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 <[email protected]> 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
