userbinator on Reflections on Curly Braces – Apple’s SSL Bug and What We Should Learn From It

Comment on Reflections on Curly Braces – Apple’s SSL Bug and What We Should Learn From It

In fact the 2nd version with braces actually makes it harder to see the doubled goto, while it stood out to me in the 1st version.

However if I were writing that code I would've probably used an OR-chain, like this:

    if ((err = SSLFreeBuffer(&hashCtx)) ||
        (err = ReadyHash(&SSLHashSHA1, &hashCtx)) ||
        (err = SSLHashSHA1.update(&hashCtx, &clientRandom)) ||
        (err = SSLHashSHA1.update(&hashCtx, &serverRandom)) ||
        (err = SSLHashSHA1.update(&hashCtx, &signedParams)) ||
        (err = SSLHashSHA1.final(&hashCtx, &hashOut)))
        goto fail;

There's also another oddness I noticed:

        if(err) {
            sslErrorLog("SSLDecodeSignedServerKeyExchange: sslRawVerify "
                        "returned %d\n", (int)err);
            goto fail;
        }
     fail:

sanderjd 12y

I think the second version with the braces really misses the point of the "use braces" argument. From my perspective, the main error cases for that code are merge conflicts and accidental line duplication. In either of those cases the new gotofail line would end up inside the braces where it would be harmless dead code.

ctdonath 12y

The programmer did something to cause doubling of that particular line. Habitual braces placed at the onset of writing the if()... would have made that act, whatever it was, obvious and obviously wrong.

The 2nd version fails as an example because it hides the act of creation then surprises you with "here, look at this, anything wrong?" with the author knowing full well he's making it wrong.

My takeaway from this whole discussion is: assume failure, prove success. The offending function failed by presuming success; it did not positively confirm every criteria for success, with even the "fail:" section executed by success.

Comments