I look at a lot of code. In my job, recreationally on weekends. Everywhere I see people using a specific coding style and, upon pushback, defend it in the sake of brevity.
I’m talking about braceless one-line conditionals. Like this:
if ( condition )
return 'first result';
if ( other_condition )
return 'second result';
Step Away From the IDE
If you’re using braceless conditionals in your code, stop. Right now. Quit it.
I work with WordPress, so the primary languages at my disposal are PHP and JavaScript. Both languages support braces in their conditionals.
As of recently, using braces for all conditionals – even one-liners – has been accepted as a coding standard by the WordPress community.[ref]It had been accepted by the jQuery community first. Since we use jQuery so heavily inside WordPress, and are trying to maintain some sense of parity between our standards, adopting it for PHP code made sense.[/ref]
Adding another bracket is easy, ultimately doesn’t sacrifice performance, and will insulate your code from potentially catastrophic failures.
Case In Point
Apple just released an update to iOS, and allegedly has a patch pending for OS X, to patch a major security hole exposed by such a one-line conditional error. Consider the following code from Apple’s published source:
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;
}
Do you see the error? An extra [cci]goto fail;[/cci] lies outside a conditional statement and, since [cci]err[/cci] will contain valid data at this point, the function will now return a valid authentication in response to attempting to verify a signed SSL certificate.
This means that SSL is unsafe and broken for any application using this core library on affected versions of iOS and OS X. This includes the default web browser and many stock apps.
Apple is working quickly to patch the well-publicized vulnerability; the point is, it exists, is exploitable, and could easily have been prevented.
Prevention
Two different tools could have been used to prevent this flaw: unit tests and proper code style.
I love unit testing, and I’ll be among the first to point out that a failure like this can be chalked up to insufficient unit tests for the function in question. If there were tests at all, there were obviously no tests for any failure condition occurring after this errant line of code. Likely, whomever was responsible for quality control failed to run a code coverage report, otherwise they would have seen immediately that a large section of their logic tree was dead code.
As much as I advocate unit testing, I also recognize that not all of our code will be properly tested before it ships. This is a horrible thing, but it happens. In the absence of proper tests (which, obviously is our reality), proper coding standards would have prevented this error as well. Adding brackets around one-line conditionals in this function would be trivial and, as it’s a compiled language the generated bytecode would end up identical regardless.
There is absolutely no excuse for poor coding style. There is also no excuse for such a mission-critical security feature to be shipped without proper unit tests and full code coverage.
Are you shipping code with potential bugs for which there is no excuse?
