Apple SSL Bug: Why goto Didn't Fail

When discussing Apple's SSL bug sometimes people mention that goto is bad and shouldn't be used anyway. They often refer to Dijkstra's famous letter to the editor “Go To Statement Considered Harmful” as a reference that it should be known since 1968 that you shouldn't use goto. However I don't think goto is to blame for this bug, but the if clause without a block.

Why do I think that way? The use of the goto statement is one of the most discussed issues in software engineering and while I would agree that goto should not be used in most modern high level programming languages, C is not what I would call a modern programming language. In C goto statements can actually improve readability because clean-up and error handling can be isolated without having to duplicate code or nesting a lot of if clauses. Modern programming languages have other language constructs for dealing with this problem. C++ has objects and object wrappers that can manage their own clean-up when a block is left or an error occurs, Java has exceptions with “finally” clauses, Python has the “with” statement. C however doesn't have such a construct therefore using goto makes the code more readable and avoids code duplication. Dijkstra even mentions in his text an old argument “to restrict the use of the go to statement to alarm exits”, that's exactly how Apple used it.

Here's what they did:

static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
    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;
}

Usually the problem with goto is, that on the jump destination it's not clear where the program flow was coming from, so the context is not clear and wrong assumptions are made. When we look at the actual bug, we see that's not the problem here, the clean-up code works just fine. The problem is that there are wrong assumptions about what is executed conditionally and what is not. This is because of the use of an if clause without wrapping the contents in a block (curly braces). This is a common problem that makes programmes vulnerable to maintenance, just add a second statement and your code will blow up, sometimes hard and fast, sometimes in more subtle ways. The solution is easy: just use the damn curly braces, it's only a few characters more to type, maybe even none if your editor/IDE automatically inserts them for you. If you think this bloats your code because of all those extra lines, you can also put the goto on the same line as the if which makes clear that someone maintaining your code can't just insert a second statement after it.

/* preferred */
if (error) {
    goto fail;
}
/* also ok */
if (error) goto fail;
 
/* DON'T DO THIS */
if (error)
    goto fail;

This if-problem is specific to C-related languages because they use brace delimited blocks and allow for non-block if statements. This becomes a problem when the visual representation of the code doesn't match the machine interpretation, because the indentation is wrong. Python and other white space sensitive languages don't have this problem because there the visual structure (indentation) defines the machine interpretation.

Some people also say that the problem should have been detected by better quality assurance, meaning dead code detection by the compiler and automated tests. I agree that having automated tests for prevalent flaws should be the standard, but it's even better if we write code that isn't prone to produce flaws or breaks when it's edited. As for compilers checking dead code: apparently gcc doesn't contain any option to detect dead code any more, and in clang it's not included in -W -Wall -Wextra according to this German rant blog post (ImperialViolet also mentions it), so compilers make it more difficult than needed. Checking whether there are non-block if clauses that are not on the same line can be done by syntax checks like lint. The standard lint doesn't have a check like this, but JSLint checks for this by default and it shouldn't be hard to write a custom check if your lint doesn't support it.

So to sum up: please write code that is not only easy to read (as it is more often read than written) but also code that is robust in the face of changes. There will always be a guy that has to maintain your code a few years from now. It might even be future-you who introduces this kind of error.