Makes Rule a shared pointer

headers/modsecurity/rule.h

@@ -100,20 +100,6 @@ class Rule {
     bool containsTag(const std::string& name, Transaction *t);
     bool containsMsg(const std::string& name, Transaction *t);
 
-    int refCountDecreaseAndCheck() {
-        m_referenceCount--;
-        if (m_referenceCount == 0) {
-            delete this;
-            return 1;
-        }
-        return 0;
-    }
-
-
-    void refCountIncrease() {
-        m_referenceCount++;
-    }
-
     void executeTransformations(
         actions::Action *a,
         std::shared_ptr<std::string> newValue,
@@ -140,7 +126,7 @@ class Rule {
     int m_phase;
     modsecurity::variables::Variables *m_variables;
     operators::Operator *m_op;
-    Rule *m_chainedRuleChild;
+    std::unique_ptr<Rule> m_chainedRuleChild;
     Rule *m_chainedRuleParent;
     std::string m_fileName;
     std::string m_marker;
@@ -150,9 +136,9 @@ class Rule {
     std::vector<actions::Action *> m_actionsRuntimePre;
     std::vector<actions::SetVar *> m_actionsSetVar;
     std::vector<actions::Tag *> m_actionsTag;
+
  private:
     bool m_unconditional;
-    int m_referenceCount;
 };
 
 

headers/modsecurity/rules.h

@@ -23,8 +23,10 @@
 #include <string>
 #include <vector>
 #include <list>
+#include <memory>
 #endif
 
+#include "modsecurity/rule.h"
 
 #ifndef HEADERS_MODSECURITY_RULES_H_
 #define HEADERS_MODSECURITY_RULES_H_
@@ -32,10 +34,55 @@
 
 #ifdef __cplusplus
 namespace modsecurity {
-class Rule;
 
-class Rules : public std::vector<Rule *> {
+
+class Rules {
  public:
+    void dump() const {
+        for (int j = 0; j < m_rules.size(); j++) {
+            std::cout << "    Rule ID: " << std::to_string(m_rules.at(j)->m_ruleId);
+            std::cout << "--" << m_rules.at(j) << std::endl;
+        }
+    }
+
+    int append(Rules *from, const std::vector<int64_t> &ids, std::ostringstream *err) {
+         size_t j = 0;
+         for (; j < from->size(); j++) {
+            Rule *rule = from->at(j).get();
+             if (std::binary_search(ids.begin(), ids.end(), rule->m_ruleId)) {
+                 if (err != NULL) {
+                     *err << "Rule id: " << std::to_string(rule->m_ruleId) \
+                        << " is duplicated" << std::endl;
+                 }
+                 return -1;
+             }
+         }
+         m_rules.insert(m_rules.end(), from->m_rules.begin(), from->m_rules.end());
+         return j;
+    }
+
+    bool insert(std::shared_ptr<Rule> rule) {
+        return insert(rule, nullptr, nullptr);
+    }
+
+    bool insert(std::shared_ptr<Rule> rule, const std::vector<int64_t> *ids, std::ostringstream *err) {
+        if (ids != nullptr && std::binary_search(ids->begin(), ids->end(), rule->m_ruleId)) {
+            if (err != nullptr) {
+                *err << "Rule id: " << std::to_string(rule->m_ruleId) \
+                    << " is duplicated" << std::endl;
+            }
+            return false;
+        }
+
+        m_rules.push_back(rule);
+        return true;
+    }
+
+    size_t size() const { return m_rules.size(); }
+    std::shared_ptr<Rule> operator[](int index) const { return m_rules[index]; }
+    std::shared_ptr<Rule> at(int index) const { return m_rules[index]; }
+
+    std::vector<std::shared_ptr<Rule> > m_rules;
 };
 
 

headers/modsecurity/rules_set_phases.h

@@ -39,10 +39,9 @@ class Rule;
 /** @ingroup ModSecurity_CPP_API */
 class RulesSetPhases {
  public:
-
     ~RulesSetPhases();
 
-    bool insert(Rule *rule);
+    bool insert(std::shared_ptr<Rule> rule);
 
     int append(RulesSetPhases *from, std::ostringstream *err);
     void dump() const;

src/parser/driver.cc

@@ -29,7 +29,7 @@ Driver::Driver()
   : RulesSetProperties(),
   trace_scanning(false),
   trace_parsing(false),
-  lastRule(NULL) { }
+  m_lastRule(nullptr) { }
 
 
 Driver::~Driver() {
@@ -43,15 +43,15 @@ Driver::~Driver() {
 
 int Driver::addSecMarker(std::string marker) {
     for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) {
-        Rule *rule = new Rule(marker);
+        std::unique_ptr<Rule> rule(new Rule(marker));
         rule->m_phase = i;
-        m_rulesSetPhases.insert(rule);
+        m_rulesSetPhases.insert(std::move(rule));
     }
     return 0;
 }
 
 
-int Driver::addSecAction(Rule *rule) {
+int Driver::addSecAction(std::unique_ptr<Rule> rule) {
     if (rule->m_phase >= modsecurity::Phases::NUMBER_OF_PHASES) {
         m_parserError << "Unknown phase: " << std::to_string(rule->m_phase);
         m_parserError << std::endl;
@@ -59,55 +59,40 @@ int Driver::addSecAction(Rule *rule) {
     }
 
 
-    m_rulesSetPhases.insert(rule);
+    m_rulesSetPhases.insert(std::move(rule));
 
     return true;
 }
 
 
-int Driver::addSecRuleScript(RuleScript *rule) {
+int Driver::addSecRuleScript(std::unique_ptr<RuleScript> rule) {
-    m_rulesSetPhases.insert(rule);
+    m_rulesSetPhases.insert(std::move(rule));
     return true;
 }
 
 
-int Driver::addSecRule(Rule *rule) {
+int Driver::addSecRule(std::unique_ptr<Rule> r) {
-    if (rule->m_phase >= modsecurity::Phases::NUMBER_OF_PHASES) {
+    if (r->m_phase >= modsecurity::Phases::NUMBER_OF_PHASES) {
-        m_parserError << "Unknown phase: " << std::to_string(rule->m_phase);
+        m_parserError << "Unknown phase: " << std::to_string(r->m_phase);
         m_parserError << std::endl;
         return false;
     }
 
-    if (lastRule && lastRule->m_chained) {
+    /* is it a chained rule? */
-        if (lastRule->m_chainedRuleChild == NULL) {
+    if (m_lastRule != nullptr && m_lastRule->m_chained) {
-            rule->m_phase = lastRule->m_phase;
+        r->m_phase = m_lastRule->m_phase;
-            if (rule->m_theDisruptiveAction) {
+        if (r->m_theDisruptiveAction) {
-                m_parserError << "Disruptive actions can only be specified by";
+            m_parserError << "Disruptive actions can only be specified by";
-                m_parserError << " chain starter rules.";
+            m_parserError << " chain starter rules.";
-                return false;
+            return false;
-            }
-            lastRule->m_chainedRuleChild = rule;
-            rule->m_chainedRuleParent = lastRule;
-            return true;
-        } else {
-            Rule *a = lastRule->m_chainedRuleChild;
-            while (a->m_chained && a->m_chainedRuleChild != NULL) {
-                a = a->m_chainedRuleChild;
-            }
-            if (a->m_chained && a->m_chainedRuleChild == NULL) {
-                a->m_chainedRuleChild = rule;
-                rule->m_chainedRuleParent = a;
-                if (a->m_theDisruptiveAction) {
-                    m_parserError << "Disruptive actions can only be ";
-                    m_parserError << "specified by chain starter rules.";
-                    return false;
-                }
-                return true;
-            }
         }
+        m_lastRule->m_chainedRuleChild = std::move(r);
+        m_lastRule->m_chainedRuleChild->m_chainedRuleParent = m_lastRule;
+        m_lastRule = m_lastRule->m_chainedRuleChild.get();
+        return true;
     }
 
-
+    std::shared_ptr<Rule> rule(std::move(r));
     /*
      * Checking if the rule has an ID and also checking if this ID is not used
      * by other rule
@@ -118,6 +103,7 @@ int Driver::addSecRule(Rule *rule) {
         m_parserError << std::to_string(rule->m_lineNumber) << std::endl;
         return false;
     }
+
     for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) {
         Rules *rules = m_rulesSetPhases[i];
         for (int j = 0; j < rules->size(); j++) {
@@ -129,14 +115,15 @@ int Driver::addSecRule(Rule *rule) {
         }
     }
 
-    lastRule = rule;
+    m_lastRule = rule.get();
     m_rulesSetPhases.insert(rule);
+
     return true;
 }
 
 
 int Driver::parse(const std::string &f, const std::string &ref) {
-    lastRule = NULL;
+    m_lastRule = nullptr;
     loc.push_back(new yy::location());
     if (ref.empty()) {
         loc.back()->begin.filename = loc.back()->end.filename = new std::string("<<reference missing or not informed>>");
@@ -155,6 +142,19 @@ int Driver::parse(const std::string &f, const std::string &ref) {
     int res = parser.parse();
     scan_end();
 
+    /*
+     * need to check for rules marked as chained but without
+     * a chained rule.
+     *
+     */
+    /*
+    if (m_lastRule != nullptr && m_lastRule->m_chained) {
+        m_parserError << "Last rule is marked as chained but there " \
+            "isn't a subsequent rule." << std::endl;
+        return false;
+    }
+    */
+
     /*
     if (m_auditLog->init(&error) == false) {
         m_parserError << "Problems while initializing the audit logs: " \

src/parser/driver.h

@@ -66,10 +66,10 @@ class Driver : public RulesSetProperties {
     Driver();
     virtual ~Driver();
 
-    int addSecRule(Rule *rule);
+    int addSecRule(std::unique_ptr<Rule> rule);
-    int addSecAction(Rule *rule);
+    int addSecAction(std::unique_ptr<Rule> rule);
     int addSecMarker(std::string marker);
-    int addSecRuleScript(RuleScript *rule);
+    int addSecRuleScript(std::unique_ptr<RuleScript> rule);
 
     bool scan_begin();
     void scan_end();
@@ -89,7 +89,7 @@ class Driver : public RulesSetProperties {
     std::list<yy::location *> loc;
 
     std::string buffer;
-    Rule *lastRule;
+    Rule *m_lastRule;
 
     RulesSetPhases m_rulesSetPhases;
 };

src/parser/location.hh

@@ -1,4 +1,4 @@
-// A Bison parser, made by GNU Bison 3.5.2.
+// A Bison parser, made by GNU Bison 3.5.3.
 
 // Locations for Bison parsers in C++
 

src/parser/position.hh

@@ -1,4 +1,4 @@
-// A Bison parser, made by GNU Bison 3.5.2.
+// A Bison parser, made by GNU Bison 3.5.3.
 
 // Starting with Bison 3.2, this file is useless: the structure it
 // used to define is now defined in "location.hh".

src/parser/seclang-parser.hh

@@ -1,4 +1,4 @@
-// A Bison parser, made by GNU Bison 3.5.2.
+// A Bison parser, made by GNU Bison 3.5.3.
 
 // Skeleton interface for Bison LALR(1) parsers in C++
 

src/parser/seclang-parser.yy

@@ -1076,16 +1076,15 @@ expression:
         }
 
         Operator *op = $3.release();
-        Rule *rule = new Rule(
+        std::unique_ptr<Rule> rule(new Rule(
             /* op */ op,
             /* variables */ v,
             /* actions */ a,
             /* file name */ *@1.end.filename,
             /* line number */ @1.end.line
-            );
+            ));
 
-        if (driver.addSecRule(rule) == false) {
+        if (driver.addSecRule(std::move(rule)) == false) {
-            delete rule;
             YYERROR;
         }
       }
@@ -1096,15 +1095,14 @@ expression:
             v->push_back(i.release());
         }
 
-        Rule *rule = new Rule(
+        std::unique_ptr<Rule> rule(new Rule(
             /* op */ $3.release(),
             /* variables */ v,
             /* actions */ NULL,
             /* file name */ *@1.end.filename,
             /* line number */ @1.end.line
-            );
+            ));
-        if (driver.addSecRule(rule) == false) {
+        if (driver.addSecRule(std::move(rule)) == false) {
-            delete rule;
             YYERROR;
         }
       }
@@ -1114,14 +1112,14 @@ expression:
         for (auto &i : *$2.get()) {
             a->push_back(i.release());
         }
-        Rule *rule = new Rule(
+        std::unique_ptr<Rule> rule(new Rule(
             /* op */ NULL,
             /* variables */ NULL,
             /* actions */ a,
             /* file name */ *@1.end.filename,
             /* line number */ @1.end.line
-            );
+            ));
-        driver.addSecAction(rule);
+        driver.addSecAction(std::move(rule));
       }
     | DIRECTIVE_SECRULESCRIPT actions
       {
@@ -1130,20 +1128,18 @@ expression:
         for (auto &i : *$2.get()) {
             a->push_back(i.release());
         }
-        RuleScript *r = new RuleScript(
+        std::unique_ptr<RuleScript> r(new RuleScript(
             /* path to script */ $1,
             /* actions */ a,
             /* file name */ *@1.end.filename,
             /* line number */ @1.end.line
-            );
+            ));
 
         if (r->init(&err) == false) {
             driver.error(@0, "Failed to load script: " + err);
-            delete r;
             YYERROR;
         }
-        if (driver.addSecRuleScript(r) == false) {
+        if (driver.addSecRuleScript(std::move(r)) == false) {
-            delete r;
             YYERROR;
         }
       }

src/parser/stack.hh

@@ -1,4 +1,4 @@
-// A Bison parser, made by GNU Bison 3.5.2.
+// A Bison parser, made by GNU Bison 3.5.3.
 
 // Starting with Bison 3.2, this file is useless: the structure it
 // used to define is now defined with the parser itself.

src/rule.cc

@@ -68,7 +68,7 @@ Rule::Rule(const std::string &marker)
     m_phase(-1),
     m_variables(NULL),
     m_op(NULL),
-    m_chainedRuleChild(NULL),
+    m_chainedRuleChild(nullptr),
     m_chainedRuleParent(NULL),
     m_fileName(""),
     m_marker(marker),
@@ -78,9 +78,7 @@ Rule::Rule(const std::string &marker)
     m_actionsRuntimePre(),
     m_actionsSetVar(),
     m_actionsTag(),
-    m_unconditional(false),
+    m_unconditional(false) { }
-    m_referenceCount(1) { }
-
 
 Rule::Rule(Operator *_op,
     variables::Variables *_variables,
@@ -103,7 +101,7 @@ Rule::Rule(Operator *_op,
     m_phase(-1),
     m_variables(_variables),
     m_op(_op),
-    m_chainedRuleChild(NULL),
+    m_chainedRuleChild(nullptr),
     m_chainedRuleParent(NULL),
     m_fileName(fileName),
     m_marker(""),
@@ -113,9 +111,7 @@ Rule::Rule(Operator *_op,
     m_actionsRuntimePre(),
     m_actionsSetVar(),
     m_actionsTag(),
-    m_unconditional(false),
+    m_unconditional(false) {
-    m_referenceCount(1)
-    {
     /* */
     organizeActions(actions);
 
@@ -149,10 +145,6 @@ Rule::~Rule() {
     if (m_variables != NULL) {
         delete m_variables;
     }
-
-    if (m_chainedRuleChild != NULL) {
-        delete m_chainedRuleChild;
-    }
 }
 
 
@@ -779,14 +771,15 @@ bool Rule::evaluate(Transaction *trans,
         goto end_exec;
     }
 
-    if (this->m_chainedRuleChild == NULL) {
+    /* FIXME: this check should happens on the parser. */
+    if (this->m_chainedRuleChild == nullptr) {
         ms_dbg_a(trans, 4, "Rule is marked as chained but there " \
             "isn't a subsequent rule.");
         goto end_clean;
     }
 
     ms_dbg_a(trans, 4, "Executing chained rule.");
-    recursiveGlobalRet = this->m_chainedRuleChild->evaluate(trans, ruleMessage);
+    recursiveGlobalRet = m_chainedRuleChild->evaluate(trans, ruleMessage);
 
     if (recursiveGlobalRet == true) {
         goto end_exec;

src/rules_set.cc

@@ -111,7 +111,7 @@ int RulesSet::evaluate(int phase, Transaction *t) {
        return 0;
     }
 
-    std::vector<Rule *> *rules = m_rulesSetPhases[phase];
+    Rules *rules = m_rulesSetPhases[phase];
 
     ms_dbg_a(t, 9, "This phase consists of " \
         + std::to_string(rules->size()) + " rule(s).");
@@ -133,7 +133,7 @@ int RulesSet::evaluate(int phase, Transaction *t) {
     //}
 
     for (int i = 0; i < rules->size(); i++) {
-        Rule *rule = rules->at(i);
+        Rule *rule = rules->at(i).get();
         if (t->m_marker.empty() == false) {
             ms_dbg_a(t, 9, "Skipped rule id '" + std::to_string(rule->m_ruleId) \
                 + "' due to a SecMarker: " + t->m_marker);

src/rules_set_phases.cc

@@ -29,24 +29,13 @@
 namespace modsecurity {
 
 RulesSetPhases::~RulesSetPhases() {
-    /** Cleanup the rules */
-    for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) {
-        Rules *rules = &m_rules[i];
-        while (rules->empty() == false) {
-            Rule *rule = rules->back();
-            rules->pop_back();
-            if (rule->refCountDecreaseAndCheck()) {
-                rule = NULL;
-            }
-        }
-    }
 }
 
-bool RulesSetPhases::insert(Rule *rule) {
+bool RulesSetPhases::insert(std::shared_ptr<Rule> rule) {
     if (rule->m_phase >= modsecurity::Phases::NUMBER_OF_PHASES) {
         return false;
     }
-    m_rules[rule->m_phase].push_back(rule);
+    m_rules[rule->m_phase].insert(rule);
     return true;
 }
 
@@ -58,7 +47,7 @@ int RulesSetPhases::append(RulesSetPhases *from, std::ostringstream *err) {
     for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) {
         v.reserve(m_rules[i].size());
         for (size_t z = 0; z < m_rules[i].size(); z++) {
-            Rule *rule_ckc = m_rules[i].at(z);
+            Rule *rule_ckc = m_rules[i].at(z).get();
             if (rule_ckc->m_secMarker == true) {
                 continue;
             }
@@ -67,19 +56,9 @@ int RulesSetPhases::append(RulesSetPhases *from, std::ostringstream *err) {
     }
     std::sort (v.begin(), v.end());
 
-    for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) {
+    for (int phase = 0; phase < modsecurity::Phases::NUMBER_OF_PHASES; phase++) {
-        for (size_t j = 0; j < from->at(i)->size(); j++) {
+        if (m_rules[phase].append(from->at(phase), v, err) < 0) {
-            Rule *rule = from->at(i)->at(j);
+            return -1;
-            if (std::binary_search(v.begin(), v.end(), rule->m_ruleId)) {
-                if (err != NULL) {
-                    *err << "Rule id: " << std::to_string(rule->m_ruleId) \
-                        << " is duplicated" << std::endl;
-                }
-                return -1;
-            }
-            amount_of_rules++;
-            rule->refCountIncrease();
-            m_rules[i].push_back(rule);
         }
     }
 
@@ -91,10 +70,7 @@ void RulesSetPhases::dump() const {
         std::cout << "Phase: " << std::to_string(i);
         std::cout << " (" << std::to_string(m_rules[i].size());
         std::cout << " rules)" << std::endl;
-        for (int j = 0; j < m_rules[i].size(); j++) {
+        m_rules[i].dump();
-            std::cout << "    Rule ID: " << std::to_string(m_rules[i][j]->m_ruleId);
-            std::cout << "--" << m_rules[i][j] << std::endl;
-        }
     }
 }
 

test/optimization/optimization.cc

@@ -40,14 +40,14 @@ int main(int argc, char **argv) {
     std::list<std::string> files;
     int total = 0;
 
-    int i = 1;
+    int p = 1;
-    while (i < argc) {
+    while (p < argc) {
         std::list<std::string> tfiles = modsecurity::utils::expandEnv(
-            argv[i], 0);
+            argv[p], 0);
         for (const auto &file : tfiles) {
             files.insert(files.begin(), file);
         }
-        i++;
+        p++;
     }
 
 
@@ -68,7 +68,7 @@ int main(int argc, char **argv) {
 
     int nphases = modsecurity::Phases::NUMBER_OF_PHASES;
     for (int j = 0; j < nphases; j++) {
-        std::vector<Rule *> *rules = modsecRules->m_rulesSetPhases[j];
+        Rules *rules = modsecRules->m_rulesSetPhases[j];
         if (rules->size() == 0) {
             continue;
         }
@@ -79,7 +79,9 @@ int main(int argc, char **argv) {
         std::unordered_map<std::string, int> operators;
         std::unordered_map<std::string, int> variables;
         std::unordered_map<std::string, int> op2var;
-        for (auto &z : *rules) {
+
+        for (int i = 0; i < rules->size(); i++) {
+            std::shared_ptr<Rule> z = rules->at(i);
             std::string key;
             if (z == NULL) {
                 continue;

test/test-cases/regression/issue-1831.json

@@ -191,7 +191,7 @@
     "rules": [
       "SecRuleEngine On",
       "SecRule ARGS \"@rx .\" \"id:954100,phase:1,block,capture,t:none,t:lowercase,msg:'Disclosure of IIS install location',logdata:'Matched Data',tag:'application-multi',tag:'language-multi',tag:'platform-iis',tag:'platform-windows',tag:'attack-disclosure',ctl:auditLogParts=+E,rev:3,ver:'OWASP_CRS/3.0.0',severity:'ERROR',chain\"",
-      "SecRule &GLOBAL:alerted_970018_iisDefLoc \"@eq 0\" \"setvar:'global.alerted_970018_iisDefLoc',setvar:'tx.msg=%{rule.msg}',setvar:'tx.outbound_anomaly_score=+%{tx.error_anomaly_score}',setvar:'tx.anomaly_score=+%{tx.error_anomaly_score}'\""
+      "SecRule ARGS \"@eq 0\" \"setvar:'global.alerted_970018_iisDefLoc',setvar:'tx.msg=%{rule.msg}',setvar:'tx.outbound_anomaly_score=+%{tx.error_anomaly_score}',setvar:'tx.anomaly_score=+%{tx.error_anomaly_score}'\""
     ]
   }
 ]