Skip to content

Conversation

martinhsv
Copy link
Contributor

Closes #2357

@martinhsv martinhsv added the 2.x Related to ModSecurity version 2.x label Jun 6, 2023
@marcstern
Copy link

Thanks for the fix Martin.
The strdup can be avoided; we can use directly re_pattern->value:
regex = msc_pregcomp_ex(msr->mp, re_pattern->value, PCRE_DOTALL | PCRE_DOLLAR_ENDONLY, &errptr, &erroffset, msc_pcre_match_limit, msc_pcre_match_limit_recursion);

@martinhsv
Copy link
Contributor Author

Yes, I had noted the possibility of omitting the additional duplication.

However, I opted to leave the pre-existing model in place (by which I mean, to the extent that it creates yet another copy of the string to pass to the regex engine) because of the data models that are in use.

The function expand_macros() populates re_pattern, which is of type msc_string. And in such structures, the 'value_len' can (in principle) be greater than the null-terminated length of 'value'. Also, just thinking from a code-safety perspective, one might wonder whether a code error (now or in the future) might result in an msc_string where the value_len is correct, but the buffer pointed to by 'value', is (incorrectly) not null-terminated (there is no such bug now -- expand_macros() will always correctly null-terminate the returned string, but, still, there can sometimes be merit in safeguarding against some future bug).

Although it makes no functional difference right now, I suspected that it might be slightly cleaner/clearer to logically differentiate the variable as an msc_string from what will be passed to the regex engine. I may revisit that prior to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Related to ModSecurity version 2.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants