From 5e1425fdf0ca4e3d699045f0096e48c1707198cf Mon Sep 17 00:00:00 2001 From: Tom Sommer Date: Sat, 19 Sep 2026 16:13:03 +0200 Subject: [PATCH] Fix memory leak of the macro expanded regex in @rx and @rxGlobal When the parameter of @rx or @rxGlobal contains a macro, the pattern has to be expanded and compiled on every evaluation, so Rx::evaluate() and RxGlobal::evaluate() allocate a temporary Utils::Regex with `new`. That object was only released by an explicit `delete` placed just before the final return statements, which the early `return false` paths never reach: * Rx::evaluate() returns early when the expanded pattern does not compile (re->hasError()) and when the match fails with a PCRE error such as MATCH_LIMIT. * RxGlobal::evaluate() returns early when the match fails with a PCRE error. Every request that hits one of those paths therefore leaks the Regex object together with the compiled pcre2 code and, when available, its JIT compiled counterpart. The operators are evaluated at least once per request and per target variable, so the leak grows without bound in a long running process. The raw pointer is replaced by a std::unique_ptr that owns the macro expanded regex for the whole scope of evaluate(), and the manual delete is removed. The non macro case still uses the pre-compiled m_re owned by the operator, which is not freed by the unique_ptr. Two regression tests are added to operator-rx.json, one per early return in Rx::evaluate(): a macro expanding to a non-compiling pattern and a macro pattern that exceeds SecPcreMatchLimit. Running that file under valgrind reports 346 (96 direct, 250 indirect) bytes in 2 blocks are definitely lost at operator new(unsigned long) by modsecurity::operators::Rx::evaluate(...) (rx.cc:49) before the change and "All heap blocks were freed -- no leaks are possible" after it. No regression test is added for RxGlobal::evaluate(). With PCRE2, which is the default, Regex::searchGlobal() always returns RegexResult::Ok, so that early return is only reachable in --with-pcre (PCRE1) builds; the leak is fixed there all the same. --- src/operators/rx.cc | 10 +-- src/operators/rx_global.cc | 10 +-- test/test-cases/regression/operator-rx.json | 97 +++++++++++++++++++++ 3 files changed, 105 insertions(+), 12 deletions(-) diff --git a/src/operators/rx.cc b/src/operators/rx.cc index de1428e2e5..647dcb963e 100644 --- a/src/operators/rx.cc +++ b/src/operators/rx.cc @@ -38,7 +38,8 @@ bool Rx::init(const std::string &arg, std::string *error) { bool Rx::evaluate(Transaction *transaction, RuleWithActions *rule, const std::string& input, RuleMessage &ruleMessage) { - Regex *re; + const Regex *re; + std::unique_ptr reOwned; if (m_param.empty() && !m_string->m_containsMacro) { return true; @@ -46,7 +47,8 @@ bool Rx::evaluate(Transaction *transaction, RuleWithActions *rule, if (m_string->m_containsMacro) { std::string eparam(m_string->evaluate(transaction)); - re = new Regex(eparam); + reOwned.reset(new Regex(eparam)); + re = reOwned.get(); } else { re = m_re; } @@ -100,10 +102,6 @@ bool Rx::evaluate(Transaction *transaction, RuleWithActions *rule, logOffset(ruleMessage, capture.m_offset, capture.m_length); } - if (m_string->m_containsMacro) { - delete re; - } - if (!captures.empty()) { return true; } diff --git a/src/operators/rx_global.cc b/src/operators/rx_global.cc index a966ed35b3..6064e01bba 100644 --- a/src/operators/rx_global.cc +++ b/src/operators/rx_global.cc @@ -38,7 +38,8 @@ bool RxGlobal::init(const std::string &arg, std::string *error) { bool RxGlobal::evaluate(Transaction *transaction, RuleWithActions *rule, const std::string& input, RuleMessage &ruleMessage) { - Regex *re; + const Regex *re; + std::unique_ptr reOwned; if (m_param.empty() && !m_string->m_containsMacro) { return true; @@ -46,7 +47,8 @@ bool RxGlobal::evaluate(Transaction *transaction, RuleWithActions *rule, if (m_string->m_containsMacro) { std::string eparam(m_string->evaluate(transaction)); - re = new Regex(eparam); + reOwned.reset(new Regex(eparam)); + re = reOwned.get(); } else { re = m_re; } @@ -95,10 +97,6 @@ bool RxGlobal::evaluate(Transaction *transaction, RuleWithActions *rule, logOffset(ruleMessage, capture.m_offset, capture.m_length); } - if (m_string->m_containsMacro) { - delete re; - } - if (captures.size() > 0) { return true; } diff --git a/test/test-cases/regression/operator-rx.json b/test/test-cases/regression/operator-rx.json index 917a67d1b0..6d926dff8a 100644 --- a/test/test-cases/regression/operator-rx.json +++ b/test/test-cases/regression/operator-rx.json @@ -284,5 +284,102 @@ "SecRule ARGS:rxtest \"@rx (w+)+$\" \"id:1,phase:1,pass,t:trim,block\"", "SecRule TX:MSC_PCRE_LIMITS_EXCEEDED \"@streq 1\" \"id:2,phase:1,pass,t:trim,block\"" ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "Testing Operator :: @rx with a macro that expands to a non-compiling pattern", + "client": { + "ip": "200.249.12.31", + "port": 123 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "localhost", + "User-Agent": "curl/7.38.0", + "Accept": "*/*", + "Content-Length": "0", + "Content-Type": "application/x-www-form-urlencoded" + }, + "uri": "/?rxtest=value1", + "method": "HEAD", + "body": [ + "" + ] + }, + "response": { + "headers": { + "Date": "Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified": "Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type": "text/html", + "Content-Length": "8" + }, + "body": [ + "no need." + ] + }, + "expected": { + "debug_log": "Error with regular expression", + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecAction \"id:1,phase:1,pass,nolog,setvar:tx.pattern=(\"", + "SecRule ARGS:rxtest \"@rx %{tx.pattern}\" \"id:2,phase:1,pass,t:trim,block\"" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "Testing Operator :: @rx with a macro pattern and PCRE match limits exceeded", + "client": { + "ip": "200.249.12.31", + "port": 123 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "localhost", + "User-Agent": "curl/7.38.0", + "Accept": "*/*", + "Content-Length": "0", + "Content-Type": "application/x-www-form-urlencoded" + }, + "uri": "/?rxtest=wwwwwwwwwwwwwwwwwwwwwowwwwwwwwwww", + "method": "HEAD", + "body": [ + "" + ] + }, + "response": { + "headers": { + "Date": "Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified": "Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type": "text/html", + "Content-Length": "8" + }, + "body": [ + "no need." + ] + }, + "expected": { + "debug_log": "rx: regex error 'MATCH_LIMIT' for pattern", + "error_log": "Matched \"Operator `StrEq' with parameter `1' against variable `TX:MSC_PCRE_LIMITS_EXCEEDED'", + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecPcreMatchLimit 2", + "SecAction \"id:1,phase:1,pass,nolog,setvar:tx.pattern=(w+)+$\"", + "SecRule ARGS:rxtest \"@rx %{tx.pattern}\" \"id:2,phase:1,pass,t:trim,block\"", + "SecRule TX:MSC_PCRE_LIMITS_EXCEEDED \"@streq 1\" \"id:3,phase:1,pass,t:trim,block\"" + ] } ]