Hacker News Reader

Comment by alex1 | Hacker News Reader

Here's a diff of that file from OS X 10.8.5 (Security-55179.13) to 10.9 (Security-55471): https://gist.github.com/alexyakoubian/9151610/revisions

Check line 631. Appears seemingly out of nowhere.

View on HN

Good find!

But, are you sure the 10.8.5 (-55179.13) version isn't in some sense a later, patched maintenance branch compared to a 10.9 (-55471) that might have been frozen earlier? The release dates are very close (~2013-10-03 for 10.8.5; ~2013-10-22 for 10.9), and they might have already been separate branches.

(Is there a 10.8.4 version to compare?)

Good point. I just checked that file ever since it was open-sourced in 10.8 and it stays exactly the same throughout all of 10.8.x (10.8 - 10.8.5) and only changes in 10.9. (You can check for yourself here: http://opensource.apple.com/)

It doesn't seem like what you said is the case here but obviously we're still missing changesets that may have been committed between 10.8.5 (Security-55179.13) and 10.9 (Security-55471). It'd be really interesting to do a git-blame on that file.

EDIT: Nevermind, that file wasn't open-sourced in 10.8. It's actually really old. (Look for directories starting with libsecurity_ssl in pre-10.8 OS X versions.) Didn't find anything particularly interesting in the old versions though.

Thanks for clarifying. I know silly errors like this can slip in, but I hope Apple does a deep x-ray on all circumstances surrounding the change.

This bug of an extra duplicate line looks like a merge issue to me.

So, what does that mean, the goto fail portion of the code. It seems like it will go to that no matter what. What is the end outcome?

Thanks in advanced

At that point the variable 'err' is still zero (noErr). So the function returns noErr, the caller thinks everything is good, and the communication is allowed.

Goto considered harmful, indeed!

Not much a problem of `goto` per se. Rather a problem with if conditions used without code blocks.

Others might say it's a problem of whitespace insensitive languages ;)

Haha love the whitespace comment. This also makes you think of the static source-code analysis done at Apple. Surely static tools would have picked this up, no...?

Clang currently does not warn about this, but I'd wager that Xcode will boast a feature in the next version that detects dead code due to early returns/gotos.

Clang may not warn about white space issues, but it certainly does warn about unreachable code if -Wunreachable is on. This code would not compile in my project because the unconditional goto leaves the following code unreachable.

Even if there were braces, the bug would still exist if the extra goto was outside the braces.

It might be more noticeable, but then, the original bug existed because no one noticed.

From a different point of view, part of the problem is the undefined state of the code at the "fail" label.

Execution will arrive there somehow, but the 'how' is unclear. The word "fail" implies you should reach that point only if there was an error, but that is a bad assumption in this case.

If the real answer to 'how did we get here?' was checked, then the bug could not hide in the undefined behavior. This would not allow a dangling goto to result in a false positive. A false negative will get someone's attention when their web page doesn't load.

Something like this could remove the undefined state:

      goto pass;
  fail:
    if ( err == 0 ) {
    	assert( err != 0 ); // BUG! variable must contain an error number
    	err = kSomeAppropriateErrorNumber;
    }
  pass:
    SSLFreeBuffer(&signedHashes);
    SSLFreeBuffer(&hashCtx);
    return err;

    return ERRCODE;

Would have produced the exact same bug.

Reply on news.ycombinator.com