Randomize

I was reading an excellent ACM article on the recent Apple security bug, and it struck me that the author skipped completely over one of the *true* root causes of this bug.

Here’s the buggy code:

if ((err = SSLFreeBuffer(&hashCtx)) != 0)
goto fail;
if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
goto fail;
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;

DRY – Don’t Repeat Yourself

The goto in itself isn’t “bad,” but the duplication of the goto is what leads to the problem.

When two conditions lead to the same result, you should favor code that “measures twice and cuts once.” You may have to engage a few more brain cells to make this code style perform as well as spaghetti branches, but it’s worth the investment.

Consider the following replacement code:

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

Sentences should not

have arbitrary line breaks and neither

should code

Sure, braces would have helped here, but IMHO, simple if statements should simply include their action on the same line, just like they did back in the ol’ line-numbered BASIC days. And, ideally, similar logic should align their actions. Consider this code:

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

While this still exhibits the same overuse of evaluating assignments and duplicating actions, at least it is abundantly clear what is going on, and an extra “goto” will stick out like a sore thumb.

I realize this is an unorthodox way to format C code, but it’s a favorite of mine. Maybe my mind is just crippled by the semester of COBOL I took in college, but I like aligned code. Like a good grid vs. a CSV text file, the alignment makes it far easier to spot inconsistencies. I use the same approach in CSS and T-SQL, where it makes sense to do so.