Merge pull request #3164 from eduar-hte/variable-origin

Improve performance of VariableOrigin instances

headers/modsecurity/anchored_set_variable_translation_proxy.h

@@ -47,11 +47,12 @@ class AnchoredSetVariableTranslationProxy {
                 VariableValue *newVariableValue = new VariableValue(name, &l->at(i)->getKey(), &l->at(i)->getKey());
                 const VariableValue *oldVariableValue = l->at(i);
                 l->at(i) = newVariableValue;
+                newVariableValue->reserveOrigin(oldVariableValue->getOrigin().size());
                 for (const auto &oldOrigin : oldVariableValue->getOrigin()) {
-                    std::unique_ptr<VariableOrigin> newOrigin(new VariableOrigin);
-                    newOrigin->m_length = oldVariableValue->getKey().size();
-                    newOrigin->m_offset = oldOrigin->m_offset - oldVariableValue->getKey().size() - 1;
-                    newVariableValue->addOrigin(std::move(newOrigin));
+                    newVariableValue->addOrigin(
+                        oldVariableValue->getKey().size(),
+                        oldOrigin.m_offset - oldVariableValue->getKey().size() - 1
+                    );
                 }
                 delete oldVariableValue;
             }

headers/modsecurity/anchored_variable.h

@@ -47,23 +47,11 @@ class AnchoredVariable {
     AnchoredVariable(const AnchoredVariable &a) = delete;
     AnchoredVariable &operator= (const AnchoredVariable &a) = delete;
 
-    /*
-        : m_transaction(a.m_transaction),
-        m_offset(a.m_offset),
-        m_name(a.m_name),
-        m_value(a.m_value),
-        m_var(a.m_var) { }
-    */
-
-    ~AnchoredVariable();
+    ~AnchoredVariable() = default;
 
     void unset();
     void set(const std::string &a, size_t offset);
     void set(const std::string &a, size_t offset, size_t offsetLen);
-    void append(const std::string &a, size_t offset,
-        bool spaceSeparator = false);
-    void append(const std::string &a, size_t offset,
-        bool spaceSeparator, int size);
 
     void evaluate(std::vector<const VariableValue *> *l);
     std::string *  evaluate();
@@ -75,7 +63,7 @@ class AnchoredVariable {
     std::string m_value;
 
  private:
-    VariableValue *m_var;
+    VariableValue m_var;
 };
 
 }  // namespace modsecurity

headers/modsecurity/variable_origin.h

@@ -15,6 +15,7 @@
 
 #ifdef __cplusplus
 #include <string>
+#include <memory>
 #endif
 
 #ifndef HEADERS_MODSECURITY_VARIABLE_ORIGIN_H_
@@ -36,14 +37,17 @@ class VariableOrigin {
     VariableOrigin()
         : m_length(0),
         m_offset(0) { }
+    VariableOrigin(size_t length, size_t offset)
+        : m_length(length),
+        m_offset(offset) { } 
 
-    std::string toText() {
-        std::string offset = std::to_string(m_offset);
-        std::string len = std::to_string(m_length);
+    std::string toText() const {
+        const auto offset = std::to_string(m_offset);
+        const auto len = std::to_string(m_length);
         return "v" + offset + "," + len;
     }
 
-    int m_length;
+    size_t m_length;
     size_t m_offset;
 };
 

headers/modsecurity/variable_value.h

@@ -18,7 +18,7 @@
 #include <string>
 #include <iostream>
 #include <memory>
-#include <list>
+#include <vector>
 #include <utility>
 #endif
 
@@ -37,7 +37,7 @@ namespace modsecurity {
 class Collection;
 class VariableValue {
  public:
-    using Origins = std::list<std::unique_ptr<VariableOrigin>>;
+    using Origins = std::vector<VariableOrigin>;
 
     explicit VariableValue(const std::string *key,
         const std::string *value = nullptr)
@@ -62,11 +62,9 @@ class VariableValue {
         m_keyWithCollection(o->m_keyWithCollection),
         m_value(o->m_value)
     {
+        reserveOrigin(o->m_orign.size());
         for (const auto &i : o->m_orign) {
-            std::unique_ptr<VariableOrigin> origin(new VariableOrigin());
-            origin->m_offset = i->m_offset;
-            origin->m_length = i->m_length;
-            m_orign.push_back(std::move(origin));
+            addOrigin(i);
         }
     }
 
@@ -98,8 +96,14 @@ class VariableValue {
     }
 
 
-    void addOrigin(std::unique_ptr<VariableOrigin> origin) {
-        m_orign.push_back(std::move(origin));
+    void addOrigin(const VariableOrigin &origin) {
+        m_orign.emplace_back(origin);
+    }
+
+
+    template<typename... Args>
+    void addOrigin(Args&&... args) {
+        m_orign.emplace_back(args...);
     }
 
 
@@ -107,6 +111,12 @@ class VariableValue {
         return m_orign;
     }
 
+
+    void reserveOrigin(Origins::size_type additionalSize) {
+        m_orign.reserve(m_orign.size() + additionalSize);
+    }
+
+
  private:
     Origins m_orign;
     std::string m_collection;

src/anchored_set_variable.cc

@@ -52,26 +52,16 @@ void AnchoredSetVariable::unset() {
 
 void AnchoredSetVariable::set(const std::string &key,
     const std::string &value, size_t offset, size_t len) {
-    std::unique_ptr<VariableOrigin> origin(new VariableOrigin());
     VariableValue *var = new VariableValue(&m_name, &key, &value);
-
-    origin->m_offset = offset;
-    origin->m_length = len;
-
-    var->addOrigin(std::move(origin));
+    var->addOrigin(len, offset);
     emplace(key, var);
 }
 
 
 void AnchoredSetVariable::set(const std::string &key,
     const std::string &value, size_t offset) {
-    std::unique_ptr<VariableOrigin> origin(new VariableOrigin());
     VariableValue *var = new VariableValue(&m_name, &key, &value);
-
-    origin->m_offset = offset;
-    origin->m_length = value.size();
-
-    var->addOrigin(std::move(origin));
+    var->addOrigin(value.size(), offset);
     emplace(key, var);
 }
 

src/anchored_variable.cc

@@ -31,19 +31,9 @@ AnchoredVariable::AnchoredVariable(Transaction *t,
     const std::string &name)
     : m_transaction(t),
     m_offset(0),
-    m_name(""),
+    m_name(name),
     m_value(""),
-    m_var(NULL) {
-        m_name.append(name);
-        m_var = new VariableValue(&m_name);
-}
-
-
-AnchoredVariable::~AnchoredVariable() {
-    if (m_var) {
-        delete (m_var);
-        m_var = NULL;
-    }
+    m_var(&name) {
 }
 
 
@@ -54,58 +44,16 @@ void AnchoredVariable::unset() {
 
 void AnchoredVariable::set(const std::string &a, size_t offset,
     size_t offsetLen) {
-    std::unique_ptr<VariableOrigin> origin(new VariableOrigin());
-
     m_offset = offset;
     m_value.assign(a.c_str(), a.size());
-    origin->m_offset = offset;
-    origin->m_length = offsetLen;
-    m_var->addOrigin(std::move(origin));
+    m_var.addOrigin(offsetLen, offset);
 }
 
 
 void AnchoredVariable::set(const std::string &a, size_t offset) {
-    std::unique_ptr<VariableOrigin> origin(new VariableOrigin());
-
     m_offset = offset;
     m_value.assign(a.c_str(), a.size());
-    origin->m_offset = offset;
-    origin->m_length = m_value.size();
-    m_var->addOrigin(std::move(origin));
-}
-
-
-void AnchoredVariable::append(const std::string &a, size_t offset,
-    bool spaceSeparator) {
-    std::unique_ptr<VariableOrigin> origin(
-        new VariableOrigin());
-
-    if (spaceSeparator && !m_value.empty()) {
-        m_value.append(" " + a);
-    } else {
-        m_value.append(a);
-    }
-    m_offset = offset;
-    origin->m_offset = offset;
-    origin->m_length = a.size();
-    m_var->addOrigin(std::move(origin));
-}
-
-
-void AnchoredVariable::append(const std::string &a, size_t offset,
-    bool spaceSeparator, int size) {
-    std::unique_ptr<VariableOrigin> origin(
-        new VariableOrigin());
-
-    if (spaceSeparator && !m_value.empty()) {
-        m_value.append(" " + a);
-    } else {
-        m_value.append(a);
-    }
-    m_offset = offset;
-    origin->m_offset = offset;
-    origin->m_length = size;
-    m_var->addOrigin(std::move(origin));
+    m_var.addOrigin(m_value.size(), offset);
 }
 
 
@@ -114,9 +62,8 @@ void AnchoredVariable::evaluate(std::vector<const VariableValue *> *l) {
         return;
     }
 
-    m_var->setValue(m_value);
-    VariableValue *m_var2 = new VariableValue(m_var);
-    l->push_back(m_var2);
+    m_var.setValue(m_value);
+    l->push_back(new VariableValue(&m_var));
 }
 
 
@@ -129,9 +76,7 @@ std::unique_ptr<std::string> AnchoredVariable::resolveFirst() {
     if (m_value.empty()) {
         return nullptr;
     }
-    std::unique_ptr<std::string> a(new std::string());
-    a->append(m_value);
-    return a;
+    return std::make_unique<std::string>(m_value);
 }
 
 

src/rule_with_operator.cc

@@ -317,8 +317,8 @@ bool RuleWithOperator::evaluate(Transaction *trans,
                 if (ret == true) {
                     ruleMessage->m_match = m_operator->resolveMatchMessage(trans,
                         key, value);
-                    for (auto &i : v->getOrigin()) {
-                        ruleMessage->m_reference.append(i->toText());
+                    for (const auto &i : v->getOrigin()) {
+                        ruleMessage->m_reference.append(i.toText());
                     }
 
                     ruleMessage->m_reference.append(*valueTemp.second);

src/variables/remote_user.cc

@@ -39,50 +39,41 @@ namespace variables {
 void RemoteUser::evaluate(Transaction *transaction,
     RuleWithActions *rule,
     std::vector<const VariableValue *> *l) {
-    size_t pos;
-    std::string base64;
-    VariableValue *var;
-    std::string header;
+    std::vector<const VariableValue *> l2;
 
-    std::vector<const VariableValue *> *l2 = \
-	new std::vector<const VariableValue *>();
-    transaction->m_variableRequestHeaders.resolve("authorization", l2);
+    transaction->m_variableRequestHeaders.resolve("authorization", &l2);
 
-    if (l2->size() < 1) {
-        goto clear;
+    if (!l2.empty()) {
+        const auto *v = l2[0];
+
+        const auto &header = v->getValue();
+
+        std::string base64;
+
+        if (header.compare(0, 6, "Basic ") == 0) {
+            base64 = std::string(header, 6, header.length());
+        }
+
+        base64 = Utils::Base64::decode(base64);
+
+        const auto pos = base64.find(":");
+        if (pos != std::string::npos) {
+            transaction->m_variableRemoteUser.assign(std::string(base64, 0, pos));
+
+            auto var = std::make_unique<VariableValue>(&v->getKeyWithCollection(),
+                &transaction->m_variableRemoteUser);
+
+            var->reserveOrigin(v->getOrigin().size());
+            for (const auto &i : v->getOrigin()) {
+                var->addOrigin(i);
+            }
+            l->push_back(var.release());
+        }
+
+        for (auto &a : l2) {
+            delete a;
+        }
     }
-
-    header = std::string(l2->at(0)->getValue());
-
-    if (header.compare(0, 6, "Basic ") == 0) {
-        base64 = std::string(header, 6, header.length());
-    }
-
-    base64 = Utils::Base64::decode(base64);
-
-    pos = base64.find(":");
-    if (pos == std::string::npos) {
-        goto clear;
-    }
-    transaction->m_variableRemoteUser.assign(std::string(base64, 0, pos));
-
-    var = new VariableValue(&l2->at(0)->getKeyWithCollection(),
-        &transaction->m_variableRemoteUser);
-
-    for (const auto &i : l2->at(0)->getOrigin()) {
-        std::unique_ptr<VariableOrigin> origin(new VariableOrigin());
-        origin->m_offset = i->m_offset;
-        origin->m_length = i->m_length;
-        var->addOrigin(std::move(origin));
-    }
-    l->push_back(var);
-
-clear:
-    for (auto &a : *l2) {
-        delete a;
-    }
-    l2->clear();
-    delete l2;
 }
 
 

src/variables/rule.h

@@ -49,15 +49,12 @@ class Rule_DictElement : public VariableDictElement { \
         if (!r || r->m_ruleId == 0) {
             return;
         }
-        std::unique_ptr<VariableOrigin> origin(new VariableOrigin());
         std::string *a = new std::string(std::to_string(r->m_ruleId));
         VariableValue *var = new VariableValue(&m_rule, &m_rule_id,
             a
         );
         delete a;
-        origin->m_offset = 0;
-        origin->m_length = 0;
-        var->addOrigin(std::move(origin));
+        var->addOrigin();
         l->push_back(var);
     }
 
@@ -75,15 +72,12 @@ class Rule_DictElement : public VariableDictElement { \
             return;
         }
 
-        std::unique_ptr<VariableOrigin> origin(new VariableOrigin());
         std::string *a = new std::string(r->m_rev);
         VariableValue *var = new VariableValue(&m_rule, &m_rule_rev,
             a
         );
         delete a;
-        origin->m_offset = 0;
-        origin->m_length = 0;
-        var->addOrigin(std::move(origin));
+        var->addOrigin();
         l->push_back(var);
     }
 
@@ -98,15 +92,12 @@ class Rule_DictElement : public VariableDictElement { \
         }
 
         if (r && r->hasSeverity()) {
-            std::unique_ptr<VariableOrigin> origin(new VariableOrigin());
             std::string *a = new std::string(std::to_string(r->severity()));
             VariableValue *var = new VariableValue(&m_rule, &m_rule_severity,
                 a
             );
             delete a;
-            origin->m_offset = 0;
-            origin->m_length = 0;
-            var->addOrigin(std::move(origin));
+            var->addOrigin();
             l->push_back(var);
         }
     }
@@ -122,15 +113,12 @@ class Rule_DictElement : public VariableDictElement { \
         }
 
         if (r && r->hasLogData()) {
-            std::unique_ptr<VariableOrigin> origin(new VariableOrigin());
             std::string *a = new std::string(r->logData(t));
             VariableValue *var = new VariableValue(&m_rule, &m_rule_logdata,
                 a
             );
             delete a;
-            origin->m_offset = 0;
-            origin->m_length = 0;
-            var->addOrigin(std::move(origin));
+            var->addOrigin();
             l->push_back(var);
         }
     }
@@ -145,15 +133,12 @@ class Rule_DictElement : public VariableDictElement { \
         }
 
         if (r && r->hasMsg()) {
-            std::unique_ptr<VariableOrigin> origin(new VariableOrigin());
             std::string *a = new std::string(r->msg(t));
             VariableValue *var = new VariableValue(&m_rule, &m_rule_msg,
                 a
             );
             delete a;
-            origin->m_offset = 0;
-            origin->m_length = 0;
-            var->addOrigin(std::move(origin));
+            var->addOrigin();
             l->push_back(var);
         }
     }

test/benchmark/basic_rules.conf

@@ -1,5 +1,3 @@
 
 Include "../../modsecurity.conf-recommended"
 
-Include "owasp-v3/crs-setup.conf.example"
-Include "owasp-v3/rules/*.conf"

test/benchmark/download-owasp-v2-rules.sh

@@ -1,56 +0,0 @@
-#!/bin/bash
-#
-#
-
-git clone https://github.com/coreruleset/coreruleset.git owasp-v2
-cd owasp-v2
-git checkout 2.2.9 -b tag2.2.9
-cd -
-
-echo 'Include "owasp-v2/base_rules/*.conf"' >> basic_rules.conf
-echo 'Include "owasp-v2/optional_rules/*.conf"' >> basic_rules.conf
-echo 'Include "owasp-v2/experimental_rules/*.conf"' >> basic_rules.conf
-echo 'Include "owasp-v2/slr_rules/modsecurity_crs_46_slr_et_xss_attacks.conf"' >> basic_rules.conf
-echo 'Include "owasp-v2/slr_rules/modsecurity_crs_46_slr_et_sqli_attacks.conf"' >> basic_rules.conf
-echo 'Include "owasp-v2/slr_rules/modsecurity_crs_46_slr_et_rfi_attacks.conf"' >> basic_rules.conf
-
-
-# Content injection not support in modsec v3
-rm owasp-v2/optional_rules/modsecurity_crs_43_csrf_protection.conf
-
-
-# Slow dos is not yet supported
-rm owasp-v2/experimental_rules/modsecurity_crs_11_slow_dos_protection.conf
-
-
-# WEBSERVER_ERROR_LOG is not supported in v3.
-cat owasp-v2/base_rules/modsecurity_crs_20_protocol_violations.conf | sed 's/SecRule WEBSERVER_ERROR_LOG/#SecRule WEBSERVER_ERROR_LOG/g' > owasp-v2/base_rules/modsecurity_crs_20_protocol_violations.conf.tmp
-mv owasp-v2/base_rules/modsecurity_crs_20_protocol_violations.conf.tmp owasp-v2/base_rules/modsecurity_crs_20_protocol_violations.conf
-
-
-# Apache specific configuration.
-cat owasp-v2/optional_rules/modsecurity_crs_49_header_tagging.conf | sed 's/RequestHeader/#RequestHeader/g' > owasp-v2/optional_rules/modsecurity_crs_49_header_tagging.conf.tmp
-mv owasp-v2/optional_rules/modsecurity_crs_49_header_tagging.conf.tmp owasp-v2/optional_rules/modsecurity_crs_49_header_tagging.conf
-
-cat owasp-v2/optional_rules/modsecurity_crs_55_application_defects.conf | sed 's/Header edit/#Header edit/g' > owasp-v2/optional_rules/modsecurity_crs_55_application_defects.conf.tmp
-mv owasp-v2/optional_rules/modsecurity_crs_55_application_defects.conf.tmp owasp-v2/optional_rules/modsecurity_crs_55_application_defects.conf
-
-cat owasp-v2/experimental_rules/modsecurity_crs_42_csp_enforcement.conf | sed 's/Header set/#Header set/g' > owasp-v2/experimental_rules/modsecurity_crs_42_csp_enforcement.conf.tmp
-mv owasp-v2/experimental_rules/modsecurity_crs_42_csp_enforcement.conf.tmp owasp-v2/experimental_rules/modsecurity_crs_42_csp_enforcement.conf
-
-
-# Disables SecGeoLookupDb
-cat owasp-v2/experimental_rules/modsecurity_crs_61_ip_forensics.conf | sed 's/SecGeoLookupDb/#SecGeoLookupDb/g' > owasp-v2/experimental_rules/modsecurity_crs_61_ip_forensics.conf.tmp
-mv owasp-v2/experimental_rules/modsecurity_crs_61_ip_forensics.conf.tmp owasp-v2/experimental_rules/modsecurity_crs_61_ip_forensics.conf
-
-cat owasp-v2/experimental_rules/modsecurity_crs_11_proxy_abuse.conf | sed 's/SecGeoLookupDb/#SecGeoLookupDb/g' > owasp-v2/experimental_rules/modsecurity_crs_11_proxy_abuse.conf.tmp
-mv owasp-v2/experimental_rules/modsecurity_crs_11_proxy_abuse.conf.tmp owasp-v2/experimental_rules/modsecurity_crs_11_proxy_abuse.conf
-
-
-# STREAM_OUTPUT_BODY is not supported
-cat owasp-v2/experimental_rules/modsecurity_crs_40_appsensor_detection_point_2.9_honeytrap.conf | sed 's/SecRule STREAM_OUTPUT_BODY/#SecRule STREAM_OUTPUT_BODY/g' > owasp-v2/experimental_rules/modsecurity_crs_40_appsensor_detection_point_2.9_honeytrap.conf.tmp
-mv owasp-v2/experimental_rules/modsecurity_crs_40_appsensor_detection_point_2.9_honeytrap.conf.tmp owasp-v2/experimental_rules/modsecurity_crs_40_appsensor_detection_point_2.9_honeytrap.conf
-
-
-echo "Done."
-

test/benchmark/download-owasp-v3-rules.sh

@@ -1,10 +1,6 @@
 #!/bin/bash
 
-
-git clone https://github.com/coreruleset/coreruleset.git owasp-v3
-cd owasp-v3
-git checkout v3.0.2 -b tag3.0.2
-cd -
+git clone -c advice.detachedHead=false --depth 1 --branch v3.0.2 https://github.com/coreruleset/coreruleset.git owasp-v3
 
 echo 'Include "owasp-v3/crs-setup.conf.example"' >> basic_rules.conf
 echo 'Include "owasp-v3/rules/*.conf"' >> basic_rules.conf

test/benchmark/download-owasp-v4-rules.sh

@@ -0,0 +1,9 @@
+#!/bin/bash
+
+git clone -c advice.detachedHead=false --depth 1 --branch v4.3.0 https://github.com/coreruleset/coreruleset.git owasp-v4
+
+echo 'Include "owasp-v4/crs-setup.conf.example"' >> basic_rules.conf
+echo 'Include "owasp-v4/rules/*.conf"' >> basic_rules.conf
+
+echo "Done."
+