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.
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.
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 :/.
"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.
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...
It's horrible code that seems to indicate a lack of production coding standards.
Errors, warnings and assertions need to be handled semantically sensibly, obviously and consistently.
Most functions, errors should be handled individually and returned as they happen and the only condition at the very bottom should be success.
My personal fav is anomaly(...) which checks for a nonfatal but suspicious condition. It always throws a warning on STDERR if it fails, even when compiled with -DNDEBUG.
One of my personal "favorites" is this gem from OpenSSH:
/* No MIN_SIZEOF here - we absolutely *must not* truncate the
* username (XXX - so check for trunc!) */
strlcpy(li->username, pw->pw_name, sizeof(li->username));
I don't think that is anything out of the ordinary, besides if pw->pw_name is longer that sizeof(li->username) it will get truncated. I was more thinking of error checking macros and error handling.
Basically the remark was that the comment went from discussing error handling, to the (much) broader scope of 'C and C++ done "right"', and as an example 5 complete, large open source projects were dumped for us to find the examples ourselves.
Just because the code doesn't misuse memory doesn't mean the code is correct. In this case, the return value of strlcpy should be checked to verify that the username is not truncated as it is added to this data structure. The comment even states that the truncation must be avoided, but rather than adding a check there is simply a XXX ;P.
If you want to get good at something, you're going to have to work at it. No one else can give you a shortcut to common sense that comes with mastery. Only pointers. That's just the way life works.
I can figure out how to look up these projects myself. The point is, if you are going to give examples on error handling be more specific. I don't need any general "life lessons" here, the point is it looked like you were trying to make a point, without having one. I.e you did not actually have any examples, you only made it appear so.
Comments
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.
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.
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 :/.
"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.
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...
Errors, warnings and assertions need to be handled semantically sensibly, obviously and consistently.
Most functions, errors should be handled individually and returned as they happen and the only condition at the very bottom should be success.
My personal fav is anomaly(...) which checks for a nonfatal but suspicious condition. It always throws a warning on STDERR if it fails, even when compiled with -DNDEBUG.
Example: https://gist.github.com/steakknife/9271284
If anyone wants to see decent examples of real C/C++ done right:
nginx [0], doom3 [1], postgres [2], varnish [3] and openssh [4]
References:
[0] http://hg.nginx.org/nginx/file/0251f2f1dc93
[1] https://github.com/TTimo/doom3.gpl
[2] https://github.com/postgres/postgres
[3] https://www.varnish-cache.org/trac/browser
[4] http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/
Do you have any specific examples? Just picking one randomly and the first error checking code I saw looked like this:
One of my personal "favorites" is this gem from OpenSSH:
Assuming loginrec.c, it's been refactored to this in 6.5p1:
Alternatively (assuming dst isn't a pointer):
Or perhaps more clear:Yay! \o/ :D
I don't think that is anything out of the ordinary, besides if pw->pw_name is longer that sizeof(li->username) it will get truncated. I was more thinking of error checking macros and error handling.
Basically the remark was that the comment went from discussing error handling, to the (much) broader scope of 'C and C++ done "right"', and as an example 5 complete, large open source projects were dumped for us to find the examples ourselves.
Just because the code doesn't misuse memory doesn't mean the code is correct. In this case, the return value of strlcpy should be checked to verify that the username is not truncated as it is added to this data structure. The comment even states that the truncation must be avoided, but rather than adding a check there is simply a XXX ;P.
I see, thanks! That's pretty funny. :D
If you want to get good at something, you're going to have to work at it. No one else can give you a shortcut to common sense that comes with mastery. Only pointers. That's just the way life works.
I can figure out how to look up these projects myself. The point is, if you are going to give examples on error handling be more specific. I don't need any general "life lessons" here, the point is it looked like you were trying to make a point, without having one. I.e you did not actually have any examples, you only made it appear so.