Performance tweaks by edding3000 · Pull Request #3468 · owasp-modsecurity/ModSecurity

edding3000 · 2025-11-20T14:29:05Z

what

Range based for loops with references are already used in this project, but in a few places not.
I am not sure why. I changed this code locations and executed unit tests.
Also use '*' for pointers when using auto keyword, makes it more readable.

why

Improve performance a little bit.

questions

"const auto&" could also be used in many places. Is there a reason why it is not used?

src/engine/lua.cc

src/rules_exceptions.cc

src/transaction.cc

edding3000 · 2025-11-20T18:49:55Z

I was not sure if there was any restriction not to use const, thats why i asked. I could extend the PR with more positions for "const auto&" if you like.

airween · 2025-11-20T19:12:07Z

I was not sure if there was any restriction not to use const, thats why i asked. I could extend the PR with more positions for "const auto&" if you like.

Sure, go! If the tests will be passed, I'll be happy :).

Btw you could review the tests too, may be you will find some lack there too (for eg. you're checking the code...)

with changes from e9bf9ad nothing is modified anymore

sonarqubecloud · 2025-11-21T12:25:00Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

airween · 2026-01-24T10:14:03Z

Hi @edding3000,

sorry for the long delay, it seems like this PR is ready to merge. Just one question: could you merge the current v3/master into this one?

airween · 2026-01-24T10:17:10Z

@edding3000,

also please take a look at the few request changes.

edding3000 · 2026-02-17T19:02:50Z

@airween I merged v3/master. cppcheck fails now. Can you see the reason?

airween · 2026-02-17T19:19:12Z

@airween I merged v3/master. cppcheck fails now. Can you see the reason?

Yes:

2026-02-17T18:48:51.1404560Z warning: src/run_time_string.cc,54,style,functionConst,The member function 'modsecurity::RunTimeString::evaluate' can be made a const function. Making this function 'const' should not cause compiler errors. Even though the function can be made const function technically it may not make sense conceptually. Think about your design and the task of the function first - is it a function that must not change object internal state?

I think cppcheck realized that you changed the loop's variable to const, and figured out the function can't change the object (I don't see the relationship what this const triggered this error), so it expects to make the fn to const - but warns us that it might be wrong.

So what you can try now:

change the function signature to const: std::string RunTimeString::evaluate(Transaction *t, Rule *r) const {
if the build will be failed, rollback that change and add an inline suppression to that line: std::string RunTimeString::evaluate(Transaction *t, Rule *r) { // cppcheck-suppress functionConst
in this case, please add this note to the PR's description

Do you have cppcheck on your local dev environment? It's important to use the latest release, and on Linux only Debian SID contains that version. Here you can see how do we use it in Docker.

sonarqubecloud · 2026-02-18T08:39:08Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

edding3000 · 2026-02-18T09:25:04Z

Thx for your hint. I build cppcheck from source, but it did not find this - perhaps I did not use correct settings. Anyway, warning is fixed now.

Copilot

Pull request overview

This PR makes small performance-oriented C++ adjustments across the ModSecurity codebase, primarily by avoiding unnecessary copies in range-based for loops and tightening const-correctness in a few helper APIs.

Changes:

Convert multiple range-based loops to iterate by const auto& (or equivalent) to avoid copies.
Adjust a few APIs/usages for const-correctness (e.g., RunTimeString::evaluate, RulesExceptions::merge, RuleWithActions::containsTag).
Replace C-style casts with reinterpret_cast for request/response body buffer pointers in regression tests.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
test/unit/unit.cc	Avoids copies in iteration and uses `auto*` for pointer-returning factory calls.
test/regression/regression.cc	Minor cleanup + safer casts to `const unsigned char*` for body buffers.
src/variables/xml.cc	Iteration tweaks while registering XML namespaces from actions.
src/variables/variable.h	Avoids string copy by passing `toOmit` argument by const reference.
src/transaction.cc	Avoids copies when iterating over rule messages and tags during JSON generation.
src/run_time_string.h	Makes `evaluate` overloads `const` (and adjusts inline `evaluate()`).
src/run_time_string.cc	Implements `RunTimeString::evaluate` as `const` and iterates elements as `const`.
src/rules_exceptions.cc	Iteration by const reference + `merge` now takes a const pointer.
src/rule_with_operator.cc	Iterates variables by const reference to avoid pointer copies.
src/rule_with_actions.cc	Iteration tweaks + makes `containsTag` `const`.
src/engine/lua.cc	Iterates variable-value pointers without copying when building Lua tables.
src/collection/backend/in_memory-per_process.cc	Avoids copying map entries during iteration.
src/actions/block.cc	Avoids copying default action smart pointers during iteration.
headers/modsecurity/rules_exceptions.h	Public header signature updated for `merge` (const pointer).
headers/modsecurity/rule_with_actions.h	Public header signature updated for `containsTag` (const method).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-06T12:54:58Z

headers/modsecurity/rules_exceptions.h

@@ -51,7 +51,7 @@ class RulesExceptions {
    bool addRange(int a, int b);
    bool addNumber(int a);
    bool contains(int a);


Changing RulesExceptions::merge to take a const RulesExceptions* alters the exported C++ symbol in an installed public header, which is an ABI-breaking change for downstream binaries linked against an older libmodsecurity. To keep backward compatibility, preserve the original signature and add a const overload (or keep the old signature and treat the parameter as const internally).

Suggested change

bool contains(int a);

bool contains(int a);

bool merge(RulesExceptions *from) {

return merge(static_cast<const RulesExceptions *>(from));

}

@edding3000 I think this suggestion makes sense.

Copilot · 2026-03-06T12:54:58Z

headers/modsecurity/rule_with_actions.h

@@ -80,7 +80,7 @@ class RuleWithActions : public Rule {

    std::vector<actions::Action *> getActionsByName(const std::string& name,
        const Transaction *t);


Making RuleWithActions::containsTag const changes the mangled name in an installed public header, which can break ABI for external connectors compiled against a previous version of the library. Consider keeping the existing non-const method for compatibility and adding a const overload (with the non-const delegating to the const implementation).

Suggested change

const Transaction *t);

const Transaction *t);

bool containsTag(const std::string& name, Transaction *t)

{

return static_cast<const RuleWithActions *>(this)->containsTag(name, t);

}

This suggestion makes sense.

Copilot · 2026-03-06T12:54:58Z

src/run_time_string.h

+    std::string evaluate(Transaction *t) const;
+    std::string evaluate(Transaction *t, Rule *r) const;
+    std::string evaluate() const{
        return evaluate(NULL);
    }


Minor formatting/style: evaluate() const{ is missing a space before {, and this still uses NULL in C++ code. Prefer consistent spacing (const {) and consider nullptr to avoid integral null pointer constants.

This one also makes sense.

airween

@edding3000 thanks, please take a look at Copilot reviews and my suggestions below.

airween · 2026-03-06T19:37:51Z

headers/modsecurity/rules_exceptions.h

@@ -51,7 +51,7 @@ class RulesExceptions {
    bool addRange(int a, int b);
    bool addNumber(int a);
    bool contains(int a);


@edding3000 I think this suggestion makes sense.

airween · 2026-03-06T19:38:22Z

src/run_time_string.h

+    std::string evaluate(Transaction *t) const;
+    std::string evaluate(Transaction *t, Rule *r) const;
+    std::string evaluate() const{
        return evaluate(NULL);
    }


This one also makes sense.

airween · 2026-03-06T19:40:44Z

headers/modsecurity/rule_with_actions.h

@@ -80,7 +80,7 @@ class RuleWithActions : public Rule {

    std::vector<actions::Action *> getActionsByName(const std::string& name,
        const Transaction *t);


This suggestion makes sense.

edding3000 added 2 commits November 20, 2025 15:24

Use reference in range-based for loop

907d7f8

Use explicit '*' with auto for pointers

12671d1

airween requested changes Nov 20, 2025

View reviewed changes

src/engine/lua.cc Outdated Show resolved Hide resolved

src/rules_exceptions.cc Outdated Show resolved Hide resolved

src/rules_exceptions.cc Outdated Show resolved Hide resolved

src/transaction.cc Outdated Show resolved Hide resolved

src/transaction.cc Outdated Show resolved Hide resolved

edding3000 added 6 commits November 21, 2025 12:35

Add const modifier to range-based for loops

e9bf9ad

Use const pointers

e4f3feb

Use explicit '*' with auto for pointers in tests

c7b2fdf

Remove c-cast from test

ef42d05

Switch to const string reference as nothing is modified

72b46c5

Add const to parameter

c9f2f35

with changes from e9bf9ad nothing is modified anymore

airween added the 3.x Related to ModSecurity version 3.x label Jan 24, 2026

Merge branch 'v3/master' into performance_tweaks

897181e

Fix const correctness warning

0f8c668

edding3000 force-pushed the performance_tweaks branch from 2539f80 to 0f8c668 Compare February 18, 2026 08:35

airween requested a review from Copilot March 6, 2026 12:49

Copilot started reviewing on behalf of airween March 6, 2026 12:50 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

airween requested changes Mar 6, 2026

View reviewed changes

		@@ -80,7 +80,7 @@ class RuleWithActions : public Rule {

		std::vector<actions::Action *> getActionsByName(const std::string& name,
		const Transaction *t);

Conversation

edding3000 commented Nov 20, 2025

what

why

questions

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edding3000 commented Nov 20, 2025

Uh oh!

airween commented Nov 20, 2025

Uh oh!

sonarqubecloud bot commented Nov 21, 2025

Quality Gate passed

Uh oh!

airween commented Jan 24, 2026

Uh oh!

airween commented Jan 24, 2026

Uh oh!

edding3000 commented Feb 17, 2026

Uh oh!

airween commented Feb 17, 2026

Uh oh!

sonarqubecloud bot commented Feb 18, 2026

Quality Gate passed

Uh oh!

edding3000 commented Feb 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

airween Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

airween Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

airween Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

airween left a comment

Choose a reason for hiding this comment

Uh oh!

airween Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

airween Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

airween Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants