[Cryptography] GOTO Considered Harmful
leichter at lrw.com
Fri Feb 28 20:19:25 EST 2014
On Feb 28, 2014, at 5:22 PM, Patrick Chkoreff <patrick at rayservers.net> wrote:
>> This was a very unusual error, and the fact there was a 'goto' in the
>> code was incidental.
> That is true, and this may be more of a case study in the benefit of
> *proper indentation*.
Actually, the *original code was indented correctly*! The *buggy* code was not.
I fault the code review process: The funny indentation should have caught the eye of a reviewer. Once you're drawn to the break in the proper indentation, you really can't miss that there are two goto's in a row. This bothers me; it suggests that Apple's code review process is just not up to snuff.
(It's been suggested that this smells like a git merge problem. Most of us have probably become pretty cavalier about merges - if the SCCS doesn't complain, and the code compiles, we figure it's fine. If this really was a merge error, it suggests that merges need to be carefully code reviewed, too. I don't know of anyone who does that.)
> Along with test coverage, of course -- which,
> *unbelievably* was absent. The most perfunctory test case ought to
> ensure that sslRawVerify is called!
It would be messy to fully test this code without constructing a rather unusual test harness. The update function for an AES computation takes a current state and a block. In raw form, there's no way for it to fail (short of accessing unmapped memory): Any bit pattern is legal for both. Many crypto API's add a bit of error checking by keeping the state in a structure that includes some kind of identification of the algorithm it's meant for. So you could get an error if you passed in an improperly initialized, or deliberately damaged, context block. But the context is allocated within the body of the function; it's not a parameter. So there's no way, from the outside, to cause any of the calls to return a non-zero value.
There probably *should* be at least a pair of tests, one of which verifies successfully, one of which doesn't. The failure (because it would succeed when it shouldn't) of the second wouldn't give any clear indication of where the problem was, but it should have lead someone to look closely. The lack of such tests is not good.
BTW, I got curious and had a look at the latest (1.0.0l) sources. The code base, excluding tests, has 6313 goto's in it; at a quick glance, most of them are used in the same pattern as the code in question. On the other hand, I wasn't able to locate the code corresponding to Apple's code - it seems to have been branched so long ago that even most of the names are different.
More information about the cryptography