Skip to content

Fix compiler warnings from GCC #3372

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

Jump to bottom
Merged
merged 2 commits into from
May 17, 2025
Merged

Fix compiler warnings from GCC #3372

merged 2 commits into from
May 17, 2025

Conversation

notroj
Copy link

@notroj notroj commented May 9, 2025

what

  • Fix compiler warnings

why

  • Compiler warnings are bad

references

airween
airween requested changes May 9, 2025
View reviewed changes
@airween
Copy link
Member

airween commented May 9, 2025

Hi @notroj,

first of all, many thanks for this PR.

There are some other warnings in different files (usually with different kind of warnings) - do you plan to fix those too?

@notroj notroj force-pushed the v2-gcc-warning-fixes branch 2 times, most recently from 64d4418 to 6919c1c Compare May 12, 2025 15:02
@notroj
Copy link
Author

notroj commented May 12, 2025 •
edited
Loading

I've repushed with discussed fixes. I am currently testing with a set of CFLAGS similar to what we build with in Fedora/RHEL, but some low value warnings turned off:

CFLAGS='-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -m64 -march=x86-64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -mtls-dialect=gnu2 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -Wno-maybe-uninitialized -Wno-deprecated-declarations -Wno-unused -Wno-pointer-sign '

There is only one outstanding warning:

In file included from /usr/include/stdio.h:970,
                 from ../apache2/modsecurity.h:18,
                 from ../apache2/msc_util.c:15:
In function ‘sprintf’,
    inlined from ‘msc_headers_to_buffer’ at ../apache2/msc_util.c:2331:17:
/usr/include/bits/stdio2.h:30:10: warning: ‘__sprintf_chk’ argument 5 overlaps destination object ‘buffer’ [-Wrestrict]
   30 |   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   31 |                                   __glibc_objsize (__s), __fmt,
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   32 |                                   __va_arg_pack ());
      |                                   ~~~~~~~~~~~~~~~~~
../apache2/msc_util.c: In function ‘msc_headers_to_buffer’:
../apache2/msc_util.c:2306:64: note: destination object referenced by ‘restrict’-qualified argument 1 was declared here
 2306 | int msc_headers_to_buffer(const apr_array_header_t *arr, char *buffer,
      |                                                          ~~~~~~^~~~~~
  CCLD     msc_test

This is similar to the one of the other problems - using the destination buffer in the snprintf format arguments. It would IMO make sense to rewrite this function to use apr_table_do which might be simpler anyway, I have a patch in progress but I wasn't able to run the tests.

I see you just fixed the tests 😄 in 8881097 and now I can this PR broke them again due to using ap_bin2hex, I will work on these.

@airween
Copy link
Member

airween commented May 12, 2025

I've repushed with discussed fixes. I am currently testing with a set of CFLAGS similar to what we build with in Fedora/RHEL, but some low value warnings turned off:
[...]
There is only one outstanding warning:
[...]
This is similar to the one of the other problems - using the destination buffer in the snprintf format arguments. It would IMO make sense to rewrite this function to use apr_table_do which might be simpler anyway, I have a patch in progress but I wasn't able to run the tests.

Thank you for taking this.

Let me know if I can help you anything.

I see you just fixed the tests 😄 in 8881097 and now I can this PR broke them again due to using ap_bin2hex, I will work on these.

Ah yes. This has been on my list for a year now.... I was talking to another developer a few days/week ago and he noticed that there were issues with the tests, so I thought I'd fix the broken parts. Sorry for the inconvenience 😃.

@airween
Copy link
Member

airween commented May 16, 2025

Hi @notroj,

just want to let you know, I plan to release a new version if mod_security2 next week. If you have enough time to make request changes, then we could merge them before the release. Thanks!

@airween airween added the 2.x Related to ModSecurity version 2.x label May 16, 2025
notroj added 2 commits May 16, 2025 09:59
@notroj
Fix compiler warnings. Reported by GCC with flags:
9d9a727 
  -Wall -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS

Note, e.g. sprintf(digest, "%s%02x", digest, ...) is undefined behaviour because
the destination and source variables overlap, and GCC warnings for this.

acmp.c:258:13: warning: 'acmp_clone_node_no_state' defined but not used [-Wunused-function]
apache2_config.c:806:9: warning: unused variable 'offset' [-Wunused-variable]
apache2_config.c:1886:23: warning: unused variable 'dcfg' [-Wunused-variable]
apache2_config.c:1942:23: warning: unused variable 'dcfg' [-Wunused-variable]
apache2_config.c:2470:23: warning: unused variable 'dcfg' [-Wunused-variable]
apache2_config.c:2538:23: warning: unused variable 'dcfg' [-Wunused-variable]
apache2_util.c:226:11: warning: unused variable 'str' [-Wunused-variable]
apache2_util.c:225:11: warning: unused variable 'saved' [-Wunused-variable]
apache2_util.c:224:11: warning: unused variable 'parse_remote' [-Wunused-variable]
apache2_util.c:223:11: warning: unused variable 'remote' [-Wunused-variable]
msc_status_engine.c:216:17: warning: unused variable 'i' [-Wunused-variable]
msc_status_engine.c:375:55: warning: the address of 'pcre' will always evaluate as 'true' [-Waddress]
msc_crypt.c:67:17: warning: unused variable 'bytes' [-Wunused-variable]
msc_crypt.c:1083:33: warning: variable 'enc' set but not used [-Wunused-but-set-variable]
msc_crypt.c:1090:29: warning: variable 'enc' set but not used [-Wunused-but-set-variable]
/usr/include/bits/stdio2.h:30:10: warning: '__sprintf_chk' argument 5 overlaps destination object 'digest' [-Wrestrict]
msc_json.c:405:11: warning: unused variable 'json_data' [-Wunused-variable]
msc_crypt.c:1097:79: warning: '%s' directive argument is null [-Wformat-overflow=]
msc_logging.c:1144:20: warning: unused variable 'now' [-Wunused-variable]
msc_remote_rules.c:729:19: warning: unused variable 'word' [-Wunused-variable]
msc_remote_rules.c:727:17: warning: unused variable 'tmp' [-Wunused-variable]
msc_remote_rules.c:805:1: warning: control reaches end of non-void function [-Wreturn-type]
msc_tree.c:836:19: warning: unused variable 'ip' [-Wunused-variable]
msc_xml.c:29:44: warning: variable 'entity' set but not used [-Wunused-but-set-variable]
msc_util.c:2627:11: warning: unused variable 'start' [-Wunused-variable]
msc_util.c:2626:17: warning: unused variable 'fd' [-Wunused-variable]
msc_util.c:2624:18: warning: unused variable 'rc' [-Wunused-variable]
msc_util.c:1077:19: warning: array subscript 1 is outside array bounds of 'unsigned char[1]' [-Warray-bounds=]
@notroj
Fix GCC warning in msc_headers_to_buffer():
de1cf63 
In file included from /usr/include/stdio.h:970,
                 from modsecurity.h:18,
                 from msc_util.c:15:
In function 'sprintf',
    inlined from 'msc_headers_to_buffer' at msc_util.c:2331:17:
/usr/include/bits/stdio2.h:30:10: warning: '__sprintf_chk' argument 5 overlaps destination object 'buffer' [-Wrestrict]
   30 |   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   31 |                                   __glibc_objsize (__s), __fmt,
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   32 |                                   __va_arg_pack ());
      |                                   ~~~~~~~~~~~~~~~~~
msc_util.c: In function 'msc_headers_to_buffer':
msc_util.c:2306:64: note: destination object referenced by 'restrict'-qualified argument 1 was declared here
 2306 | int msc_headers_to_buffer(const apr_array_header_t *arr, char *buffer,
      |                                                          ~~~~~~^~~~~~
@notroj notroj force-pushed the v2-gcc-warning-fixes branch from 6919c1c to de1cf63 Compare May 16, 2025 08:59
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
2 Security Hotspots
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@notroj
Copy link
Author

notroj commented May 16, 2025

@airween I went with simpler fixes and repushed. This addresses every warning and make check passes.

airween
airween approved these changes May 17, 2025
View reviewed changes
Copy link
Member

@airween airween left a comment

Choose a reason for hiding this comment

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

LGTM

@airween airween merged commit 38850f9 into owasp-modsecurity:v2/master May 17, 2025
81 of 82 checks passed
@airween
Copy link
Member

airween commented May 17, 2025

Thanks, @notroj!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@airween airween airween approved these changes

Assignees
No one assigned
Labels
2.x Related to ModSecurity version 2.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@notroj @airween