From 46f40899e7603dcd0c09349603767a0bbc6ef417 Mon Sep 17 00:00:00 2001 From: "tomasz.ziolkowski" Date: Sun, 6 Mar 2022 14:30:27 +0100 Subject: [PATCH 1/6] Fix parallel lmdb readonly transactions --- src/collection/backend/lmdb.cc | 92 +++++++++++++++++++++++++++------- src/collection/backend/lmdb.h | 49 ++++++++++++++++++ src/modsecurity.cc | 3 ++ 3 files changed, 127 insertions(+), 17 deletions(-) diff --git a/src/collection/backend/lmdb.cc b/src/collection/backend/lmdb.cc index 4f58eb7c..5893c433 100644 --- a/src/collection/backend/lmdb.cc +++ b/src/collection/backend/lmdb.cc @@ -22,6 +22,8 @@ #include #include +#include + #include "modsecurity/variable_value.h" #include "src/utils/regex.h" #include "src/variables/variable.h" @@ -36,21 +38,22 @@ namespace backend { #ifdef WITH_LMDB LMDB::LMDB(std::string name) : - Collection(name), m_env(NULL) { - MDB_txn *txn; - mdb_env_create(&m_env); - mdb_env_open(m_env, "./modsec-shared-collections", - MDB_WRITEMAP | MDB_NOSUBDIR, 0664); - mdb_txn_begin(m_env, NULL, 0, &txn); - mdb_dbi_open(txn, NULL, MDB_CREATE | MDB_DUPSORT, &m_dbi); - mdb_txn_commit(txn); -} + Collection(name), m_env(NULL), isOpen(false) {} LMDB::~LMDB() { mdb_env_close(m_env); } +int LMDB::txn_begin(unsigned int flags, MDB_txn **ret) { + if (!isOpen) { + MDBEnvProvider* provider = MDBEnvProvider::GetInstance(); + m_env = provider->GetEnv(); + m_dbi = *(provider->GetDBI()); + isOpen = true; + } + return mdb_txn_begin(m_env, NULL, flags, ret); +} void LMDB::string2val(const std::string& str, MDB_val *val) { val->mv_size = sizeof(char)*(str.size()); @@ -159,7 +162,7 @@ std::unique_ptr LMDB::resolveFirst(const std::string& var) { string2val(var, &mdb_key); - rc = mdb_txn_begin(m_env, NULL, MDB_RDONLY, &txn); + rc = txn_begin(MDB_RDONLY, &txn); lmdb_debug(rc, "txn", "resolveFirst"); if (rc != 0) { goto end_txn; @@ -192,7 +195,7 @@ bool LMDB::storeOrUpdateFirst(const std::string &key, string2val(key, &mdb_key); string2val(value, &mdb_value); - rc = mdb_txn_begin(m_env, NULL, 0, &txn); + rc = txn_begin(0, &txn); lmdb_debug(rc, "txn", "storeOrUpdateFirst"); if (rc != 0) { goto end_txn; @@ -240,7 +243,7 @@ void LMDB::resolveSingleMatch(const std::string& var, MDB_val mdb_value_ret; MDB_cursor *cursor; - rc = mdb_txn_begin(m_env, NULL, MDB_RDONLY, &txn); + rc = txn_begin(MDB_RDONLY, &txn); lmdb_debug(rc, "txn", "resolveSingleMatch"); if (rc != 0) { goto end_txn; @@ -271,7 +274,7 @@ void LMDB::store(std::string key, std::string value) { int rc; MDB_stat mst; - rc = mdb_txn_begin(m_env, NULL, 0, &txn); + rc = txn_begin(0, &txn); lmdb_debug(rc, "txn", "store"); if (rc != 0) { goto end_txn; @@ -310,7 +313,7 @@ bool LMDB::updateFirst(const std::string &key, MDB_val mdb_value; MDB_val mdb_value_ret; - rc = mdb_txn_begin(m_env, NULL, 0, &txn); + rc = txn_begin(0, &txn); lmdb_debug(rc, "txn", "updateFirst"); if (rc != 0) { goto end_txn; @@ -364,7 +367,7 @@ void LMDB::del(const std::string& key) { MDB_val mdb_value_ret; MDB_stat mst; - rc = mdb_txn_begin(m_env, NULL, 0, &txn); + rc = txn_begin(0, &txn); lmdb_debug(rc, "txn", "del"); if (rc != 0) { goto end_txn; @@ -411,7 +414,7 @@ void LMDB::resolveMultiMatches(const std::string& var, size_t keySize = var.size(); MDB_cursor *cursor; - rc = mdb_txn_begin(m_env, NULL, MDB_RDONLY, &txn); + rc = txn_begin(MDB_RDONLY, &txn); lmdb_debug(rc, "txn", "resolveMultiMatches"); if (rc != 0) { goto end_txn; @@ -465,7 +468,7 @@ void LMDB::resolveRegularExpression(const std::string& var, Utils::Regex r(var, true); - rc = mdb_txn_begin(m_env, NULL, MDB_RDONLY, &txn); + rc = txn_begin(MDB_RDONLY, &txn); lmdb_debug(rc, "txn", "resolveRegularExpression"); if (rc != 0) { goto end_txn; @@ -503,6 +506,61 @@ end_txn: return; } + +MDBEnvProvider* MDBEnvProvider::provider_ = nullptr;; + +MDBEnvProvider* MDBEnvProvider::GetInstance() { + if (provider_==nullptr) { + provider_ = new MDBEnvProvider(); + } + return provider_; +} + +void MDBEnvProvider::Finalize() { + if (provider_!=nullptr) { + provider_->close(); + provider_ = nullptr; + } +} + +MDBEnvProvider::MDBEnvProvider() : + m_env(NULL), initialized(false) { + pthread_mutex_init(&m_lock, NULL); +} + +MDB_env* MDBEnvProvider::GetEnv() { + init(); + return m_env; +} + +MDB_dbi* MDBEnvProvider::GetDBI() { + init(); + return &m_dbi; +} + +void MDBEnvProvider::init() { + pthread_mutex_lock(&m_lock); + if (!initialized) { + MDB_txn *txn; + mdb_env_create(&m_env); + mdb_env_open(m_env, "./modsec-shared-collections", + MDB_WRITEMAP | MDB_NOSUBDIR, 0664); + mdb_txn_begin(m_env, NULL, 0, &txn); + mdb_dbi_open(txn, NULL, MDB_CREATE | MDB_DUPSORT, &m_dbi); + mdb_txn_commit(txn); + } + pthread_mutex_unlock(&m_lock); +} + +void MDBEnvProvider::close() { + pthread_mutex_lock(&m_lock); + if (initialized) { + mdb_dbi_close(m_env, m_dbi); + mdb_env_close(m_env); + } + pthread_mutex_unlock(&m_lock); +} + #endif } // namespace backend diff --git a/src/collection/backend/lmdb.h b/src/collection/backend/lmdb.h index dc44c078..dca5e55a 100644 --- a/src/collection/backend/lmdb.h +++ b/src/collection/backend/lmdb.h @@ -48,6 +48,53 @@ namespace modsecurity { namespace collection { namespace backend { + +/** + * The MDBEnvProvider class defines the `GetInstance` method that serves as an + * alternative to constructor and lets clients access the same instance of this + * class over and over. Its used to provide single MDB_env instance for each collection + * that uses lmdb to store and retrieve data. That approach satisfies lmdb requirement: + * + * "LMDB uses POSIX locks on files, and these locks have issues if one process opens + * a file multiple times. Because of this, do not mdb_env_open() a file multiple + * times from a single process." + * + * Creation of MDB_env is delayed to moment when first transaction is opened. + * This approach prevents passing env object to forked processes. + * In that way next lmdb requirement be satisfied: + * + * "Use an MDB_env* in the process which opened it, without fork()ing." + */ +class MDBEnvProvider { + protected: + static MDBEnvProvider* provider_; + MDBEnvProvider(); + public: + MDBEnvProvider(MDBEnvProvider &other) = delete; + void operator=(const MDBEnvProvider &) = delete; + + /** + * This is the static method that controls the access to the singleton + * instance. On the first run, it creates a singleton object and places it + * into the static field. On subsequent runs, it returns the client existing + * object stored in the static field. + */ + static MDBEnvProvider* GetInstance(); + static void Finalize(); + + MDB_env* GetEnv(); + MDB_dbi* GetDBI(); + + private: + MDB_env *m_env; + MDB_dbi m_dbi; + pthread_mutex_t m_lock; + + bool initialized; + void init(); + void close(); +}; + class LMDB : public Collection { public: @@ -75,11 +122,13 @@ class LMDB : variables::KeyExclusions &ke) override; private: + int txn_begin(unsigned int flags, MDB_txn **ret); void string2val(const std::string& str, MDB_val *val); void inline lmdb_debug(int rc, std::string op, std::string scope); MDB_env *m_env; MDB_dbi m_dbi; + bool isOpen; }; } // namespace backend diff --git a/src/modsecurity.cc b/src/modsecurity.cc index 854ec31e..2bb92c64 100644 --- a/src/modsecurity.cc +++ b/src/modsecurity.cc @@ -106,6 +106,9 @@ ModSecurity::~ModSecurity() { delete m_ip_collection; delete m_session_collection; delete m_user_collection; +#ifdef WITH_LMDB + collection::backend::MDBEnvProvider::Finalize(); +#endif } From 1fa95ec2e8f05a729ff0444453dc2484faec3b12 Mon Sep 17 00:00:00 2001 From: "tomasz.ziolkowski" Date: Tue, 8 Mar 2022 11:21:43 +0100 Subject: [PATCH 2/6] set initialized flag, remove unnecessary semicolon --- src/collection/backend/lmdb.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/collection/backend/lmdb.cc b/src/collection/backend/lmdb.cc index 5893c433..917cfbbe 100644 --- a/src/collection/backend/lmdb.cc +++ b/src/collection/backend/lmdb.cc @@ -507,7 +507,7 @@ end_txn: } -MDBEnvProvider* MDBEnvProvider::provider_ = nullptr;; +MDBEnvProvider* MDBEnvProvider::provider_ = nullptr; MDBEnvProvider* MDBEnvProvider::GetInstance() { if (provider_==nullptr) { @@ -542,6 +542,7 @@ void MDBEnvProvider::init() { pthread_mutex_lock(&m_lock); if (!initialized) { MDB_txn *txn; + initialized = true; mdb_env_create(&m_env); mdb_env_open(m_env, "./modsec-shared-collections", MDB_WRITEMAP | MDB_NOSUBDIR, 0664); From 3b50b2634b1e5157c5c5e1764ed4b1b04d1801a5 Mon Sep 17 00:00:00 2001 From: "tomasz.ziolkowski" Date: Tue, 8 Mar 2022 12:27:08 +0100 Subject: [PATCH 3/6] remove destructor, close environment only once --- src/collection/backend/lmdb.cc | 5 ----- src/collection/backend/lmdb.h | 1 - 2 files changed, 6 deletions(-) diff --git a/src/collection/backend/lmdb.cc b/src/collection/backend/lmdb.cc index 917cfbbe..413c9534 100644 --- a/src/collection/backend/lmdb.cc +++ b/src/collection/backend/lmdb.cc @@ -40,11 +40,6 @@ namespace backend { LMDB::LMDB(std::string name) : Collection(name), m_env(NULL), isOpen(false) {} - -LMDB::~LMDB() { - mdb_env_close(m_env); -} - int LMDB::txn_begin(unsigned int flags, MDB_txn **ret) { if (!isOpen) { MDBEnvProvider* provider = MDBEnvProvider::GetInstance(); diff --git a/src/collection/backend/lmdb.h b/src/collection/backend/lmdb.h index dca5e55a..3b359566 100644 --- a/src/collection/backend/lmdb.h +++ b/src/collection/backend/lmdb.h @@ -99,7 +99,6 @@ class LMDB : public Collection { public: explicit LMDB(std::string name); - ~LMDB(); void store(std::string key, std::string value) override; bool storeOrUpdateFirst(const std::string &key, From 89186b7e3a180dad8e8055aa9e939a04b48c426e Mon Sep 17 00:00:00 2001 From: "tomasz.ziolkowski" Date: Sat, 19 Mar 2022 09:12:43 +0100 Subject: [PATCH 4/6] update lines for modsecurity.cc on supress list for static check --- test/cppcheck_suppressions.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/cppcheck_suppressions.txt b/test/cppcheck_suppressions.txt index 64d0d2b6..646c6b07 100644 --- a/test/cppcheck_suppressions.txt +++ b/test/cppcheck_suppressions.txt @@ -44,14 +44,14 @@ functionStatic:src/engine/lua.h:71 functionConst:src/utils/geo_lookup.h:49 useInitializationList:src/operators/rbl.h:69 constStatement:test/common/modsecurity_test.cc:82 -danglingTemporaryLifetime:src/modsecurity.cc:206 +danglingTemporaryLifetime:src/modsecurity.cc:209 functionStatic:src/operators/geo_lookup.h:35 duplicateBreak:src/operators/validate_utf8_encoding.cc duplicateBranch:src/request_body_processor/multipart.cc:91 syntaxError:src/transaction.cc:62 noConstructor:src/variables/variable.h:152 duplicateBranch:src/request_body_processor/multipart.cc:93 -danglingTempReference:src/modsecurity.cc:206 +danglingTempReference:src/modsecurity.cc:209 knownConditionTrueFalse:src/operators/validate_url_encoding.cc:77 knownConditionTrueFalse:src/operators/verify_svnr.cc:87 rethrowNoCurrentException:headers/modsecurity/transaction.h:307 From 00483e4009aba5f659c5492d0ea670a6e62866fd Mon Sep 17 00:00:00 2001 From: "tomasz.ziolkowski" Date: Thu, 28 Apr 2022 07:15:02 +0200 Subject: [PATCH 5/6] swtich singleton to thread safe version --- src/collection/backend/lmdb.cc | 21 ++------------------- src/collection/backend/lmdb.h | 14 +++++++------- src/modsecurity.cc | 3 --- test/cppcheck_suppressions.txt | 4 ++-- 4 files changed, 11 insertions(+), 31 deletions(-) diff --git a/src/collection/backend/lmdb.cc b/src/collection/backend/lmdb.cc index 413c9534..2d9c9e2b 100644 --- a/src/collection/backend/lmdb.cc +++ b/src/collection/backend/lmdb.cc @@ -42,9 +42,8 @@ LMDB::LMDB(std::string name) : int LMDB::txn_begin(unsigned int flags, MDB_txn **ret) { if (!isOpen) { - MDBEnvProvider* provider = MDBEnvProvider::GetInstance(); - m_env = provider->GetEnv(); - m_dbi = *(provider->GetDBI()); + m_env = MDBEnvProvider::GetInstance().GetEnv(); + m_dbi = *(MDBEnvProvider::GetInstance().GetDBI()); isOpen = true; } return mdb_txn_begin(m_env, NULL, flags, ret); @@ -502,22 +501,6 @@ end_txn: } -MDBEnvProvider* MDBEnvProvider::provider_ = nullptr; - -MDBEnvProvider* MDBEnvProvider::GetInstance() { - if (provider_==nullptr) { - provider_ = new MDBEnvProvider(); - } - return provider_; -} - -void MDBEnvProvider::Finalize() { - if (provider_!=nullptr) { - provider_->close(); - provider_ = nullptr; - } -} - MDBEnvProvider::MDBEnvProvider() : m_env(NULL), initialized(false) { pthread_mutex_init(&m_lock, NULL); diff --git a/src/collection/backend/lmdb.h b/src/collection/backend/lmdb.h index 3b359566..d2bfc240 100644 --- a/src/collection/backend/lmdb.h +++ b/src/collection/backend/lmdb.h @@ -66,9 +66,7 @@ namespace backend { * "Use an MDB_env* in the process which opened it, without fork()ing." */ class MDBEnvProvider { - protected: - static MDBEnvProvider* provider_; - MDBEnvProvider(); + public: MDBEnvProvider(MDBEnvProvider &other) = delete; void operator=(const MDBEnvProvider &) = delete; @@ -77,11 +75,12 @@ class MDBEnvProvider { * This is the static method that controls the access to the singleton * instance. On the first run, it creates a singleton object and places it * into the static field. On subsequent runs, it returns the client existing - * object stored in the static field. + * object stored in the static field (Meyers Singleton implementation). */ - static MDBEnvProvider* GetInstance(); - static void Finalize(); - + static MDBEnvProvider& GetInstance() { + static MDBEnvProvider instance; + return instance; + } MDB_env* GetEnv(); MDB_dbi* GetDBI(); @@ -90,6 +89,7 @@ class MDBEnvProvider { MDB_dbi m_dbi; pthread_mutex_t m_lock; + MDBEnvProvider(); bool initialized; void init(); void close(); diff --git a/src/modsecurity.cc b/src/modsecurity.cc index 2bb92c64..854ec31e 100644 --- a/src/modsecurity.cc +++ b/src/modsecurity.cc @@ -106,9 +106,6 @@ ModSecurity::~ModSecurity() { delete m_ip_collection; delete m_session_collection; delete m_user_collection; -#ifdef WITH_LMDB - collection::backend::MDBEnvProvider::Finalize(); -#endif } diff --git a/test/cppcheck_suppressions.txt b/test/cppcheck_suppressions.txt index 646c6b07..64d0d2b6 100644 --- a/test/cppcheck_suppressions.txt +++ b/test/cppcheck_suppressions.txt @@ -44,14 +44,14 @@ functionStatic:src/engine/lua.h:71 functionConst:src/utils/geo_lookup.h:49 useInitializationList:src/operators/rbl.h:69 constStatement:test/common/modsecurity_test.cc:82 -danglingTemporaryLifetime:src/modsecurity.cc:209 +danglingTemporaryLifetime:src/modsecurity.cc:206 functionStatic:src/operators/geo_lookup.h:35 duplicateBreak:src/operators/validate_utf8_encoding.cc duplicateBranch:src/request_body_processor/multipart.cc:91 syntaxError:src/transaction.cc:62 noConstructor:src/variables/variable.h:152 duplicateBranch:src/request_body_processor/multipart.cc:93 -danglingTempReference:src/modsecurity.cc:209 +danglingTempReference:src/modsecurity.cc:206 knownConditionTrueFalse:src/operators/validate_url_encoding.cc:77 knownConditionTrueFalse:src/operators/verify_svnr.cc:87 rethrowNoCurrentException:headers/modsecurity/transaction.h:307 From 82326ffe2b4c34aad73b5e4fe8cafb71523d369c Mon Sep 17 00:00:00 2001 From: "tomasz.ziolkowski" Date: Fri, 29 Apr 2022 06:49:00 +0200 Subject: [PATCH 6/6] shift lmdb initialization to provider constructor which is called only once --- src/collection/backend/lmdb.cc | 38 ++++++++++------------------------ src/collection/backend/lmdb.h | 6 +----- 2 files changed, 12 insertions(+), 32 deletions(-) diff --git a/src/collection/backend/lmdb.cc b/src/collection/backend/lmdb.cc index 2d9c9e2b..2e512fa1 100644 --- a/src/collection/backend/lmdb.cc +++ b/src/collection/backend/lmdb.cc @@ -501,43 +501,27 @@ end_txn: } -MDBEnvProvider::MDBEnvProvider() : - m_env(NULL), initialized(false) { - pthread_mutex_init(&m_lock, NULL); +MDBEnvProvider::MDBEnvProvider() : m_env(NULL) { + MDB_txn *txn; + mdb_env_create(&m_env); + mdb_env_open(m_env, "./modsec-shared-collections", + MDB_WRITEMAP | MDB_NOSUBDIR, 0664); + mdb_txn_begin(m_env, NULL, 0, &txn); + mdb_dbi_open(txn, NULL, MDB_CREATE | MDB_DUPSORT, &m_dbi); + mdb_txn_commit(txn); } MDB_env* MDBEnvProvider::GetEnv() { - init(); return m_env; } MDB_dbi* MDBEnvProvider::GetDBI() { - init(); return &m_dbi; } -void MDBEnvProvider::init() { - pthread_mutex_lock(&m_lock); - if (!initialized) { - MDB_txn *txn; - initialized = true; - mdb_env_create(&m_env); - mdb_env_open(m_env, "./modsec-shared-collections", - MDB_WRITEMAP | MDB_NOSUBDIR, 0664); - mdb_txn_begin(m_env, NULL, 0, &txn); - mdb_dbi_open(txn, NULL, MDB_CREATE | MDB_DUPSORT, &m_dbi); - mdb_txn_commit(txn); - } - pthread_mutex_unlock(&m_lock); -} - -void MDBEnvProvider::close() { - pthread_mutex_lock(&m_lock); - if (initialized) { - mdb_dbi_close(m_env, m_dbi); - mdb_env_close(m_env); - } - pthread_mutex_unlock(&m_lock); +MDBEnvProvider::~MDBEnvProvider() { + mdb_dbi_close(m_env, m_dbi); + mdb_env_close(m_env); } #endif diff --git a/src/collection/backend/lmdb.h b/src/collection/backend/lmdb.h index d2bfc240..933f88c7 100644 --- a/src/collection/backend/lmdb.h +++ b/src/collection/backend/lmdb.h @@ -83,16 +83,12 @@ class MDBEnvProvider { } MDB_env* GetEnv(); MDB_dbi* GetDBI(); - + ~MDBEnvProvider(); private: MDB_env *m_env; MDB_dbi m_dbi; - pthread_mutex_t m_lock; MDBEnvProvider(); - bool initialized; - void init(); - void close(); }; class LMDB :