[Cryptography] GOTO Considered Harmful

[Owen Shepherd]

On 1 March 2014 01:19, Jerry Leichter <leichter at lrw.com> wrote:

> Two things:
>
> 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.
>

Good coding conventions and style guides can act as defense in depth
against this:

   - Requiring that all if bodies be either on the same line or braced
   avoids the indentation issue
   - If you're using Allman or similar indentation styles, the result of
   this is that the double goto triples in side:
   {
       goto fail;
   }
   {
       goto fail;
   }
   - If you're using K&R style, my preference, a double goto resulting from
   a merge probably doesn't compile:
   if (err = DoSomeOperation(...)) {
       goto fail;
   }
       goto fail;
   }
   - The label name "fail" is inappropriate. "cleanup" would be better and
   highlight the purpose. In particular, it would make it more obvious to
   someone who spotted the double goto but was puzzled by it that it was wrong
   ("Two fails can't cause spurious success results" vs "Why is it jumping
   past this code to cleanup?")
   - Testing, testing, tetsing. "Full functional" testing (i.e. connecting
   the TLS stack to various working and broken servers) is ideal but tricky
   (Note Chrome uses a whole second, somewhat hacky TLS stack to verify
   various behaviors, but unit testing is entirely feasible.
   - Use of C++ RAII, Rust styled scoped deterministic lifetimes, and other
   languages with integral resource management functionality is ideal
      - An adage of mine: The C developer is scared of the code they
*can't*see. The C++ developer is scared of the code they
      *can*. In this kind of code, the stuff that C++ "hides" is the
      boilerplate code to do with resource management, removing it as "noise"
      from the surrounding actually important code. Plus, once I've seen the
      implementation of std::unique_ptr (or some smart pointer or your own
      creation) once I can be confident of its behavior anywhere (Modulo
      misunderstandings of the depressingly large specification. Yay Rust, for
      swapping the tradeoffs C++ made away from C compatibility and in the
      direction of safety), letting me "ignore" it and focus in on the critical
      security code.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.metzdowd.com/pipermail/cryptography/attachments/20140301/9b853bc9/attachment.html>