To everyone claiming "it looks like a merge gone bad": That's a bit hard to believe. There's no other changes around the extra goto, so it's pretty weird for a duplicate line to appear like that out of nowhere. It's also incredibly visible in any diff viewer, so how such a checkin could have gone in without being noticed is just unfathomable.
It's hard to believe if you pay no attention to bugs caused by code refactorings, but easy to believe if you, for instance, saw the OpenBSD IPSEC team refactor authentication checking on IPSEC packets out of the kernel by accident, in almost the exact same manner as this bug.
No, because the OpenBSD case was NSA as well (via NETSEC, who worked on the OpenBSD IPSEC stack, and have since admitted to sneaking backdoors in). Exact same MO, 15 years later!
(I don't actually believe this, but it was too convenient and amusing not to call out.)
Pretty yak-shavy. Looks like some symbols were renamed and many of the APIs were (probably pointlessly) mutated. In total, unreviewable.
But as you say, this may not have been the internal diff seen by someone at Apple. They may have seen individual, focussed patches that did single things like changing from the 2-argument to the 1-argument SSLFreeBuffer, another patch for changing selectedCipherSpec to selectedCipherSpecParams (hello, yak shaving), renaming noErr to errSecSuccess, and so forth.
The question is, in any of those changes, the extra "goto fail;" line would have stuck out as irrelevant. In a gigantic 500-line delta the reviewer's eyes may have been glazed over by the time they got to that one.
I'm shocked and find that hard to believe; even without separate commits, it's not a huge single diff and it should really not be too much to ask for a proper review.
You'd think one would be a little less sloppy when hacking on the very core of the SSL libraries being deployed on hundreds of million of devices.
