fix: strlen related variables should be of type size_t#3503
fix: strlen related variables should be of type size_t#3503fzipi wants to merge 2 commits intoowasp-modsecurity:v2/masterfrom
Conversation
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
There was a problem hiding this comment.
Pull request overview
This PR updates several strlen()-related variables and function parameters to use size_t (instead of narrower integer types) to address type-safety and platform-specific warnings (notably on Windows) in the Apache 2.x connector code.
Changes:
- Converted multiple length variables (derived from
strlen) fromint/unsigned shorttosize_t. - Updated
parse_pm_contentdeclaration/definition to acceptsize_t op_len, and propagated some call-site length type updates. - Adjusted several internal helper length variables across request processing, logging/sanitization, multipart handling, and hashing codepaths.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| apache2/re_variables.c | Uses size_t for request-line length in FULL_REQUEST generation. |
| apache2/re_operators.c | Updates several operator parameter length variables to size_t and propagates into parsing paths. |
| apache2/msc_util.h | Changes parse_pm_content prototype to take size_t op_len. |
| apache2/msc_util.c | Changes parse_pm_content definition and updates internal length usage. |
| apache2/msc_status_engine.c | Changes base32 helper function parameters/locals to size_t where derived from strlen. |
| apache2/msc_multipart.c | Updates some strlen-related locals to size_t in multipart parsing helpers. |
| apache2/msc_logging.c | Updates several sanitization loop counters/offsets to size_t. |
| apache2/msc_crypt.c | Changes bytes accumulator (based on strlen) to size_t. |
| apache2/apache2_util.c | Uses size_t for message length derived from strlen in internal logging helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ret = verify_gsb(gsb, msr, match, match_length); | ||
|
|
There was a problem hiding this comment.
match_length/canon_length were changed to size_t, but verify_gsb still takes an unsigned int match_length (see its signature) and is called with a size_t argument. This can truncate lengths > UINT_MAX and typically triggers MSVC warnings. Update verify_gsb to take size_t (or apr_size_t) and adjust the apr_md5_update call accordingly.
| op_len = strlen(buf); | ||
| processed = apr_pstrdup(rule->ruleset->mp, parse_pm_content(buf, op_len, rule, error_msg)); |
There was a problem hiding this comment.
op_len in msre_op_pmFromFile_param_init is still declared as unsigned short int but is assigned from strlen(buf) and passed to parse_pm_content (now size_t). This truncates lengths over 65535 and can reintroduce the Windows warnings this PR is targeting. Change op_len to size_t and update any dependent logic accordingly.
…tion Keep masking offset as signed int in msc_logging.c to prevent negative pos from disabling sanitization via unsigned comparison. Propagate size_t to loop indices, function signatures, and format specifiers. Revert size_t where downstream consumers are all int.
|
1 participant
what
strlenreturns asize_tvariable, that depends on the architecture and environmentwhy
references