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.

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

Remove the need of -Wno-stringop-overflow warning suppression #11357

Merged
merged 2 commits into from
May 16, 2024

Conversation

freak82
Copy link
Contributor

@freak82 freak82 commented May 15, 2024

The problem was that GCC complained for possibly incorrect usage of strncat due to the last argument being equal to the source string length. The last argument of strncat is supposed to be the remaining free space in the destination buffer.
Replaced the usage of strncat with the ATS function ink_strlcat.
More info about this can be found in this issue

The problem was that GCC complained for possibly incorrect usage of
strncat due to the last argument being equal to the source string length.
The last argument of strncat is supposed to be the remaining free space in
the destination buffer.
Replaced the usage of strncat with the ATS function ink_strlcat.
@brbzull0 brbzull0 added this to the 10.1.0 milestone May 15, 2024
@ezelkow1
Copy link
Member

[approve ci]

@JosiahWI
Copy link
Collaborator

JosiahWI commented May 15, 2024

In function 'size_t str_concat(char*, size_t, const char*, size_t)',
    inlined from 'TSHttpStatus S3Request::authorizeV2(S3Config*)' at ../../../plugins/s3_auth/s3_auth.cc:920:25:
../../../plugins/s3_auth/s3_auth.cc:761:12: error: 'char* strncat(char*, const char*, size_t)' specified bound 1 equals source length [-Werror=stringop-overflow=]
  761 |     strncat(dst, src, to_copy);
      |     ~~~~~~~^~~~~~~~~~~~~~~~~~~
In function 'size_t str_concat(char*, size_t, const char*, size_t)',
    inlined from 'TSHttpStatus S3Request::authorizeV2(S3Config*)' at ../../../plugins/s3_auth/s3_auth.cc:926:25:
../../../plugins/s3_auth/s3_auth.cc:761:12: error: 'char* strncat(char*, const char*, size_t)' specified bound 1 equals source length [-Werror=stringop-overflow=]
  761 |     strncat(dst, src, to_copy);
      |     ~~~~~~~^~~~~~~~~~~~~~~~~~~

It looks like there is a possible similar incorrect usage of strncat in the s3_auth plugin that the CentOS build is now complaining about.

@freak82
Copy link
Contributor Author

freak82 commented May 15, 2024

Hmm, I built all of the plugins and there was no other complains from GCC 13.2 on Debian.
I can see that the s3_auth plugin has been built. It has not been skipped.
Most likely this is due to different GCC versions.

It seems to me that in the plugins/s3_auth/s3_auth.cc the GCC analyzer is wrong about this warning:
It most likely get confused from this usage str_concat(&left[loff], (left_size - loff), "/", 1); and because in the str_concat there is std::min(dst_len, src_len) for the strncat size argument.

Maybe it'll be better if I change all of the remaining 5 usages of strncat in

plugins/s3_auth/s3_auth.cc (1)
plugins/experimental/url_sig/url_sig.cc (4)

with ink_strlcat?

Or the suppression can be left as it is at global level?

Or another option is the suppression to be set in the CMakeLists.txt of the s3_auth plugin?

@maskit
Copy link
Member

maskit commented May 15, 2024

I don't mind leaving the warning suppression. Sounds like the change is enough for you, and I'm not going to have you fix all the places. We can remove the warning suppression later when we are ready to do so. But if you are willing to make additional changes for the plugin code, please use TSstrlcat, which is exactly the same as ink_strlcat but it's exported for plugins.

The change is to use TSstrlcat instead of strncat.
It seems to me the GCC analyzer is wrong in this case and it issues the
warning because of such usage:
str_concat(&left[loff], (left_size - loff), "/", 1);
and the str_concat internally does
std::min(dst_len, src_len) for the strncat size argument.
So GCC sees the possibility that the last argument of strncat can be
equal to the legnth of the source argument and issues the warning.
@freak82
Copy link
Contributor Author

freak82 commented May 16, 2024

I replaced the strncat usage in the s3_auth plugin with TSstrlcat.
Tested the build again with both GCC 11, which was giving the warning in the plugin, and with GCC 13.

Copy link
Member

@maskit maskit left a comment

Choose a reason for hiding this comment

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

Thank you!

@maskit maskit merged commit 7220d70 into apache:master May 16, 2024
15 checks passed
@freak82 freak82 deleted the remove-Wno-stringop-overflow branch May 17, 2024 06:58
@cmcfarlen cmcfarlen modified the milestones: 10.1.0, 10.0.0 May 24, 2024
@cmcfarlen
Copy link
Contributor

Cherry-picked to v10.0.x

cmcfarlen pushed a commit that referenced this pull request May 24, 2024
* Remove the need of -Wno-stringop-overflow warning suppression

The problem was that GCC complained for possibly incorrect usage of
strncat due to the last argument being equal to the source string length.
The last argument of strncat is supposed to be the remaining free space in
the destination buffer.
Replaced the usage of strncat with the ATS function ink_strlcat.

* Fix bogus warning for strncat usage in s3_auth plugin given by GCC 11

The change is to use TSstrlcat instead of strncat.
It seems to me the GCC analyzer is wrong in this case and it issues the
warning because of such usage:
str_concat(&left[loff], (left_size - loff), "/", 1);
and the str_concat internally does
std::min(dst_len, src_len) for the strncat size argument.
So GCC sees the possibility that the last argument of strncat can be
equal to the legnth of the source argument and issues the warning.

(cherry picked from commit 7220d70)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: picked-10.0.0
Development

Successfully merging this pull request may close these issues.

None yet

6 participants