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:
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).
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.
I've done that type of copy/paste error - inadvertently duplicating a line - more than once when using an unfamiliar and "helpful" IDE. I hate it when an IDE screen flashes and I'm left wondering what the HELL did that just do???
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.
It would be interesting to know how the whitespace (tabs vs spaces) before the extra goto compares to that one the other goto lines.
(in case anyone is curious, the "goto fail" lines in that part of the code are identical: all spaces; and the file in general does have an unfortunate mishmash of spaces and tabs)
Comments
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:
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!
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).
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.
Looking at the diff between the two versions released by apple http://opensource.apple.com/source/Security/Security-55471/l... and http://opensource.apple.com/source/Security/Security-55179.1...
the only thing that changed in the relevant part was that "goto fail;" was added.
this makes a copy & paste error highly unlikely.I've done that type of copy/paste error - inadvertently duplicating a line - more than once when using an unfamiliar and "helpful" IDE. I hate it when an IDE screen flashes and I'm left wondering what the HELL did that just do???
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.
(in case anyone is curious, the "goto fail" lines in that part of the code are identical: all spaces; and the file in general does have an unfortunate mishmash of spaces and tabs)