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
Open
wants to merge 2 commits into
base: v2/master
Choose a base branch
from
Open

Fix compiler warnings from GCC #3372

wants to merge 2 commits into from

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
apache2/acmp.c Outdated
@@ -251,6 +251,7 @@ static void acmp_add_node_to_parent(acmp_node_t *parent, acmp_node_t *child) {
}
}

#if 0
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch. It seems like we don't use this function at all - do we want to keep this unused code?

apache2/msc_xml.c Outdated
@@ -132,29 +132,19 @@ static void msc_xml_on_characters(void *ctx, const xmlChar *ch, int len) {
}


static xmlParserInputBufferPtr
Copy link
Member

Choose a reason for hiding this comment

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

Have you removed this function because it always returned a NULL?

Copy link
Author

Choose a reason for hiding this comment

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

Ah, this is a mistake too. Since entity was set but never used in xml_init I assumed this was safe to remove, but xmlParserInputBufferCreateFilenameDefault should still be called here. I'll restore it and fix the warning another way.

apache2/re_operators.c Outdated
@@ -4231,11 +4231,6 @@ static int msre_op_fuzzy_hash_init(msre_rule *rule, char **error_msg)
return 1;
#endif
return 1;

invalid_parameters:
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this label is still referenced here and here. Do we really want to remove this?

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I missed that because I don't have that feature enabled. I'll restore this.

@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 from 82c5c3a to 64d4418 Compare May 12, 2025 14:59
notroj added 2 commits May 12, 2025 16:00
@notroj
Fix compiler warnings. Reported by GCC with flags:
67293dc 
  -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
Remove unused acmp_clone_node_no_state() function entirely.
6919c1c
@notroj notroj force-pushed the v2-gcc-warning-fixes branch from 64d4418 to 6919c1c Compare May 12, 2025 15:02
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
C 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 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 😃.

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

@airween airween airween requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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