fix: make setvar =+N / =-N an atomic update in the collection backend

headers/modsecurity/collection/collection.h

@@ -69,6 +69,30 @@
         std::vector<const VariableValue *> *l,
         variables::KeyExclusions &ke) = 0;
 
+    /* incrementFirst: add `delta` to the integer stored under `key` (missing,
+     * expired or non-numeric values count as 0) and return the new value.
+     * Backends do the read-modify-write as one atomic step; this default is
+     * only a non-atomic fallback for backends that do not override it. */
+    virtual int incrementFirst(const std::string &key, int delta) {
+        int value = 0;
+        if (auto current = resolveFirst(key)) {
+            try {
+                value = std::stoi(*current);
+            } catch (...) {
+                value = 0;
+            }
+        }
+        value += delta;
+        storeOrUpdateFirst(key, std::to_string(value));
+        return value;
+    }
+
+    virtual int incrementFirst(const std::string &key,
+        std::string compartment, std::string compartment2, int delta) {
+        std::string nkey = compartment + "::" + compartment2 + "::" + key;
+        return incrementFirst(nkey, delta);
+    }
+
 
     /* storeOrUpdateFirst */
     virtual bool storeOrUpdateFirst(const std::string &key,

src/actions/set_var.cc

@@ -90,36 +90,37 @@
         }
         goto end;
     } else {
-        int pre = 0;
+        /* =+N / =-N: the backend reads, adds and writes in one atomic step */
+        int delta = 0;
         int value = 0;
 
         try {
-            pre = stoi(resolvedPre);
+            delta = stoi(resolvedPre);
         } catch (...) {
-            pre = 0;
+            delta = 0;
         }
-
-        try {
-            std::vector<const VariableValue *> l;
-            RuleWithOperator *rr = dynamic_cast<RuleWithOperator *>(rule);
-            m_variable->evaluate(t, rr, &l);
-            if (l.size() == 0) {
-                value = 0;
-            } else {
-                value = stoi(l[0]->getValue());
-                for (auto &i : l) {
-                    delete i;
-                }
-            }
-        } catch (...) {
-            value = 0;
+        if (m_operation == substractAndSetOperation) {
+            delta = -delta;
         }
 
-        if (m_operation == sumAndSetOperation) {
-            targetValue = std::to_string(value + pre);
-        } else if (m_operation == substractAndSetOperation) {
-            targetValue = std::to_string(value - pre);
+        if (tx) {
+            value = tx->incrementFirst(t, m_variableNameExpanded, delta);
+        } else if (session) {
+            value = session->incrementFirst(t, m_variableNameExpanded, delta);
+        } else if (ip) {
+            value = ip->incrementFirst(t, m_variableNameExpanded, delta);
+        } else if (resource) {
+            value = resource->incrementFirst(t, m_variableNameExpanded, delta);
+        } else if (global) {
+            value = global->incrementFirst(t, m_variableNameExpanded, delta);
+        } else if (user) {
+            value = user->incrementFirst(t, m_variableNameExpanded, delta);
         }
+
+        ms_dbg_a(t, 8, "Saving variable: " + m_variable->m_collectionName \
+            + ":" + m_variableNameExpanded + " with value: " \
+            + std::to_string(value));
+        goto end;
     }
 
     ms_dbg_a(t, 8, "Saving variable: " + m_variable->m_collectionName \

src/collection/backend/in_memory-per_process.cc

@@ -91,6 +91,33 @@
 }
 
 
+int InMemoryPerProcess::incrementFirst(const std::string &key, int delta) {
+    const std::lock_guard lock(m_mutex); // write lock (exclusive access)
+    int value = 0;
+
+    auto search = m_map.find(key);
+    if (search != m_map.end() && search->second.isExpired()) {
+        m_map.erase(search);
+        search = m_map.end();
+    }
+    if (search != m_map.end() && search->second.hasValue()) {
+        try {
+            value = std::stoi(search->second.getValue());
+        } catch (...) {
+            value = 0;
+        }
+    }
+    value += delta;
+
+    if (search != m_map.end()) {
+        search->second.setValue(std::to_string(value));
+    } else {
+        __store(m_map, key, std::to_string(value));
+    }
+    return value;
+}
+
+
 void InMemoryPerProcess::del(const std::string& key) {
     const std::lock_guard lock(m_mutex); // write lock (exclusive access)
     m_map.erase(key);

src/collection/backend/in_memory-per_process.h

@@ -83,6 +83,8 @@
     bool updateFirst(const std::string &key,
         const std::string &value) override;
 
+    int incrementFirst(const std::string &key, int delta) override;
+
     void del(const std::string& key) override;
 
     void delIfExpired(const std::string& key);

src/collection/backend/lmdb.cc

@@ -355,6 +355,77 @@
 }
 
 
+int LMDB::incrementFirst(const std::string &key, int delta) {
+    int rc;
+    int value = 0;
+    MDB_txn *txn;
+    MDB_val mdb_key;
+    MDB_val mdb_value;
+    MDB_val mdb_value_ret;
+    CollectionData previous_data;
+    CollectionData new_data;
+    std::string serializedData;
+
+    string2val(key, &mdb_key);
+
+    /* A write transaction holds the LMDB writer lock, so the read, the
+     * add and the write below are one atomic step across threads and
+     * processes. */
+    rc = txn_begin(0, &txn);
+    lmdb_debug(rc, "txn", "incrementFirst");
+    if (rc != 0) {
+        goto end_txn;
+    }
+
+    rc = mdb_get(txn, m_dbi, &mdb_key, &mdb_value_ret);
+    lmdb_debug(rc, "get", "incrementFirst");
+    if (rc == 0) {
+        previous_data.setFromSerialized(reinterpret_cast<char *>(mdb_value_ret.mv_data), mdb_value_ret.mv_size);
+        if (previous_data.hasValue() && !previous_data.isExpired()) {
+            try {
+                value = std::stoi(previous_data.getValue());
+            } catch (...) {
+                value = 0;
+            }
+        }
+        rc = mdb_del(txn, m_dbi, &mdb_key, &mdb_value_ret);
+        lmdb_debug(rc, "del", "incrementFirst");
+        if (rc != 0) {
+            goto end_del;
+        }
+    }
+    value += delta;
+
+    if (previous_data.hasExpiry() && !previous_data.isExpired()) {
+        new_data = previous_data;
+    }
+    new_data.setValue(std::to_string(value));
+    serializedData = new_data.getSerialized();
+    string2val(serializedData, &mdb_value);
+
+    rc = mdb_put(txn, m_dbi, &mdb_key, &mdb_value, 0);
+    lmdb_debug(rc, "put", "incrementFirst");
+    if (rc != 0) {
+        goto end_put;
+    }
+
+    rc = mdb_txn_commit(txn);
+    lmdb_debug(rc, "commit", "incrementFirst");
+    if (rc != 0) {
+        goto end_commit;
+    }
+
+end_put:
+end_del:
+    if (rc != 0) {
+        mdb_txn_abort(txn);
+    }
+end_commit:
+end_txn:
+    return value;
+}
+
+
 void LMDB::resolveSingleMatch(const std::string& var,
     std::vector<const VariableValue *> *l) {
     int rc;

src/collection/backend/lmdb.h

@@ -103,6 +103,8 @@ class LMDB :
     bool updateFirst(const std::string &key,
         const std::string &value) override;
 
+    int incrementFirst(const std::string &key, int delta) override;
+
     void del(const std::string& key) override;
 
     void setExpiry(const std::string& key, int32_t expiry_seconds) override;

src/variables/global.h

@@ -111,6 +111,14 @@ class Global_DynamicElement : public Variable {
             t->m_rules->m_secWebAppId.m_value, expiry_seconds);
     }
 
+    static int incrementFirst(Transaction *t, const std::string &var,
+        int delta) {
+        return t->m_collections.m_global_collection->incrementFirst(
+            var, t->m_collections.m_global_collection_key,
+            t->m_rules->m_secWebAppId.m_value,
+            delta);
+    }
+
     static void storeOrUpdateFirst(Transaction *t, const std::string &var,
         const std::string &value) {
         t->m_collections.m_global_collection->storeOrUpdateFirst(

src/variables/ip.h

@@ -110,6 +110,14 @@ class Ip_DynamicElement : public Variable {
             t->m_rules->m_secWebAppId.m_value, expiry_seconds);
     }
 
+    static int incrementFirst(Transaction *t, const std::string &var,
+        int delta) {
+        return t->m_collections.m_ip_collection->incrementFirst(
+            var, t->m_collections.m_ip_collection_key,
+            t->m_rules->m_secWebAppId.m_value,
+            delta);
+    }
+
     static void storeOrUpdateFirst(Transaction *t, const std::string &var,
         const std::string &value) {
         t->m_collections.m_ip_collection->storeOrUpdateFirst(

src/variables/resource.h

@@ -111,6 +111,14 @@ class Resource_DynamicElement : public Variable {
     }
 
 
+    static int incrementFirst(Transaction *t, const std::string &var,
+        int delta) {
+        return t->m_collections.m_resource_collection->incrementFirst(
+            var, t->m_collections.m_resource_collection_key,
+            t->m_rules->m_secWebAppId.m_value,
+            delta);
+    }
+
     static void storeOrUpdateFirst(Transaction *t, const std::string &var,
         const std::string &value) {
         t->m_collections.m_resource_collection->storeOrUpdateFirst(

src/variables/session.h

@@ -110,6 +110,14 @@ class Session_DynamicElement : public Variable {
             t->m_rules->m_secWebAppId.m_value, expiry_seconds);
     }
 
+    static int incrementFirst(Transaction *t, const std::string &var,
+        int delta) {
+        return t->m_collections.m_session_collection->incrementFirst(
+            var, t->m_collections.m_session_collection_key,
+            t->m_rules->m_secWebAppId.m_value,
+            delta);
+    }
+
     static void storeOrUpdateFirst(Transaction *t, const std::string &var,
         const std::string &value) {
         t->m_collections.m_session_collection->storeOrUpdateFirst(

src/variables/tx.h

@@ -103,6 +103,11 @@ class Tx_DynamicElement : public Variable {
         t->m_collections.m_tx_collection->storeOrUpdateFirst(var, value);
     }
 
+    static int incrementFirst(Transaction *t, const std::string &var,
+        int delta) {
+        return t->m_collections.m_tx_collection->incrementFirst(var, delta);
+    }
+
     std::unique_ptr<RunTimeString> m_string;
 };
 

src/variables/user.h

@@ -110,6 +110,14 @@ class User_DynamicElement : public Variable {
             t->m_rules->m_secWebAppId.m_value, expiry_seconds);
     }
 
+    static int incrementFirst(Transaction *t, const std::string &var,
+        int delta) {
+        return t->m_collections.m_user_collection->incrementFirst(
+            var, t->m_collections.m_user_collection_key,
+            t->m_rules->m_secWebAppId.m_value,
+            delta);
+    }
+
     static void storeOrUpdateFirst(Transaction *t, const std::string &var,
         const std::string &value) {
         t->m_collections.m_user_collection->storeOrUpdateFirst(

test/test-cases/regression/issue-2306.json

@@ -0,0 +1,91 @@
+[
+  {
+    "enabled": 1,
+    "version_min": 300000,
+    "version_max": 0,
+    "title": "Testing Issue #2306 - setvar =+N adds to the stored counter",
+    "client": {
+      "ip": "200.249.12.31",
+      "port": 2313
+    },
+    "server": {
+      "ip": "200.249.12.31",
+      "port": 80
+    },
+    "request": {
+      "headers": {
+        "Host": "www.modsecurity.org",
+        "User-Agent": "Mozilla/5.0",
+        "Content-Length": "0"
+      },
+      "uri": "/",
+      "method": "GET",
+      "http_version": 1.1,
+      "body": [
+        ""
+      ]
+    },
+    "response": {
+      "headers": {
+        "Content-Length": "0"
+      },
+      "body": [
+        ""
+      ]
+    },
+    "expected": {
+      "debug_log": "Saving variable: IP:counter with value: 5",
+      "http_code": 403
+    },
+    "rules": [
+      "SecRuleEngine On",
+      "SecAction \"id:1,phase:1,pass,nolog,initcol:ip=%{REMOTE_ADDR}\"",
+      "SecAction \"id:2,phase:1,pass,nolog,setvar:ip.counter=+2,setvar:ip.counter=+3\"",
+      "SecRule IP:counter \"@eq 5\" \"id:3,phase:1,deny,status:403\""
+    ]
+  },
+  {
+    "enabled": 1,
+    "version_min": 300000,
+    "version_max": 0,
+    "title": "Testing Issue #2306 - setvar =-N on a non-numeric value counts it as 0",
+    "client": {
+      "ip": "200.249.12.31",
+      "port": 2313
+    },
+    "server": {
+      "ip": "200.249.12.31",
+      "port": 80
+    },
+    "request": {
+      "headers": {
+        "Host": "www.modsecurity.org",
+        "User-Agent": "Mozilla/5.0",
+        "Content-Length": "0"
+      },
+      "uri": "/",
+      "method": "GET",
+      "http_version": 1.1,
+      "body": [
+        ""
+      ]
+    },
+    "response": {
+      "headers": {
+        "Content-Length": "0"
+      },
+      "body": [
+        ""
+      ]
+    },
+    "expected": {
+      "debug_log": "Saving variable: TX:counter with value: -3",
+      "http_code": 403
+    },
+    "rules": [
+      "SecRuleEngine On",
+      "SecAction \"id:1,phase:1,pass,nolog,setvar:tx.counter=abc,setvar:tx.counter=-3\"",
+      "SecRule TX:counter \"@eq -3\" \"id:2,phase:1,deny,status:403\""
+    ]
+  }
+]