Re: Miscompilation under strict C aliasing rules

[Solar Designer <solar@openwall.com>]

On Sun, Feb 27, 2022 at 01:00:16PM -0800, Colin Percival wrote:
> On 2/24/22 00:42, Quentin Carbonneaux wrote:
> >The compiler is technically allowed to forward the store
> >because blkcpy (and also blkxor) breaks the strict aliasing
> >rules of C. (See 6.5.7. in the C99 standard, for example.)
> >
> >On my end, I will replace blkcpy() with memcpy() and have
> >blkxor() act on bytes. I hope this can be fixed upstream
> >as well.
> 
> We should probably make it operate on uint32_t

Yes.

> rather than bytes,
> since the data in question is used as arrays of uint32_t elsewhere?

If these two blk*() functions were operating on bytes - or rather
"unsigned char" - we would be fine.  That type is allowed to alias any
other type.  However, they operate on size_t, and on systems where
size_t is of different size than uint32_t, we violate the rules.

It is possible to use a union type, if we care to optimize this for
speed on 64-bit, but then it's best to merge the functionality into
blockmix and thus avoid the copying for even greater speedup.

In John the Ripper, we have:

commit 50262affd1a7b4e2fc0a58a3fee6e74fefac5304
Author: jfoug <jfoug@cox.net>
Date:   Tue Jan 27 11:50:15 2015 -0600

    scrypt: corrected pointer alias issues causing scrypt to fail on MIPS64.  #1032

That was a temporary fix.  It used a union type.  By that time, the
issue had already been fixed in yescrypt as submitted to PHC in 2014
(but I was not yet aware of it actually being triggerable in 2014).

In current yescrypt, the blk*() are gone in the optimized implementation
(their functionality is merged into blockmix), and in the reference
implementation they use uint32_t, so there are no casts nor a union.

I was under impression this was fixed in scrypt long ago - but now it
looks like I share responsibility for not reporting it back then. :-(

Alexander