Archive for the ‘Coding Standards’ Category

Apple’s #gotofail SSL Security Bug was Easily Preventable

Monday, March 3rd, 2014 Michael Barr

If programmers at Apple had simply followed a couple of the rules in the Embedded C Coding Standard, they could have prevented the very serious `Gotofail` SSL bug from entering the iOS and OS X operating systems. Here’s a look at the programming mistakes involved and the easy-to-follow coding standard rules that could have easily prevent the bug.

In case you haven’t been following the computer security news, Apple last week posted security updates for users of devices running iOS 6, iOS 7, and OS X 10.9 (Mavericks). This was prompted by a critical bug in Apple’s implementation of the SSL/TLS protocol, which has apparently been lurking for over a year.

In a nutshell, the bug is that a bunch of important C source code lines containing digital signature certificate checks were never being run because an extraneous goto fail; statement in a portion of the code was always forcing a jump. This is a bug that put millions of people around the world at risk for man-in-the-middle attacks on their apparently-secure encrypted connections. Moreover, Apple should be embarrassed that this particular bug also represents a clear failure of software process at Apple.

There is debate about whether this may have been a clever insider-enabled security attack against all of Apple’s users, e.g., by a certain government agency. However, whether it was an innocent mistake or an attack designed to look like an innocent mistake, Apple could have and should have prevented this error by writing the relevant portion of code in a simple manner that would have always been more reliable as well as more secure. And thus, in my opinion, Apple was clearly negligent.

Here are the lines of code at issue (from Apple’s open source code server), with the extraneous goto in bold:

static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams, ...)
{
    OSStatus  err;
    ...

    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
        goto fail;
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
        goto fail;
        goto fail;
    if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
        goto fail;
    ...

fail:
    SSLFreeBuffer(&signedHashes);
    SSLFreeBuffer(&hashCtx);
    return err;
}

The code above violates at least two rules from Barr Group‘s Embedded C Coding Standard book. Importantly, had Apple followed at least the first of these rules, in particular, this dangerous bug should almost certainly have been prevented from ever getting into even a single device.

Rule 1.3.a

Braces shall always surround the blocks of code (a.k.a., compound statements), following if, else, switch, while, do, and for statements; single statements and empty statements following these keywords shall also always be surrounded by braces.

Had Apple not violated this always-braces rule in the SSL/TLS code above, there would have been either just one set of curly braces after each if test or a very odd looking hard-to-miss chunk of code with two sets of curly braces after the if with two gotos. Either way, this bug was preventable by following this rule and performing code review.

Rule 1.7.c

The goto keyword shall not be used.

Had Apple not violated this never-goto rule in the SSL/TLS code above, there would not have been a double goto fail; line to create the unreachable code situation. Certainly if that forced each of the goto lines to be replaced with more than one line of code, it would have forced programmers to use curly braces.

On a final note, Apple should be asking its engineers and engineering managers about the failures of process (at several layers) that must have occurred for this bug to have gone into end user’s devices. Specifically:

  • Where was the peer code review that should have spotted this, or how did the reviewers fail to spot this?
  • Why wasn’t a coding standard rule adopted to make such bugs easier to spot during peer code reviews?
  • Why wasn’t a static analysis tool, such as Klocwork, used, or how did it fail to detect the unreachable code that followed? Or was it users of such a tool, at Apple, who failed to act?
  • Where was the regression test case for a bad SSL certificate signature, or how did that test fail?

Dangerous bugs, like this one from Apple, often result from a combination of accumulated errors in the face of flawed software development processes. Too few programmers recognize that many bugs can be kept entirely out of a system simply by adopting (and rigorously enforcing) a coding standard that is designed to keep bugs out.

How to Combine Volatile with Struct

Friday, November 9th, 2012 Michael Barr

C’s volatile keyword is a qualifier that can be used to declare a variable in such a way that the compiler will never optimize away any of the reads and writes. Though there are several important types of variables to declare volatile, this obscure keyword is especially valuable when you are interacting with hardware peripheral registers and such via memory-mapped I/O.

Sometimes a memory-mapped I/O device could be as simple as just having a single 8-bit control register at a fixed address. In that case, the placement of volatile should be to the left of the * operator in the declaration of the pointer to that address, as in:


uint8_t volatile * p_ledreg = 0x10000000;

In the above code, the variable p_legreg is a pointer to a volatile 8-bit unsigned register located at address 0x10000000.

However, it is far more common that memory-mapped peripherals have at least a half dozen registers. In this more complicated scenario, a C struct can be defined to encapsulate these registers as a set and a pointer to said data structure can be declared. Here’s an example of such a declaration that does not feature the volatile keyword at all:


typedef struct
{
uint8_t reg1;
uint8_t reg2;
uint8_t _reserved;
uint8_t reg3;

} mydevice_t;

mydevice_t * p_mydevice = 0x10000000;

In this scenario, there are three possible places for the volatile keyword. First, the first line could be modified so that the new type “mydevice_t” always contains the volatile keyword, as in:


typedef volatile struct

Or the last line could be modified so that the pointer “p_mydevice” is a pointer to a volatile mydevice_t:


mydevice_t volatile * p_mydevice = 0x10000000;

Note that the difference between these first two volatile placements is whether all instances of said struct are volatile or only the pointer’s instance is volatile. If there is only one instance of the struct in the whole program and it is the pointer p_mydevice, then that difference is immaterial.

Finally, the third option is to place one or more volatile keywords within the struct definition itself. With this placement, only the specific registers within the struct that are declared volatile will be treated, by the compiler, as subject to asynchronous change. Reads and writes from or to other, non-volatile-declared, registers in the struct may potentially be optimized away. Here’s an example:


typedef struct
{
uint8_t volatile reg1;
uint8_t volatile reg2;
uint8_t const _reserved;
uint8_t reg3;
}

mydevice_t * p_mydevice = 0x10000000;

Given that there are multiple choices for the placement of volatile, where is the best place to put the volatile keyword in practice? My preferred placement is typically in the pointer declaration. That way, all of the registers in the struct will be treated, by the compiler, as volatile and yet it is possible to have other (e.g. RAM-based shadows) instances of said struct that are not volatile because they are not actually hardware registers underneath.

Combining C’s volatile and const Keywords

Tuesday, January 24th, 2012 Michael Barr

Does it ever make sense to declare a variable in C or C++ as both volatile (i.e., “ever-changing”) and const (“read-only”)? If so, why? And how should you combine volatile and const properly?

One of the most consistently popular articles on the Netrino website is about C’s volatile keyword. The volatile keyword, like const, is a type qualifier. These keywords can be used by themselves or together in variable declarations.

I’ve written about volatile and const individually before. If you haven’t previously used the volatile keyword, I recommend you read How to Use C’s volatile Keyword before going on. As that article makes plain:

C’s volatile keyword is a qualifier that is applied to a variable when it is declared. It tells the compiler that the value of the variable may change at any time–without any action being taken by the code the compiler finds nearby.

How to Use C’s volatile Keyword

By declaring a variable volatile you are effectively asking the compiler to be as inefficient as possible when it comes to reading or writing that variable. Specifically, the compiler should generate object code to perform each and every read from a volatile variable and each and every write to a volatile variable–even if you write it twice in a row or read it and ignore the result. No read or write can be skipped. Effectively no optimizations are allowed with respect to volatile variables.

The use of volatile variables also creates additional sequence points in C and C++ programs. The order of accesses of volatile variables A and B in the object code must be the same as the order of those accesses in the source code. The compiler is not allowed to reorder volatile variable accesses for any reason.

Here are a couple of examples of declarations of volatile variables:

int volatile g_flag_shared_with_isr;

uint8_t volatile * p_led_reg = (uint8_t *) 0x00080000;

The first example declares a global flag that can be shared between an ISR and some other part of the code (e.g., a background processing loop in main() or an RTOS task) without fear that the compiler will optimize (i.e., “delete”) the code you write to check for asynchronous changes to the flag’s value. It is important to use volatile to declare all variables that are shared by asynchronous software entities, which is important in any kind of multithreaded programming. (Remember, though, that access to global variables shared by tasks or with an ISR must always also be controlled via a mutex or interrupt disable, respectively.)

The second example declares a pointer to a hardware register at a known physical memory address (80000h)–in this case to manipulate the state of one or more LEDs. Because the pointer to the hardware register is declared volatile, the compiler must always perform each individual write. Even if you write C code to turn an LED on followed immediately by code to turn the same LED off, you can trust that the hardware really will receive both instructions. Because of the sequence point restrictions, you are also guaranteed that the LED will be off after both lines of the C code have been executed. The volatile keyword should always be used with creating pointers to memory-mapped I/O such as this.

[See Coding Standard Rule #4: Use volatile Whenever Possible for more on the use of volatile by itself.]

How to Use C’s const Keyword

The const keyword is can be used to modify parameters as well as in variable declarations. Here we are only interested in the use of const as a type qualifier, as in:

uint16_t const max_temp_in_c = 1000;

This declaration creates a 16-bit unsigned integer value of 1,000 with a scoped name of max_temp_in_c. In C, this variable will exist in memory at run-time, but will typically be located, by the linker, in a non-volatile memory area such as ROM or flash. Any reference to the const variable will read from that location. (In C++, a const integer may no longer exist as an addressable location in run-time memory.)

Any attempt the code makes to write to a const variable directly (i.e., by its name) will result in a compile-time error. To the extent that the const variable is located in ROM or flash, an indirect write (i.e., via a pointer to its address) will also be thwarted–though at run-time, obviously.

Another use of const is to mark a hardware register as read-only. For example:

uint8_t const * p_latch_reg = 0x10000000;

Declaring the pointer this way, any attempt to write to that physical memory address via the pointer (e.g., *p_latch_reg = 0xFF;) should result in a compile-time error.

[See Coding Standard Rule #2: Use const Whenever Possible for more on the use of const by itself.]

How to Use const and volatile Together

Though the essence of the volatile (“ever-changing”) and const (“read-only”) decorators may seem at first glance opposed, there are some times when it makes sense to use them both to declare one variable. The scenarios I’ve run across have involved pointers to memory-mapped hardware registers and shared memory areas.

(#1) Constant Addresses of Hardware Registers

The following declaration uses both const and volatile in the frequently useful scenario of declaring a constant pointer to a volatile hardware register.

uint8_t volatile * const p_led_reg = (uint8_t *) 0x00080000;

The proper way to read a complex declaration like this is from the name of the variable back to the left, as in:

p_led_reg IS A constant pointer TO A volatile 8-bit unsigned integer.

Reading it that way, we can see that the keyword const modifies only the pointer (i.e., the fixed address 80000h), which does not change at run-time. Whereas the keyword volatile modifies only the type of integer. This is actually quite useful and is a much safer version of the declaration of a p_led_reg that appears at the top of this article. In particular, adding const means that the simple typo of a missed pointer dereference (‘*’) will be caught at compile time. That is, the mistaken code p_led_reg = LED1_ON; won’t overwrite the address with the non-80000h value of LED1_ON. The compiler error leads us to correct this to *p_led_reg = LED1_ON;, which is almost certainly what we meant to write in the first place.

(#2) Read-Only Shared-Memory Buffer

Another use for a combination of const and volatile is where you have two processors communicating via a shared memory area and you are coding the side of this communications that will only be reading from a shared memory buffer. In this case you could declare variables such as:

int const volatile comm_flag;

uint8_t const volatile comm_buffer[BUFFER_SIZE];

Of course, you’d usually want to instruct the linker to place these global variables at the correct addresses in the shared memory area or to declare the above as pointers to specific physical memory addresses. In the case of pointers, the use of const and volatile may become even more complex, as in the next category.

(#3) Read-Only Hardware Register

Sometimes you will run across a read-only hardware register. In addition to enforcing compile-time checking so that the software doesn’t try to overwrite the memory location, you also need to be sure that each and every requested read actually occurs. By declaring your variable IS A (constant) pointer TO A constant and volatile memory location you request all of the appropriate protections, as in:

uint8_t const volatile * const p_latch_reg = (uint8_t *) 0x10000000;

As you can see, the declarations of variables that involve both the volatile and const decorators can quickly become complicated to read. But the technique of combining C’s volatile and const keywords can be useful and even important. This is definitely something you should learn to master to be a master embedded software engineer.

Don’t Follow These 5 Dangerous Coding Standard Rules

Tuesday, August 30th, 2011 Michael Barr

Over the summer I happened across a brief blog post by another firmware developer in which he presented ten C coding rules for better embedded C code. I had an immediate strong negative reaction to half of his rules and later came to dislike a few more, so I’m going to describe what I don’t like about each. I’ll refer to this author as BadAdvice. I hope that if you have followed rules like the five below my comments will persuade you to move away from those toward a set of embedded C coding rules that keep bugs out. If you disagree, please start a constructive discussion in the comments.

Bad Rule #1: Do not divide; use right shift.

As worded, the above rule is way too broad. It’s not possible to always avoid C’s division operator. First of all, right shifting only works as a substitute for division when it is integer division and the denominator is a power of two (e.g., right shift by one bit to divide by 2, two bits to divide by 4, etc.). But I’ll give BadAdvice the benefit of the doubt and assume that he meant to say you should “use right shift as a substitute for division whenever possible”.

For his example, BadAdvice shows code to compute an average over 16 integer data samples, which are accumulated into a variable sum, during the first 16 iterations of a loop. On the 17th iteration, the average is computed by right shifting sum by 4 bits (i.e., dividing by 16). Perhaps the worst thing about this example code is how much it is tied a pair of #defines for the magic numbers 16 and 4. A simple but likely refactoring to average over 15 instead of 16 samples would break the entire example–you’d have to change from the right shift to a divide proper. It’s also easy to imagine someone changing AVG_COUNT from 16 to 15 without realizing about the shift; and if you didn’t change this, you’d get a bug in that the sum of 15 samples would still be right shifted by 4 bits.

Better Rule: Shift bits when you mean to shift bits and divide when you mean to divide.

There are many sources of bugs in software programs. The original programmer creates some bugs. Other bugs result from misunderstandings by those who later maintain, extend, port, and/or reuse the code. Thus coding rules should emphasize readability and portability most highly. The choice to deviate from a good coding rule in favor of efficiency should be taken only within a subset of the code. Unless there is a very specific function or construct that needs to be hand optimized, efficiency concerns should be left to the compiler.

Bad Rule #2: Use variable types in relation to the maximum value that variable may take.

BadAdvice gives the example of a variable named seconds, which holds integer values from 0 to 59. And he shows choosing char for the type over int. His stated goal is to reduce memory use.

In principle, I agree with the underlying practices of not always declaring variables int and choosing the type (and signedness) based on the maximum range of values. However, I think it essential that any practice like this be matched with a corresponding practice of always declaring specifically sized variables using C99’s portable fixed-width integer types.

It is impossible to understand the reasoning of the original programmer from unsigned char seconds;. Did he choose char because it is big enough or for some other reason? (Remember too that a plain char may be naturally signed or unsigned, depending on the compiler. Perhaps the original programmer even knows his compiler’s chars are default unsigned and omits that keyword.) The intent behind variables declared short and long is at least as difficult to decipher. A short integer may be 16-bits or 32-bits (or something else), depending on the compiler; a width the original programmer may have (or may not have) relied upon.

Better Rule: Whenever the width of an integer matters, use C99’s portable fixed-width integer types.

A variable declared uint16_t leaves no doubt about the original intent as it is very clearly meant to be a container for an unsigned integer value no wider than 16-bits. This type selection adds new and useful information to the source code and makes programs both more readable and more portable. Now that C99 has standardized the names of fixed-width integer types, declarations involving short and long should no longer be used. Even char should only be used for actual character (i.e., ASCII) data. (Of course, there may still be int variables around, where size does not matter, such as in loop counters.)

Bad Rule #3: Avoid >= and use <.

As worded above, I can’t say I understand this rule or its goal sufficiently, but to illustrate it BadAdvice gives the specific example of an if-else if wherein he recommends if (speed < 100) ... else if (speed > 99) instead of if (speed < 100) ... else if (speed >= 100). Say what? First of all, why not just use else for that specific scenario, as speed must be either below 100 or 100 or above.

Even if we assume we need to test for less than 100 first and then for greater than or equal to 100 second, why would anyone in their right mind prefer to use greater than 99? That would be confusing to any reader of the code. To me it reads like a bug and I need to keep going back over it to find the logical problem with the apparently mismatched range checks. Additionally, I believe that BadAdvice’s terse rationale that “Benefits: Lesser Code” is simply untrue. Any half decent compiler should be able to optimize either comparison as needed for the underlying processor.

Better Rule: Use whatever comparison operator is easiest to read in a given situation.

One of the very best things any embedded programmer can do is to make their code as readable as possible to as broad an audience as possible. That way another programmer who needs to modify your code, a peer doing code review to help you find bugs, or even you years later, will find the code hard to misinterpret.

Bad Rule #4: Avoid variable initialization while defining.

BadAdvice says that following the above rule will make initialization faster. He gives the example of unsigned char MyVariable = 100; (not preferred) vs:


#define INITIAL_VALUE 100
unsigned char MyVariable;
// Before entering forever loop in main
MyVariable = INITIAL_VALUE

Though it’s unclear from the above, let’s assume that MyVariable is a local stack variable. (It could also be global, the way his pseudo code is written.) I don’t think there should be a (portably) noticeable efficiency gain from switching to the latter. And I do think that following this rule creates an opening to forget to do the initialization or to unintentionally place the initialization code within a conditional clause.

Better Rule: Initialize every variable as soon as you know the initial value.

I’d much rather see every variable initialized on creation with perhaps the creation of the variable postponed as long as possible. If you’re using a C99 or C++ compiler, you can declare a variable anywhere within the body of a function.

Bad Rule #5: Use #defines for constant numbers.

The example given for this rule is of defining three constant values, including #define ON 1 and #define OFF 0. The rationale is “Increased convenience of changing values in a single place for the whole file. Provides structure to the code.” And I agree that using named constants instead of magic numbers elsewhere in the code is a valuable practice. However, I think there is an even better way to go about this.

Better Rule: Declare constants using const or enum.

C’s const keyword can be used to declare a variable of any type as unable to be changed at run-time. This is a preferable way of declaring constants, as they are in this way given a type that can be used to make comparisons properly and enabling them to be type-checked by the compiler if they are passed as parameters to function calls. Enumeration sets may be used instead for integer constants that come in groups, such as enum { OFF = 0, ON };.

Final Thoughts

There are two scary things about these and a few of the other rules on BadAdvice’s blog. First, is that they are out there on the Internet to be found with a search for embedded C coding rules. Second, is that BadAdvice’s bio says he works on medical device design. I’m not sure which is worse. But I do hope the above reasoning and proposed better rules gets you thinking about how to develop more reliable embedded software with fewer bugs.

How to Enforce Coding Standards Using PC-Lint

Thursday, June 16th, 2011 Michael Barr

In an earlier blog post, I introduced the concept of automatic enforcement of coding standards by stating that:

Enforcement of coding standards too often depends on programmers already under deadline pressure to be disciplined while they code and/or to make time to perform peer code reviews. … To ensure your selected coding standard is followed, and thus effective, your team should find as many automated ways to enforce as many of its rules as possible. And you should make such automated rule checking part of the everyday software build process.

One of the tools we have found to be indispensible for this purpose is Gimpel Software‘s PC-Lint (and the multi-platform FlexeLint). At just a few hundred dollars per seat, PC-Lint happens to also be one of the least expensive tools we own and thus a source of tremendous value to our team.

The following options can be set (in version 9.00) to assist in the automated enforcement of the identified rules from Barr Group’s Embedded C Coding Standard.

 

Rule(s) Tool Configuration Notes
Which C (1.1) 975 (unrecognized pragma)

+pragma(name, action)

-pragma(name)

Braces (1.3) PC-Lint can help identify missing braces through indentation checking:

525 (negative indentation)

539 (did not expect positive indentation)

725 (expected positive indentation)

Parentheses (1.4) 665 (Unparenthesized parameter in macro is passed an expression)

773 (Expression-like macro not parenthesized)

821 (Right hand side of assignment not parenthesized)

834 (Operator ‘Name’ followed by operator ‘Name’ is confusing. Use parentheses)

973 (Unary operator in macro ‘Symbol‘ not parenthesized)

Casts (1.6) 507, 511, 519 (Size incompatibility in cast)

549, 571, 611 (suspicious cast)

643 (loss of precision in pointer cast)

680 (suspicious truncation in arithmetic expression converted to pointer)

688 (cast used within preprocessor conditional statement)

740, 741 (unusual pointer cast)

920 (cast from ‘type’ to ‘void’)

921-930 (cast from ‘type’ to ‘type’)

1773 (attempt to cast away const or volatile)

Keywords to avoid (1.7) -deprecate( keyword, auto, violates coding standard )

-deprecate( keyword, register, violates coding standard )

-deprecate( keyword, goto, violates coding standard )

-deprecate( keyword, continue, violates coding standard )

Keywords to Frequent (1.8) 818 (Pointer parameter could be declared ptr to const)

843 (Variable could be declared as const)

844 (Pointer variable could be declared as const)

952 (parameter could be declared as const)

953 (variable could be declared as const)

954 (pointer variable could be declared as pointing to a const)

765 (external symbol could be made static)

Comments, Acceptable Formats (2.1) 602 (comment within comment)
Alignment (3.2)

Indentation (3.4)

525 (negative indentation)

539 (did not expect positive indentation)

725 (expected positive indentation)

Header Files (4.2)

Source Files (4.3)

451 (header file repeatedly included but does not have a standard include guard)

766 (Include of header file not used in module)

966 (indirectly included header file not used in module)

967 (Header file does not have a standard include guard)

Fixed Width Integers (5.2)

Signed Integers (5.3)

569 (Loss of information — Integer bits to Integer bits)

570 (loss of sign, assignment being made from a negative constant to an unsigned quantity)

573 (signed-unsigned mix with divide)

574 (signed-unsigned mix with relational)

701, 703 (shift left of signed quantity)

702, 704 (shift right of signed quantity)

712 (loss of precision, one type is larger than another type)

713 (loss of precision, signed to unsigned)

734 (loss of precision, to a quantity smaller than an int)

Structure and Unions (5.5) 38 (Offset of symbol inconsistent — A member of a class or struct appears in a different position (offset from the start of the structure) than an earlier declaration)

39 (Redefinition of symbol conflicts with ‘location’ — a struct or a union is being redefined)

Function-Like Macros (6.3) 665 (Unparenthesized parameter in macro is passed an expression)

773 (Expression-like macro not parenthesized)

Initialization (7.2) 35 (initializer has side-effects)

133 (too many initializers for aggregate)

134 (missing initializer)

530, 603 (symbol not initialized)

540 (excessive size — a string initializer required more space than what was allocated)

644 (variable may not have been initialized)

651 (potentially confusing initializer)

708 (union initialization)

727, 728, 729, 738 (symbol not explicitly initialized)

771, 772 (symbol conceivably not initialized)

784 (nul character truncated from string — During initialization of an array with a string constant there was not enough room to hold the trailing NUL character)

785, 943 (too few initializers for aggregate)

940 (omitted braces within an initializer)

Switch Statements (8.3) 44 (A case or default statement occurred outside a switch)

108 (Invalid context — A continue or break statement was encountered without an appropriate surrounding context)

137 (constant used twice within switch)

408 (type mismatch with switch expression)

744 (switch statement has no default)

764 (switch statement does not have a case)

825 (control flows into case/default)

Equivalence Tests (8.6) 720 (Boolean test of assignment)

 

As a convenience to readers who follow Barr Group’s Embedded C Coding Standard and may like to use RSM and/or PC-Lint to inexpensively and automatically enforce as many of the rules as possible, here are starter configuration files: