[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Miscompilation under strict C aliasing rules
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
> 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:
Author: jfoug <email@example.com>
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. :-(