<div dir="ltr">On 1 March 2014 01:19, Jerry Leichter <span dir="ltr"><<a href="mailto:leichter@lrw.com" target="_blank">leichter@lrw.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Two things:<br>
<div class=""><br>
On Feb 28, 2014, at 5:22 PM, Patrick Chkoreff <<a href="mailto:patrick@rayservers.net">patrick@rayservers.net</a>> wrote:<br>
>> This was a very unusual error, and the fact there was a 'goto' in the<br>
>> code was incidental.<br>
><br>
> That is true, and this may be more of a case study in the benefit of<br>
> *proper indentation*.<br>
</div>Actually, the *original code was indented correctly*!  The *buggy* code was not.<br>
<br>
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.<br>

<br>
(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.<br>
</blockquote><div><br></div><div>Good coding conventions and style guides can act as defense in depth against this:<br><ul><li>Requiring that all if bodies be either on the same line or braced avoids the indentation issue <br>
</li><li>If you're using Allman or similar indentation styles, the result of this is that the double goto triples in side:<br><div style="margin-left:40px">{<br>    goto fail;<br>}<br>{<br>    goto fail;<br>}<br></div>
</li><li>If you're using K&R style, my preference, a double goto resulting from a merge probably doesn't compile:<br><div style="margin-left:40px">if (err = DoSomeOperation(...)) {<br>    goto fail;<br>}<br>    goto fail;<br>
}<br></div></li><li>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?")</li>
<li>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.</li>
<li>Use of C++ RAII, Rust styled scoped deterministic lifetimes, and other languages with integral resource management functionality is ideal</li><ul><li>An adage of mine: The C developer is scared of the code they <i>can't</i> see. The C++ developer is scared of the code they <i>can</i>. 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.<br>
</li></ul></ul></div></div></div></div>