From f62dc287c9998c87e943cb7a7143160d8443b315 Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Wed, 28 Dec 2016 01:38:43 -0300 Subject: [PATCH] Uses pointer instead of std::string copies while applying transformations --- headers/modsecurity/rule.h | 2 +- src/rule.cc | 64 ++++++++++++++++++++++---------------- src/variables/xml.cc | 7 +++-- 3 files changed, 43 insertions(+), 30 deletions(-) diff --git a/headers/modsecurity/rule.h b/headers/modsecurity/rule.h index 6e5f5091..8809a859 100644 --- a/headers/modsecurity/rule.h +++ b/headers/modsecurity/rule.h @@ -55,7 +55,7 @@ class Rule { std::vector getFinalVars(Transaction *trasn); void executeActionsAfterFullMatch(Transaction *trasn, bool containsDisruptive, RuleMessage *ruleMessage); - std::vector executeSecDefaultActionTransofrmations( + std::vector executeSecDefaultActionTransofrmations( Transaction *trasn, const std::string &value, bool multiMatch); bool executeOperatorAt(Transaction *trasn, std::string key, std::string value); diff --git a/src/rule.cc b/src/rule.cc index 49991597..33cdf09e 100644 --- a/src/rule.cc +++ b/src/rule.cc @@ -284,12 +284,15 @@ bool Rule::executeOperatorAt(Transaction *trasn, std::string key, // FIXME: this should be a list instead of a vector, keeping the but // of v2 alive. -std::vector Rule::executeSecDefaultActionTransofrmations( - Transaction *trasn, const std::string &value2, bool multiMatch) { +std::vector Rule::executeSecDefaultActionTransofrmations( + Transaction *trasn, const std::string &in, bool multiMatch) { int none = 0; int transformations = 0; - std::vector ret; - std::string value = std::string(value2); + std::vector ret; + + std::string *value = new std::string(in); + std::string *newValue = NULL; + if (multiMatch == true) { ret.push_back(value); @@ -309,49 +312,51 @@ std::vector Rule::executeSecDefaultActionTransofrmations( for (Action *a : trasn->m_rules->defaultActions[this->phase]) { if (a->action_kind \ == actions::Action::RunTimeBeforeMatchAttemptKind) { - std::string oldValue = std::string(value); - if (multiMatch) { - oldValue = ret.back(); - } - std::string newValue = a->evaluate(oldValue, trasn); + newValue = new std::string(a->evaluate(*value, trasn)); + if (multiMatch == true) { - if (newValue != oldValue) { + if (*newValue != *value) { ret.push_back(newValue); + } else { + delete value; } } else { - value = newValue; + delete value; } - + value = newValue; trasn->debug(9, "(SecDefaultAction) T (" + \ std::to_string(transformations) + ") " + \ a->m_name + ": \"" + \ - utils::string::limitTo(80, newValue) +"\""); + utils::string::limitTo(80, *value) +"\""); transformations++; } } } + + for (Action *a : this->m_actionsRuntimePre) { if (none == 0) { - std::string oldValue = std::string(value); - if (multiMatch) { - oldValue = ret.back(); - } + newValue = new std::string(a->evaluate(*value, trasn)); - std::string newValue = a->evaluate(oldValue, trasn); if (multiMatch == true) { - if (newValue != oldValue) { + if (*value != *newValue) { ret.push_back(newValue); + value = newValue; + } else { + delete value; } } else { - value = newValue; + delete value; } + value = newValue; trasn->debug(9, " T (" + \ std::to_string(transformations) + ") " + \ a->m_name + ": \"" + \ - utils::string::limitTo(80, newValue) + "\""); + utils::string::limitTo(80, *value) + "\""); + transformations++; } if (a->m_isNone) { @@ -364,8 +369,8 @@ std::vector Rule::executeSecDefaultActionTransofrmations( trasn->debug(9, "multiMatch is enabled. " \ + std::to_string(ret.size()) + \ " values to be tested."); - for (const std::string &a : ret) { - trasn->debug(9, " - " + a); + for (const std::string *a : ret) { + trasn->debug(9, " - " + *a); } } else { ret.push_back(value); @@ -414,6 +419,8 @@ std::vector Rule::getFinalVars( trasn->debug(9, "Variable: " + *key + " is part of the exclusion list, skipping..."); #endif + delete v; + v = NULL; continue; } @@ -431,6 +438,8 @@ std::vector Rule::getFinalVars( } } if (ignoreVariable) { + delete v; + v = NULL; continue; } @@ -448,6 +457,8 @@ std::vector Rule::getFinalVars( } } if (ignoreVariable) { + delete v; + v = NULL; continue; } @@ -574,16 +585,16 @@ bool Rule::evaluate(Transaction *trasn) { const std::string value = *(v->m_value); const std::string key = *(v->m_key); - std::vector values; + std::vector values; bool multiMatch = getActionsByName("multimatch").size() > 0; values = executeSecDefaultActionTransofrmations(trasn, value, multiMatch); - for (const std::string &valueTemp : values) { + for (const std::string *valueTemp : values) { bool ret; - ret = executeOperatorAt(trasn, key, valueTemp); + ret = executeOperatorAt(trasn, key, *valueTemp); if (ret == true) { ruleMessage.m_match = resolveMatchMessage(key, value); updateMatchedVars(trasn, key, value); @@ -591,6 +602,7 @@ bool Rule::evaluate(Transaction *trasn) { &containsDisruptive, &ruleMessage); globalRet = true; } + delete valueTemp; } } diff --git a/src/variables/xml.cc b/src/variables/xml.cc index dce6de46..0124afec 100644 --- a/src/variables/xml.cc +++ b/src/variables/xml.cc @@ -123,9 +123,10 @@ void XML::evaluateInternal(Transaction *t, content = reinterpret_cast( xmlNodeGetContent(nodes->nodeTab[i])); if (content != NULL) { - // FIXME: Memory leak - l->push_back(new collection::Variable(&m_name, - new std::string(content))); + collection::Variable *var = new collection::Variable(&m_name, + new std::string(content)); + var->m_dynamic_value = true; + l->push_back(var); xmlFree(content); } }