Discussion:
some gcc warnings
(too old to reply)
Bruno Haible
2016-10-16 15:06:11 UTC
Permalink
Raw Message
Hi Paul, Simon,

When I compile a gnulib testdir on a glibc system with "gcc -Wall", I see the
following warnings (among others):

rijndael-api-fst.c: In function 'rijndaelBlockEncrypt':
rijndael-api-fst.c:234:11: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
rijndael-api-fst.c: In function 'rijndaelPadEncrypt':
rijndael-api-fst.c:317:11: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
rijndael-api-fst.c: In function 'rijndaelBlockDecrypt':
rijndael-api-fst.c:390:11: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
rijndael-api-fst.c: In function 'rijndaelPadDecrypt':
rijndael-api-fst.c:484:11: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
rijndael-api-fst.c:484:11: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
rijndael-api-fst.c:495:7: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
rijndael-api-fst.c:495:7: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

test-quotearg.h:123:1: warning: 'use_quote_double_quotes' defined but not used [-Wunused-function]

Do you see a simple and reliable way to correct or silence these warnings?

Bruno
--
In memoriam Marie Antoinette <http://en.wikipedia.org/wiki/Marie_Antoinette>
Paul Eggert
2016-10-20 22:36:11 UTC
Permalink
Raw Message
Post by Bruno Haible
test-quotearg.h:123:1: warning: 'use_quote_double_quotes' defined but not used [-Wunused-function]
Do you see a simple and reliable way to correct or silence these warnings?
For test-quotearg.h the attached patch (installed) should do the trick.
I'll leave rijndael-api-fst.c for Simon.
Bruno Haible
2017-05-04 22:24:18 UTC
Permalink
Raw Message
Post by Paul Eggert
I'll leave rijndael-api-fst.c for Simon.
I'm still seeing the warnings with gcc-7.1:


rijndael-api-fst.c: In function 'rijndaelBlockEncrypt':
rijndael-api-fst.c:234:11: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
((uint32_t *) block)[0] = ((uint32_t *) input)[0] ^
^
rijndael-api-fst.c: In function 'rijndaelPadEncrypt':
rijndael-api-fst.c:317:11: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
((uint32_t *) block)[0] = ((uint32_t *) input)[0] ^
^
rijndael-api-fst.c: In function 'rijndaelBlockDecrypt':
rijndael-api-fst.c:390:11: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
((uint32_t *) block)[0] ^= ((uint32_t *) iv)[0];
^
rijndael-api-fst.c: In function 'rijndaelPadDecrypt':
rijndael-api-fst.c:484:11: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
((uint32_t *) block)[0] ^= ((uint32_t *) cipher->IV)[0];
^
rijndael-api-fst.c:484:11: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
rijndael-api-fst.c:495:7: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
((uint32_t *) block)[0] ^= ((uint32_t *) cipher->IV)[0];
^
rijndael-api-fst.c:495:7: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]


In gnupg's libgcrypt, Werner fixed it like this:
https://dev.gnupg.org/rCf17d50bbd31b1faa24af1e46c10bba845becf585
https://dev.gnupg.org/rCdfb4673da8ee52d95e0a62c9f49ca8599943f22e

But this fix is only effective for GCC. How could a compiler-independent fix
look like?

Bruno
Paul Eggert
2017-05-04 22:55:37 UTC
Permalink
Raw Message
Post by Bruno Haible
https://dev.gnupg.org/rCf17d50bbd31b1faa24af1e46c10bba845becf585
https://dev.gnupg.org/rCdfb4673da8ee52d95e0a62c9f49ca8599943f22e
But this fix is only effective for GCC. How could a compiler-independent fix
look like?
Why do we need a compiler-independent fix? The code itself is portable
as-is, right? So the only reason the change is needed is to pacify GCC
when its warnings are cranked up too high.
Bruno Haible
2017-05-05 16:30:37 UTC
Permalink
Raw Message
Hi Paul,
Post by Paul Eggert
Post by Bruno Haible
https://dev.gnupg.org/rCf17d50bbd31b1faa24af1e46c10bba845becf585
https://dev.gnupg.org/rCdfb4673da8ee52d95e0a62c9f49ca8599943f22e
But this fix is only effective for GCC. How could a compiler-independent fix
look like?
Why do we need a compiler-independent fix? The code itself is portable
as-is, right? So the only reason the change is needed is to pacify GCC
when its warnings are cranked up too high.
I'm not so sure about it.

This is the code:

char block[16];
...
((uint32_t *) block)[0] = ((uint32_t *) input)[0] ^
((uint32_t *) iv)[0];
((uint32_t *) block)[1] = ((uint32_t *) input)[1] ^
((uint32_t *) iv)[1];
((uint32_t *) block)[2] = ((uint32_t *) input)[2] ^
((uint32_t *) iv)[2];
((uint32_t *) block)[3] = ((uint32_t *) input)[3] ^
((uint32_t *) iv)[3];
rijndaelEncrypt (key->rk, key->Nr, block, outBuffer);

I think GCC is trying to tell us that, if the rijndaelEncrypt function got
inlined, the 4 assignments to '((uint32_t *) block)' might be reordered to
occur _after_ the read accesses to 'block' inside the function.

This danger is the same for all compilers that do alias-based analysis -
at least GCC, xlc [1][2], and SUNWspro C [3].

This appears to not be a mistake in GCC's analysis, because [4] says
"It's always assumed that char* aliases other types. However this
won't work the other way: there's no assumption that your struct
aliases a buffer of chars."

To disable this possible optimization and the associated warning - without
changing compiler options globally - there appear to be three approaches:

(1) Not use ((uint32_t *) block); use the 'char *' based gnulib module
'memxor' instead.
Drawback: This is a de-optimization, because memxor does not have
a fast path for when the alignment of both source and destination is 4.
(2) Use a union.
(3) Use the '__may_alias__' attribute.[5][6]
The drawbacks of this approach are:
- It may trigger internal compiler errors in GCC. [7]
- It has no effect on SUNWspro C, since this compiler does not
support this attribute.
- In the present form, it has no effect on xlc, since Werner Koch's
patch [8] did not enable it for xlc.

Therefore I would propose to go with the union approach:

union { char bytes[16]; uint32_t words[4]; } block;
...
block.words[0] = ((uint32_t *) input)[0] ^
((uint32_t *) iv)[0];
block.words[1] = ((uint32_t *) input)[1] ^
((uint32_t *) iv)[1];
block.words[2] = ((uint32_t *) input)[2] ^
((uint32_t *) iv)[2];
block.words[3] = ((uint32_t *) input)[3] ^
((uint32_t *) iv)[3];
rijndaelEncrypt (key->rk, key->Nr, block.bytes, outBuffer);

This code style should still work in 10 years, when compilers have attained
even better inlining and type propagation capabilities.

Bruno

[1] https://www.ibm.com/support/knowledgecenter/en/SSXVZZ_13.1.1/com.ibm.xlcpp1311.lelinux.doc/compiler_ref/opt_alias.html
[2] https://www.ibm.com/support/knowledgecenter/SSGH2K_13.1.0/com.ibm.xlc131.aix.doc/compiler_ref/opt_alias.html
[3] https://docs.oracle.com/cd/E19205-01/819-5265/bjaso/index.html
[4] http://stackoverflow.com/questions/98650/what-is-the-strict-aliasing-rule
[5] https://gcc.gnu.org/onlinedocs/gcc-7.1.0/gcc/Common-Type-Attributes.html
[6] https://www.ibm.com/support/knowledgecenter/SSGH3R_13.1.2/com.ibm.xlcpp1312.aix.doc/language_ref/type_attr_may_alias.html
[7] http://www.liblfds.org/wordpress/?p=520
[8] https://dev.gnupg.org/rCf17d50bbd31b1faa24af1e46c10bba845becf585


2017-05-05 Bruno Haible <***@clisp.org>

crypto/rijndael: Fix "strict-aliasing rules" warnings.
* lib/rijndael-api-fst.c (rijndaelBlockEncrypt): Declare 'block' as a
union.
(rijndaelPadEncrypt, rijndaelBlockDecrypt): Likewise.
(rijndaelPadDecrypt): Likewise. Use local variable 'iv' to cache the
value of cipher->IV.

diff --git a/lib/rijndael-api-fst.c b/lib/rijndael-api-fst.c
index b416601..c4972dc 100644
--- a/lib/rijndael-api-fst.c
+++ b/lib/rijndael-api-fst.c
@@ -203,7 +203,8 @@ rijndaelBlockEncrypt (rijndaelCipherInstance *cipher,
size_t inputLen, char *outBuffer)
{
size_t i, k, t, numBlocks;
- char block[16], *iv;
+ union { char bytes[16]; uint32_t words[4]; } block;
+ char *iv;

if (cipher == NULL || key == NULL || key->direction == RIJNDAEL_DIR_DECRYPT)
{
@@ -231,15 +232,11 @@ rijndaelBlockEncrypt (rijndaelCipherInstance *cipher,
iv = cipher->IV;
for (i = numBlocks; i > 0; i--)
{
- ((uint32_t *) block)[0] = ((uint32_t *) input)[0] ^
- ((uint32_t *) iv)[0];
- ((uint32_t *) block)[1] = ((uint32_t *) input)[1] ^
- ((uint32_t *) iv)[1];
- ((uint32_t *) block)[2] = ((uint32_t *) input)[2] ^
- ((uint32_t *) iv)[2];
- ((uint32_t *) block)[3] = ((uint32_t *) input)[3] ^
- ((uint32_t *) iv)[3];
- rijndaelEncrypt (key->rk, key->Nr, block, outBuffer);
+ block.words[0] = ((uint32_t *) input)[0] ^ ((uint32_t *) iv)[0];
+ block.words[1] = ((uint32_t *) input)[1] ^ ((uint32_t *) iv)[1];
+ block.words[2] = ((uint32_t *) input)[2] ^ ((uint32_t *) iv)[2];
+ block.words[3] = ((uint32_t *) input)[3] ^ ((uint32_t *) iv)[3];
+ rijndaelEncrypt (key->rk, key->Nr, block.bytes, outBuffer);
memcpy (cipher->IV, outBuffer, 16);
input += 16;
outBuffer += 16;
@@ -253,8 +250,8 @@ rijndaelBlockEncrypt (rijndaelCipherInstance *cipher,
memcpy (outBuffer, input, 16);
for (k = 0; k < 128; k++)
{
- rijndaelEncrypt (key->ek, key->Nr, iv, block);
- outBuffer[k >> 3] ^= (block[0] & 0x80U) >> (k & 7);
+ rijndaelEncrypt (key->ek, key->Nr, iv, block.bytes);
+ outBuffer[k >> 3] ^= (block.bytes[0] & 0x80U) >> (k & 7);
for (t = 0; t < 15; t++)
{
iv[t] = (iv[t] << 1) | (iv[t + 1] >> 7);
@@ -281,7 +278,8 @@ rijndaelPadEncrypt (rijndaelCipherInstance *cipher,
size_t inputOctets, char *outBuffer)
{
size_t i, numBlocks, padLen;
- char block[16], *iv;
+ union { char bytes[16]; uint32_t words[4]; } block;
+ char *iv;

if (cipher == NULL || key == NULL || key->direction == RIJNDAEL_DIR_DECRYPT)
{
@@ -305,24 +303,20 @@ rijndaelPadEncrypt (rijndaelCipherInstance *cipher,
}
padLen = 16 - (inputOctets - 16 * numBlocks);
assert (padLen > 0 && padLen <= 16);
- memcpy (block, input, 16 - padLen);
- memset (block + 16 - padLen, padLen, padLen);
- rijndaelEncrypt (key->rk, key->Nr, block, outBuffer);
+ memcpy (block.bytes, input, 16 - padLen);
+ memset (block.bytes + 16 - padLen, padLen, padLen);
+ rijndaelEncrypt (key->rk, key->Nr, block.bytes, outBuffer);
break;

case RIJNDAEL_MODE_CBC:
iv = cipher->IV;
for (i = numBlocks; i > 0; i--)
{
- ((uint32_t *) block)[0] = ((uint32_t *) input)[0] ^
- ((uint32_t *) iv)[0];
- ((uint32_t *) block)[1] = ((uint32_t *) input)[1] ^
- ((uint32_t *) iv)[1];
- ((uint32_t *) block)[2] = ((uint32_t *) input)[2] ^
- ((uint32_t *) iv)[2];
- ((uint32_t *) block)[3] = ((uint32_t *) input)[3] ^
- ((uint32_t *) iv)[3];
- rijndaelEncrypt (key->rk, key->Nr, block, outBuffer);
+ block.words[0] = ((uint32_t *) input)[0] ^ ((uint32_t *) iv)[0];
+ block.words[1] = ((uint32_t *) input)[1] ^ ((uint32_t *) iv)[1];
+ block.words[2] = ((uint32_t *) input)[2] ^ ((uint32_t *) iv)[2];
+ block.words[3] = ((uint32_t *) input)[3] ^ ((uint32_t *) iv)[3];
+ rijndaelEncrypt (key->rk, key->Nr, block.bytes, outBuffer);
memcpy (cipher->IV, outBuffer, 16);
input += 16;
outBuffer += 16;
@@ -331,13 +325,13 @@ rijndaelPadEncrypt (rijndaelCipherInstance *cipher,
assert (padLen > 0 && padLen <= 16);
for (i = 0; i < 16 - padLen; i++)
{
- block[i] = input[i] ^ iv[i];
+ block.bytes[i] = input[i] ^ iv[i];
}
for (i = 16 - padLen; i < 16; i++)
{
- block[i] = (char) padLen ^ iv[i];
+ block.bytes[i] = (char) padLen ^ iv[i];
}
- rijndaelEncrypt (key->rk, key->Nr, block, outBuffer);
+ rijndaelEncrypt (key->rk, key->Nr, block.bytes, outBuffer);
memcpy (cipher->IV, outBuffer, 16);
break;

@@ -355,7 +349,8 @@ rijndaelBlockDecrypt (rijndaelCipherInstance *cipher,
size_t inputLen, char *outBuffer)
{
size_t i, k, t, numBlocks;
- char block[16], *iv;
+ union { char bytes[16]; uint32_t words[4]; } block;
+ char *iv;

if (cipher == NULL
|| key == NULL
@@ -386,13 +381,13 @@ rijndaelBlockDecrypt (rijndaelCipherInstance *cipher,
iv = cipher->IV;
for (i = numBlocks; i > 0; i--)
{
- rijndaelDecrypt (key->rk, key->Nr, input, block);
- ((uint32_t *) block)[0] ^= ((uint32_t *) iv)[0];
- ((uint32_t *) block)[1] ^= ((uint32_t *) iv)[1];
- ((uint32_t *) block)[2] ^= ((uint32_t *) iv)[2];
- ((uint32_t *) block)[3] ^= ((uint32_t *) iv)[3];
+ rijndaelDecrypt (key->rk, key->Nr, input, block.bytes);
+ block.words[0] ^= ((uint32_t *) iv)[0];
+ block.words[1] ^= ((uint32_t *) iv)[1];
+ block.words[2] ^= ((uint32_t *) iv)[2];
+ block.words[3] ^= ((uint32_t *) iv)[3];
memcpy (cipher->IV, input, 16);
- memcpy (outBuffer, block, 16);
+ memcpy (outBuffer, block.bytes, 16);
input += 16;
outBuffer += 16;
}
@@ -405,13 +400,13 @@ rijndaelBlockDecrypt (rijndaelCipherInstance *cipher,
memcpy (outBuffer, input, 16);
for (k = 0; k < 128; k++)
{
- rijndaelEncrypt (key->ek, key->Nr, iv, block);
+ rijndaelEncrypt (key->ek, key->Nr, iv, block.bytes);
for (t = 0; t < 15; t++)
{
iv[t] = (iv[t] << 1) | (iv[t + 1] >> 7);
}
iv[15] = (iv[15] << 1) | ((input[k >> 3] >> (7 - (k & 7))) & 1);
- outBuffer[k >> 3] ^= (block[0] & 0x80U) >> (k & 7);
+ outBuffer[k >> 3] ^= (block.bytes[0] & 0x80U) >> (k & 7);
}
outBuffer += 16;
input += 16;
@@ -432,7 +427,8 @@ rijndaelPadDecrypt (rijndaelCipherInstance *cipher,
size_t inputOctets, char *outBuffer)
{
size_t i, numBlocks, padLen;
- char block[16];
+ union { char bytes[16]; uint32_t words[4]; } block;
+ char *iv;

if (cipher == NULL || key == NULL || key->direction == RIJNDAEL_DIR_ENCRYPT)
{
@@ -460,55 +456,56 @@ rijndaelPadDecrypt (rijndaelCipherInstance *cipher,
outBuffer += 16;
}
/* last block */
- rijndaelDecrypt (key->rk, key->Nr, input, block);
- padLen = block[15];
+ rijndaelDecrypt (key->rk, key->Nr, input, block.bytes);
+ padLen = block.bytes[15];
if (padLen >= 16)
{
return RIJNDAEL_BAD_DATA;
}
for (i = 16 - padLen; i < 16; i++)
{
- if (block[i] != padLen)
+ if (block.bytes[i] != padLen)
{
return RIJNDAEL_BAD_DATA;
}
}
- memcpy (outBuffer, block, 16 - padLen);
+ memcpy (outBuffer, block.bytes, 16 - padLen);
break;

case RIJNDAEL_MODE_CBC:
+ iv = cipher->IV;
/* all blocks but last */
for (i = numBlocks - 1; i > 0; i--)
{
- rijndaelDecrypt (key->rk, key->Nr, input, block);
- ((uint32_t *) block)[0] ^= ((uint32_t *) cipher->IV)[0];
- ((uint32_t *) block)[1] ^= ((uint32_t *) cipher->IV)[1];
- ((uint32_t *) block)[2] ^= ((uint32_t *) cipher->IV)[2];
- ((uint32_t *) block)[3] ^= ((uint32_t *) cipher->IV)[3];
- memcpy (cipher->IV, input, 16);
- memcpy (outBuffer, block, 16);
+ rijndaelDecrypt (key->rk, key->Nr, input, block.bytes);
+ block.words[0] ^= ((uint32_t *) iv)[0];
+ block.words[1] ^= ((uint32_t *) iv)[1];
+ block.words[2] ^= ((uint32_t *) iv)[2];
+ block.words[3] ^= ((uint32_t *) iv)[3];
+ memcpy (iv, input, 16);
+ memcpy (outBuffer, block.bytes, 16);
input += 16;
outBuffer += 16;
}
/* last block */
- rijndaelDecrypt (key->rk, key->Nr, input, block);
- ((uint32_t *) block)[0] ^= ((uint32_t *) cipher->IV)[0];
- ((uint32_t *) block)[1] ^= ((uint32_t *) cipher->IV)[1];
- ((uint32_t *) block)[2] ^= ((uint32_t *) cipher->IV)[2];
- ((uint32_t *) block)[3] ^= ((uint32_t *) cipher->IV)[3];
- padLen = block[15];
+ rijndaelDecrypt (key->rk, key->Nr, input, block.bytes);
+ block.words[0] ^= ((uint32_t *) iv)[0];
+ block.words[1] ^= ((uint32_t *) iv)[1];
+ block.words[2] ^= ((uint32_t *) iv)[2];
+ block.words[3] ^= ((uint32_t *) iv)[3];
+ padLen = block.bytes[15];
if (padLen <= 0 || padLen > 16)
{
return RIJNDAEL_BAD_DATA;
}
for (i = 16 - padLen; i < 16; i++)
{
- if (block[i] != padLen)
+ if (block.bytes[i] != padLen)
{
return RIJNDAEL_BAD_DATA;
}
}
- memcpy (outBuffer, block, 16 - padLen);
+ memcpy (outBuffer, block.bytes, 16 - padLen);
break;

default:
Bruno Haible
2017-05-05 16:36:54 UTC
Permalink
Raw Message
By the way, the proposed patch not only fixes the gcc warnings about aliasing,
but also the alignment issue: a declaration of a local variable
char block[16];
does not ensure that 'block' will has the necessary alignment for uint32_t
accesses.

Bruno
Paul Eggert
2017-05-05 22:16:48 UTC
Permalink
Raw Message
Yes, that sounds like the best way to fix the problem now. Thanks for
explaining it.
Bruno Haible
2017-05-05 23:27:16 UTC
Permalink
Raw Message
Post by Paul Eggert
Yes, that sounds like the best way to fix the problem now. Thanks for
explaining it.
OK, I've pushed it.

Bruno

Loading...