From a145de332503992656cfaf879a721b3a118b6de8 Mon Sep 17 00:00:00 2001 From: Eduardo Arias Date: Tue, 20 Aug 2024 08:05:58 -0700 Subject: [PATCH] Use fmt::format to compute keys in variables::collection::Collection - Refactored code to avoid code duplication - Use std::string_view in variables::collection::Collection - compartment & compartment2 arguments were being passed by value and creating an unnecessary std::string (heap-allocation & copy) - Replace const std::string& with std::string_view to relax client requirements (do not need to create a std::string instance) --- headers/modsecurity/collection/collection.h | 134 ++++++++---------- src/collection/backend/collection_data.h | 13 +- .../backend/in_memory-per_process.cc | 18 +-- .../backend/in_memory-per_process.h | 25 ++-- src/collection/backend/lmdb.cc | 4 +- src/collection/backend/lmdb.h | 4 +- 6 files changed, 92 insertions(+), 106 deletions(-) diff --git a/headers/modsecurity/collection/collection.h b/headers/modsecurity/collection/collection.h index 25d86b9a67..763370de05 100644 --- a/headers/modsecurity/collection/collection.h +++ b/headers/modsecurity/collection/collection.h @@ -16,11 +16,10 @@ #ifdef __cplusplus #include -#include -#include #include #include -#include +#include +#include #endif @@ -43,14 +42,14 @@ namespace collection { class Collection { public: - explicit Collection(const std::string &a) : m_name(a) { } + explicit Collection(std::string_view a) : m_name(a) { } virtual ~Collection() { } - virtual bool storeOrUpdateFirst(const std::string &key, - const std::string &value) = 0; + virtual bool storeOrUpdateFirst(const std::string& key, + std::string_view value) = 0; - virtual bool updateFirst(const std::string &key, - const std::string &value) = 0; + virtual bool updateFirst(const std::string& key, + std::string_view value) = 0; virtual void del(const std::string& key) = 0; @@ -70,131 +69,124 @@ class Collection { /* storeOrUpdateFirst */ - virtual bool storeOrUpdateFirst(const std::string &key, - std::string compartment, const std::string &value) { - std::string nkey = compartment + "::" + key; - return storeOrUpdateFirst(nkey, value); + bool storeOrUpdateFirst(std::string_view key, + std::string_view compartment, std::string_view value) { + return storeOrUpdateFirst(nkey(compartment, key), value); } - virtual bool storeOrUpdateFirst(const std::string &key, - std::string compartment, std::string compartment2, - const std::string &value) { - std::string nkey = compartment + "::" + compartment2 + "::" + key; - return storeOrUpdateFirst(nkey, value); + bool storeOrUpdateFirst(std::string_view key, + std::string_view compartment, std::string_view compartment2, + std::string_view value) { + return storeOrUpdateFirst(nkey(compartment, compartment2, key), value); } /* updateFirst */ - virtual bool updateFirst(const std::string &key, std::string compartment, - const std::string &value) { - std::string nkey = compartment + "::" + key; - return updateFirst(nkey, value); + bool updateFirst(std::string_view key, std::string_view compartment, + std::string_view value) { + return updateFirst(nkey(compartment, key), value); } - virtual bool updateFirst(const std::string &key, std::string compartment, - std::string compartment2, const std::string &value) { - std::string nkey = compartment + "::" + compartment2 + "::" + key; - return updateFirst(nkey, value); + bool updateFirst(std::string_view key, std::string_view compartment, + std::string_view compartment2, std::string_view value) { + return updateFirst(nkey(compartment, compartment2, key), value); } /* del */ - virtual void del(const std::string& key, std::string compartment) { - std::string nkey = compartment + "::" + key; - del(nkey); + void del(std::string_view key, std::string_view compartment) { + del(nkey(compartment, key)); } - virtual void del(const std::string& key, std::string compartment, - std::string compartment2) { - std::string nkey = compartment + "::" + compartment2 + "::" + key; - del(nkey); + void del(std::string_view key, std::string_view compartment, + std::string_view compartment2) { + del(nkey(compartment, compartment2, key)); } /* setExpiry */ - virtual void setExpiry(const std::string& key, std::string compartment, + void setExpiry(std::string_view key, std::string_view compartment, int32_t expiry_seconds) { - std::string nkey = compartment + "::" + key; - setExpiry(nkey, expiry_seconds); + setExpiry(nkey(compartment, key), expiry_seconds); } - virtual void setExpiry(const std::string& key, std::string compartment, - std::string compartment2, int32_t expiry_seconds) { - std::string nkey = compartment + "::" + compartment2 + "::" + key; - setExpiry(nkey, expiry_seconds); + void setExpiry(std::string_view key, std::string_view compartment, + std::string_view compartment2, int32_t expiry_seconds) { + setExpiry(nkey(compartment, compartment2, key), expiry_seconds); } /* resolveFirst */ - virtual std::unique_ptr resolveFirst(const std::string& var, - std::string compartment) { - std::string nkey = compartment + "::" + var; - return resolveFirst(nkey); + std::unique_ptr resolveFirst(std::string_view var, + std::string_view compartment) { + return resolveFirst(nkey(compartment, var)); } - virtual std::unique_ptr resolveFirst(const std::string& var, - std::string compartment, std::string compartment2) { - std::string nkey = compartment + "::" + compartment2 + "::" + var; - return resolveFirst(nkey); + std::unique_ptr resolveFirst(std::string_view var, + std::string_view compartment, std::string_view compartment2) { + return resolveFirst(nkey(compartment, compartment2, var)); } /* resolveSingleMatch */ - virtual void resolveSingleMatch(const std::string& var, - std::string compartment, std::vector *l) { - std::string nkey = compartment + "::" + var; - resolveSingleMatch(nkey, l); + void resolveSingleMatch(std::string_view var, + std::string_view compartment, std::vector *l) { + resolveSingleMatch(nkey(compartment, var), l); } - virtual void resolveSingleMatch(const std::string& var, - std::string compartment, std::string compartment2, + void resolveSingleMatch(std::string_view var, + std::string_view compartment, std::string_view compartment2, std::vector *l) { - std::string nkey = compartment + "::" + compartment2 + "::" + var; - resolveSingleMatch(nkey, l); + resolveSingleMatch(nkey(compartment, compartment2, var), l); } /* resolveMultiMatches */ - virtual void resolveMultiMatches(const std::string& var, - std::string compartment, std::vector *l, + void resolveMultiMatches(std::string_view var, + std::string_view compartment, std::vector *l, variables::KeyExclusions &ke) { - std::string nkey = compartment + "::" + var; - resolveMultiMatches(nkey, l, ke); + resolveMultiMatches(nkey(compartment, var), l, ke); } - virtual void resolveMultiMatches(const std::string& var, - std::string compartment, std::string compartment2, + void resolveMultiMatches(std::string_view var, + std::string_view compartment, std::string_view compartment2, std::vector *l, variables::KeyExclusions &ke) { - std::string nkey = compartment + "::" + compartment2 + "::" + var; - resolveMultiMatches(nkey, l, ke); + resolveMultiMatches(nkey(compartment, compartment2, var), l, ke); } /* resolveRegularExpression */ - virtual void resolveRegularExpression(const std::string& var, - std::string compartment, std::vector *l, + void resolveRegularExpression(std::string_view var, + std::string_view compartment, std::vector *l, variables::KeyExclusions &ke) { - std::string nkey = compartment + "::" + var; - resolveRegularExpression(nkey, l, ke); + resolveRegularExpression(nkey(compartment, var), l, ke); } - virtual void resolveRegularExpression(const std::string& var, - std::string compartment, std::string compartment2, + void resolveRegularExpression(std::string_view var, + std::string_view compartment, std::string_view compartment2, std::vector *l, variables::KeyExclusions &ke) { - std::string nkey = compartment + "::" + compartment2 + "::" + var; - resolveRegularExpression(nkey, l, ke); + resolveRegularExpression(nkey(compartment, compartment2, var), l, ke); } std::string m_name; + + protected: + + static inline std::string nkey(std::string_view compartment, std::string_view key) { + return fmt::format("{}::{}", compartment, key); + } + static inline std::string nkey(std::string_view compartment, std::string_view compartment2, std::string_view key) { + return fmt::format("{}::{}::{}", compartment, compartment2, key); + } }; } // namespace collection diff --git a/src/collection/backend/collection_data.h b/src/collection/backend/collection_data.h index 8b1b25d817..49f1ee78ec 100644 --- a/src/collection/backend/collection_data.h +++ b/src/collection/backend/collection_data.h @@ -16,6 +16,7 @@ #ifdef __cplusplus #include +#include #include #endif @@ -24,9 +25,7 @@ #define SRC_COLLECTION_DATA_H_ #ifdef __cplusplus -namespace modsecurity { -namespace collection { -namespace backend { +namespace modsecurity::collection::backend { class CollectionData { public: @@ -34,12 +33,12 @@ class CollectionData { m_hasValue(false), m_hasExpiryTime(false) { } - CollectionData(const std::string &value) : + CollectionData(std::string_view value) : m_hasValue(true), m_hasExpiryTime(false), m_value(value) { } - void setValue(const std::string &value) { + void setValue(std::string_view value) { m_value = value; m_hasValue = true; } @@ -60,9 +59,7 @@ class CollectionData { std::chrono::system_clock::time_point m_expiryTime; }; -} // namespace backend -} // namespace collection -} // namespace modsecurity +} // namespace modsecurity::collection::backend #endif #endif // SRC_COLLECTION_DATA_H_ diff --git a/src/collection/backend/in_memory-per_process.cc b/src/collection/backend/in_memory-per_process.cc index b16ee843ac..3f378ef998 100644 --- a/src/collection/backend/in_memory-per_process.cc +++ b/src/collection/backend/in_memory-per_process.cc @@ -35,7 +35,7 @@ namespace collection { namespace backend { -InMemoryPerProcess::InMemoryPerProcess(const std::string &name) : +InMemoryPerProcess::InMemoryPerProcess(std::string_view name) : Collection(name) { m_map.reserve(1000); } @@ -46,7 +46,7 @@ InMemoryPerProcess::~InMemoryPerProcess() { template -inline void __store(Map &map, std::string key, std::string value) { +inline void __store(Map &map, const std::string& key, std::string_view value) { // NOTE: should be called with write-lock previously acquired map.emplace(key, value); @@ -55,8 +55,8 @@ inline void __store(Map &map, std::string key, std::string value) { template inline bool __updateFirst(Map &map, - const std::string &key, - const std::string &value) { + const std::string& key, + std::string_view value) { // NOTE: should be called with write-lock previously acquired if (auto search = map.find(key); search != map.end()) { @@ -68,14 +68,14 @@ inline bool __updateFirst(Map &map, } -void InMemoryPerProcess::store(const std::string &key, const std::string &value) { +void InMemoryPerProcess::store(const std::string& key, std::string_view value) { const std::lock_guard lock(m_mutex); // write lock (exclusive access) __store(m_map, key, value); } -bool InMemoryPerProcess::storeOrUpdateFirst(const std::string &key, - const std::string &value) { +bool InMemoryPerProcess::storeOrUpdateFirst(const std::string& key, + std::string_view value) { const std::lock_guard lock(m_mutex); // write lock (exclusive access) if (__updateFirst(m_map, key, value) == false) { __store(m_map, key, value); @@ -84,8 +84,8 @@ bool InMemoryPerProcess::storeOrUpdateFirst(const std::string &key, } -bool InMemoryPerProcess::updateFirst(const std::string &key, - const std::string &value) { +bool InMemoryPerProcess::updateFirst(const std::string& key, + std::string_view value) { const std::lock_guard lock(m_mutex); // write lock (exclusive access) return __updateFirst(m_map, key, value); } diff --git a/src/collection/backend/in_memory-per_process.h b/src/collection/backend/in_memory-per_process.h index 4aa7b1d076..825db09fd0 100644 --- a/src/collection/backend/in_memory-per_process.h +++ b/src/collection/backend/in_memory-per_process.h @@ -73,15 +73,15 @@ struct MyHash{ class InMemoryPerProcess : public Collection { public: - explicit InMemoryPerProcess(const std::string &name); + explicit InMemoryPerProcess(std::string_view name); ~InMemoryPerProcess() override; - void store(const std::string &key, const std::string &value); + void store(const std::string& key, std::string_view value); - bool storeOrUpdateFirst(const std::string &key, - const std::string &value) override; + bool storeOrUpdateFirst(const std::string& key, + std::string_view value) override; - bool updateFirst(const std::string &key, - const std::string &value) override; + bool updateFirst(const std::string& key, + std::string_view value) override; void del(const std::string& key) override; @@ -101,17 +101,14 @@ class InMemoryPerProcess : variables::KeyExclusions &ke) override; /* store */ - virtual void store(const std::string &key, std::string &compartment, - std::string value) { - const auto nkey = compartment + "::" + key; - store(nkey, value); + void store(std::string_view key, std::string_view compartment, + std::string_view value) { + store(nkey(compartment, key), value); } - - virtual void store(const std::string &key, const std::string &compartment, + void store(const std::string &key, const std::string &compartment, std::string compartment2, std::string value) { - const auto nkey = compartment + "::" + compartment2 + "::" + key; - store(nkey, value); + store(nkey(compartment, compartment2, key), value); } private: diff --git a/src/collection/backend/lmdb.cc b/src/collection/backend/lmdb.cc index ffad0c7675..f76289b57f 100644 --- a/src/collection/backend/lmdb.cc +++ b/src/collection/backend/lmdb.cc @@ -296,7 +296,7 @@ void LMDB::delIfExpired(const std::string& key) { } bool LMDB::storeOrUpdateFirst(const std::string &key, - const std::string &value) { + std::string_view value) { int rc; MDB_txn *txn; MDB_val mdb_key; @@ -400,7 +400,7 @@ void LMDB::resolveSingleMatch(const std::string& var, bool LMDB::updateFirst(const std::string &key, - const std::string &value) { + std::string_view value) { int rc; MDB_txn *txn; MDB_val mdb_key; diff --git a/src/collection/backend/lmdb.h b/src/collection/backend/lmdb.h index 49633e37b9..db3bb2f27d 100644 --- a/src/collection/backend/lmdb.h +++ b/src/collection/backend/lmdb.h @@ -98,10 +98,10 @@ class LMDB : explicit LMDB(const std::string &name); bool storeOrUpdateFirst(const std::string &key, - const std::string &value) override; + std::string_view value) override; bool updateFirst(const std::string &key, - const std::string &value) override; + std::string_view value) override; void del(const std::string& key) override;