Fix GCC -Wall warnings on Fedora/RHEL builds#3567
Open
mikelolasagasti wants to merge 20 commits into
Open
Conversation
Use vector::size_type for the loop index when iterating over resolved
variable values, avoiding a signed/unsigned comparison with size().
Fixes GCC -Wsign-compare:
../headers/modsecurity/anchored_set_variable_translation_proxy.h:46:31:
warning: comparison of integer expressions of different signedness:
'int' and 'std::vector<const modsecurity::VariableValue*>::size_type'
{aka 'long unsigned int'} [-Wsign-compare]
Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Mark intervention::{reset,clean,freeUrl,freeLog,free} as inline so they
are not emitted as unused static functions in every translation unit
that includes intervention.h.
Fixes GCC -Wunused-function:
../headers/modsecurity/intervention.h:39:17: warning: 'void
modsecurity::intervention::clean(...)' defined but not used
[-Wunused-function]
../headers/modsecurity/intervention.h:59:17: warning: 'void
modsecurity::intervention::free(...)' defined but not used
[-Wunused-function]
Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Use std::string::size_type for the length limit so it matches str.length() and assign()'s count parameter. Fixes GCC -Wsign-compare: ../src/utils/string.h:94:22: warning: comparison of integer expressions of different signedness: 'std::__cxx11::basic_string<char>::size_type' and 'int' [-Wsign-compare] Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Initialize the Variable base class before m_dictElement, matching member declaration order. Fixes GCC -Wreorder: ../src/variables/variable.h:635:17: warning: 'm_dictElement' will be initialized after base 'Variable' [-Wreorder] Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Initialize the Variable base class before m_r and m_regex, matching member declaration order. Fixes GCC -Wreorder: ../src/variables/variable.h:648:17: warning: 'm_regex' will be initialized after base 'Variable' [-Wreorder] Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Use std::string::size_type for the position returned by find(), avoiding comparison with std::string::npos as a signed int. Fixes GCC -Wsign-compare: actions/init_col.cc:37:19: warning: comparison of integer expressions of different signedness [-Wsign-compare] Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Cast parsed highlight offsets to size_t before comparing with content and variable lengths. Fixes GCC -Wsign-compare: modsecurity.cc:271:30: warning: comparison of integer expressions of different signedness [-Wsign-compare] modsecurity.cc:350:30: warning: comparison of integer expressions of different signedness [-Wsign-compare] Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Initialize the Operator base class before m_re, matching member declaration order. Fixes GCC -Wreorder: ../src/operators/rx.h:62:12: warning: 'm_re' will be initialized after base 'Operator' [-Wreorder] ../src/operators/rx_global.h:62:12: warning: 'm_re' will be initialized after base 'Operator' [-Wreorder] Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Use size_t for the rule loop index when comparing against rules->size(). Fixes GCC -Wsign-compare: rules_set.cc:147:23: warning: comparison of integer expressions of different signedness [-Wsign-compare] Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Drop dead `char z = name.at(0)` assignments from RUN_TIME_VAR_* grammar actions; the first character was never used. Fixes GCC -Wunused-variable: seclang-parser.yy:2591:14: warning: unused variable 'z' [-Wunused-variable] Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Align integer types for bit/count comparisons in CPTAddElement, remove an unused variable, and compute CIDR slash position safely in TreeAddIP. Fixes -Wsign-compare and -Wunused-variable in CPTAddElement and TreeAddIP. Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Use sizeof(unicode) for the hex snprintf buffer size and size_t for hex digit length and loop indices. Fixes -Wsizeof-pointer-memaccess and -Wsign-compare in actions/transformations/utf8_to_unicode.cc. Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Use std::string::size_type for the copy loop index to match copy. Fixes GCC -Wsign-compare: actions/transformations/html_entity_decode.cc:157:28: warning: comparison of integer expressions of different signedness [-Wsign-compare] Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Cast the in-place compression length to std::string::size_type before comparing with value.length(). Fixes GCC -Wsign-compare: actions/transformations/compress_whitespace.cc:42:34: warning: comparison of integer expressions of different signedness [-Wsign-compare] Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Compare upload file count against SecUploadFileLimit using uint32_t on both sides. Fixes GCC -Wsign-compare: request_body_processor/multipart.cc:561:26: warning: comparison of integer expressions of different signedness [-Wsign-compare] Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Use uint64_t for the scan index to match input_length. Fixes -Wsign-compare in operators/validate_url_encoding.cc:36 and :38. Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Use size_t for the loop index when scanning characters before '#'. Fixes GCC -Wsign-compare: operators/pm_from_file.cc:36:27: warning: comparison of integer expressions of different signedness [-Wsign-compare] Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Use size_t for the rule index when checking for duplicate rule IDs. Fixes GCC -Wsign-compare: parser/driver.cc:111:27: warning: comparison of integer expressions of different signedness [-Wsign-compare] Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Initialize TransactionAnchoredVariables before m_logCbData in the member initializer list. Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
Use uint64_t for reqbodyNoFilesLength when comparing against SecRequestBodyNoFilesLimit. Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
mikelolasagasti
force-pushed
the
fix-warnings
branch
from
905e100 to
5a76e19
Compare
Author
|
Fixed 3 SonarCloud warnings. The "10.9% Duplication on New Code (required ≤ 3%)" Quality Gate is out of the scope of this PR. |
|
Member
|
Hi @mikelolasagasti, could you pick these changes or update your branch? Thanks! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces GCC -Wall (Fedora/RHEL-style) build warnings across ModSecurity by addressing common warning sources (initializer reordering, signed/unsigned mismatches, unused variables, and safer string/offset handling) in core transaction/rules logic, operators, parser, utilities, and public headers.
Changes:
- Reordered constructor/base/member initializers and added explicit default member initialization to silence
-Wreorder/uninitialized warnings. - Replaced several
intloop/index/length variables withsize_t/std::string::size_type/ unsigned types and added casts where needed to silence sign/size warnings. - Fixed a correctness issue in
utf8_to_unicode(snprintfbuffer size) and made IP parsing more robust when no CIDR slash is present.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/variables/variable.h | Reorders initializer lists to match declaration order (-Wreorder). |
| src/utils/string.h | Uses std::string::size_type for truncation length. |
| src/utils/msc_tree.cc | Fixes signed/unsigned warnings and avoids strchr(...)-buffer when no / is present. |
| src/transaction.cc | Fixes base/member init order and widens request-body length type. |
| src/rules_set.cc | Uses size_t for rule vector indexing. |
| src/request_body_processor/multipart.cc | Fixes signed/unsigned comparison for upload file limit. |
| src/parser/seclang-parser.yy | Removes unused local variables in grammar actions. |
| src/parser/driver.cc | Uses size_t for rule vector indexing. |
| src/operators/validate_url_encoding.cc | Uses uint64_t index to match input_length type. |
| src/operators/rx.h | Moves m_re initialization to in-class default initializer. |
| src/operators/rx_global.h | Moves m_re initialization to in-class default initializer. |
| src/operators/pm_from_file.cc | Uses size_t loop index when scanning comment prefix. |
| src/modsecurity.cc | Adds explicit casts for offset bounds checks when parsing highlight data. |
| src/actions/transformations/utf8_to_unicode.cc | Fixes snprintf size argument and uses size_t for derived lengths/loops. |
| src/actions/transformations/html_entity_decode.cc | Uses std::string::size_type for loop index. |
| src/actions/transformations/compress_whitespace.cc | Fixes pointer-diff type warning via explicit cast and clarifies changed type. |
| src/actions/init_col.cc | Uses std::string::size_type for find() result. |
| headers/modsecurity/intervention.h | Switches header-defined helpers from static to inline to reduce warnings. |
| headers/modsecurity/anchored_set_variable_translation_proxy.h | Uses vector size_type for indexing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+36
to
39
| for (size_t i = 0; i < pos; i++) { | ||
| if (!std::isspace(s[i])) { | ||
| return false; | ||
| } |
Comment on lines
+271
to
274
| if (static_cast<size_t>(stoi(startingAt)) >= len) { | ||
| *err = "Offset is out of the content limits."; | ||
| return -1; | ||
| } |
Comment on lines
+350
to
353
| if (static_cast<size_t>(stoi(startingAt)) >= varValue.size()) { | ||
| *err = "Offset is out of the variable limits."; | ||
| return -1; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
-Wall, hardening, ...).why
references