6 ms·

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

28 points by omnibrain 13y ago

userbinator 13y ago
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:
- ctdonath 13y ago
  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.
- sanderjd 13y ago
  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.
saurik 13y ago
This article points out other simplications that can be made to help with this error, but frankly the core problem is using insanely verbose boilerplate to handle errors. At bare minimum, the "if ((error = ...) != success) goto fail;" pattern should have been hidden behind a macro such as "attempt(...)"; just refactoring the code to use if statements differently is not sufficient. Further, in a more reasonable language (C++), the "goto fail;" paradigm (that is being used to free manually managed memory) could be replaced by a deconstructor (alternatively, in languages like D, a scope guard), which simplifies the logic. One could also use exceptions (or, if using a language like Haskell, a Maybe monad) to structure the error handling and remove the boilerplate entirely (which also has the incredibly valuable side effect of making the error handling impossible to avoid: forgetting to check a return value, or checking the return value using the wrong error constants, is a serious issue that leads to security bugs; Rage Against the Cage is a simple example from Android). There is really no excuse to be writing code like this in C when it is just asking for these kinds of mistakes by not providing even the most bare of abstractions to keep the programmer from ending up with code that is 50% boilerplate :(. But again, lest people get caught up in "I hate C++" land, even C provided something to help the programmer not type this code a million times (#define macros, which still leaves aome boilerplate in the code, but drops the percentage dramatically, removing any impetus to end up with a pattern here that requires multiple lines of code to express), which despite being an incomplete solution to the core problems of this pattern of error checking, would make this code infinitely easier to edit without introducing errors in the boilerplate.
- andyjohnson0 13y ago
  I'm going to disagree with you on the "if ((error = ...) != success) goto fail;" pattern. I've written a lot of C/C++ during my career, but mostly C# in the last ten years, and that pattern looks totally obvious to me. This isn't because I'm super-smart (I'm not) but because I've internalised the language rules and common patterns and I don't have to think hard about what such a code line is doing. It just seems like a normal c/c++ construct. I understand that it might be more opaque to a ruby/scala/whatever dev, but they probably wouldn't be poking around in libssl unless they knew C/C++. To me, wrapping such a simple test in a macro just obscures what is going on by introducing unnecessary abstraction. Making the decision about whether to use a macro in this case seems to me rather like the decision to use operator overloading in C++. There are occasions when you might want to do it to hide complexity, but you'd better have a good reason.
DangerousPie 13y ago
The question the author seems to ignore is how the second "goto fail" appeared in the first place. In all likelihood it was caused by an erroneous copy + paste where the programmer accidentally included the `goto fail;` from above the `if`, like so: goto fail; if ((err = SSLHashMD5.update(&hashCtx, &clientRandom)) != 0) goto fail; Now, what would have happened if there had been braces? To get the two `goto fail;`s he would have had to duplicate something like this, which would have immediately caused a syntax error! goto fail; } if ((err = SSLHashMD5.update(&hashCtx, &clientRandom)) != 0) { goto fail; }
- saurik 13y ago
  This is a very poor form of checksum on copy/paste mistakes. As I describe in my top-level comment, a more complete solution to this problem is to avoid the developer ever having to copy/paste multiple lines of silly error-handling boilerplate in the first place: hell, the programmer shouldn't be feeling the need to copy/paste at all, as that's just asking for trouble :(. Instead, a #define macro can be used that would replace the "if ((error = ...) != success) goto fail;" pattern with "attempt(...);" (or, for an even more complete solution that solves other issues that are possible in this kind of code, such as resource management mistakes caused by using goto for unwind semantics or simply forgetting to check an error entirely, C++ deconstructors and exceptions can entirely remove the error handling boilerplate from this function, letting the programmer concentrate on behavior and not error conditions).
  - DangerousPie 13y ago
    I totally agree with you. I just don't think it's fair to say that braces wouldn't have helped in this situation, because they would have. That doesn't mean that a pattern like this was a good solution in the first place of course.
- andyjohnson0 13y ago
  It would be interesting to know how the whitespace (tabs vs spaces) before the extra goto compares to that on the other goto lines. This might provide a clue as to whether copy/paste was involved. On the subject of how the extra line appeared in the first place, my take on possible causes are, in order of likelihood: 1. Line added during developer testing to force a fail and not removed. 2. Copy/paste error during construction. 3. Artefact from a bad code merge. 4. Deliberately introduced by NSA mole. Presumably, one someone goes through the version control history, the who and when will be known - though maybe not publicly. I'm not sure we'll ever know why, though.
hrjet 13y ago
The original website seems to have gone offline. But based on the title, I think the lesson should be to have more stringent reviews and tests. Things which can't be checked statically at compile time have to be thoroughly reviewed and tested and a coverage tool used to checked for completeness (in the very least). I have seen teams following thorough review and testing processes in far less critical software!
- nickzoic 13y ago
  Agreed, this is a case where a small number of test cases would have been sufficient to cover the critical parts of the code and avoid this particular error and many like it.
paul_f 13y ago
I tend to agree with the OP, the problem is not the braces per se. IMO, better would be a coding convention to skip curly braces but if and only if the entire instruction is on one line. Then the bug would have stood out more prominently.
- SideburnsOfDoom 13y ago
  > IMO, better would be a coding convention to skip curly braces but if and only if... IMHO, even better and simpler would be a coding convention to not skip curly braces. The end. (as per last time this was raised: https://news.ycombinator.com/item?id=7283767 https://news.ycombinator.com/item?id=7283767 ) Otherwise, the OP is right on the money; there are multiple levels that would have caught this bug - a layout convention, a linter, a code review, pairing, a unit test, an integration test. Or just a refactor of the big old method for clarity.
  - vkjv 13y ago
    This. In addition, a linter would have caught his example with braces because of the improper indent. And, IMHO, without that indent it becomes glaringly obvious.
    - SideburnsOfDoom 13y ago
      > This. In addition, a linter would have caught his example with braces because of the improper indent That and the dead code after it.
ZoFreX 13y ago
I would also add: * For critical code, enable as many compiler warnings as you can while remaining sane. This would definitely include unreachable code warnings. * Consider using static analysis * Initialise your error variable in the "error" state not in the "everything is ok" state
- drone 13y ago
  Here's one more to add: * Stop copying and pasting the same code over and over. If you do a complex series of operations several times, encapsulate them in a function/method.
- EvanAnderson 13y ago
  I came here to make your third point. I feel strongly that code should be written to "fail safe"-- error state should be assumed until calls return otherwise, etc.
bcl 13y ago
If this had been Python it wouldn't have been an issue ;) I find it hard to believe that this code had a proper peer review before being committed. A 2nd and 3rd set of eyes experienced at reading C should have spotted this. Peer review is especially important when dealing with core security components like this where simple mistakes can have huge consequences.
- naterator 13y ago
  Not sure if it is/was, but having at least three people review code before merge should be standard practice for security critical stuff like this. Something I plan to implement in the future.
- staticshock 13y ago
  Of course, different languages come with different trade-offs, but I wholeheartedly agree that Python picked the good side of this particular trade-off. If proper indentation is so critical to a human parsing of the code that every style guide in the world demands some type of indentation (note the lack of style guides demanding a complete lack of indentation,) then why not use that very same convention to convey that very same information to the compiler? The C-family of languages is on the unfortunate side of this trade-off: there's one method to convey statement blocks to the compiler (brackets) and another to convey it to the reader (the indentation convention.) TL;DR: Multiple channels with redundant information run the risk of disagreeing with each other. I don't know if anyone's come up with a catchy phrase for this, or coined some sort of "law", but it's very real. As soon as you have redundancy of information, you automatically inherit the problem of synchronizing the information between channels.
  - eyeface 13y ago
    The idea that was hammered into my head in school is that you should have a single point of truth.
- userbinator 13y ago
  If this has been Python, a missing/extra indent might've caused a similar issue, or even a duplicate function call. What evidence is there to say that if a C programmer can sometimes miss duplicate lines of code, a Python programmer won't miss duplicate lines of code or a missing/extra indent? I find it hard to believe that this code had a proper peer review before being committed. A 2nd and 3rd set of eyes experienced at reading C should have spotted this. They could've spotted this, and maybe due to some other factors (e.g. some people just don't want to point out other's mistakes, like hierarchical cultures) let it pass; or the other reviewers were at the same competency as the one who wrote the code.
SeanLuke 13y ago
All that text, so many points, and somehow the author managed to avoid the 800-pound gorilla in the room: the use of goto.
- seabee 13y ago
  Ah yes, the use of goto conjures up the 800-year-old essay in the room. The way they use it is widely understood and accepted in C error handling. It's also not the only way you can screw up, either, which is a good reason why you should add a static analyser to your build process.
- ufo 13y ago
  goto in C is perfectly fine in this sort of situation. Its the only way to do sane error handling.
  - Pxtl 13y ago
    That is because C is not a good language. Don't get be wrong, it's the best language for the space that it occupies. There is no better language to implement such things. But it still isn't good. It's just that it's failed to be replaced by anything better.
bakul 13y ago
You can't fix carelessness by devising "better" coding standards.
agentultra 13y ago
Some nice suggestions in the review and most importantly the author leaves the programmer out of it and focuses on the code... except for one bit where the author cites the opinion of the masses (implying ei shares the same views). It's important that when reviewing code you only talk about the code and not the people or person who wrote it. It will happen to you one day and someone else will come along and make all sorts of remarks of your character and ability. And it will be completely unjust and unwarranted. So don't do it. The conclusion I found wanting. "Should," is a word like, "obvious," that is best avoided. It is a sign of weak, wishful thinking and stinks of condescension. Could, would, or should have... it happened and was dealt with, it seems, in an appropriate manner. Armed with good suggestions on how to avoid it in the future perhaps we won't see the same mistake made again (hah).
ctdonath 13y ago
Did anyone notice that the "fail:" section is misnamed? It's a completion section, run regardless of failure or success. Marked as such, the reader assumes "if it succeeds it won't go here" before actually (if at all) reading it, processing all the logic, and concluding that it's run regardless.
stevehawk 13y ago
why are we blaming curly braces instead of unit tests? If the code was always going to return a success then that tells me it wasn't unit tested /at all/.
mathenk2 13y ago
This is why my repos reject code that doesn't explicitly use curly braces and semicolons. There is no legitimate reason for leaving them off.

28 points by omnibrain 13y ago

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:

ctdonath 13y ago
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.
sanderjd 13y ago
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.

saurik 13y ago

This article points out other simplications that can be made to help with this error, but frankly the core problem is using insanely verbose boilerplate to handle errors. At bare minimum, the "if ((error = ...) != success) goto fail;" pattern should have been hidden behind a macro such as "attempt(...)"; just refactoring the code to use if statements differently is not sufficient. Further, in a more reasonable language (C++), the "goto fail;" paradigm (that is being used to free manually managed memory) could be replaced by a deconstructor (alternatively, in languages like D, a scope guard), which simplifies the logic. One could also use exceptions (or, if using a language like Haskell, a Maybe monad) to structure the error handling and remove the boilerplate entirely (which also has the incredibly valuable side effect of making the error handling impossible to avoid: forgetting to check a return value, or checking the return value using the wrong error constants, is a serious issue that leads to security bugs; Rage Against the Cage is a simple example from Android). There is really no excuse to be writing code like this in C when it is just asking for these kinds of mistakes by not providing even the most bare of abstractions to keep the programmer from ending up with code that is 50% boilerplate :(. But again, lest people get caught up in "I hate C++" land, even C provided something to help the programmer not type this code a million times (#define macros, which still leaves aome boilerplate in the code, but drops the percentage dramatically, removing any impetus to end up with a pattern here that requires multiple lines of code to express), which despite being an incomplete solution to the core problems of this pattern of error checking, would make this code infinitely easier to edit without introducing errors in the boilerplate.

andyjohnson0 13y ago
I'm going to disagree with you on the "if ((error = ...) != success) goto fail;" pattern. I've written a lot of C/C++ during my career, but mostly C# in the last ten years, and that pattern looks totally obvious to me. This isn't because I'm super-smart (I'm not) but because I've internalised the language rules and common patterns and I don't have to think hard about what such a code line is doing. It just seems like a normal c/c++ construct. I understand that it might be more opaque to a ruby/scala/whatever dev, but they probably wouldn't be poking around in libssl unless they knew C/C++. To me, wrapping such a simple test in a macro just obscures what is going on by introducing unnecessary abstraction. Making the decision about whether to use a macro in this case seems to me rather like the decision to use operator overloading in C++. There are occasions when you might want to do it to hide complexity, but you'd better have a good reason.

DangerousPie 13y ago

The question the author seems to ignore is how the second "goto fail" appeared in the first place. In all likelihood it was caused by an erroneous copy + paste where the programmer accidentally included the `goto fail;` from above the `if`, like so: goto fail; if ((err = SSLHashMD5.update(&hashCtx, &clientRandom)) != 0) goto fail; Now, what would have happened if there had been braces? To get the two `goto fail;`s he would have had to duplicate something like this, which would have immediately caused a syntax error! goto fail; } if ((err = SSLHashMD5.update(&hashCtx, &clientRandom)) != 0) { goto fail; }

saurik 13y ago
This is a very poor form of checksum on copy/paste mistakes. As I describe in my top-level comment, a more complete solution to this problem is to avoid the developer ever having to copy/paste multiple lines of silly error-handling boilerplate in the first place: hell, the programmer shouldn't be feeling the need to copy/paste at all, as that's just asking for trouble :(. Instead, a #define macro can be used that would replace the "if ((error = ...) != success) goto fail;" pattern with "attempt(...);" (or, for an even more complete solution that solves other issues that are possible in this kind of code, such as resource management mistakes caused by using goto for unwind semantics or simply forgetting to check an error entirely, C++ deconstructors and exceptions can entirely remove the error handling boilerplate from this function, letting the programmer concentrate on behavior and not error conditions).
- DangerousPie 13y ago
  I totally agree with you. I just don't think it's fair to say that braces wouldn't have helped in this situation, because they would have. That doesn't mean that a pattern like this was a good solution in the first place of course.
andyjohnson0 13y ago
It would be interesting to know how the whitespace (tabs vs spaces) before the extra goto compares to that on the other goto lines. This might provide a clue as to whether copy/paste was involved. On the subject of how the extra line appeared in the first place, my take on possible causes are, in order of likelihood: 1. Line added during developer testing to force a fail and not removed. 2. Copy/paste error during construction. 3. Artefact from a bad code merge. 4. Deliberately introduced by NSA mole. Presumably, one someone goes through the version control history, the who and when will be known - though maybe not publicly. I'm not sure we'll ever know why, though.

hrjet 13y ago

The original website seems to have gone offline. But based on the title, I think the lesson should be to have more stringent reviews and tests. Things which can't be checked statically at compile time have to be thoroughly reviewed and tested and a coverage tool used to checked for completeness (in the very least). I have seen teams following thorough review and testing processes in far less critical software!

nickzoic 13y ago
Agreed, this is a case where a small number of test cases would have been sufficient to cover the critical parts of the code and avoid this particular error and many like it.

paul_f 13y ago

I tend to agree with the OP, the problem is not the braces per se. IMO, better would be a coding convention to skip curly braces but if and only if the entire instruction is on one line. Then the bug would have stood out more prominently.

SideburnsOfDoom 13y ago
> IMO, better would be a coding convention to skip curly braces but if and only if... IMHO, even better and simpler would be a coding convention to not skip curly braces. The end. (as per last time this was raised: https://news.ycombinator.com/item?id=7283767 https://news.ycombinator.com/item?id=7283767 ) Otherwise, the OP is right on the money; there are multiple levels that would have caught this bug - a layout convention, a linter, a code review, pairing, a unit test, an integration test. Or just a refactor of the big old method for clarity.
- vkjv 13y ago
  This. In addition, a linter would have caught his example with braces because of the improper indent. And, IMHO, without that indent it becomes glaringly obvious.
  - SideburnsOfDoom 13y ago
    > This. In addition, a linter would have caught his example with braces because of the improper indent That and the dead code after it.

ZoFreX 13y ago

I would also add: * For critical code, enable as many compiler warnings as you can while remaining sane. This would definitely include unreachable code warnings. * Consider using static analysis * Initialise your error variable in the "error" state not in the "everything is ok" state

drone 13y ago
Here's one more to add: * Stop copying and pasting the same code over and over. If you do a complex series of operations several times, encapsulate them in a function/method.
EvanAnderson 13y ago
I came here to make your third point. I feel strongly that code should be written to "fail safe"-- error state should be assumed until calls return otherwise, etc.

bcl 13y ago

If this had been Python it wouldn't have been an issue ;) I find it hard to believe that this code had a proper peer review before being committed. A 2nd and 3rd set of eyes experienced at reading C should have spotted this. Peer review is especially important when dealing with core security components like this where simple mistakes can have huge consequences.

naterator 13y ago
Not sure if it is/was, but having at least three people review code before merge should be standard practice for security critical stuff like this. Something I plan to implement in the future.
staticshock 13y ago
Of course, different languages come with different trade-offs, but I wholeheartedly agree that Python picked the good side of this particular trade-off. If proper indentation is so critical to a human parsing of the code that every style guide in the world demands some type of indentation (note the lack of style guides demanding a complete lack of indentation,) then why not use that very same convention to convey that very same information to the compiler? The C-family of languages is on the unfortunate side of this trade-off: there's one method to convey statement blocks to the compiler (brackets) and another to convey it to the reader (the indentation convention.) TL;DR: Multiple channels with redundant information run the risk of disagreeing with each other. I don't know if anyone's come up with a catchy phrase for this, or coined some sort of "law", but it's very real. As soon as you have redundancy of information, you automatically inherit the problem of synchronizing the information between channels.
- eyeface 13y ago
  The idea that was hammered into my head in school is that you should have a single point of truth.
userbinator 13y ago
If this has been Python, a missing/extra indent might've caused a similar issue, or even a duplicate function call. What evidence is there to say that if a C programmer can sometimes miss duplicate lines of code, a Python programmer won't miss duplicate lines of code or a missing/extra indent? I find it hard to believe that this code had a proper peer review before being committed. A 2nd and 3rd set of eyes experienced at reading C should have spotted this. They could've spotted this, and maybe due to some other factors (e.g. some people just don't want to point out other's mistakes, like hierarchical cultures) let it pass; or the other reviewers were at the same competency as the one who wrote the code.

SeanLuke 13y ago

All that text, so many points, and somehow the author managed to avoid the 800-pound gorilla in the room: the use of goto.

seabee 13y ago
Ah yes, the use of goto conjures up the 800-year-old essay in the room. The way they use it is widely understood and accepted in C error handling. It's also not the only way you can screw up, either, which is a good reason why you should add a static analyser to your build process.
ufo 13y ago
goto in C is perfectly fine in this sort of situation. Its the only way to do sane error handling.
- Pxtl 13y ago
  That is because C is not a good language. Don't get be wrong, it's the best language for the space that it occupies. There is no better language to implement such things. But it still isn't good. It's just that it's failed to be replaced by anything better.

bakul 13y ago

You can't fix carelessness by devising "better" coding standards.

agentultra 13y ago

Some nice suggestions in the review and most importantly the author leaves the programmer out of it and focuses on the code... except for one bit where the author cites the opinion of the masses (implying ei shares the same views). It's important that when reviewing code you only talk about the code and not the people or person who wrote it. It will happen to you one day and someone else will come along and make all sorts of remarks of your character and ability. And it will be completely unjust and unwarranted. So don't do it. The conclusion I found wanting. "Should," is a word like, "obvious," that is best avoided. It is a sign of weak, wishful thinking and stinks of condescension. Could, would, or should have... it happened and was dealt with, it seems, in an appropriate manner. Armed with good suggestions on how to avoid it in the future perhaps we won't see the same mistake made again (hah).

ctdonath 13y ago

Did anyone notice that the "fail:" section is misnamed? It's a completion section, run regardless of failure or success. Marked as such, the reader assumes "if it succeeds it won't go here" before actually (if at all) reading it, processing all the logic, and concluding that it's run regardless.

stevehawk 13y ago

why are we blaming curly braces instead of unit tests? If the code was always going to return a success then that tells me it wasn't unit tested /at all/.

mathenk2 13y ago

This is why my repos reject code that doesn't explicitly use curly braces and semicolons. There is no legitimate reason for leaving them off.