Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GCC 15 Wunterminated-string-initialization issues #1543

Closed
wants to merge 1 commit into from
Closed

Fix GCC 15 Wunterminated-string-initialization issues #1543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/nxt_http_parse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,10 @@ nxt_http_parse_request_line(nxt_http_request_parse_t *rp, u_char **pos,
nxt_http_ver_t ver;
nxt_http_target_traps_e trap;

static const nxt_http_ver_t http11 = { "HTTP/1.1" };
static const nxt_http_ver_t http10 = { "HTTP/1.0" };
static const nxt_http_ver_t http11 =
{{ 'H', 'T', 'T', 'P', '/', '1', '.', '1' }};
static const nxt_http_ver_t http10 =
{{ 'H', 'T', 'T', 'P', '/', '1', '.', '0' }};
Comment on lines -167 to +170
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep this as it was. It was much more readable. I'd make it a [[gnu::nonstring]], and wait for a future compiler to re-enable the diagnostic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it looks hideous, but it meant we could keep the warning enabled... but then that whole nxt_http_ver_t thing is only used in that function and it seems to be purely to allow for doing a string comparison using unit64_t's...


p = *pos;

Expand Down Expand Up @@ -516,7 +518,8 @@ nxt_http_parse_field_name(nxt_http_request_parse_t *rp, u_char **pos,
size_t len;
uint32_t hash;

static const u_char normal[256] nxt_aligned(64) =
/* The last '\0' is not needed because the string is NUL terminated */
static const u_char normal[] nxt_aligned(64) =
Comment on lines -519 to +522
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ac000 ,

That comment is incorrect:

/* The last '\0' is not needed because the string is NUL terminated */

Even though this is a "string literal", it is not a string. The standard prohibits having NUL bytes in the middle of a string, and this is thus not a string. See footnote 75 in C23 (n3220): https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3220.pdf#subsection.6.4.5.

75)A string literal may not be a string (see 7.1.1), because a null character can be embedded in it by a \0 escape sequence.

So I would disable the diagnostic. This change (I'm referring specifically to this hunk, and not the entire patch; most of the patch, I like it) is bad for readability, IMO. Instead, I'd take the opportunity to fix the few places where it wouldn't hurt to adapt to the warning, but then disable it until [[gnu::nonstring()]] allows you to re-enable it.

Cheers,
Alex

Cc: @thesamesam

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add the [[gnu::nonstring]] attribute already, so that later you only need to re-enable the diagnostic. That would already enhance readability.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ac000 ,

That comment is incorrect:

/* The last '\0' is not needed because the string is NUL terminated */

Admittedly I just yanked that comment out of nginx, but In this case I would take string to just mean "array of characters".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ac000 ,
That comment is incorrect:

/* The last '\0' is not needed because the string is NUL terminated */

Admittedly I just yanked that comment out of nginx,

Ughhh. Maybe tell them that their comment is actively harmful?

but In this case I would take string to just mean "array of characters".

Yeah, but I this one is really too obscure. The new code feels like a hybrid between a string and a nonstring. And I'd say the [256] was actually informative, since it's not obvious to see how many zeros there are.

"the string is NUL terminated" reminds just so much to a proper string, that it will most likely confuse someone at some point.

I'd keep the old version here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, can you point to the comment in nginx? I can't find it with grep.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior related commits:

nginx/nginx@e5efadb

nginx/nginx@3338cfd

(sounds like this confused Igor himself.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this context, I would assume "string" just means "string of characters" where string is synonymous with 'series', like you might say "string of robberies".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this context, I would assume "string" just means "string of characters" where string is synonymous with 'series', like you might say "string of robberies".

I'd use "array" instead of string. Or "string literal", if you want. But using string seems inducing confusion.

Maybe I'm too much of a pedantic regarding wording issues; that's my job. :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, if we're being pedantic then I think the word would be pedant.

Anyway, I'm probably just going to close this PR and simply disable the warning for now, and then look at sprinkling __attribute__((nonstring))'s (nicer named) around when Kees' patch makes it in, maybe GCC 16, maybe even GCC 15, which would be prefect, or we may be sitting here in two years time still waiting for it to land. Never can tell... (No, think positive thoughts...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, if we're being pedantic then I think the word would be pedant.

Lol. True.

Anyway, I'm probably just going to close this PR and simply disable the warning for now,

I do like that one, however: #1543 (comment)

I'd do that change, and then turn the diagnostic off.

and then look at sprinkling __attribute__((nonstring))'s (nicer named) around when Kees' patch makes it in, maybe GCC 16, maybe even GCC 15, which would be prefect, or we may be sitting here in two years time still waiting for it to land. Never can tell... (No, think positive thoughts...)

:-)

"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"
/* \s ! " # $ % & ' ( ) * + , . / : ; < = > ? */
"\0\1\0\1\1\1\1\1\0\0\1\1\0" "-" "\1\0" "0123456789" "\0\0\0\0\0\0"
Expand All @@ -529,7 +532,7 @@ nxt_http_parse_field_name(nxt_http_request_parse_t *rp, u_char **pos,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0";
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0";

p = *pos + rp->field_name.length;
hash = rp->field_hash;
Expand Down
4 changes: 2 additions & 2 deletions src/nxt_sprintf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ nxt_vsprintf(u_char *buf, u_char *end, const char *fmt, va_list args)
nxt_sprintf_t spf;
nxt_file_name_t *fn;

static const u_char hexadecimal[16] = "0123456789abcdef";
static const u_char HEXADECIMAL[16] = "0123456789ABCDEF";
static const u_char hexadecimal[] = "0123456789abcdef";
static const u_char HEXADECIMAL[] = "0123456789ABCDEF";
Comment on lines -115 to +116
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one I like it. Everything that need not be a nonstring, should be a string. Everybody knows hexadecimal enough that the lost information is negligible.

static const u_char nan[] = "[nan]";
static const u_char null[] = "[null]";
static const u_char infinity[] = "[infinity]";
Expand Down
15 changes: 7 additions & 8 deletions src/nxt_string.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,14 +592,15 @@ nxt_decode_uri_plus(u_char *dst, u_char *src, size_t length)
}


static const u_char nxt_hex[] = "0123456789ABCDEF";


uintptr_t
nxt_encode_uri(u_char *dst, u_char *src, size_t length)
{
u_char *end;
nxt_uint_t n;

static const u_char hex[16] = "0123456789ABCDEF";

end = src + length;

if (dst == NULL) {
Expand All @@ -624,8 +625,8 @@ nxt_encode_uri(u_char *dst, u_char *src, size_t length)

if (nxt_uri_escape[*src >> 5] & (1U << (*src & 0x1f))) {
*dst++ = '%';
*dst++ = hex[*src >> 4];
*dst++ = hex[*src & 0xf];
*dst++ = nxt_hex[*src >> 4];
*dst++ = nxt_hex[*src & 0xf];

} else {
*dst++ = *src;
Expand All @@ -644,8 +645,6 @@ nxt_encode_complex_uri(u_char *dst, u_char *src, size_t length)
u_char *reserved, *end, ch;
nxt_uint_t n;

static const u_char hex[16] = "0123456789ABCDEF";

reserved = (u_char *) "?#\0";

end = src + length;
Expand Down Expand Up @@ -689,8 +688,8 @@ nxt_encode_complex_uri(u_char *dst, u_char *src, size_t length)

} else {
*dst++ = '%';
*dst++ = hex[ch >> 4];
*dst++ = hex[ch & 0xf];
*dst++ = nxt_hex[ch >> 4];
*dst++ = nxt_hex[ch & 0xf];
continue;
}
}
Expand Down
Loading