From dc0a06fc70a1e460637560a39bac2208e7d2c397 Mon Sep 17 00:00:00 2001 From: Eduardo Arias Date: Sat, 1 Jun 2024 14:54:49 +0000 Subject: [PATCH] Improve performance of VariableOrigin instances - The previous approach would create a std::unique_ptr and store it in a std::list in VariableValue (Origins) - The new approach now stores Origins in a std::vector and constructs VariableOrigin elements in-place on insertion. - Instead of having two heap-allocations for every added VariableOrigin instance, this performs only one. - If multiple origins are added, std::vector's growth strategy may even prevent a heap-allocation. There's a cost on growing the size of the vector, because a copy of current elements will be necessary. - Introduced reserveOrigin method to notify that multiple insertions will be made, so that we can use std::vector's reserve and do a single allocation (and copy of previous elements), and then just initialize the new elements in-place. --- .../anchored_set_variable_translation_proxy.h | 9 ++- headers/modsecurity/anchored_variable.h | 2 +- headers/modsecurity/variable_origin.h | 12 ++- headers/modsecurity/variable_value.h | 26 +++++-- src/anchored_set_variable.cc | 14 +--- src/anchored_variable.cc | 28 ++----- src/rule_with_operator.cc | 4 +- src/variables/remote_user.cc | 73 ++++++++----------- src/variables/rule.h | 25 ++----- 9 files changed, 78 insertions(+), 115 deletions(-) diff --git a/headers/modsecurity/anchored_set_variable_translation_proxy.h b/headers/modsecurity/anchored_set_variable_translation_proxy.h index 165e3cad..f36c69b1 100644 --- a/headers/modsecurity/anchored_set_variable_translation_proxy.h +++ b/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 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; } diff --git a/headers/modsecurity/anchored_variable.h b/headers/modsecurity/anchored_variable.h index 703a4f9d..3d777128 100644 --- a/headers/modsecurity/anchored_variable.h +++ b/headers/modsecurity/anchored_variable.h @@ -63,7 +63,7 @@ class AnchoredVariable { 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); + bool spaceSeparator, size_t size); void evaluate(std::vector *l); std::string * evaluate(); diff --git a/headers/modsecurity/variable_origin.h b/headers/modsecurity/variable_origin.h index 80ec177b..4bcab143 100644 --- a/headers/modsecurity/variable_origin.h +++ b/headers/modsecurity/variable_origin.h @@ -15,6 +15,7 @@ #ifdef __cplusplus #include +#include #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; }; diff --git a/headers/modsecurity/variable_value.h b/headers/modsecurity/variable_value.h index 06cf854a..78f17217 100644 --- a/headers/modsecurity/variable_value.h +++ b/headers/modsecurity/variable_value.h @@ -18,7 +18,7 @@ #include #include #include -#include +#include #include #endif @@ -37,7 +37,7 @@ namespace modsecurity { class Collection; class VariableValue { public: - using Origins = std::list>; + using Origins = std::vector; 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 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 origin) { - m_orign.push_back(std::move(origin)); + void addOrigin(const VariableOrigin &origin) { + m_orign.emplace_back(origin); + } + + + template + 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; diff --git a/src/anchored_set_variable.cc b/src/anchored_set_variable.cc index ec087775..efc6e574 100644 --- a/src/anchored_set_variable.cc +++ b/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 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 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); } diff --git a/src/anchored_variable.cc b/src/anchored_variable.cc index 63128bb2..3ead9dbe 100644 --- a/src/anchored_variable.cc +++ b/src/anchored_variable.cc @@ -54,58 +54,40 @@ void AnchoredVariable::unset() { void AnchoredVariable::set(const std::string &a, size_t offset, size_t offsetLen) { - std::unique_ptr 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 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)); + m_var->addOrigin(m_value.size(), offset); } void AnchoredVariable::append(const std::string &a, size_t offset, bool spaceSeparator) { - std::unique_ptr 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)); + m_var->addOrigin(a.size(), offset); } void AnchoredVariable::append(const std::string &a, size_t offset, - bool spaceSeparator, int size) { - std::unique_ptr origin( - new VariableOrigin()); - + bool spaceSeparator, size_t size) { 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({size, offset}); } diff --git a/src/rule_with_operator.cc b/src/rule_with_operator.cc index 5146c6d4..3a5ff385 100644 --- a/src/rule_with_operator.cc +++ b/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); diff --git a/src/variables/remote_user.cc b/src/variables/remote_user.cc index cc357aea..23bc6487 100644 --- a/src/variables/remote_user.cc +++ b/src/variables/remote_user.cc @@ -39,50 +39,41 @@ namespace variables { void RemoteUser::evaluate(Transaction *transaction, RuleWithActions *rule, std::vector *l) { - size_t pos; - std::string base64; - VariableValue *var; - std::string header; + std::vector l2; - std::vector *l2 = \ - new std::vector(); - 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(&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 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; } diff --git a/src/variables/rule.h b/src/variables/rule.h index f9e2f989..3d3cbcc0 100644 --- a/src/variables/rule.h +++ b/src/variables/rule.h @@ -49,15 +49,12 @@ class Rule_DictElement : public VariableDictElement { \ if (!r || r->m_ruleId == 0) { return; } - std::unique_ptr 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 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 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 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 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); } }