Merge pull request #3216 from eduar-hte/inmemory-collection-shared-mutex

Prevent concurrent access to data in InMemoryPerProcess' resolveXXX methods

.github/workflows/ci.yml

@@ -132,11 +132,11 @@ jobs:
         configuration: [Release]
         configure:
           - {label: "full",       opt: "" }
-          - {label: "wo curl",    opt: "-DWITHOUT_CURL=ON" }
-          - {label: "wo lmdb",    opt: "-DWITHOUT_LMDB=ON" }
-          - {label: "wo lua",     opt: "-DWITHOUT_LUA=ON" }
-          - {label: "wo maxmind", opt: "-DWITHOUT_MAXMIND=ON" }
-          - {label: "wo libxml",  opt: "-WITHOUT_LIBXML2=ON" }
+          - {label: "wo curl",    opt: "-DWITH_CURL=OFF" }
+          - {label: "wo lua",     opt: "-DWITH_LUA=OFF" }
+          - {label: "wo maxmind", opt: "-DWITH_MAXMIND=OFF" }
+          - {label: "wo libxml",  opt: "-DWITH_LIBXML2=OFF" }
+          - {label: "with lmdb",  opt: "-DWITH_LMDB=ON" }
     steps:
       - uses: actions/checkout@v4
         with:

.gitignore

@@ -50,6 +50,7 @@ ltmain.sh
 examples/simple_example_using_c/test
 /tools/rules-check/modsec-rules-check
 examples/multiprocess_c/multi
+examples/multithread/multithread
 examples/reading_logs_via_rule_message/simple_request
 examples/reading_logs_with_offset/read
 examples/using_bodies_in_chunks/simple_request

build/win32/CMakeLists.txt

@@ -2,13 +2,13 @@ cmake_minimum_required(VERSION 3.24)
 
 set(BASE_DIR ${CMAKE_CURRENT_LIST_DIR}/../..)
 
-option(WITHOUT_LMDB    "Include LMDB support"    OFF)
-option(WITHOUT_LUA     "Include LUA support"     OFF)
-option(WITHOUT_LIBXML2 "Include LibXML2 support" OFF)
-option(WITHOUT_MAXMIND "Include MaxMind support" OFF)
-option(WITHOUT_CURL    "Include CURL support"    OFF)
+option(WITH_LMDB    "Include LMDB support"    OFF)
+option(WITH_LUA     "Include LUA support"     ON)
+option(WITH_LIBXML2 "Include LibXML2 support" ON)
+option(WITH_MAXMIND "Include MaxMind support" ON)
+option(WITH_CURL    "Include CURL support"    ON)
 
-option(USE_ASAN        "Build with Address Sanitizer" OFF)
+option(USE_ASAN     "Build with Address Sanitizer" OFF)
 
 # common compiler settings
 
@@ -93,24 +93,23 @@ set(HAVE_SSDEEP   0)    # should always be zero, no conan package available
 
 macro(enable_feature flag option)
   if(${option})
-    set(${flag} 0)
+    set(${flag} 1)  # ON
   else()
-    set(${flag} 1)
+    set(${flag} 0)  # OFF
   endif()
 endmacro()
 
-enable_feature(HAVE_LMDB ${WITHOUT_LMDB})
-enable_feature(HAVE_LUA ${WITHOUT_LUA})
-enable_feature(HAVE_LIBXML2 ${WITHOUT_LIBXML2})
-enable_feature(HAVE_MAXMIND ${WITHOUT_MAXMIND})
-enable_feature(HAVE_CURL ${WITHOUT_CURL})
+enable_feature(HAVE_LMDB ${WITH_LMDB})
+enable_feature(HAVE_LUA ${WITH_LUA})
+enable_feature(HAVE_LIBXML2 ${WITH_LIBXML2})
+enable_feature(HAVE_MAXMIND ${WITH_MAXMIND})
+enable_feature(HAVE_CURL ${WITH_CURL})
 
 include(${CMAKE_CURRENT_LIST_DIR}/ConfigureChecks.cmake)
 
 configure_file(config.h.cmake ${BASE_DIR}/src/config.h)
 
 find_package(PCRE2 REQUIRED)
-find_package(PThreads4W REQUIRED)
 find_package(Poco REQUIRED)
 find_package(dirent REQUIRED)		# used only by tests (check dirent::dirent refernces)
 
@@ -139,7 +138,7 @@ add_library(libModSecurity SHARED ${libModSecuritySources})
 
 target_compile_definitions(libModSecurity PRIVATE WITH_PCRE2)
 target_include_directories(libModSecurity PRIVATE ${BASE_DIR} ${BASE_DIR}/headers ${BASE_DIR}/others ${MBEDTLS_DIR}/include)
-target_link_libraries(libModSecurity PRIVATE pcre2::pcre2 pthreads4w::pthreads4w libinjection mbedcrypto Poco::Poco Iphlpapi.lib)
+target_link_libraries(libModSecurity PRIVATE pcre2::pcre2 libinjection mbedcrypto Poco::Poco Iphlpapi.lib)
 
 macro(add_package_dependency project compile_definition link_library flag)
   if(${flag})
@@ -255,12 +254,15 @@ setExampleTargetProperties(using_bodies_in_chunks)
 # reading_logs_via_rule_message
 add_executable(reading_logs_via_rule_message ${BASE_DIR}/examples/reading_logs_via_rule_message/simple_request.cc)
 setExampleTargetProperties(reading_logs_via_rule_message)
-target_link_libraries(reading_logs_via_rule_message PRIVATE libModSecurity pthreads4w::pthreads4w)
 
 # reading_logs_with_offset
 add_executable(reading_logs_with_offset ${BASE_DIR}/examples/reading_logs_with_offset/read.cc)
 setExampleTargetProperties(reading_logs_with_offset)
 
+# multithread
+add_executable(multithread ${BASE_DIR}/examples/multithread/multithread.cc)
+setExampleTargetProperties(multithread)
+
 # tools
 #
 

build/win32/README.md

@@ -51,6 +51,7 @@ Built files will be located in the directory: `build\win32\build\[build_configur
     * `using_bodies_in_chunks.exe`
     * `reading_logs_via_rule_message.exe`
     * `reading_logs_with_offset.exe`
+    * `multithread.exe`
  * Executable files for tools
     * `rules_check.exe`
 

build/win32/conanfile.txt

@@ -1,7 +1,6 @@
 [requires]
 yajl/2.1.0
 pcre2/10.42
-pthreads4w/3.0.0
 libxml2/2.12.6
 lua/5.4.6
 libcurl/8.6.0

configure.ac

@@ -423,6 +423,7 @@ AM_COND_IF([EXAMPLES],
             examples/Makefile \
             examples/simple_example_using_c/Makefile \
             examples/multiprocess_c/Makefile \
+            examples/multithread/Makefile \
             examples/reading_logs_with_offset/Makefile \
             examples/reading_logs_via_rule_message/Makefile \
             examples/using_bodies_in_chunks/Makefile \

examples/Makefile.am

@@ -4,6 +4,7 @@ ACLOCAL_AMFLAGS = -I build
 
 SUBDIRS = \
 	multiprocess_c \
+	multithread \
 	reading_logs_with_offset \
 	reading_logs_via_rule_message \
 	simple_example_using_c \

examples/multiprocess_c/Makefile.am

@@ -15,7 +15,6 @@ multi_LDFLAGS = \
 	-L$(top_builddir)/src/.libs/ \
 	$(GEOIP_LDFLAGS) \
 	-lmodsecurity \
-	-lpthread \
 	-lm \
 	-lstdc++ \
 	$(LUA_LDFLAGS) \

examples/multithread/Makefile.am

@@ -0,0 +1,54 @@
+
+
+noinst_PROGRAMS = multithread
+
+multithread_SOURCES = \
+	multithread.cc
+
+multithread_LDADD = \
+	$(CURL_LDADD) \
+	$(GEOIP_LDADD) \
+	$(GLOBAL_LDADD) \
+	$(LIBXML2_LDADD) \
+	$(LMDB_LDADD) \
+	$(MAXMIND_LDADD) \
+	$(LUA_LDADD) \
+	$(PCRE_LDADD) \
+	$(SSDEEP_LDADD) \
+	$(YAJL_LDADD)
+
+multithread_LDFLAGS = \
+	-L$(top_builddir)/src/.libs/ \
+	$(GEOIP_LDFLAGS) \
+	-lmodsecurity \
+	-lm \
+	-lstdc++ \
+	$(LMDB_LDFLAGS) \
+	$(LUA_LDFLAGS) \
+	$(MAXMIND_LDFLAGS) \
+	$(SSDEEP_LDFLAGS) \
+	$(YAJL_LDFLAGS)
+
+multithread_CPPFLAGS = \
+	$(GLOBAL_CFLAGS) \
+	-I$(top_builddir)/headers \
+	-I$(top_builddir) \
+	-g \
+	-I../others \
+	-fPIC \
+	-O3 \
+	$(CURL_CFLAGS) \
+	$(GEOIP_CFLAGS) \
+	$(GLOBAL_CPPFLAGS) \
+	$(MODSEC_NO_LOGS) \
+	$(YAJL_CFLAGS) \
+	$(LMDB_CFLAGS) \
+	$(LUA_CFLAGS) \
+	$(PCRE_CFLAGS) \
+	$(LIBXML2_CFLAGS)
+
+
+MAINTAINERCLEANFILES = \
+	Makefile.in
+
+

examples/multithread/basic_rules.conf

@@ -0,0 +1,14 @@
+SecDebugLog debug.log
+SecDebugLogLevel 9
+
+
+SecRule REQUEST_HEADERS:User-Agent ".*" "id:1,phase:1,t:sha1,t:hexEncode,setvar:tx.ua_hash=%{MATCHED_VAR}"
+
+SecAction "id:2,phase:2,initcol:ip=%{REMOTE_ADDR}_%{tx.ua_hash}"
+
+SecRule REQUEST_HEADERS:User-Agent "@rx .*" "id:3,phase:2,setvar:ip.auth_attempt=+1"
+
+SecRule ARGS:foo "@rx herewego" "id:4,phase:2,setvar:ip.foo=bar,expirevar:ip.foo=2"
+#SecRule ARGS:foo "@rx herewego" "id:4,phase:2,setvar:ip.foo=bar"
+SecRule IP "@rx bar" "id:5,phase:2,pass"
+SecRule IP:auth_attempt "@rx bar" "id:6,phase:2,pass"

examples/multithread/multithread.cc

@@ -0,0 +1,68 @@
+#include <iostream>
+#include <thread>
+#include <array>
+
+#include <modsecurity/modsecurity.h>
+#include <modsecurity/transaction.h>
+#include <modsecurity/rules_set.h>
+
+static void process_request(modsecurity::ModSecurity *modsec, modsecurity::RulesSet *rules, int tid) {
+    std::cout << "Hello World! It's me, thread #" << tid << std::endl;
+
+    for(int i = 0; i != 1'000; i++) {
+        auto modsecTransaction = std::make_unique<modsecurity::Transaction>(modsec, rules, nullptr);
+
+        modsecTransaction->processConnection("127.0.0.1", 12345, "127.0.0.1", 80);
+        modsecTransaction->processURI(
+            "https://www.modsecurity.org/test?foo=herewego",
+            "GET", "1.1");
+
+        modsecTransaction->addRequestHeader("User-Agent",
+            "Basic ModSecurity example");
+        modsecTransaction->processRequestHeaders();
+        modsecTransaction->processRequestBody();
+
+        modsecTransaction->addResponseHeader("HTTP/1.1",
+            "200 OK");
+        modsecTransaction->processResponseHeaders(200, "HTTP 1.2");
+        modsecTransaction->processResponseBody();
+
+        modsecTransaction->processLogging();
+
+        std::this_thread::sleep_for(std::chrono::microseconds(100));
+    }
+
+    std::cout << "Thread #" << tid << " exits" << std::endl;
+}
+
+int main (int argc, char *argv[]) {
+    auto modsec = std::make_unique<modsecurity::ModSecurity>();
+    modsec->setConnectorInformation("ModSecurity-test v0.0.1-alpha (Simple " \
+        "example on how to use ModSecurity API");
+
+    char main_rule_uri[] = "basic_rules.conf";
+    auto rules = std::make_unique<modsecurity::RulesSet>();
+    if (rules->loadFromUri(main_rule_uri) < 0) {
+        std::cerr << "Problems loading the rules..." << std::endl;
+        std::cerr << rules->m_parserError.str() << std::endl;
+        return -1;
+    }
+
+    constexpr auto NUM_THREADS = 100;
+    std::array<std::thread, NUM_THREADS> threads;
+
+    for (auto i = 0; i != threads.size(); ++i) {
+        threads[i] = std::thread(
+            [&modsec, &rules, i]() {
+                process_request(modsec.get(), rules.get(), i);
+            });
+    }
+
+    std::this_thread::sleep_for(std::chrono::microseconds(10000));
+
+    for (auto i = 0; i != threads.size(); ++i) {
+        threads[i].join();
+    }
+
+    return 0;
+}

examples/reading_logs_via_rule_message/Makefile.am

@@ -21,7 +21,6 @@ simple_request_LDFLAGS = \
 	-L$(top_builddir)/src/.libs/ \
 	$(GEOIP_LDFLAGS) \
 	-lmodsecurity \
-	-lpthread \
 	-lm \
 	-lstdc++ \
 	$(LMDB_LDFLAGS) \

examples/reading_logs_via_rule_message/reading_logs_via_rule_message.h

@@ -13,14 +13,19 @@
  *
  */
 
+#ifndef EXAMPLES_READING_LOGS_VIA_RULE_MESSAGE_READING_LOGS_VIA_RULE_MESSAGE_H_
+#define EXAMPLES_READING_LOGS_VIA_RULE_MESSAGE_READING_LOGS_VIA_RULE_MESSAGE_H_
+
 #include <string>
 #include <memory>
 #include <thread>
+#include <array>
 #include <chrono>
-#include <pthread.h>
+
+#include "modsecurity/rule_message.h"
 
 
-#define NUM_THREADS 100
+constexpr auto NUM_THREADS = 100;
 
 
 char request_header[] =  "" \
@@ -62,40 +67,21 @@ char response_body[] = "" \
 
 char ip[] = "200.249.12.31";
 
-#include "modsecurity/rule_message.h"
 
-#ifndef EXAMPLES_READING_LOGS_VIA_RULE_MESSAGE_READING_LOGS_VIA_RULE_MESSAGE_H_
-#define EXAMPLES_READING_LOGS_VIA_RULE_MESSAGE_READING_LOGS_VIA_RULE_MESSAGE_H_
+static void process_request(modsecurity::ModSecurity *modsec, modsecurity::RulesSet *rules) {
+    for (auto z = 0; z < 10000; z++) {
+        auto modsecTransaction = std::make_unique<modsecurity::Transaction>(modsec, rules, nullptr);
 
-
-struct data_ms {
-    modsecurity::ModSecurity *modsec;
-    modsecurity::RulesSet *rules;
-};
-
-#if defined _MSC_VER
-#pragma warning(push)
-#pragma warning(disable:4716)   // avoid error C4716: 'process_request': must return a value, as MSVC C++ compiler doesn't support [[noreturn]]
-#pragma warning(disable:4715)   // avoid warning c4715: 'process_request' : not all control paths return a value
-#endif
-
-[[noreturn]] static void *process_request(void *data) {
-    struct data_ms *a = (struct data_ms *)data;
-    modsecurity::ModSecurity *modsec = a->modsec;
-    modsecurity::RulesSet *rules = a->rules;
-    int z = 0;
-
-    for (z = 0; z < 10000; z++) {
-        modsecurity::Transaction *modsecTransaction = \
-            new modsecurity::Transaction(modsec, rules, NULL);
         modsecTransaction->processConnection(ip, 12345, "127.0.0.1", 80);
         modsecTransaction->processURI(request_uri, "GET", "1.1");
 
         std::this_thread::sleep_for(std::chrono::microseconds(10));
+
         modsecTransaction->addRequestHeader("Host",
             "net.tutsplus.com");
         modsecTransaction->processRequestHeaders();
         modsecTransaction->processRequestBody();
+
         modsecTransaction->addResponseHeader("HTTP/1.1",
             "200 OK");
         modsecTransaction->processResponseHeaders(200, "HTTP 1.2");
@@ -103,18 +89,11 @@ struct data_ms {
             (const unsigned char*)response_body,
             strlen((const char*)response_body));
         modsecTransaction->processResponseBody();
+
         modsecTransaction->processLogging();
-
-        delete modsecTransaction;
     }
-
-    pthread_exit(nullptr);
 }
 
-#if defined _MSC_VER
-#pragma warning(pop)
-#endif
-
 class ReadingLogsViaRuleMessage {
  public:
     ReadingLogsViaRuleMessage(char *request_header,
@@ -134,11 +113,6 @@ class ReadingLogsViaRuleMessage {
         { }
 
     int process() const {
-        pthread_t threads[NUM_THREADS];
-        int i;
-        struct data_ms dms;
-        void *status;
-
         auto modsec = std::make_unique<modsecurity::ModSecurity>();
         modsec->setConnectorInformation("ModSecurity-test v0.0.1-alpha" \
             " (ModSecurity test)");
@@ -152,18 +126,19 @@ class ReadingLogsViaRuleMessage {
             return -1;
         }
 
-        dms.modsec = modsec.get();
-        dms.rules = rules.get();
+        std::array<std::thread, NUM_THREADS> threads;
 
-        for (i = 0; i < NUM_THREADS; i++) {
-            pthread_create(&threads[i], NULL, process_request,
-		reinterpret_cast<void *>(&dms));
+        for (auto i = 0; i != threads.size(); ++i) {
+            threads[i] = std::thread(
+                [&modsec, &rules]() {
+                    process_request(modsec.get(), rules.get());
+                });
         }
 
         std::this_thread::sleep_for(std::chrono::microseconds(10000));
 
-        for (i=0; i < NUM_THREADS; i++) {
-            pthread_join(threads[i], &status);
+        for (auto i = 0; i != threads.size(); ++i) {
+            threads[i].join();
             std::cout << "Main: completed thread id :" << i << std::endl;
         }
 

examples/reading_logs_with_offset/Makefile.am

@@ -21,7 +21,6 @@ read_LDFLAGS = \
 	-L$(top_builddir)/src/.libs/ \
 	$(GEOIP_LDFLAGS) \
 	-lmodsecurity \
-	-lpthread \
 	-lm \
 	-lstdc++ \
 	$(LMDB_LDFLAGS) \

examples/using_bodies_in_chunks/Makefile.am

@@ -21,12 +21,10 @@ simple_request_LDFLAGS = \
 	-L$(top_builddir)/src/.libs/ \
 	$(GEOIP_LDFLAGS) \
 	-lmodsecurity \
-	-lpthread \
 	-lm \
 	-lstdc++ \
 	$(MAXMIND_LDFLAGS) \
 	$(LMDB_LDFLAGS) \
-	-lpthread \
 	$(LUA_LDFLAGS) \
 	$(SSDEEP_LDFLAGS) \
 	$(YAJL_LDFLAGS)

src/collection/backend/in_memory-per_process.cc

@@ -25,8 +25,6 @@
 #include <memory>
 #endif
 
-#include <pthread.h>
-
 #include "modsecurity/variable_value.h"
 #include "src/utils/regex.h"
 #include "src/utils/string.h"
@@ -39,26 +37,48 @@ namespace backend {
 
 InMemoryPerProcess::InMemoryPerProcess(const std::string &name) :
     Collection(name) {
-    this->reserve(1000);
-    pthread_mutex_init(&m_lock, NULL);
+    m_map.reserve(1000);
 }
 
 InMemoryPerProcess::~InMemoryPerProcess() {
-    this->clear();
-    pthread_mutex_destroy(&m_lock);
+    m_map.clear();
 }
 
-void InMemoryPerProcess::store(std::string key, std::string value) {
-    pthread_mutex_lock(&m_lock);
-    this->emplace(key, value);
-    pthread_mutex_unlock(&m_lock);
+
+template<typename Map>
+inline void __store(Map &map, std::string key, std::string value) {
+    // NOTE: should be called with write-lock previously acquired
+
+    map.emplace(key, value);
+}
+
+
+template<typename Map>
+inline bool __updateFirst(Map &map,
+    const std::string &key,
+    const std::string &value) {
+    // NOTE: should be called with write-lock previously acquired
+
+    if (auto search = map.find(key); search != map.end()) {
+        search->second.setValue(value);
+        return true;
+    }
+
+    return false;
+}
+
+
+void InMemoryPerProcess::store(const std::string &key, const std::string &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) {
-    if (updateFirst(key, value) == false) {
-        store(key, value);
+    const std::lock_guard lock(m_mutex); // write lock (exclusive access)
+    if (__updateFirst(m_map, key, value) == false) {
+        __store(m_map, key, value);
     }
     return true;
 }
@@ -66,66 +86,59 @@ bool InMemoryPerProcess::storeOrUpdateFirst(const std::string &key,
 
 bool InMemoryPerProcess::updateFirst(const std::string &key,
     const std::string &value) {
-    pthread_mutex_lock(&m_lock);
-
-    if (auto search = this->find(key); search != this->end()) {
-        search->second.setValue(value);
-        pthread_mutex_unlock(&m_lock);
-        return true;
-    }
-
-    pthread_mutex_unlock(&m_lock);
-    return false;
+    const std::lock_guard lock(m_mutex); // write lock (exclusive access)
+    return __updateFirst(m_map, key, value);
 }
 
 
 void InMemoryPerProcess::del(const std::string& key) {
-    pthread_mutex_lock(&m_lock);
-    this->erase(key);
-    pthread_mutex_unlock(&m_lock);
+    const std::lock_guard lock(m_mutex); // write lock (exclusive access)
+    m_map.erase(key);
 }
 
 void InMemoryPerProcess::delIfExpired(const std::string& key) {
-    pthread_mutex_lock(&m_lock);
+    const std::lock_guard lock(m_mutex); // write lock (exclusive access)
     // Double check the status while within the mutex
-    auto iter = this->find(key);
-    if ((iter != this->end()) && (iter->second.isExpired())) {
-        this->erase(key);
+    const auto iter = std::find_if(m_map.begin(), m_map.end(),
+        [&key](const auto &x) { return x.first == key && x.second.isExpired(); });
+    if (iter != m_map.end()) {
+        m_map.erase(key);
     }
-    pthread_mutex_unlock(&m_lock);
 }
 
 void InMemoryPerProcess::setExpiry(const std::string& key, int32_t expiry_seconds) {
-    pthread_mutex_lock(&m_lock);
+    const std::lock_guard lock(m_mutex); // write lock (exclusive access)
 
-    if (auto search = this->find(key); search != this->end()) {
+    if (const auto search = m_map.find(key); search != m_map.end()) {
         search->second.setExpiry(expiry_seconds);
-        pthread_mutex_unlock(&m_lock);
         return;
     }
 
     // We allow an expiry value to be set for a key that has not (yet) had a value set.
-    auto iter = this->emplace(key, CollectionData());
+    const auto iter = m_map.emplace(key, CollectionData());
     iter->second.setExpiry(expiry_seconds);
-
-    pthread_mutex_unlock(&m_lock);
 }
 
 
 void InMemoryPerProcess::resolveSingleMatch(const std::string& var,
     std::vector<const VariableValue *> *l) {
     std::list<std::string> expiredVars;
-    auto range = this->equal_range(var);
 
-    for (auto it = range.first; it != range.second; ++it) {
-        if (it->second.isExpired()) {
-            expiredVars.push_back(it->first);
-	} else if (it->second.hasValue() == false) {
-            // No-op. A non-expired expiry exists for the key but there is no actual value
-	} else {
-            l->push_back(new VariableValue(&m_name, &it->first, &it->second.getValue()));
-	}
+    {
+        const std::shared_lock lock(m_mutex); // read lock (shared access)
+
+        const auto range = m_map.equal_range(var);
+        for (auto it = range.first; it != range.second; ++it) {
+            if (it->second.isExpired()) {
+                expiredVars.push_back(it->first);
+            } else if (it->second.hasValue() == false) {
+                // No-op. A non-expired expiry exists for the key but there is no actual value
+            } else {
+                l->push_back(new VariableValue(&m_name, &it->first, &it->second.getValue()));
+            }
+        }
     }
+
     for (const auto& expiredVar : expiredVars) {
         delIfExpired(expiredVar);
     }
@@ -134,40 +147,45 @@ void InMemoryPerProcess::resolveSingleMatch(const std::string& var,
 
 void InMemoryPerProcess::resolveMultiMatches(const std::string& var,
     std::vector<const VariableValue *> *l, variables::KeyExclusions &ke) {
-    size_t keySize = var.size();
+    const auto keySize = var.size();
     l->reserve(15);
     std::list<std::string> expiredVars;
 
-    if (keySize == 0) {
-        for (auto &i : *this) {
-            if (ke.toOmit(i.first)) {
-                continue;
+    {
+        const std::shared_lock lock(m_mutex); // read lock (shared access)
+
+        if (keySize == 0) {
+            for (auto &i : m_map) {
+                if (ke.toOmit(i.first)) {
+                    continue;
+                }
+                if (i.second.isExpired()) {
+                    expiredVars.push_back(i.first);
+                } else if (i.second.hasValue() == false) {
+                    // No-op. A non-expired expiry exists for the key but there is no actual value
+                } else {
+                    l->insert(l->begin(), new VariableValue(&m_name, &i.first,
+                        &i.second.getValue()));
+                }
             }
-            if (i.second.isExpired()) {
-                expiredVars.push_back(i.first);
-	    } else if (i.second.hasValue() == false) {
-                // No-op. A non-expired expiry exists for the key but there is no actual value
-	    } else {
-                l->insert(l->begin(), new VariableValue(&m_name, &i.first,
-                    &i.second.getValue()));
-	    }
-        }
-    } else {
-        auto range = this->equal_range(var);
-        for (auto it = range.first; it != range.second; ++it) {
-            if (ke.toOmit(var)) {
-                continue;
+        } else {
+            const auto range = m_map.equal_range(var);
+            for (auto it = range.first; it != range.second; ++it) {
+                if (ke.toOmit(var)) {
+                    continue;
+                }
+                if (it->second.isExpired()) {
+                    expiredVars.push_back(it->first);
+                } else if (it->second.hasValue() == false) {
+                    // No-op. A non-expired expiry exists for the key but there is no actual value
+                } else {
+                    l->insert(l->begin(), new VariableValue(&m_name, &var,
+                        &it->second.getValue()));
+                }
             }
-            if (it->second.isExpired()) {
-                expiredVars.push_back(it->first);
-	    } else if (it->second.hasValue() == false) {
-                // No-op. A non-expired expiry exists for the key but there is no actual value
-	    } else {
-                l->insert(l->begin(), new VariableValue(&m_name, &var,
-                    &it->second.getValue()));
-	    }
         }
     }
+
     for (const auto& expiredVar : expiredVars) {
         delIfExpired(expiredVar);
     }
@@ -176,47 +194,30 @@ void InMemoryPerProcess::resolveMultiMatches(const std::string& var,
 
 void InMemoryPerProcess::resolveRegularExpression(const std::string& var,
     std::vector<const VariableValue *> *l, variables::KeyExclusions &ke) {
-
-    //if (var.find(":") == std::string::npos) {
-    //    return;
-    //}
-    //if (var.size() < var.find(":") + 3) {
-    //    return;
-    //}
-    //std::string col = std::string(var, 0, var.find(":"));
-    //std::string name = std::string(var, var.find(":") + 2,
-    //    var.size() - var.find(":") - 3);
-    //size_t keySize = col.size();
     Utils::Regex r(var, true);
     std::list<std::string> expiredVars;
 
-    for (const auto& x : *this) {
-        //if (x.first.size() <= keySize + 1) {
-        //    continue;
-        //}
-        //if (x.first.at(keySize) != ':') {
-        //    continue;
-        //}
-        //if (std::string(x.first, 0, keySize) != col) {
-        //    continue;
-        //}
-        //std::string content = std::string(x.first, keySize + 1,
-         //                                 x.first.size() - keySize - 1);
-        int ret = Utils::regex_search(x.first, r);
-        if (ret <= 0) {
-            continue;
+    {
+        const std::shared_lock lock(m_mutex); // read lock (shared access)
+
+        for (const auto& x : m_map) {
+            const auto ret = Utils::regex_search(x.first, r);
+            if (ret <= 0) {
+                continue;
+            }
+            if (ke.toOmit(x.first)) {
+                continue;
+            }
+            if (x.second.isExpired()) {
+                expiredVars.push_back(x.first);
+            } else if (x.second.hasValue() == false) {
+                // No-op. A non-expired expiry exists for the key but there is no actual value
+            } else {
+                l->insert(l->begin(), new VariableValue(&m_name, &x.first, &x.second.getValue()));
+            }
         }
-        if (ke.toOmit(x.first)) {
-            continue;
-        }
-        if (x.second.isExpired()) {
-            expiredVars.push_back(x.first);
-	} else if (x.second.hasValue() == false) {
-            // No-op. A non-expired expiry exists for the key but there is no actual value
-	} else {
-            l->insert(l->begin(), new VariableValue(&m_name, &x.first, &x.second.getValue()));
-	}
     }
+
     for (const auto& expiredVar : expiredVars) {
         delIfExpired(expiredVar);
     }
@@ -225,18 +226,29 @@ void InMemoryPerProcess::resolveRegularExpression(const std::string& var,
 
 std::unique_ptr<std::string> InMemoryPerProcess::resolveFirst(
     const std::string& var) {
-    auto range = equal_range(var);
-    for (auto it = range.first; it != range.second; ++it) {
-        if (it->second.isExpired()) {
-            delIfExpired(it->second.getValue());
-	} else if (it->second.hasValue() == false) {
-            // No-op. A non-expired expiry exists for the key but there is no actual value
-	} else {
-            return std::unique_ptr<std::string>(new std::string(it->second.getValue()));
-	}
+    std::unique_ptr<std::string> ret;
+    std::list<std::string> expiredVars;
+
+    {
+        const std::shared_lock lock(m_mutex); // read lock (shared access)
+
+        const auto range = m_map.equal_range(var);
+        for (auto it = range.first; it != range.second; ++it) {
+            if (it->second.isExpired()) {
+                expiredVars.push_back(it->first);
+            } else if (it->second.hasValue() == false) {
+                // No-op. A non-expired expiry exists for the key but there is no actual value
+            } else {
+                ret = std::make_unique<std::string>(it->second.getValue());
+            }
+        }
     }
 
-    return NULL;
+    for (const auto& expiredVar : expiredVars) {
+        delIfExpired(expiredVar);
+    }
+
+    return ret;
 }
 
 

src/collection/backend/in_memory-per_process.h

@@ -12,7 +12,6 @@
  * directly using the email address security@modsecurity.org.
  *
  */
-#include <pthread.h>
 
 
 #ifdef __cplusplus
@@ -24,6 +23,7 @@
 #include <vector>
 #include <algorithm>
 #include <memory>
+#include <shared_mutex>
 #endif
 
 
@@ -71,13 +71,11 @@ struct MyHash{
 };
 
 class InMemoryPerProcess :
-    public std::unordered_multimap<std::string, CollectionData,
-        /*std::hash<std::string>*/MyHash, MyEqual>,
     public Collection {
  public:
     explicit InMemoryPerProcess(const std::string &name);
     ~InMemoryPerProcess();
-    void store(std::string key, std::string value);
+    void store(const std::string &key, const std::string &value);
 
     bool storeOrUpdateFirst(const std::string &key,
         const std::string &value) override;
@@ -103,21 +101,23 @@ class InMemoryPerProcess :
         variables::KeyExclusions &ke) override;
 
     /* store */
-    virtual void store(std::string key, std::string compartment,
+    virtual void store(const std::string &key, std::string &compartment,
         std::string value) {
-        std::string nkey = compartment + "::" + key;
+        const auto nkey = compartment + "::" + key;
         store(nkey, value);
     }
 
 
-    virtual void store(std::string key, std::string compartment,
+    virtual void store(const std::string &key, const std::string &compartment,
         std::string compartment2, std::string value) {
-        std::string nkey = compartment + "::" + compartment2 + "::" + key;
+        const auto nkey = compartment + "::" + compartment2 + "::" + key;
         store(nkey, value);
     }
 
  private:
-    pthread_mutex_t m_lock;
+    std::unordered_multimap<std::string, CollectionData,
+        /*std::hash<std::string>*/MyHash, MyEqual> m_map;
+    std::shared_mutex m_mutex;
 };
 
 }  // namespace backend

src/collection/backend/lmdb.cc

@@ -27,8 +27,6 @@
 #include <string>
 #include <memory>
 
-#include <pthread.h>
-
 #include "modsecurity/variable_value.h"
 #include "src/utils/regex.h"
 #include "src/variables/variable.h"

src/collection/backend/lmdb.h

@@ -27,12 +27,10 @@
 
 #ifdef WITH_LMDB
 #include <lmdb.h>
-#include <semaphore.h>
 #endif  // WITH_LMDB
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <fcntl.h>
-#include <pthread.h>
 
 #include "modsecurity/variable_value.h"
 #include "modsecurity/collection/collection.h"

src/operators/pm.cc

@@ -39,9 +39,6 @@ Pm::~Pm() {
 
     free(m_p);
     m_p = NULL;
-#ifdef MODSEC_MUTEX_ON_PM
-    pthread_mutex_destroy(&m_lock);
-#endif
 }
 
 
@@ -89,11 +86,12 @@ bool Pm::evaluate(Transaction *transaction, RuleWithActions *rule,
     pt.ptr = NULL;
     const char *match = NULL;
 #ifdef MODSEC_MUTEX_ON_PM
-    pthread_mutex_lock(&m_lock);
+    {
+    const std::lock_guard lock(m_mutex);
 #endif
     rc = acmp_process_quick(&pt, &match, input.c_str(), input.length());
 #ifdef MODSEC_MUTEX_ON_PM
-    pthread_mutex_unlock(&m_lock);
+    }
 #endif
 
     if (rc >= 0 && transaction) {
@@ -118,9 +116,6 @@ bool Pm::init(const std::string &file, std::string *error) {
     std::istringstream *iss;
     const char *err = NULL;
 
-#ifdef MODSEC_MUTEX_ON_PM
-    pthread_mutex_init(&m_lock, NULL);
-#endif
     char *content = parse_pm_content(m_param.c_str(), m_param.length(), &err);
     if (content == NULL) {
         iss = new std::istringstream(m_param);

src/operators/pm.h

@@ -20,6 +20,7 @@
 #include <list>
 #include <memory>
 #include <utility>
+#include <mutex>
 
 #include "src/operators/operator.h"
 #include "src/utils/acmp.h"
@@ -56,7 +57,7 @@ class Pm : public Operator {
 #ifdef MODSEC_MUTEX_ON_PM
 
  private:
-    pthread_mutex_t m_lock;
+    std::mutex m_mutex;
 #endif
 };