[PATCH] Avoid undefined behavior for hashes using XOF

Peter Wu peter at lekensteyn.nl
Thu Mar 24 13:01:54 CET 2016

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

More information about the Gcrypt-devel mailing list