[PATCH] Avoid undefined behavior for hashes using XOF

[Peter Wu]

On Thu, Mar 24, 2016 at 11:25:04AM +0100, Werner Koch wrote:
> On Thu, 24 Mar 2016 00:29, peter at lekensteyn.nl said:
> 
> > While the functions could simply shortcircuit and return early, let's
> > perform the hash calculations anyway such that the benchmarks can be
> > run. Copying zero bytes is valid according to the documentation of
> > gcry_md_hash_buffer{,s} as gcry_md_get_algo_dlen() returns 0.
> 
> Your code is now:
> 
>   if (md_digest_length (algo))
>     memcpy (digest, md_read (h, algo), md_digest_length (algo));
> 
> By adding the condition you avoid calling md_read which would return
> NULL in the case of SHAKE128.  So the UB seems to be that memcpy (foo,
> NULL, 0) is not defined - impractical but obviously another gcc/clang
> annoyance.
> 
> I would suggest not to test for md_digest_length but to
> 
>   const void *tmp = md_read (h, algo);
>   if (tmp)
>     memcpy (digest, tmp, md_digest_length (algo));
> 
> which uses the real cause for the condition.

The UB warning comes from the md_read() function returning NULL, but
that only happens because SHAKE128 has no read function for outputting a
fixed hash length.

I think that md_digest_length() is still more suitable here, the message
is not "avoid memcpy on a NULL source", but "avoid a memcpy if there is
nothing to copy". The former might hide hypothetical errors where the
length is non-zero, but md_read returns NULL for some reason.

> _gcry_md_hash_buffers should however return an error and not silently
> ignore it.  Even if that means to adjust the tests ;-)

Failing with an error indeed seems a better hint for the user, I will
fix that for the next patch. The benchmark is not quite accurate anyway
for XOFs, what about skipping tests for such functions?
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl