SEI CERT C Coding Standard

Space shortcuts

Page tree

The size of a structure is not always equal to the sum of the sizes of its members. Subclause 6.7.2.1 of the C Standard states, "There may be unnamed padding within a structure object, but not at its beginning" [ISO/IEC 9899:2011].

This unnamed padding is often called structure padding. Structure members are arranged in memory as they are declared in the program text. Padding may be added to the structure to ensure the structure is properly aligned in memory. Structure padding allows for faster member access on many architectures.

Rearranging the fields in a struct can change the size of the struct. It is possible to minimize padding anomalies if the fields are arranged in such a way that fields of the same size are grouped together.

Padding is also called struct member alignment. Many compilers provide a flag that controls how the members of a structure are packed into memory. Modifying this flag may cause the size of the structures to vary. Most compilers also include a keyword that removes all padding; the resulting structures are called packed structures. Overriding the default behavior is often unwise because it leads to interface compatibility problems (the nominally same struct has its layout interpreted differently in different modules).

Noncompliant Code Example

This noncompliant code example assumes that the size of struct buffer is equal to the sum of the size of its individual components, which may not be the case [Dowd 2006]. The size of struct buffer may actually be larger because of structure padding.

enum { buffer_size = 50 };

struct buffer {
  size_t size;
  char bufferC[buffer_size];
};

/* ... */

void func(const struct buffer *buf) {

  /*
   * Incorrectly assumes sizeof(struct buffer) =
   * sizeof(size_t) + sizeof(bufferC)
   */
  struct buffer *buf_cpy = (struct buffer *)malloc(
    sizeof(size_t) + (buffer_size * sizeof(char) /* 1 */)
  );

  if (buf_cpy == NULL) {
    /* Handle malloc() error */
  }

  /* 
   * With padding, sizeof(struct buffer) may be greater than
   * sizeof(size_t) + sizeof(buff.bufferC), causing some data  
   * to be written outside the bounds of the memory allocated.
   */
  memcpy(buf_cpy, buf, sizeof(struct buffer));

  /* ... */

  free(buf_cpy);
}

Compliant Solution

Accounting for structure padding prevents these types of errors:

enum { buffer_size = 50 };

struct buffer {
  size_t size;
  char bufferC[buffer_size];
};

/* ... */

void func(const struct buffer *buf) {

  struct buffer *buf_cpy = 
    (struct buffer *)malloc(sizeof(struct buffer));

  if (buf_cpy == NULL) {
    /* Handle malloc() error */
  }

  /* ... */

  memcpy(buf_cpy, buf, sizeof(struct buffer));

  /* ... */

  free(buf_cpy);
}

Risk Assessment

Failure to correctly determine the size of a structure can lead to subtle logic errors and incorrect calculations, the effects of which can lead to abnormal program termination, memory corruption, or execution of arbitrary code.

Recommendation

Severity

Likelihood

Remediation Cost

Priority

Level

EXP03-C

High

Unlikely

High

P3

L3

Automated Detection

Tool

Version

Checker

Description

Astrée
24.04

Supported: Astrée reports accesses outside the bounds of allocated memory.
Helix QAC

2024.1

C0697
LDRA tool suite
9.7.1

578 S

Enhanced enforcement

Related Vulnerabilities

Search for vulnerabilities resulting from the violation of this rule on the CERT website.

Related Guidelines

SEI CERT C++ Coding StandardVOID EXP03-CPP. Do not assume the size of a class or struct is the sum of the sizes of its members

Bibliography

[Dowd 2006]Chapter 6, "C Language Issues" ("Structure Padding," pp. 284–287)
[ISO/IEC 9899:2011]Subclause 6.7.2.1, "Structure and Union Specifiers"
[Sloss 2004]Section 5.7, "Structure Arrangement"


18 Comments

  1. Abhijit Rao

    The compliant solution must be improved upon - it does not compile, it has memory leak and the name of the struct is buffer and the field in it is also named buffer . 

    here is the compliant code

    struct buffer {
    size_t size;
    char bufferC[50];
};

/* ... */

void func(struct buffer *buf) {

  struct buffer *buf_cpy = malloc(sizeof(struct buffer));
  if (buf_cpy == NULL) {
    /* Handle malloc() error */
  }

  /* ... */

  memcpy(buf_cpy, buf, sizeof(struct buffer));   /* ... */  free(buf_cpy); }

    Perhaps other aspects of Padding must also be discussed. Padding is also refered to as "Struct Member Alignment" - it is compiler flag. By changing this option - the size can vary. Most compilers include a keyword that removes all padding, such structures are referred to as Packed structures.

    It is also important to emphasize that rearranging the fields in a struct can change the size of the struct. It is possible to minimize padding anomalies if the fields are arranged  in such a way that fields of the same size are *grouped* together.

     Reference

    [ARM system Developer's Guide] Chapter 5, Efficient C Programming, Section 5.7 Structure Arrangement 

    1. Robert Seacord

      I've incorporated all your suggestions (to the best of my ability) above.

  2. Douglas A. Gwyn

    It should be explained that structure padding allows for faster member access, on many architectures, and that overriding this default is unwise since it leads to interface compatibility problems (the nominally same struct has its layout interpreted differently in different modules).

    1. Robert Seacord

      OK, I've added words to this effect.

      Do we need another recommendation that says something like "avoid unaligned references on packed structures?" Anyone want to write it? ;^)

  3. David Svoboda

    It's impossible for a static analysis tool to infer the intentions of a programmer, so ROSE can't tell that the programmer summed up the size of a struct's elements and substituted that for the size of a struct. Besides, such an error would cause a programmer to issue magic numbers (54) to a malloc function, or some function that expected a sizeof() operator. We have rules to catch magic numbers and a rule to catch malloc without a sizeof().

    1. Robert Seacord

      I'm leaning towards this being analyzable. I rewrote the noncompliant code example to not use magic numbers (and still be wrong). I think it is analyzable however because a tool could keep track of the size of buf_cpy and determine that the subsequent memcpy() is copying more memory then has been allocated.

      1. David Svoboda

        Changed to 'rose-partial'. The NCCE is now analyzeable. But I still think that many, many instances of code will not be amenable to static analysis.

  4. David Svoboda

    If violating this rule can lead to arbitrary code execution, its severity should be High. I've made this change.

    1. Aaron Ballman

      Good catch, thank you!

  5. Christian

    memcpy(buf_cpy, buf, sizeof(struct buffer));
    Correct me if I'm wrong, but shouldn't it be 'buff' and not 'buf' in memcpy?

    This would apply to the non compliant example as well as the compliant example.

    1. Joseph C. Sible

      There's a global called buff and a function parameter called buf. I think they're each being used correctly right now.

      1. David Svoboda

        Agreed, the code is correct. For the sake of clarity, I took out the 'buff' type since it is not used anywhere.

        1. Joseph C. Sible

          Uh, that is used: sizeof(buff.bufferC) 

          1. Yozo TODA

            sizeof(buf→bufferC) or buffer_size.

            I suppose buffer_size is better here (-:

            1. David Svoboda

              Agreed, I made this change.

      2. Christian

        Thanks I was so concentrated on the buf and buff I completely missed the function parameter.

  6. Marcin Gebus

    When allocating memory, it is better to use sizeof(*pointer) than sizeof(struct_name). Moreover, casting the return value which is a void pointer is redundant.  So, in the Compliant Solution it is better to write

    struct buffer *buf_cpy = malloc(sizeof(*buf_cpy));

    than

    struct buffer *buf_cpy = (struct buffer *)malloc(sizeof(struct buffer));

    It improves readability and protects from a bug when someone needs to change the type of the variable but forgets to change the argument of sizeof.

    It is also recommended to do so by Linux kernel coding style https://www.kernel.org/doc/html/v4.10/process/coding-style.html#allocating-memory

    1. David Svoboda

      Marcin:
      You might be interested in related recommendations MEM02-C. Immediately cast the result of a memory allocation function call into a pointer to the allocated type and ARR01-C. Do not apply the sizeof operator to a pointer when taking the size of an array.  I suggest that future discussion go in the comment section of one of those recommendations.  While MEM02-C recommends explicitly casting the output of malloc(), it says nothing about what whether malloc() should get a type or an expression (from which it infers the type). ARR01-C is the closest we come to specifying what goes in malloc, and that is specific to arrays (where getting the type wrong can cause obvious security problems)..