
Reflections on Curly Braces – Apple’s SSL Bug and What We Should Learn From It - omnibrain
https://blog.codecentric.de/en/2014/02/curly-braces/
======
saurik
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
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.

~~~
saurik
The issue here isn't that the code is confusing or somehow not "obvious" in
function: the issue is that there are numerous ways we can mistype it,
especially as we (apparently) are prone to copy/pasting it because it involves
typing an additional 30 keystrokes (in this case, assuming "err" and "0"). The
goal isn't to "hide complexity", the goal is to "remove things the developer
can do wrong".

Trust me: I glance at that code, and it is 100% clear to me in that moment
what it is doing and why it is doing it--I _have_ been programming primarily
in C/C++ now for almost 20 years--but I simply do not believe you that an
experienced C/C++ programmer glances at this code and knows 100% for certain
that it is _correct_. If nothing else, clearly the person who wrote this code
screwed it up ;P.

This pattern just lends itself to silly mistakes: you might check the error
using the wrong constant (!= 0 when you need == -1; this could even work
temporarily!) or think that a function can't return an error when it actually
can (the setuid mistake that burned Android, see Rage Against the Cage); by
using structured error handling you aren't just "hiding" complexity: you are
_removing it_.

Now, I happily admit (and already did in my original comment) that the macro
doesn't solve all of these problems as well as "use a language that provides
better abstractions (even C++!)"; however, it does mean that you can't
accidentally miss the set of parentheses (assigning the comparison result),
double up the equal sign (comparing instead of assigning), add an extra
semicolon (breaking the comparison, so as to always "goto fail;"), copy/paste
the boilerplate incorrectly (which you might have even done in a misguided
attempt to avoid the previous errors ;P) leaving you with a second copy of
"goto fail;" (as the developer here probably did; it might also have been a
merge failure: thankfully, that also becomes impossible, as the error check
now is naturally part of the same line as the code being checked), or
otherwise make any silly "it still compiles, it looks almost identical, but
now it doesn't work" errors. That's _valuable_ :/.

~~~
andyjohnson0
_" but I simply do not believe you that an experienced C/C++ programmer
glances at this code and knows 100% for certain that it is correct."_

I agree, but then I didn't actually say that I would know it was _correct_ (to
any percentage) by glancing at it. What I tried to say was that the
"boilerplate" does not detract from the code exposing its meaning or
intention.

Again, I agree that the the wrong error constant could be used. But thats also
true of a macro is used - especially if the constant is embedded in the macro
which is elsewhere.

I didn't want to get into your point about whether this would be better
written in another language, because we are where we are. libssl has bee
around for a while and has accumulated plenty of dependencies. I don't think a
re-write is going to happen any time soon.

~~~
saurik
You seem to have ignored the long list of "possible typos similar to this one"
that were my arguments for why the macro was better than having the developer
copy/paste the code every time. The only argument you seem to be making is:
"abstraction is bad"; I mean, this same argument (that abstraction hides
functionality and values behind opaque names) is also the reason why
developers should not write functions...

------
userbinator
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
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.

------
ZoFreX
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
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.

------
bcl
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.

~~~
staticshock
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
The idea that was hammered into my head in school is that you should have a
_single point of truth._

------
hrjet
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
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.

------
agentultra
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
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.

------
DangerousPie
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
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
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.

------
paul_f
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
> 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
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
> This. In addition, a linter would have caught his example with braces
> because of the improper indent

That and the dead code after it.

------
mathenk2
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.

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

------
stevehawk
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/.

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

~~~
ufo
goto in C is perfectly fine in this sort of situation. Its the only way to do
sane error handling.

~~~
Pxtl
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.

