Fixed -Wunterminated-string-initialization with GCC 15. #134
Conversation
|
Seems like gcc-15 is in its early stages, so maybe it's too early to fix this. Generally I like the patch, just a couple of small comments. Plus the commit log should be in line with nginx standards, see https://github.com/nginx/nginx/blob/master/CONTRIBUTING.md. |
51b6e1d to
df5486a
Compare
gcc-15 build failuregcc-15 build failure on -Werror=unterminated-string-initialization.
|
Reworded the commit message. |
df5486a to
08f6439
Compare
gcc-15 build failure on -Werror=unterminated-string-initialization.gcc-15 build failure on -Werror=unterminated-string-initialization.
|
Good point. Applied the similar change to |
There was a problem hiding this comment.
Concerning the commit log.
We don't use multiple prefixes in the commit log.
If commit comes as a general fix touching unrelated places, the prefix is omitted.
Looking at the commit history to get similar commit messages, this leads to the next simple commit log:
Fixed -Wunterminated-string-initialization with GCC 15.
| static const uint8_t salt[20] = { | ||
| 0x38, 0x76, 0x2c, 0xf7, 0xf5, 0x59, 0x34, 0xb3, 0x4d, 0x17, | ||
| 0x9a, 0xe6, 0xa4, 0xc8, 0x0c, 0xad, 0xcc, 0xbb, 0x7f, 0x0a, | ||
| }; |
There was a problem hiding this comment.
I don't think this is a change in the right direction.
What I'd like to see is declaring with unspecified array size, e.g. similar to the grpc module. This allows to keep using the initialization string unmodified, written in a more compact form, compared to a byte sequence.
Calculating the number of array elements with sizeof() - 1 is a common approach used in nginx as well.
$ grep -E 'sizeof\([^)]+\) - 1' src/http/v2/ngx_http_v2_filter_module.c
(ngx_http_v2_integer_octets(sizeof(h) - 1) + sizeof(h) - 1)
len += 1 + sizeof(nginx) - 1;
len += sizeof("; charset=") - 1 + r->headers_out.charset.len;
location.len = sizeof("https://") - 1 + host.len
location.len += sizeof(":65535") - 1;
p = ngx_cpymem(location.data, "http", sizeof("http") - 1);
len += 1 + sizeof(accept_encoding) - 1;
sizeof(NGINX_VER) - 1, tmp);
sizeof(NGINX_VER_BUILD) - 1, tmp);
pos = ngx_cpymem(pos, nginx, sizeof(nginx) - 1);
len = r->headers_out.content_type.len + sizeof("; charset=") - 1
p = ngx_cpymem(p, "; charset=", sizeof("; charset=") - 1);
len = sizeof("Wed, 31 Dec 1986 18:00:00 GMT") - 1;
pos = ngx_cpymem(pos, accept_encoding, sizeof(accept_encoding) - 1);
This covers both C arrays and literal strings.
As a bonus, this allows logging the salt value without variable length modifier:
@@ -152,8 +152,8 @@ ngx_quic_keys_set_initial_secret(ngx_quic_keys_t *keys, ngx_str_t *secret,
ngx_log_debug0(NGX_LOG_DEBUG_EVENT, log, 0,
"quic ngx_quic_set_initial_secret");
#ifdef NGX_QUIC_DEBUG_CRYPTO
- ngx_log_debug3(NGX_LOG_DEBUG_EVENT, log, 0,
- "quic salt len:%uz %*xs", sizeof(salt), sizeof(salt), salt);
+ ngx_log_debug2(NGX_LOG_DEBUG_EVENT, log, 0,
+ "quic salt len:%uz %xs", sizeof(salt) - 1, salt);
ngx_log_debug3(NGX_LOG_DEBUG_EVENT, log, 0,
"quic initial secret len:%uz %*xs", is_len, is_len, is);
#endif
`gcc-15` added a new warning in https://gcc.gnu.org/PR115185: src/http/v2/ngx_http_v2_filter_module.c: In function 'ngx_http_v2_header_filter': src/http/v2/ngx_http_v2_filter_module.c:118:36: error: initializer-string for array of 'unsigned char' is too long [-Werror=unterminated-string-initialization] 118 | static const u_char nginx[5] = "\x84\xaa\x63\x55\xe7"; | ^~~~~~~~~~~~~~~~~~~~~~ src/http/v2/ngx_http_v2_filter_module.c:121:9: error: initializer-string for array of 'unsigned char' is too long [-Werror=unterminated-string-initialization] 121 | "\x8b\x84\x84\x2d\x69\x5b\x05\x44\x3c\x86\xaa\x6f"; | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This change initializes the array bytewise to avoid string literal assignnment (and avoid trailing "\0" truncation).
08f6439 to
df14b77
Compare
gcc-15 build failure on -Werror=unterminated-string-initialization.|
Switched |
|
Please apply same in the rest places. |
|
On the other hand, removing explicit size from key_data and nonce may be error-prone, because they are passed without explicit size (the object size is implied by a selected cipher suite). And it could be a security issue, e.g. once NGX_QUIC_IV_LEN is enlarged. |
|
iiuc this was closed & not merged? i hit it with current release + GCC15, as on Fedora 42+. e.g.: for Fedora41-release, with GCC14 @ https://gist.github.com/pgnd/23a3dd25ec9fe19d34f1b0850323d734#file-gistfile1-txt-L4900 for Fedora42-prerelease, with GCC15 @ https://gist.github.com/pgnd/f40533deaea3f1defe82906bee25c77d#file-gistfile1-txt-L3669 |
|
Looks like it was closed as a coincidence. |
got it, thx.
yup. the workaround works for now; GCC15 builds for F42 & Rawhide are good. |
|
It was closed because the scope of the fix kept growing without the sight of completion. The comments were too vague for me to clearly understand the desired end state. After another merge conflict I gave up. It's just taking too much time from me to fix a trivial issue. I ended up disabling |
|
FWIW we hit the same issue in Unit. For now we've disabled this warning until hopefully this patch (or similar) makes it into GCC then we can re-enable the warning and tag such arrays with the "nonstring" variable attribute. |
|
FWIW |
Hmm, which version of clang is that? AFAICT clang has no such option... However seeing as the above mentioned patch has now been merged into GCC 15 this issue can be handled properly... |
do you have a link for that? |
The linked commit will allow static const char hex[16] __attribute__(("nonstring")) = "0123456789ABCDEF";This will allow you to stick to defining string literals as E.g. For something like int hexit(int c)
{
static const char hex[16] = "0123456789ABCDEF";
return hex[c % 16];
}GCC 15 now produces $ gcc -Wunterminated-string-initialization -c nonstring.c nonstring.c: In function ‘hexit’:
nonstring.c:13:49: warning: initializer-string for array of ‘char’ truncates NUL terminator but destination lacks ‘nonstring’ attribute (17 chars into 16 available) [-Wunterminated-string-initialization]
13 | static const char hex[16] = "0123456789ABCDEF";
| ^~~~~~~~~~~~~~~~~~(Note: So you can do int hexit(int c)
{
static const char hex[16] __attribute__((nonstring)) = "0123456789ABCDEF";
return hex[c % 16];
}$ gcc -Wunterminated-string-initialization -c nonstring.c
$This attribute already existed, it just didn't originally effect this new warning. |
This is a wrapper around __attribute__((nonstring)). Traditionally this was used to mark char array variables that intentionally lacked a terminating NUL byte, this would then cause warnings to either be quelled or emitted for various memory/string functions. GCC 15 introduced a new warning, Wunterminated-string-initialization, which will always warn on things like static const char hex[16] = "0123456789ABCDEF"; However this is very much intentionally not NUL terminated. The above attribute also quells this new warning (which is also enabled by -Wextra). So the above example would become static const char hex[16] NGX_NONSTRING = "0123456789ABCDEF"; This issue was attempted to be fixed in a previous pull-request, but I believe this to be a superior solution (although the use of "nonstring" to quell this warning wasn't available at that time). Link: <https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=44c9403ed1833ae71a59e84f9e37af3182be0df5> Link: <https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=622968990beee7499e951590258363545b4a3b57> Link: <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117178#c21> Link: <nginx#134> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
gcc-15added a new warning in https://gcc.gnu.org/PR115185:This change initializes the array bytewise to avoid string literal
assignnment (and avoid trailing "\0" truncation).