Skip to content
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.

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

Adjust reference to modsecurity::utils::string::VALID_HEX #3243

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

eduar-hte
Copy link
Contributor

what

Fix build after integrating PR #3231 & #3233 because of reference to modsecurity::utils::string::VALID_HEX in pm.cc's parse_pm_content.

why

These two PRs were independent of each other, but when we added a reference to VALID_HEX to simplify the code, we introduced an incompatibility between both PRs because PR #3231 moved VALID_HEX into the namespace modsecurity::utils::string (alongside all other helper functions in src/utils/string.h) and adjusted all call sites to this function (previously a macro). However, in that branch the call added to parse_pm_content that we introduced later in PR #3233 was not present and thus was not adjusted.

- This function (previously a #define) was previously in the global
  namespace and was moved into modsecurity::utils::string in commit
  a6d64bf.
@airween
Copy link
Member

airween commented Aug 28, 2024

Ahm, thanks - meanwhile I also started to work on this fix 😄

Btw I don't understand the situation, because the PR's check were successful, but later I get reports about failed checks.

Seems like now everything is fine.

I also added two new tests (a positive and a negative) - do you think that would be useful to check the @pm's correct behavior? If yes, could you pick up those modifications?

@eduar-hte
Copy link
Contributor Author

Btw I don't understand the situation, because the PR's check were successful, but later I get reports about failed checks.

VALID_HEX used to live in the global namespace because it was a macro (#define), and in PR #3231 I converted it to a function (in order to continue removing preprocessor dependencies), and the function was included in the namespace modsecurity::utils::string (alongside the rest of the functions in src/utils/string.h, which is where VALID_HEX was defined).

You can see in commit a6d64bf that I adjusted the source files that were using this macro because the macro/function was no longer in the global namespace.

When we introduced the use of VALID_HEX in PR #3233 to simplify the code in parse_pm_content, a new reference to this macro/function was introduced, which needed to be adjusted to the namespace change.

I also added two new tests (a positive and a negative) - do you think that would be useful to check the @pm's correct behavior? If yes, could you pick up those modifications?

ok, I'll do that and push an update to this PR.

@eduar-hte
Copy link
Contributor Author

ok, I'll do that and push an update to this PR.

done. I added your repository as a remote in my local repository in order to cherry-pick your commit and preserve your authorship. :-)

Copy link

sonarcloud bot commented Aug 28, 2024

@airween
Copy link
Member

airween commented Aug 28, 2024

in PR #3231 I converted it to a function

Oh, thanks - I didn't noticed that. I looked up for that and found as macro so that was confused.

Never mind, thanks for all again. If the tests will passed I'm going to merge this PR.

@airween airween merged commit f180e64 into owasp-modsecurity:v3/master Aug 28, 2024
49 checks passed
airween added a commit to airween/ModSecurity that referenced this pull request Aug 28, 2024
@eduar-hte eduar-hte deleted the valid-hex-fix branch August 28, 2024 14:08
@airween
Copy link
Member

airween commented Aug 28, 2024

Hmm... after this PR was merged, I receive notifications about failed tests, especially on macOS, with SecRemoteRules - like this one.

Never mind, may be I forget to update that branch where I get from the notifications.

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

Successfully merging this pull request may close these issues.

2 participants