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.
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 ???).
-Wall
-Wunreachable-code
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
niallcooling, I'm happy that I'm not the only one who thinks that way.
Personally I never use if, for, while, etc. without curly braces. Never - not even if it's a single-line if/for/while/whatever.
And I have not been using the 'goto' keyword in C ever.
Last time I used goto was in GfA-Basic for the Atari ST back in the 90's.
Instead, I re-arrange my if-blocks, so that a label and a goto is not necessary.
Yes, I know, a goto translates directly into a single machine-code instruction called a branch, and executes very, very quickly.
I am also aware that an if/else-block makes such branch instructions as well, but after rearranging my code, I often find that the code can be reduced, and suddenly I get a much better overview.
Apart from that, all my own sources compiles without any warning, because to me, a warning often means some kind of bug/error/typo.
-Even worse, I've seen libraries (fatfs?), which spit out so many warnings, that your own errors/warnings are drowned and you won't spot them.
I'm not telling everyone to do the same thing, but I encourage a few of you to try, perhaps someone will adopt some of my strange behaviour.
Perhaps -Wunreachable-code isn't considered a 'serious' enough error/warning to be grouped with '-Wall' ?
But I absolutely agree. Please report to the gcc developers.