From e2b3c9594ff47a0d9abb71dfe234f73ecbf9816e Mon Sep 17 00:00:00 2001 From: Eduardo Arias Date: Wed, 7 Aug 2024 08:33:03 -0700 Subject: [PATCH 1/4] Prevent concurrent access to data structure in resolve methods - As reported in #3054, the resolve methods in InMemoryPerProcess are not acquiring a lock/mutex to prevent concurrent access to the data structures that may be modified at the same time from other threads, and thus triggering undefined behaviour. - Replace inheritance of std::unordered_multimap data structure with data member to prevent potential clients to use it without acquiring the mutex to protect concurrent access. - Replace pthreads lock with std C++11 std::shared_mutex - Provides exclusive/shared lock access so that multiple readers can access the data at the same time, but only one writer. this is used to favor query performance by allowing more concurrent access to the data until an update needs to be performed. - Simplifies acquisition and disposal of lock/mutex with std::lock_guard, which has RAII semantics. - NOTE: Because std::shared_mutex is not recursive, calls to another function that tries to acquire the lock will fail. Introduced __store & __updateFirst helper methods to workaround this. - Updates to InMemoryPerProcess::resolveFirst - Updated the code to store the expired var in 'expiredVars' to delete them after iterating over the range (and releasing the read lock, as 'delIfExpired' needs to acquire it for exclusive access), as the current call to 'delIfExpired' would invalidate the range triggering undefined behaviour on the following iteration. - Noticed that in commit 118e1b3 the call to 'delIfExpired' in this function is done using 'it->second.getValue()'' instead of 'it->first', which seems incorrect (based on similar code in other resolveXXX functions). - Updated InMemoryPerProcess::delIfExpired to use 'std::find_if' (with a lambda that matches both the key and the 'isExpired' condition) because the data structure is a multimap. The version introduced in commit 118e1b3 could find an entry (not necessarily the first, because the map is unordered) where 'isExpired' is 'false' and exit, while another entry could be expired. --- .../backend/in_memory-per_process.cc | 248 +++++++++--------- .../backend/in_memory-per_process.h | 18 +- 2 files changed, 140 insertions(+), 126 deletions(-) diff --git a/src/collection/backend/in_memory-per_process.cc b/src/collection/backend/in_memory-per_process.cc index a4ca5e2e5d..57e87072f9 100644 --- a/src/collection/backend/in_memory-per_process.cc +++ b/src/collection/backend/in_memory-per_process.cc @@ -39,93 +39,108 @@ 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 +inline void __store(Map &map, std::string key, std::string value) { + // NOTE: should be called with write-lock previously acquired -bool InMemoryPerProcess::storeOrUpdateFirst(const std::string &key, - const std::string &value) { - if (updateFirst(key, value) == false) { - store(key, value); - } - return true; + map.emplace(key, value); } -bool InMemoryPerProcess::updateFirst(const std::string &key, +template +inline bool __updateFirst(Map &map, + const std::string &key, const std::string &value) { - pthread_mutex_lock(&m_lock); + // NOTE: should be called with write-lock previously acquired - if (auto search = this->find(key); search != this->end()) { + if (auto search = map.find(key); search != map.end()) { search->second.setValue(value); - pthread_mutex_unlock(&m_lock); return true; } - pthread_mutex_unlock(&m_lock); 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) { + 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; +} + + +bool InMemoryPerProcess::updateFirst(const std::string &key, + const std::string &value) { + 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 *l) { std::list 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 +149,45 @@ void InMemoryPerProcess::resolveSingleMatch(const std::string& var, void InMemoryPerProcess::resolveMultiMatches(const std::string& var, std::vector *l, variables::KeyExclusions &ke) { - size_t keySize = var.size(); + const auto keySize = var.size(); l->reserve(15); std::list 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 +196,30 @@ void InMemoryPerProcess::resolveMultiMatches(const std::string& var, void InMemoryPerProcess::resolveRegularExpression(const std::string& var, std::vector *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 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; - } - if (ke.toOmit(x.first)) { - 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 (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 +228,29 @@ void InMemoryPerProcess::resolveRegularExpression(const std::string& var, std::unique_ptr 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(new std::string(it->second.getValue())); - } + std::unique_ptr ret; + std::list 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(it->second.getValue()); + } + } + } + + for (const auto& expiredVar : expiredVars) { + delIfExpired(expiredVar); } - return NULL; + return ret; } diff --git a/src/collection/backend/in_memory-per_process.h b/src/collection/backend/in_memory-per_process.h index 3cacdca0d2..2e283d2582 100644 --- a/src/collection/backend/in_memory-per_process.h +++ b/src/collection/backend/in_memory-per_process.h @@ -12,7 +12,6 @@ * directly using the email address security@modsecurity.org. * */ -#include #ifdef __cplusplus @@ -24,6 +23,7 @@ #include #include #include +#include #endif @@ -71,13 +71,11 @@ struct MyHash{ }; class InMemoryPerProcess : - public std::unordered_multimap*/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*/MyHash, MyEqual> m_map; + std::shared_mutex m_mutex; }; } // namespace backend From 4e15f9ef7118088d3c414dabe4fb1d3cc62b4e13 Mon Sep 17 00:00:00 2001 From: Eduardo Arias Date: Wed, 7 Aug 2024 11:50:55 -0700 Subject: [PATCH 2/4] Turn off LMDB by default in Windows build to align with defaults for other platforms - Replaced WITHOUT_XXX build options with WITH_XXX to make it easier to understand and configure. - Updated GitHub workflow to align with these changes and include a build 'with lmdb' (again, analogous to non-Windows configurations) --- .github/workflows/ci.yml | 10 +++++----- build/win32/CMakeLists.txt | 26 +++++++++++++------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 10ac514454..71ca3c7c57 100644 --- a/.github/workflows/ci.yml +++ b/.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: diff --git a/build/win32/CMakeLists.txt b/build/win32/CMakeLists.txt index bdb9a45f22..83b020a798 100644 --- a/build/win32/CMakeLists.txt +++ b/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,17 +93,17 @@ 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) From 293cd214c79c8104c87d52188987956ca3ce1b36 Mon Sep 17 00:00:00 2001 From: Eduardo Arias Date: Wed, 7 Aug 2024 13:54:58 -0700 Subject: [PATCH 3/4] Removed usage of pthreads and replaced with std C++ features - Replaced pthread_mutex_t in modsecurity::operators::Pm with std::mutex - Replaced pthread's thread usage in reading_logs_via_rule_message example with std::thread. - Simplified and modernized C++ code. - Removed unnecessary includes of pthread.h --- build/win32/CMakeLists.txt | 4 +- build/win32/conanfile.txt | 1 - examples/multiprocess_c/Makefile.am | 1 - .../reading_logs_via_rule_message/Makefile.am | 1 - .../reading_logs_via_rule_message.h | 67 ++++++------------- examples/reading_logs_with_offset/Makefile.am | 1 - examples/using_bodies_in_chunks/Makefile.am | 2 - .../backend/in_memory-per_process.cc | 2 - src/collection/backend/lmdb.cc | 2 - src/collection/backend/lmdb.h | 2 - src/operators/pm.cc | 11 +-- src/operators/pm.h | 3 +- 12 files changed, 27 insertions(+), 70 deletions(-) diff --git a/build/win32/CMakeLists.txt b/build/win32/CMakeLists.txt index 83b020a798..1e6303fdb6 100644 --- a/build/win32/CMakeLists.txt +++ b/build/win32/CMakeLists.txt @@ -110,7 +110,6 @@ 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,7 +254,6 @@ 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) diff --git a/build/win32/conanfile.txt b/build/win32/conanfile.txt index b1a6982156..b8f9721d0a 100644 --- a/build/win32/conanfile.txt +++ b/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 diff --git a/examples/multiprocess_c/Makefile.am b/examples/multiprocess_c/Makefile.am index 85c2648726..726d1d9057 100644 --- a/examples/multiprocess_c/Makefile.am +++ b/examples/multiprocess_c/Makefile.am @@ -15,7 +15,6 @@ multi_LDFLAGS = \ -L$(top_builddir)/src/.libs/ \ $(GEOIP_LDFLAGS) \ -lmodsecurity \ - -lpthread \ -lm \ -lstdc++ \ $(LUA_LDFLAGS) \ diff --git a/examples/reading_logs_via_rule_message/Makefile.am b/examples/reading_logs_via_rule_message/Makefile.am index 210edef3bc..55d3a93f5e 100644 --- a/examples/reading_logs_via_rule_message/Makefile.am +++ b/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) \ diff --git a/examples/reading_logs_via_rule_message/reading_logs_via_rule_message.h b/examples/reading_logs_via_rule_message/reading_logs_via_rule_message.h index 6b2800482c..33148cd7d0 100644 --- a/examples/reading_logs_via_rule_message/reading_logs_via_rule_message.h +++ b/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 #include #include +#include #include -#include + +#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_ - -struct data_ms { - modsecurity::ModSecurity *modsec; - modsecurity::RulesSet *rules; -}; +static void process_request(modsecurity::ModSecurity *modsec, modsecurity::RulesSet *rules) { + for (auto z = 0; z < 10000; z++) { + auto modsecTransaction = std::make_unique(modsec, rules, nullptr); -#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; + modsecTransaction->processLogging(); } - - 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(); 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 threads; - for (i = 0; i < NUM_THREADS; i++) { - pthread_create(&threads[i], NULL, process_request, - reinterpret_cast(&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; } diff --git a/examples/reading_logs_with_offset/Makefile.am b/examples/reading_logs_with_offset/Makefile.am index f845c5837a..3ecda10cbb 100644 --- a/examples/reading_logs_with_offset/Makefile.am +++ b/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) \ diff --git a/examples/using_bodies_in_chunks/Makefile.am b/examples/using_bodies_in_chunks/Makefile.am index aa28fe0782..5d64537992 100644 --- a/examples/using_bodies_in_chunks/Makefile.am +++ b/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) diff --git a/src/collection/backend/in_memory-per_process.cc b/src/collection/backend/in_memory-per_process.cc index 57e87072f9..b16ee843ac 100644 --- a/src/collection/backend/in_memory-per_process.cc +++ b/src/collection/backend/in_memory-per_process.cc @@ -25,8 +25,6 @@ #include #endif -#include - #include "modsecurity/variable_value.h" #include "src/utils/regex.h" #include "src/utils/string.h" diff --git a/src/collection/backend/lmdb.cc b/src/collection/backend/lmdb.cc index 85c68e3cdd..4a77f5fb81 100644 --- a/src/collection/backend/lmdb.cc +++ b/src/collection/backend/lmdb.cc @@ -27,8 +27,6 @@ #include #include -#include - #include "modsecurity/variable_value.h" #include "src/utils/regex.h" #include "src/variables/variable.h" diff --git a/src/collection/backend/lmdb.h b/src/collection/backend/lmdb.h index 15c4fa9f72..49633e37b9 100644 --- a/src/collection/backend/lmdb.h +++ b/src/collection/backend/lmdb.h @@ -27,12 +27,10 @@ #ifdef WITH_LMDB #include -#include #endif // WITH_LMDB #include #include #include -#include #include "modsecurity/variable_value.h" #include "modsecurity/collection/collection.h" diff --git a/src/operators/pm.cc b/src/operators/pm.cc index ebf31c4077..d92fc163f2 100644 --- a/src/operators/pm.cc +++ b/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); diff --git a/src/operators/pm.h b/src/operators/pm.h index b090ec5dc5..ed5649af85 100644 --- a/src/operators/pm.h +++ b/src/operators/pm.h @@ -20,6 +20,7 @@ #include #include #include +#include #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 }; From 4bf9616f9e88ea758f0c334e697261702bc07d39 Mon Sep 17 00:00:00 2001 From: Eduardo Arias Date: Fri, 9 Aug 2024 10:56:36 -0700 Subject: [PATCH 4/4] Adding multithreaded example from issue #3054 (by airween) - Rewritten to use C++ libModSecurity API and std::thread (instead of pthreads) --- .gitignore | 1 + build/win32/CMakeLists.txt | 4 ++ build/win32/README.md | 1 + configure.ac | 1 + examples/Makefile.am | 1 + examples/multithread/Makefile.am | 54 +++++++++++++++++++++ examples/multithread/basic_rules.conf | 14 ++++++ examples/multithread/multithread.cc | 68 +++++++++++++++++++++++++++ 8 files changed, 144 insertions(+) create mode 100644 examples/multithread/Makefile.am create mode 100644 examples/multithread/basic_rules.conf create mode 100644 examples/multithread/multithread.cc diff --git a/.gitignore b/.gitignore index be0de1557c..9515fb56ec 100644 --- a/.gitignore +++ b/.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 diff --git a/build/win32/CMakeLists.txt b/build/win32/CMakeLists.txt index 1e6303fdb6..cebf4c9be2 100644 --- a/build/win32/CMakeLists.txt +++ b/build/win32/CMakeLists.txt @@ -259,6 +259,10 @@ setExampleTargetProperties(reading_logs_via_rule_message) 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 # diff --git a/build/win32/README.md b/build/win32/README.md index 7b70cfbdf8..85fd868edb 100644 --- a/build/win32/README.md +++ b/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` diff --git a/configure.ac b/configure.ac index 3a0fbb4855..34281a3f01 100644 --- a/configure.ac +++ b/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 \ diff --git a/examples/Makefile.am b/examples/Makefile.am index 609cb93ef0..2dbb21ace7 100644 --- a/examples/Makefile.am +++ b/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 \ diff --git a/examples/multithread/Makefile.am b/examples/multithread/Makefile.am new file mode 100644 index 0000000000..40e81dd15c --- /dev/null +++ b/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 + + diff --git a/examples/multithread/basic_rules.conf b/examples/multithread/basic_rules.conf new file mode 100644 index 0000000000..cff1834261 --- /dev/null +++ b/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" diff --git a/examples/multithread/multithread.cc b/examples/multithread/multithread.cc new file mode 100644 index 0000000000..4233bc71fa --- /dev/null +++ b/examples/multithread/multithread.cc @@ -0,0 +1,68 @@ +#include +#include +#include + +#include +#include +#include + +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(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(); + 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(); + 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 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; +} \ No newline at end of file