sha2: fix aliasing violation #24
Conversation
|
I've used the same patch from the linked bugs but it's also the obvious patch for the problem. |
|
See also Canonical Bug: #2033405 |
There was a problem hiding this comment.
Hi @thesamesam,
I'm with you that this violates strict aliasing and should be fixed.
From a quick look at the unpatched code my impression is that:
context->bitcountisSHA256_CTX.bitcountin functionSHA256_Finalwhich is of typeuint64_t, 8 bytes.context->bitcountisSHA512_CTX.bitcountin functionSHA512_Lastwhich is of typeuint64_t[2], 2 times 8 bytes.sha2_word64is eitheruint64_toru_int64_t, both 8 bytes.
From that I would superficially include:
- Changing this to
memcpyshould be safe and copy the exact same amounts of bytes as before and intended, assuming a "friendly compiler". - That the compiler is "likely" to do the right thing even unpatched, even with
-O3.
What I do not yet understand is:
- If this is a theoretical or a practical problem at runtime also?
- How to invoke GCC to make it detect this issue — I tried and it kept silent.
What do you think?
Best, Sebastian
Please see https://gcc.gnu.org/PR114698 where this actually did break I did mention that in the commit message but maybe it wasn't explicit.
Unfortunately, GCC's strict aliasing warnings are known to be deficient and easy to fool. See https://gcc.gnu.org/PR113151. You can sometimes get GCC to warn on things by using |
|
Hi, @hartwork!
I also tried to reproduce the issue presented on https://gcc.gnu.org/PR114698 by using a Debian Sid ppc64el image emulated under QEMU. I had no wrong results after building dcfldd using GCC 13 with Regards, |
Well, I must had done something wrong last time I tested, because I had tests failing now after building with |
`&context->buffer` is `uint8_t*`, but we try to access it as `sha2_word64*`, which is an aliasing violation (undefined behaviour). Use memcpy instead to avoid being miscompiled by e.g. >= GCC 12. This is just as fast with any modern compiler. Bug: https://gcc.gnu.org/PR114698 Bug: NetBSD/pkgsrc#122 Bug: archiecobbs/libnbcompat#4 Bug: https://bugs.launchpad.net/ubuntu-power-systems/+bug/2033405 Signed-off-by: Sam James <sam@gentoo.org>
|
Thanks for your help! |
When reviewing resurrecting-open-source-projects#24, @davidpolverari pointed out a mistake I made in one revision - oops. While fixing that, I noticed: ``` sha2.c: In function ‘SHA384_End’: sha2.c:1052:45: error: argument to ‘sizeof’ in ‘memset’ call is the same expression as the destination; did you mean to dereference it? [-Werror=sizeof-pointer-memaccess] 1052 | MEMSET_BZERO(context, sizeof(context)); | ^ sha2.c:176:49: note: in definition of macro ‘MEMSET_BZERO’ 176 | #define MEMSET_BZERO(p,l) memset((p), 0, (l)) | ^ cc1: some warnings being treated as errors ``` Let's fix that too, while we're at it. Signed-off-by: Sam James <sam@gentoo.org>
|
Thank you! |
When reviewing #24, @davidpolverari pointed out a mistake I made in one revision - oops. While fixing that, I noticed: ``` sha2.c: In function ‘SHA384_End’: sha2.c:1052:45: error: argument to ‘sizeof’ in ‘memset’ call is the same expression as the destination; did you mean to dereference it? [-Werror=sizeof-pointer-memaccess] 1052 | MEMSET_BZERO(context, sizeof(context)); | ^ sha2.c:176:49: note: in definition of macro ‘MEMSET_BZERO’ 176 | #define MEMSET_BZERO(p,l) memset((p), 0, (l)) | ^ cc1: some warnings being treated as errors ``` Let's fix that too, while we're at it. Signed-off-by: Sam James <sam@gentoo.org>
|
@davidpolverari is there a chance to release 1.9.2 with this critical patch? Without a new release, all downstreams will have to bundle the patch downstream and become aware some way also. A new release would help awareness while reducing multiplication of work. What do you think? |
sha2: fix aliasing violation
&context->bufferisuint8_t*, but we try to access it assha2_word64*, whichis an aliasing violation (undefined behaviour).
Use memcpy instead to avoid being miscompiled by e.g. >= GCC 12. This is
just as fast with any modern compiler.
Bug: https://gcc.gnu.org/PR114698
Bug: NetBSD/pkgsrc#122
Bug: archiecobbs/libnbcompat#4
Bug: https://bugs.launchpad.net/ubuntu-power-systems/+bug/2033405
Signed-off-by: Sam James sam@gentoo.org