3.8.2 gcc 13 warnings `-Wstringop-overflow`, `-Wdangling-pointer`
Closed this issue · 12 comments
In function 'make_kn',
inlined from 'make_kn' at ./libressl/crypto/cmac/cmac.c:80:1,
inlined from 'CMAC_Init' at ./libressl/crypto/cmac/cmac.c:191:3:
./libressl/crypto/cmac/cmac.c:92:28: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
92 | k1[bl - 1] ^= bl == 16 ? 0x87 : 0x1b;
| ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
./libressl/crypto/cmac/cmac.c: In function 'CMAC_Init':
./libressl/crypto/cmac/cmac.c:66:23: note: at offset [-2147483649, -1] into destination object 'k1' of size 32
66 | unsigned char k1[EVP_MAX_BLOCK_LENGTH];
| ^~
./libressl/crypto/compat/timegm.c: In function '__year_to_secs':
./libressl/crypto/compat/timegm.c:91:45: warning: dangling pointer 'is_leap' to an unnamed temporary may be used [-Wdangling-pointer=]
91 | leaps += 97*cycles + 24*centuries - *is_leap;
| ^~~~~~~~
./libressl/crypto/compat/timegm.c:62:39: note: unnamed temporary defined here
62 | if (!is_leap) is_leap = &(int){0};
| ^
./libressl/crypto/compat/timegm.c:70:26: warning: dangling pointer 'is_leap' to an unnamed temporary may be used [-Wdangling-pointer=]
70 | *is_leap = 1;
| ~~~~~~~~~^~~
./libressl/crypto/compat/timegm.c:62:39: note: unnamed temporary defined here
62 | if (!is_leap) is_leap = &(int){0};
| ^
./libressl/crypto/compat/timegm.c:82:34: warning: dangling pointer 'is_leap' to an unnamed temporary may be used [-Wdangling-pointer=]
82 | *is_leap = 0;
| ~~~~~~~~~^~~
./libressl/crypto/compat/timegm.c:62:39: note: unnamed temporary defined here
62 | if (!is_leap) is_leap = &(int){0};
| ^
./libressl/crypto/compat/timegm.c:87:34: warning: dangling pointer 'is_leap' to an unnamed temporary may be used [-Wdangling-pointer=]
87 | *is_leap = !rem;
| ~~~~~~~~~^~~~~~
./libressl/crypto/compat/timegm.c:62:39: note: unnamed temporary defined here
62 | if (!is_leap) is_leap = &(int){0};
| ^
In function 'make_kn', inlined from 'make_kn' at ./libressl/crypto/cmac/cmac.c:80:1, inlined from 'CMAC_Init' at ./libressl/crypto/cmac/cmac.c:191:3: ./libressl/crypto/cmac/cmac.c:92:28: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=] 92 | k1[bl - 1] ^= bl == 16 ? 0x87 : 0x1b; | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~ ./libressl/crypto/cmac/cmac.c: In function 'CMAC_Init': ./libressl/crypto/cmac/cmac.c:66:23: note: at offset [-2147483649, -1] into destination object 'k1' of size 32 66 | unsigned char k1[EVP_MAX_BLOCK_LENGTH]; | ^~
This assumes that bl <= 0 which should never happen with a block cipher (and if you do CMAC with a non-block cipher you get what you asked for). But there are various issues in this code...
I wonder if the warning goes away if we do this, but I'm not sure if gcc is smart enough:
diff --git a/lbressl/crypto/cmac/cmac.c b/libressl/crypto/cmac/cmac.c
index 257bd21ccad..75930fd3cfe 100644
--- a/libressl/crypto/cmac/cmac.c
+++ b/libressl/crypto/cmac/cmac.c
@@ -186,7 +186,8 @@ CMAC_Init(CMAC_CTX *ctx, const void *key, size_t keylen,
return 0;
if (!EVP_EncryptInit_ex(&ctx->cctx, NULL, NULL, key, zero_iv))
return 0;
- bl = EVP_CIPHER_CTX_block_size(&ctx->cctx);
+ if ((bl = EVP_CIPHER_CTX_block_size(&ctx->cctx)) <= 0)
+ return 0;
if (!EVP_Cipher(&ctx->cctx, ctx->tbl, zero_iv, bl))
return 0;
make_kn(ctx->k1, ctx->tbl, bl);The second pile of warnings could be "solved" by deleting or commenting out this line:
portable/crypto/compat/timegm.c
Line 62 in e454895
However, it should be noted that __year_to_secs() is never called with is_leap == NULL so all the whining is irrelevant in the first place. Moreover, I'm pretty sure this is perfectly fine use of an unnamed temporary object, which should have static storage duration.
Also note that this is a copy of musl code.
I tried both of your suggestions and they do make these warnings
disappear with gcc 13.
[ Luckily, I'm not building musl from source :) ]
Thanks for testing. Then I'll land a few fixes in the vicinity of that particular problem zone in cmac.c. I think it would be fine to comment out the line in the timegm.c code and explicitly say why it is done.
@bob-beck there is one call left in x509_verify_asn1_time_to_time_t() in x509_verify.c:
Unfortunately there are a few more in libtls which are a bit more annoying to fix.
The warning in the CMAC code should also be gone with openbsd/src@8865b67
@botovq Great, thanks! I was looking, but in the wrong repo (https://github.com/libressl-portable/openbsd).
Closing this as resolved.
It's also there. I usually link to commits the openbsd/src repo since the commit hashes there are guaranteed to be stable. This is not the case for libressl/openbsd due to limitations in the conversion tool.
Found it indeed: libressl/openbsd@0423d36
I have no excuses, not sure how I missed it!