Fix parallel lmdb readonly transactions

Author: ziollek

src/collection/backend/lmdb.cc

@@ -22,6 +22,8 @@
 #include <string>
 #include <memory>
 
+#include <pthread.h>
+
 #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<std::string> 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 @@ void LMDB::resolveRegularExpression(const std::string& var,
     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

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

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
 }