[Cryptography] GOTO Considered Harmful

[ianG]

On 28/02/2014 22:22 pm, Patrick Chkoreff wrote:
> Jerry Leichter wrote, On 02/28/2014 03:13 PM:

>> 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.  ...
> 
> It's a good point about efficiency.  One should measure it, and see if
> and where it matters -- and take the *utmost* precaution that the
> pursuit of efficiency does not degrade the pursuits of reliability and
> productivity.


It's a terrible point.  This is security code, it tests tricky things to
do with certs.  It's also intermingled with slow message digests, there
is really no excuse here for using "fast code" instead of careful code.


....
> If I were writing something like this from scratch, I'm sure I would
> quickly chafe at the need to check the error status at every turn.  At
> that point I would probably create a helper function which simply
> bugged-out with a "return" upon hitting any error.


Yes.  That is what return is for.  Always bug out when you can, as soon
as you can.  That makes the code following that point far simpler,
because it eliminates whole paths of choice.  That is what functions are
for.


> Then the parent function could look like this:
> 
> static OSStatus
> SSLVerifySignedServerKeyExchange(...)
>   {
>   /* Set up. */
>   OSStatus err;
>   SSLBuffer hashOut, ...
>   ...
>   signedHashes.data = 0;
>   hashCtx.data = 0;
>   ...
> 
>   /* Call the function which does all the crypto stuff. */
>   err = do_the_actual_crypto(...)
> 
>   /* Clean up. */
>   SSLFreeBuffer(&signedHashes);
>   SSLFreeBuffer(&hashCtx);
>   return err;
>   }
> 
> The child function could then do things like this:
> 
>   if ((err = ReadyHash(&SSLHashMD5, &hashCtx)) != 0)
>       return err;
>   if ((err = SSLHashMD5.update(&hashCtx, &clientRandom)) != 0)
>       return err;


That.  Do.  Of course.

> OK, it's an extra function I know, and instead of saying "goto fail"
> we're saying "return err".  Big whoop.  But do we really have to use
> "goto" just because we have a create-use-destroy pattern?  For *that* we
> gotta drag out a goto?  This isn't kernel spinner code here.


Yeah.

> I also don't often favor assignments in expressions, so the child
> function might look more like this:
> 
>   err = ReadyHash(&SSLHashMD5, &hashCtx);
>   if (err) return err;
> 
>   err = SSLHashMD5.update(&hashCtx, &clientRandom);
>   if (err) return err;
> 
> In fact, with all the strict compiler flags I like to use, if I changed
> a = to == the compiler would warn me like this:
> 
>    error: value computed is not used [-Werror=unused-value]
> 
> So I got that going for me.


Sure... If and when the code is changed to be structured optimally for
security, then we can have that discussion...



iang