From a7c7b3f4c37b9235ebdd2ac8cf410ba50b9a3ec9 Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Mon, 6 Apr 2020 20:27:10 -0300 Subject: [PATCH] Moves default actions to be part of the rules --- CHANGES | 10 ++++++++++ headers/modsecurity/rules.h | 2 ++ headers/modsecurity/rules_set_properties.h | 22 ---------------------- src/actions/block.cc | 2 +- src/parser/seclang-parser.cc | 4 ++-- src/parser/seclang-parser.yy | 4 ++-- src/rule_with_actions.cc | 4 ++-- src/rules_set_phases.cc | 7 +++++++ 8 files changed, 26 insertions(+), 29 deletions(-) diff --git a/CHANGES b/CHANGES index 3759dfde..d8156478 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,16 @@ v3.x.y - YYYY-MMM-DD (to be released) ------------------------------------- + - Using std::shared_ptr instead of generates its own references counters + for Rules and related. + [@zimmerle] + - Better handle shared_pointers on messages aiming for better performance. + [@zimmerle] + - Better handle memory usage on transformations aiming for better + performance. + [@zimmerle] + - Coding refactoring on the Rule class. The Rule class is now refactored + into RuleWithOperator, RuleWithActions, and RuleUnconditional. - EXPERIMENTAL: Add new transformation call phpArgsNames [Issue #2387 - @marshal09] - Having ARGS_NAMES, variables proxied diff --git a/headers/modsecurity/rules.h b/headers/modsecurity/rules.h index 1aaa65b5..3c8ccff4 100644 --- a/headers/modsecurity/rules.h +++ b/headers/modsecurity/rules.h @@ -84,6 +84,8 @@ class Rules { std::shared_ptr operator[](int index) const { return m_rules[index]; } std::shared_ptr at(int index) const { return m_rules[index]; } + std::vector > m_defaultActions; + std::vector > m_rules; }; diff --git a/headers/modsecurity/rules_set_properties.h b/headers/modsecurity/rules_set_properties.h index 13519fcb..b48d3682 100644 --- a/headers/modsecurity/rules_set_properties.h +++ b/headers/modsecurity/rules_set_properties.h @@ -201,16 +201,6 @@ class RulesSetProperties { RulesSetProperties &operator =(const RulesSetProperties &r) = delete; ~RulesSetProperties() { - int i = 0; - - for (i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) { - std::vector > *tmp = \ - &m_defaultActions[i]; - while (tmp->empty() == false) { - tmp->pop_back(); - } - } - delete m_debugLog; delete m_auditLog; } @@ -410,16 +400,6 @@ class RulesSetProperties { to->m_responseBodyTypeToBeInspected.m_set = true; } - for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) { - std::vector > *actions_from = \ - &from->m_defaultActions[i]; - std::vector > *actions_to = \ - &to->m_defaultActions[i]; - for (size_t j = 0; j < actions_from->size(); j++) { - actions_to->push_back(actions_from->at(j)); - } - } - if (to->m_auditLog) { std::string error; to->m_auditLog->merge(from->m_auditLog, &error); @@ -481,8 +461,6 @@ class RulesSetProperties { ConfigString m_uploadTmpDirectory; ConfigString m_secArgumentSeparator; ConfigString m_secWebAppId; - std::vector > \ - m_defaultActions[modsecurity::Phases::NUMBER_OF_PHASES]; ConfigUnicodeMap m_unicodeMapTable; }; diff --git a/src/actions/block.cc b/src/actions/block.cc index bde5e634..a6b91480 100644 --- a/src/actions/block.cc +++ b/src/actions/block.cc @@ -33,7 +33,7 @@ bool Block::evaluate(RuleWithActions *rule, Transaction *transaction, std::shared_ptr rm) { ms_dbg_a(transaction, 8, "Marking request as disruptive."); - for (auto &a : transaction->m_rules->m_defaultActions[rule->getPhase()]) { + for (auto &a : transaction->m_rules->m_rulesSetPhases[rule->getPhase()]->m_defaultActions) { if (a->isDisruptive() == false) { continue; } diff --git a/src/parser/seclang-parser.cc b/src/parser/seclang-parser.cc index 9e8f5f86..67fbc497 100644 --- a/src/parser/seclang-parser.cc +++ b/src/parser/seclang-parser.cc @@ -2437,7 +2437,7 @@ namespace yy { YYERROR; } - if (!driver.m_defaultActions[definedPhase].empty()) { + if (!driver.m_rulesSetPhases[definedPhase]->m_defaultActions.empty()) { std::stringstream ss; ss << "SecDefaultActions can only be placed once per phase and configuration context. Phase "; ss << secRuleDefinedPhase; @@ -2447,7 +2447,7 @@ namespace yy { } for (actions::Action *a : checkedActions) { - driver.m_defaultActions[definedPhase].push_back( + driver.m_rulesSetPhases[definedPhase]->m_defaultActions.push_back( std::unique_ptr(a)); } diff --git a/src/parser/seclang-parser.yy b/src/parser/seclang-parser.yy index 75ee7c08..6f49bbe3 100644 --- a/src/parser/seclang-parser.yy +++ b/src/parser/seclang-parser.yy @@ -1209,7 +1209,7 @@ expression: YYERROR; } - if (!driver.m_defaultActions[definedPhase].empty()) { + if (!driver.m_rulesSetPhases[definedPhase]->m_defaultActions.empty()) { std::stringstream ss; ss << "SecDefaultActions can only be placed once per phase and configuration context. Phase "; ss << secRuleDefinedPhase; @@ -1219,7 +1219,7 @@ expression: } for (actions::Action *a : checkedActions) { - driver.m_defaultActions[definedPhase].push_back( + driver.m_rulesSetPhases[definedPhase]->m_defaultActions.push_back( std::unique_ptr(a)); } diff --git a/src/rule_with_actions.cc b/src/rule_with_actions.cc index 5ac17a26..bc4b5238 100644 --- a/src/rule_with_actions.cc +++ b/src/rule_with_actions.cc @@ -222,7 +222,7 @@ void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans, bool containsBlock, std::shared_ptr ruleMessage) { bool disruptiveAlreadyExecuted = false; - for (auto &a : trans->m_rules->m_defaultActions[getPhase()]) { + for (auto &a : trans->m_rules->m_rulesSetPhases[getPhase()]->m_defaultActions) { if (a.get()->action_kind != actions::Action::RunTimeOnlyIfMatchKind) { continue; } @@ -356,7 +356,7 @@ void RuleWithActions::executeTransformations( // Notice that first we make sure that won't be a t:none // on the target rule. if (none == 0) { - for (auto &a : trans->m_rules->m_defaultActions[getPhase()]) { + for (auto &a : trans->m_rules->m_rulesSetPhases[getPhase()]->m_defaultActions) { if (a->action_kind \ != actions::Action::RunTimeBeforeMatchAttemptKind) { continue; diff --git a/src/rules_set_phases.cc b/src/rules_set_phases.cc index 1d93330c..f0d572cb 100644 --- a/src/rules_set_phases.cc +++ b/src/rules_set_phases.cc @@ -61,6 +61,13 @@ int RulesSetPhases::append(RulesSetPhases *from, std::ostringstream *err) { return res; } amount_of_rules = amount_of_rules + res; + + std::vector > *actions_from = &from->at(phase)->m_defaultActions; + std::vector > *actions_to = &at(phase)->m_defaultActions; + + for (size_t j = 0; j < actions_from->size(); j++) { + actions_to->push_back(actions_from->at(j)); + } } return amount_of_rules;