goto fail and the ARMCC Compiler

I can't imagine anyone reading this posting hasn't already read about the Apple "goto fail" bug in SSL. My reaction was one of incredulity; I really couldn't believe this code could have got into the wild on so many levels.

First we've got to consider the testing (or lack thereof) for this codebase. The side effect of the bug was that all SSL certificates passed, even malformed ones. This implies positive testing (i.e. we can demonstrate it works), but no negative testing (i.e. a malformed SSL certificate), or even no dynamic SSL certificate testing at all?

What I haven't established* is whether the bug came about through code removal (e.g. there was another 'if' statement before the second goto) or, due to trial-and-error programming, the extra goto got added (with other code) that then didn't get removed on a clean-up. There are, of course, some schools of thought that believe it was deliberately put in as part of prism!

Then you have to query regression testing; have they never tested for malformed SSL certificates (I can't believe that; mind you I didn't believe Lance was doping!) or did they use a regression-subset for this release which happened to miss this bug? Regression testing vs. product release is always a massive pressure. Automation of regression testing through continuous integration is key, but even so, for very large code bases it is simplistic to say "rerun all tests"; we live in a world of compromises.

Next, if we actually analyse the code then I can imagine the MISRA-C group jumping around saying "look, look, if only they’d followed MISRA-C this couldn't of happened" (yes Chris, it's you I'm envisaging) and of course they're correct. This code breaks a number of MISRA rules, but most notably:

15.6 (Required) The body of an iteration-statement or selection-statement shall be a compound-statement

Which boils down to all if-statements must use a block structure, so the code would go from (ignoring the glaring coding error of two gotos):

       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;

to

       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;
       }

This would then stop the unconditional goto be executed. But would cause a further rule violation:

Rule 2.1 (Required) A project shall not contain unreachable code

Nevertheless, what might surprise you (unless you follow MISRA-C closely) is that the use of the goto statement ISallowed:

Rule 15.2 (Required) The goto statement shall jump to a label declared in the same function.

but discouraged:

Rule 15.1 (Advisory) The goto statement should not be used.

In addition, I would expect all static analysis tools to flag this error; indicating even rudimentary static analysis is not being applied to this codebase. But that's not really what struck me when I first saw the code. My reaction was

"They must have ignored compiler warnings, as any compiler worth its salt  would warn about unreachable code"

Now I do most of my work, here at Feabhas using either the ARM/Keil compiler (armcc) or the IAR Compiler (iccarm) and am very used to seeing this warning as it is common to have infinite loops in multi-tasking code. Sure enough, give the following code:

int calculate_F(void);
int simpleTest(int p) 
{
   int ret_val = 1;
   if(p & 0x000F)
      goto out;
   if(p & 0x00F0)
      goto out;
      goto out;
   if(p & 0x0F00)
      goto out;
      ret_val = calculate_F();
out:
       return ret_val;
}

As expected, the ARM compiler reports a warning regarding unreachable code.

goto fail

So how could this happen? To my utter amazement, compiling this with GCC and -Wall (Warnings all) doesn’t report any warnings. Also apparently neither does Clang (the default compiler on OSX) with -Wall, but does if you specify -Wunreachable-code (which isn’t part of all ???).

So what’s my takeaway from this? I’ve always advocated the use of Static Analysis tools as an integral part of the build cycle (i.e. not trying to apply it retrospectively to 100,000’s of lines of code) rather than relying on the compiler to generate appropriate warnings. And if you weren’t convinced before, this is just another, now well documented, example of why you should.

* Please let me know if it has

Anonymous
  • No GCC isn't a problem if you're using a good static analysis tool. Unfortunatley I see too many companies not prepared to invest in even (good) entry level tools such as pc-lint.

  • To demonstrate (some of) the danger(s) of omitting curly braces, consider this (insane) example:

    #define goto     printf("Executing 'goto' in File: %s, Line: %s\n", __FILE__, __LINE__); goto

    Using curly braces...

         if(p & 0x00f0)

         {

              goto out;

         }

    ...then you're safe, but as soon as you remove the curly braces, guess what ?

  • So is this a GCC problem?  We've got issues with GCC sometimes, too.  I'm not quite sure what to think of the open source tools. 

  • Placement and alignment... No matter what we say and do, this will always be our own opinion and what we've gotten used to.

    I no longer want to inflict my opinion on other people, as I've seen it may do more damage than good.

    But earlier, I worked in a fairly large software company. This company had rules for the alignment and placement of curly braces.

    I adopted this style quickly, and I prefer it at all times, as it's very easy to overview and the formatting is quite pretty.

    But if I contribute to open source projects, I choose to try and keep the style of the project, in order to keep it consistent.

    Unfortunately there are some alignment/placement rules that confuses me so much that I can not read the code.

    So regarding those issues, I try my best to follow the existing style; fortunately all rules allow me to always place curly braces.

  • Testing, or lack of, aside, I'm with both of you on one aspect of this. I am quite sure that this code would never have got into the wild if the programmer had followed the simple rule of using curly braces at all times and in all places. The offending statement would have stood out visually for a start.

    As to the placement and alignment of those braces... well, I still have the scars from the last time I got involved in that debate!

    Chris

Related