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.

Author: eduar-hte

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<typename Map>
+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<typename Map>
+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<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 +149,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 +196,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;
-        }
-        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<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());
+            }
+        }
+    }
+
+    for (const auto& expiredVar : expiredVars) {
+        delIfExpired(expiredVar);
     }
 
-    return NULL;
+    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