More on the Apple SSL bug from ImperialViolet

[maxomai]

ImperialViolet has this concise explanation of what caused the problems in Apple's SSL stack. In short, it has to do with a duplicate goto statement.

Actually, no, it doesn't. While goto statements might be a bad idea (PDF), it's the proximate cause of the problem, but not the root cause. The root causes are much more difficult to fix:

• The SSL stack is actually very hard to unit-test thoroughly, and therefore even a proper test wouldn't necessarily have caught this bug.


• Programmers assume that code behaves according to its indentation, but it doesn't (unless its Python-like). So, even if another coder caught this errant goto, they might have assumed that the errant goto was tied to the previous if statement, and would thus never run.

Apple is deploying patches to fix the problem on iOS devices (iPhones, iPads, certain iPods). They have yet to deploy a patch to OS X, which, as a Mac user, I find personally irritating.

Comments

[yes-justice.livejournal.com]

I heard goto statements made optimization difficult for the compiler/linker.

I see it a lot for error handling.

if blah
goto error

[...]

error:
do a bunch of cleanup

[yes-justice.livejournal.com]

Just look at the example.

Yeah, you have to construct code so drunk people can maintaining. Doubling a goto statement seems a curious instance.

[maxomai.livejournal.com]

Goto in this code seems perfectly defensible to me, from both an optimization POV and a maintainability POV. From an optimization pov the goto simply tells the compiler, "put jmp here." The CPU takes a hit, but if the code is correct (!) then they should only see the jmp on an error condition. And "goto error" makes perfect sense even to a relatively inexperienced C coder.

Nonetheless, this could be written better, simply by getting into the habit of wrapping dependent code in curly brackets, even if it's just one line.

So instead of this:

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

You have this:

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

Notice what happens here. The expectation in the former code (which is not met) is that the redundant goto never executes. In the former code, the goto in fact runs if none of the above error conditions occur. In the latter code, the redundant goto never happens, which is what we'd expect.

For the cost of some more bytes of source and a little more clutter, you have improved readability a little and fixed the bug.

[yes-justice.livejournal.com]

You make a reasoned case for explicit bracing! The compiler may even offer us a warning in case 2, or have an easier time doing so.

Also, regarding the jmp situation, its one of those things I was told as opposed to observed. Religious lore of code, avoid goto spaghetti code. But in the error handling application, it always appeared elegant.

The conversation prompted me to find:
http://en.wikipedia.org/wiki/Goto#Criticism_and_decline

"Programming style, like writing style, is somewhat of an art and cannot be codified by inflexible rules, although discussions about style often seem to center exclusively around such rules. In the case of the goto statement, it has long been observed that unfettered use of goto's quickly leads to unmaintainable spaghetti code. However, a simple, unthinking ban on the goto statement does not necessarily lead immediately to beautiful programming: an unstructured programmer is just as capable of constructing a Byzantine tangle without using any goto's (perhaps substituting oddly-nested loops and Boolean control variables, instead). Many programmers adopt a moderate stance: goto's are usually to be avoided, but are acceptable in a few well-constrained situations, if necessary: as multi-level break statements, to coalesce common actions inside a switch statement, or to centralize cleanup tasks in a function with several error returns. (...) Blindly avoiding certain constructs or following rules without understanding them can lead to just as many problems as the rules were supposed to avert. Furthermore, many opinions on programming style are just that: opinions. They may be strongly argued and strongly felt, they may be backed up by solid-seeming evidence and arguments, but the opposing opinions may be just as strongly felt, supported, and argued. It's usually futile to get dragged into "style wars", because on certain issues, opponents can never seem to agree, or agree to disagree, or stop arguing."


It would seem the bolded solution I find readable and elegant, moreso than repeating the clean up work around the code.

[daotoad.livejournal.com]

This sort of thing is EXACTLY why you bracket all your ifs in C-like languages.


Apple needs to update their coding standards.