From 7ae9b073d073f9d7433fd0a228800184331c2f5c Mon Sep 17 00:00:00 2001 From: Tom Sommer Date: Sat, 19 Sep 2026 16:14:40 +0200 Subject: [PATCH] Fix memory leak in setvar when the current value is not numeric The += and -= branch of SetVar::evaluate() resolves the current value of the target variable with m_variable->evaluate(), which fills a std::vector whose elements are owned by the caller. The elements were deleted only after stoi() had converted the first one: value = stoi(l[0]->getValue()); for (auto &i : l) { delete i; } stoi() throws std::invalid_argument when the current value does not start with a number and std::out_of_range when it does not fit in an int. Both exceptions are swallowed by the surrounding catch (...), which only resets value to 0, so the delete loop is skipped and every VariableValue in the vector is leaked. This happens on any request that increments a variable holding a non numeric value, for example a rule set that does setvar:tx.score=+1 on a tx.score that was previously assigned a string, so the leak is per request and unbounded in a long running process. The value is now copied into a local std::string before the vector is emptied, and stoi() is called afterwards. The behaviour is unchanged: the conversion still runs inside the same try block and value stays 0 when it throws. A regression test is added to collection-tx.json: tx.something is first set to "not_a_number" and then incremented by 10, which must yield 10. Running collection-tx.json under valgrind reports 152 bytes in 1 blocks are definitely lost at operator new(unsigned long) by modsecurity::collection::backend::InMemoryPerProcess::resolveMultiMatches(...) by modsecurity::variables::Tx_DynamicElement::evaluate(...) by modsecurity::actions::SetVar::evaluate(...) (set_var.cc:105) before the change and "All heap blocks were freed -- no leaks are possible" after it. --- src/actions/set_var.cc | 5 +- test/test-cases/regression/collection-tx.json | 62 +++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/src/actions/set_var.cc b/src/actions/set_var.cc index 70c07d065d..c9e9a22d26 100644 --- a/src/actions/set_var.cc +++ b/src/actions/set_var.cc @@ -106,10 +106,13 @@ bool SetVar::evaluate(RuleWithActions *rule, Transaction *t) { if (l.size() == 0) { value = 0; } else { - value = stoi(l[0]->getValue()); + /* copy: the VariableValue objects are deleted below */ + // cppcheck-suppress redundantCopyLocalConst + const std::string currentValue(l[0]->getValue()); for (auto &i : l) { delete i; } + value = stoi(currentValue); } } catch (...) { value = 0; diff --git a/test/test-cases/regression/collection-tx.json b/test/test-cases/regression/collection-tx.json index 587fb33053..9a03132079 100644 --- a/test/test-cases/regression/collection-tx.json +++ b/test/test-cases/regression/collection-tx.json @@ -347,5 +347,67 @@ "SecRule REQUEST_HEADERS:Cookie|REQUEST_HEADERS:Cookie2 \"@contains ookie\" \"id:4,t:lowercase,t:removewhitespace,multimatch,setvar:tx.anomaly_score=+%{tx.critical_anomaly_score}\"", "SecRule TX \"@contains to_test\" \"id:100\"" ] + }, + { + "enabled": 1, + "version_min": 300000, + "version_max": 0, + "title": "Testing collection :: TX (6/n)", + "client": { + "ip": "200.249.12.31", + "port": 2313 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "User-Agent": "Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 (.NET CLR 3.5.30729)", + "Accept": "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8", + "Accept-Language": "en-us,en;q=0.5", + "Accept-Encoding": "gzip,deflate", + "Accept-Charset": "ISO-8859-1,utf-8;q=0.7,*;q=0.7", + "Keep-Alive": "300", + "Connection": "keep-alive", + "Cookie": "PHPSESSID=rAAAAAAA2t5uvjq435r4q7ib3vtdjq120 - cookie I", + "Cookie2": "PHPSESSID=rAAAAAAA2t5uvjq435r4q7ib3vtdjq120 - cookie II", + "Pragma": "no-cache", + "Cache-Control": "no-cache", + "Content-Length": "0" + }, + "uri": "/test.pl?param1= test ¶m2=test2", + "method": "GET", + "http_version": 1.1, + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Type": "text/xml; charset=utf-8\n\r", + "Content-Length": "384" + }, + "body": [ + "\n\r", + "\n\r", + " \n\r", + " \n\r", + " string\n\r", + " \n\r", + " \n\r", + "\n\r" + ] + }, + "expected": { + "debug_log": "Target value: \"10\" \\(Variable: TX:something\\)", + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecRule REQUEST_HEADERS:Cookie \"@contains PHPSESSID\" \"id:1,t:none,setvar:tx.something=not_a_number\"", + "SecRule REQUEST_HEADERS:Cookie \"@contains PHPSESSID\" \"id:2,t:none,setvar:tx.something=+10\"", + "SecRule TX \"@contains to_test\" \"id:3,t:lowercase,t:none\"" + ] } ]