Skip to content

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

Merged
maskit merged 2 commits into
apache:masterfrom
x3me:remove-Wno-stringop-overflow
May 16, 2024
Merged

Remove the need of -Wno-stringop-overflow warning suppression#11357
maskit merged 2 commits into
apache:masterfrom
x3me:remove-Wno-stringop-overflow

Conversation

@freak82
Copy link
Copy Markdown
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 CMake work related to CMakes scripts or issues Core and removed Core labels May 15, 2024
@brbzull0 brbzull0 added this to the 10.1.0 milestone May 15, 2024
@ezelkow1
Copy link
Copy Markdown
Member

[approve ci]

@JosiahWI
Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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
Copy Markdown
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

CMake work related to CMakes scripts or issues

Projects

Status: picked-10.0.0

Development

Successfully merging this pull request may close these issues.

6 participants