[Cryptography] GOTO Considered Harmful

[Jerry Leichter]

On Feb 28, 2014, at 1:07 PM, Patrick Chkoreff <patrick at rayservers.net> wrote:
> I refactored Apple's code:
Apple's or OpenSSL's?  Apple has modified this stuff, but most of it is from OpenSSL.  (I don't know about this code specifically; nor do I know about any analogous code else in OpenSSL.)

> http://fexl.com/goto-considered-harmful
> 
> You're welcome.
One of many, many ways to do this.

The original code was more efficient as it didn't repeatedly test 'failed'.  Whether that makes any noticeable difference is something one can debate.  (Really, one should measure it.  I'll bet if you actually try, not with this particular code - which deals with a relatively small, fixed-length header, but with a loop over a long run of memory - that you would be able to measure the effect, but that it would be a fraction of a percent.  On the other hand, as AES calculations get faster - e.g., using the new AES acceleration instructions on recent chips - the effect of extra tests becomes more noticeable.  But this is just numbers; whether they matter in a real application is an entirely different question.)

In any case, this is exactly why the goto-style is used by kernel hackers and others who feel they must be concerned with every instruction.

This was a very unusual error, and the fact there was a 'goto' in the code was incidental.  A simple typo breaks yours, too.  Just accidentally change any '||=' to '='.  Or change the "if (!failed)" to "if (failed)".  Or change "err =" to "err ==".

Somehow the fact there happened to be goto's in this code has become a lightning rod for ressurrecting a 45-year-old (justified) rant about programming languages and programming styles that vanished decades ago.  (If you don't remember what the extended range of a DO loop was in FORTRAN; or if you think the proof that finding the shortest program representation on a VAX with different-sized branch instructions is NP-complete relies on code no one could ever possibly write; then you don't understand why Dijkstra wrote what he wrote.)

In fact, if you go back to what bothered Dijkstra in that article (the CACM version is behind a paywall, but a version from Dijkstra's files is at https://www.cs.utexas.edu/users/EWD/ewd02xx/EWD215.PDF) you'll see that the particular style of "forward goto" used here would likely not have been a concern.  On the other hand, virtually everything about a functional programming style would be covered by that Dijkstra was getting at.

As far as I'm concerned, the best recommendation here, which avoids changing code that's worked correctly for years prior to this bug, is to add a sanity check:

  ...
  goto cleanup;

fail:
  assert(err != 0);  // Whatever you want here to die *in production code*!

cleanup:
  SSLFreeBuffer(...)
  ...
  return err;

(You can also flow through into the cleanup code, then jump back to it from the fail code.  Choose your poison.  Both code organizations were common back in assembler days.)

This turns the meaning of "fail" into something specific:  We go here when there's an error.  If we try to go here when there's no error, the code will die.  Then the code does what it says, and says what it does.  (In he original code, executing the "fail" statement doesn't mean there was a failure!)

                                                        -- Jerry

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4813 bytes
Desc: not available
URL: <http://www.metzdowd.com/pipermail/cryptography/attachments/20140228/dad93bd7/attachment.bin>