Having RunTimeString in a better shape

This is an effort towards better understanding the issues
reported on #2376

src/operators/operator.cc

@@ -122,7 +122,7 @@ bool Operator::evaluate(Transaction *transaction,
 Operator *Operator::instantiate(std::string op, std::string param_str) {
     std::string op_ = utils::string::tolower(op);
     std::unique_ptr<RunTimeString> param(new RunTimeString());
-    param->appendText(param_str);
+    param->append(param_str);
 
     IF_MATCH(beginswith) { return new BeginsWith(std::move(param)); }
     IF_MATCH(contains) { return new Contains(std::move(param)); }

src/parser/seclang-parser.cc

@@ -5621,7 +5621,7 @@ namespace yy {
   case 438: // run_time_string: run_time_string "FREE_TEXT_QUOTE_MACRO_EXPANSION"
 #line 3036 "seclang-parser.yy"
       {
-        yystack_[1].value.as < std::unique_ptr<RunTimeString> > ()->appendText(yystack_[0].value.as < std::string > ());
+        yystack_[1].value.as < std::unique_ptr<RunTimeString> > ()->append(yystack_[0].value.as < std::string > ());
         yylhs.value.as < std::unique_ptr<RunTimeString> > () = std::move(yystack_[1].value.as < std::unique_ptr<RunTimeString> > ());
       }
 #line 5628 "seclang-parser.cc"
@@ -5630,7 +5630,7 @@ namespace yy {
   case 439: // run_time_string: run_time_string var
 #line 3041 "seclang-parser.yy"
       {
-        yystack_[1].value.as < std::unique_ptr<RunTimeString> > ()->appendVar(std::move(yystack_[0].value.as < std::unique_ptr<Variable> > ()));
+        yystack_[1].value.as < std::unique_ptr<RunTimeString> > ()->append(std::move(yystack_[0].value.as < std::unique_ptr<Variable> > ()));
         yylhs.value.as < std::unique_ptr<RunTimeString> > () = std::move(yystack_[1].value.as < std::unique_ptr<RunTimeString> > ());
       }
 #line 5637 "seclang-parser.cc"
@@ -5640,7 +5640,7 @@ namespace yy {
 #line 3046 "seclang-parser.yy"
       {
         std::unique_ptr<RunTimeString> r(new RunTimeString());
-        r->appendText(yystack_[0].value.as < std::string > ());
+        r->append(yystack_[0].value.as < std::string > ());
         yylhs.value.as < std::unique_ptr<RunTimeString> > () = std::move(r);
       }
 #line 5647 "seclang-parser.cc"
@@ -5650,7 +5650,7 @@ namespace yy {
 #line 3052 "seclang-parser.yy"
       {
         std::unique_ptr<RunTimeString> r(new RunTimeString());
-        r->appendVar(std::move(yystack_[0].value.as < std::unique_ptr<Variable> > ()));
+        r->append(std::move(yystack_[0].value.as < std::unique_ptr<Variable> > ()));
         yylhs.value.as < std::unique_ptr<RunTimeString> > () = std::move(r);
       }
 #line 5657 "seclang-parser.cc"

src/parser/seclang-parser.yy

@@ -3034,24 +3034,24 @@ setvar_action:
 run_time_string:
     run_time_string FREE_TEXT_QUOTE_MACRO_EXPANSION
       {
-        $1->appendText($2);
+        $1->append($2);
         $$ = std::move($1);
       }
     | run_time_string var
       {
-        $1->appendVar(std::move($2));
+        $1->append(std::move($2));
         $$ = std::move($1);
       }
     | FREE_TEXT_QUOTE_MACRO_EXPANSION
       {
         std::unique_ptr<RunTimeString> r(new RunTimeString());
-        r->appendText($1);
+        r->append($1);
         $$ = std::move(r);
       }
     | var
       {
         std::unique_ptr<RunTimeString> r(new RunTimeString());
-        r->appendVar(std::move($1));
+        r->append(std::move($1));
         $$ = std::move(r);
       }
     ;

src/run_time_string.cc

@@ -31,38 +31,22 @@
 namespace modsecurity {
 
 
-void RunTimeString::appendText(const std::string &text) {
+void RunTimeString::append(const std::string &text) {
-    std::unique_ptr<RunTimeElementHolder> r(new RunTimeElementHolder);
+    m_elements.emplace_back(new ElementHolder(text));
-    r->m_string = text;
-    m_elements.push_back(std::move(r));
 }
 
 
-void RunTimeString::appendVar(
+void RunTimeString::append(std::unique_ptr<Variable> var) {
-    std::unique_ptr<modsecurity::variables::Variable> var) {
+    m_elements.emplace_back(new ElementHolder(std::move(var)));
-    std::unique_ptr<RunTimeElementHolder> r(new RunTimeElementHolder);
-    r->m_variable = std::move(var);
-    m_elements.push_back(std::move(r));
     m_containsMacro = true;
 }
 
 
-std::string RunTimeString::evaluate(Transaction *transaction) {
+std::string RunTimeString::evaluate(/* const */ Transaction *transaction) const noexcept {
     std::string retString;
     // FIXME: Educated guess the size of retString based on the size of the elements.
     for (auto &element : m_elements) {
-        if (element->m_string.size() > 0) {
+        element->appendValueTo(transaction, retString);
-            retString.append(element->m_string);
-        } else if (element->m_variable != nullptr && transaction != nullptr) {
-            std::vector<const VariableValue *> l;
-            element->m_variable->evaluate(transaction, &l);
-            if (!l.empty()) {
-                retString.append(l[0]->getValue());
-            }
-            for (auto &i : l) {
-                delete i;
-            }
-        }
     }
     return retString;
 }

src/run_time_string.h

@@ -33,33 +33,11 @@
 
 namespace modsecurity {
 
-class RunTimeElementHolder {
- public:
-    RunTimeElementHolder()
-        : m_string(""),
-        m_variable(nullptr)
-    { };
-
-
-    RunTimeElementHolder(const RunTimeElementHolder &other)
-        : m_string(other.m_string),
-        m_variable(other.m_variable) {
-            variables::RuleVariable *rv = dynamic_cast<variables::RuleVariable *>(m_variable.get());
-            if (rv != nullptr) {
-                auto nrv = rv->clone();
-                rv = dynamic_cast<variables::RuleVariable *>(nrv);
-                rv->populate(nullptr);
-                m_variable = std::unique_ptr<variables::Variable>(nrv);
-            }
-        };
-
- /* protected: */
-    std::string m_string;
-    std::shared_ptr<variables::Variable> m_variable;
-};
-
 class RunTimeString {
  public:
+    using Variable = variables::Variable;
+    using RuleVariable = variables::RuleVariable;
+
     RunTimeString()
         : m_containsMacro(false),
         m_elements()
@@ -71,37 +49,124 @@ class RunTimeString {
         m_elements()
     {
         for (auto &m : other.m_elements) {
-            m_elements.push_back(std::unique_ptr<RunTimeElementHolder>(new RunTimeElementHolder(*m.get())));
+            m_elements.emplace_back(new ElementHolder(*m.get()));
         }
     };
 
-
+    RunTimeString& operator=(RunTimeString other)
-    void appendText(const std::string &text);
+    {
-    void appendVar(std::unique_ptr<modsecurity::variables::Variable> var);
+        m_containsMacro = other.m_containsMacro;
-
+        for (auto &m : other.m_elements) {
-
+            m_elements.emplace_back(new ElementHolder(*m.get()));
-    std::string evaluate(Transaction *t);
+        }
-
+        return *this;
-    inline std::string evaluate() {
-        return evaluate(NULL);
     }
 
 
-    inline bool containsMacro() const { return m_containsMacro; }
+    void append(const std::string &text);
+    void append(std::unique_ptr<Variable> var);
 
 
-    void populate(RuleWithActions *rule) {
+    /*
+     *
+     * FIXME: Transaction should be const here. Variables resolution does
+     *        not change anything on transaction instance.
+     *
+     */
+    std::string evaluate(/* const */ Transaction *t = nullptr) const noexcept;
+
+
+    inline bool containsMacro() const noexcept {
+        return m_containsMacro;
+    }
+
+
+    void populate(RuleWithActions *rule) noexcept {
         for (auto &a : m_elements) {
-            modsecurity::variables::RuleVariable *vrule = dynamic_cast<variables::RuleVariable *>(a->m_variable.get());
+            a->populate(rule);
+        }
+    }
+
+
+    class ElementHolder {
+     public:
+        ElementHolder()
+            : m_string(""),
+            m_variable(nullptr)
+        { };
+    
+        explicit ElementHolder(std::unique_ptr<Variable> variable)
+            : m_string(""),
+            m_variable(std::move(variable))
+        { };
+    
+        explicit ElementHolder(const std::string &str)
+            : m_string(str),
+            m_variable(nullptr)
+        { };
+
+        ElementHolder(const ElementHolder &other)
+            : m_string(other.m_string),
+            m_variable(nullptr) {
+                RuleVariable *rv = dynamic_cast<RuleVariable *>(other.m_variable.get());
+                if (rv != nullptr) {
+                    auto nrv = rv->clone();
+                    rv = dynamic_cast<RuleVariable *>(nrv);
+                    rv->populate(nullptr);
+                    m_variable = std::unique_ptr<Variable>(nrv);
+                    /* m_variable = nullptr; */
+                } else {
+                    m_variable = other.m_variable;
+                }
+
+            };
+
+
+        void appendValueTo(/* const */ Transaction *transaction, std::string &v) const noexcept {
+            if (m_variable && transaction) {
+                std::vector<const VariableValue *> l;
+                m_variable->evaluate(transaction, &l);
+                if (!l.empty()) {
+                    v.append(l[0]->getValue());
+                }
+                for (auto &i : l) {
+                    delete i;
+                }
+
+                return;
+            }
+
+            v.append(m_string);
+        }
+    
+
+        void populate(RuleWithActions *rule) noexcept {
+            if (!m_variable) {
+                return;
+            }
+
+            RuleVariable *vrule = dynamic_cast<RuleVariable *>(m_variable.get());
             if (vrule != nullptr) {
                 vrule->populate(rule);
             }
         }
-    }
 
+     private:
+        std::string m_string;
+        /*
+         *
+         * FIXME: In the current state m_variable should be a unique_ptr. There
+         *        is no copy for variables, thus having a shared pointer here.
+         *        As an optimization we can have it as a shared_ptr to reduce the
+         *        memory footprint in anchored variables.
+         *
+         */
+        std::shared_ptr<Variable> m_variable;
+    };
+    
  private:
-    bool m_containsMacro;
+    bool m_containsMacro:1;
-    std::list<std::unique_ptr<RunTimeElementHolder>> m_elements;
+    std::vector<std::unique_ptr<ElementHolder>> m_elements;
 };