Add atomic updates for persistent counters. See #20.

CHANGES

@@ -1,6 +1,8 @@
 09 May 2008 - trunk
 -------------------
 
+* Persistent counter updates are now atomic.
+
 * Force modules "mod_rpaf-2.0.c" and "mod_custom_header.c" to run before
   ModSecurity so that the correct IP is used.
 

apache2/modsecurity.c

@@ -302,6 +302,8 @@ apr_status_t modsecurity_tx_init(modsec_rec *msr) {
     msr->geo_vars = apr_table_make(msr->mp, 8);
     if (msr->geo_vars == NULL) return -1;
 
+    msr->collections_original = apr_table_make(msr->mp, 8);
+    if (msr->collections_original == NULL) return -1;
     msr->collections = apr_table_make(msr->mp, 8);
     if (msr->collections == NULL) return -1;
     msr->collections_dirty = apr_table_make(msr->mp, 8);

apache2/modsecurity.h

@@ -351,6 +351,7 @@ struct modsec_rec {
     int                  upload_remove_files;
 
     /* other */
+    apr_table_t         *collections_original;
     apr_table_t         *collections;
     apr_table_t         *collections_dirty;
 

apache2/persist_dbm.c

@@ -31,17 +31,26 @@ static apr_table_t *collection_unpack(modsec_rec *msr, const unsigned char *blob
 
         var->name_len = (blob[blob_offset] << 8) + blob[blob_offset + 1];
         if (var->name_len == 0) {
-            /* This should never happen as the length includes the terminating
+            /* Is the length a name length, or just the end of the blob? */
-             * NUL and should be 1 for ""
+            if (blob_offset < blob_size - 2) {
-             */
+                /* This should never happen as the name length
-            msr_log(msr, 4, "Possiblly corrupted database: var name length = 0 at blob offset %u-%u.", blob_offset, blob_offset + 1);
+                 * includes the terminating NUL and should be 1 for ""
+                 */
+                if (msr->txcfg->debuglog_level >= 9) {
+                    msr_log(msr, 9, "BLOB[%d]: %s", blob_offset, log_escape_hex(msr->mp, blob + blob_offset, blob_size - blob_offset));
+                }
+                msr_log(msr, 4, "Possibly corrupted database: var name length = 0 at blob offset %u-%u.", blob_offset, blob_offset + 1);
+            }
             break;
         }
         else if (var->name_len > 65536) {
             /* This should never happen as the length is restricted on store
              * to 65536.
              */
-            msr_log(msr, 4, "Possiblly corrupted database: var name length > 65536 (0x%04x) at blob offset %u-%u.", var->name_len, blob_offset, blob_offset + 1);
+            if (msr->txcfg->debuglog_level >= 9) {
+                msr_log(msr, 9, "BLOB[%d]: %s", blob_offset, log_escape_hex(msr->mp, blob + blob_offset, blob_size - blob_offset));
+            }
+            msr_log(msr, 4, "Possibly corrupted database: var name length > 65536 (0x%04x) at blob offset %u-%u.", var->name_len, blob_offset, blob_offset + 1);
             break;
         }
 
@@ -74,7 +83,7 @@ static apr_table_t *collection_unpack(modsec_rec *msr, const unsigned char *blob
 /**
  *
  */
-apr_table_t *collection_retrieve(modsec_rec *msr, const char *col_name,
+static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec *msr, const char *col_name,
     const char *col_key, int col_key_len)
 {
     char *dbm_filename = NULL;
@@ -100,17 +109,19 @@ apr_table_t *collection_retrieve(modsec_rec *msr, const char *col_name,
     key.dptr = (char *)col_key;
     key.dsize = col_key_len + 1;
 
-    rc = apr_sdbm_open(&dbm, dbm_filename, APR_READ | APR_SHARELOCK,
+    if (existing_dbm == NULL) {
-        CREATEMODE, msr->mp);
+        rc = apr_sdbm_open(&dbm, dbm_filename, APR_READ | APR_SHARELOCK,
-    if (rc != APR_SUCCESS) {
+            CREATEMODE, msr->mp);
-        return NULL;
+        if (rc != APR_SUCCESS) {
+            return NULL;
+        }
+    }
+    else {
+        dbm = existing_dbm;
     }
 
     value = (apr_sdbm_datum_t *)apr_pcalloc(msr->mp, sizeof(apr_sdbm_datum_t));
     rc = apr_sdbm_fetch(dbm, value, key);
-
-    apr_sdbm_close(dbm);
-
     if (rc != APR_SUCCESS) {
         msr_log(msr, 1, "Failed to read from DBM file \"%s\": %s", log_escape(msr->mp,
             dbm_filename), get_apr_error(msr->mp, rc));
@@ -130,6 +141,12 @@ apr_table_t *collection_retrieve(modsec_rec *msr, const char *col_name,
     col = collection_unpack(msr, (const unsigned char *)value->dptr, value->dsize, 1);
     if (col == NULL) return NULL;
 
+    /* We have to close *after* we use "value" from the fetch or the memory
+     * may be overwritten. */
+    if (existing_dbm == NULL) {
+        apr_sdbm_close(dbm);
+    }
+
     /* Remove expired variables. */
     do {
         arr = apr_table_elts(col);
@@ -168,18 +185,20 @@ apr_table_t *collection_retrieve(modsec_rec *msr, const char *col_name,
      * time.
      */
     if (apr_table_get(col, "KEY") == NULL) {
-        rc = apr_sdbm_open(&dbm, dbm_filename, APR_CREATE | APR_WRITE | APR_SHARELOCK,
+        if (existing_dbm == NULL) {
-            CREATEMODE, msr->mp);
+            rc = apr_sdbm_open(&dbm, dbm_filename, APR_CREATE | APR_WRITE | APR_SHARELOCK,
-        if (rc != APR_SUCCESS) {
+                CREATEMODE, msr->mp);
-            msr_log(msr, 1, "Failed to access DBM file \"%s\": %s",
+            if (rc != APR_SUCCESS) {
-                log_escape(msr->mp, dbm_filename), get_apr_error(msr->mp, rc));
+                msr_log(msr, 1, "Failed to access DBM file \"%s\": %s",
-            return NULL;
+                    log_escape(msr->mp, dbm_filename), get_apr_error(msr->mp, rc));
+                return NULL;
+            }
+        }
+        else {
+            dbm = existing_dbm;
         }
 
         rc = apr_sdbm_delete(dbm, key);
-
-        apr_sdbm_close(dbm);
-
         if (rc != APR_SUCCESS) {
             msr_log(msr, 1, "Failed deleting collection (name \"%s\", "
                 "key \"%s\"): %s", log_escape(msr->mp, col_name),
@@ -187,6 +206,11 @@ apr_table_t *collection_retrieve(modsec_rec *msr, const char *col_name,
             return NULL;
         }
 
+
+        if (existing_dbm == NULL) {
+            apr_sdbm_close(dbm);
+        }
+
         if (expired && (msr->txcfg->debuglog_level >= 9)) {
             msr_log(msr, 9, "Collection expired (name \"%s\", key \"%s\").", col_name, log_escape_ex(msr->mp, col_key, col_key_len));
         }
@@ -242,6 +266,15 @@ apr_table_t *collection_retrieve(modsec_rec *msr, const char *col_name,
     return col;
 }
 
+/**
+ *
+ */
+apr_table_t *collection_retrieve(modsec_rec *msr, const char *col_name,
+    const char *col_key, int col_key_len) {
+    return collection_retrieve_ex(NULL, msr, col_name, col_key, col_key_len);
+}
+
+
 /**
  *
  */
@@ -257,6 +290,8 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
     const apr_array_header_t *arr;
     apr_table_entry_t *te;
     int i;
+    const apr_table_t *stored_col = NULL;
+    const apr_table_t *orig_col = NULL;
 
     var_name = (msc_string *)apr_table_get(col, "__name");
     if (var_name == NULL) {
@@ -330,7 +365,35 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
      * convert back to table form
      */
 
-    /* Calculate the size first. */
+    rc = apr_sdbm_open(&dbm, dbm_filename, APR_CREATE | APR_WRITE | APR_SHARELOCK,
+        CREATEMODE, msr->mp);
+    if (rc != APR_SUCCESS) {
+        msr_log(msr, 1, "Failed to access DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
+            get_apr_error(msr->mp, rc));
+        return -1;
+    }
+
+        /* We only need to lock so we can pull in the stored data again. */
+        rc = apr_sdbm_lock(dbm, APR_FLOCK_EXCLUSIVE);
+        if (rc != APR_SUCCESS) {
+            msr_log(msr, 1, "Failed to exclusivly lock DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
+                get_apr_error(msr->mp, rc));
+            apr_sdbm_close(dbm);
+            return -1;
+        }
+
+    /* If there is an original value, then we need to create a delta and
+     * apply the delta to the current value */
+    orig_col = (const apr_table_t *)apr_table_get(msr->collections_original, var_name->value);
+    if (orig_col != NULL) {
+        if (msr->txcfg->debuglog_level >= 9) {
+            msr_log(msr, 9, "Re-retrieving collection prior to store: %s", apr_psprintf(msr->mp, "%.*s", var_name->value_len, var_name->value));
+        }
+
+        stored_col = (const apr_table_t *)collection_retrieve_ex(dbm, msr, var_name->value, var_key->value, var_key->value_len);
+    }
+
+    /* Merge deltas and calculate the size first. */
     blob_size = 3 + 2;
     arr = apr_table_elts(col);
     te = (apr_table_entry_t *)arr->elts;
@@ -338,6 +401,33 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
         msc_string *var = (msc_string *)te[i].val;
         int len;
 
+        /* If there is an original value, then we need to apply the delta
+         * to the latest stored value */
+        if (stored_col != NULL) {
+            const msc_string *orig_var = (const msc_string *)apr_table_get(orig_col, var->name);
+            if (orig_var != NULL) {
+                const msc_string *stored_var = (const msc_string *)apr_table_get(stored_col, var->name);
+                if (stored_var != NULL) {
+                    int origval = atoi(orig_var->value);
+                    int ourval = atoi(var->value);
+                    int storedval = atoi(stored_var->value);
+                    int delta = ourval - origval;
+                    int value = storedval + delta;
+
+                    if (value < 0) value = 0; /* Counters never go below zero. */
+
+                    var->value = apr_psprintf(msr->mp, "%d", value);
+                    var->value_len = strlen(var->value);
+                    if (msr->txcfg->debuglog_level >= 9) {
+                        msr_log(msr, 9, "Delta applied for %s.%s %d->%d (%d): %d + (%d) = %d [%s,%d]",
+                        log_escape_ex(msr->mp, var_name->value, var_name->value_len),
+                        log_escape_ex(msr->mp, var->name, var->name_len),
+                        origval, ourval, delta, storedval, delta, value, var->value, var->value_len);
+                    }
+                }
+            }
+        }
+
         len = var->name_len + 1;
         if (len >= 65536) len = 65536;
         blob_size += len + 2;
@@ -401,24 +491,17 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
     value.dptr = (char *)blob;
     value.dsize = blob_size;
 
-    rc = apr_sdbm_open(&dbm, dbm_filename, APR_CREATE | APR_WRITE | APR_SHARELOCK,
-        CREATEMODE, msr->mp);
-    if (rc != APR_SUCCESS) {
-        msr_log(msr, 1, "Failed to access DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
-            get_apr_error(msr->mp, rc));
-        return -1;
-    }
-
     rc = apr_sdbm_store(dbm, key, value, APR_SDBM_REPLACE);
-
-    apr_sdbm_close(dbm);
-
     if (rc != APR_SUCCESS) {
         msr_log(msr, 1, "Failed to write to DBM file \"%s\": %s", dbm_filename,
             get_apr_error(msr->mp, rc));
         return -1;
     }
 
+    /* ENH: Do we need to unlock()?  Or will just close() suffice? */
+    apr_sdbm_unlock(dbm);
+    apr_sdbm_close(dbm);
+
     if (msr->txcfg->debuglog_level >= 4) {
         msr_log(msr, 4, "Persisted collection (name \"%s\", key \"%s\").",
             log_escape_ex(msr->mp, var_name->value, var_name->value_len), log_escape_ex(msr->mp, var_key->value, var_key->value_len));

apache2/re.h

@@ -41,6 +41,8 @@ typedef struct msre_cache_rec msre_cache_rec;
 
 /* Actions, variables, functions and operator functions */
 
+apr_status_t DSOLOCAL collection_original_setvar(modsec_rec *msr, const char *col_name, const msc_string *orig_var);
+
 int DSOLOCAL expand_macros(modsec_rec *msr, msc_string *var, msre_rule *rule, apr_pool_t *mptmp);
 
 apr_status_t DSOLOCAL msre_parse_targets(msre_ruleset *ruleset, const char *text,

apache2/re_actions.c

@@ -299,6 +299,64 @@ int expand_macros(modsec_rec *msr, msc_string *var, msre_rule *rule, apr_pool_t
     return 1;
 }
 
+/**
+ * Record the original collection values to use to calculate deltas.
+ * This can be called multiple times and will not overwrite the first
+ * value that is set.
+ */
+apr_status_t collection_original_setvar(modsec_rec *msr, const char *col_name, const msc_string *orig_var) {
+    apr_table_t *table = NULL;
+    msc_string *var = NULL;
+    const char *var_name = NULL;
+
+    if (orig_var == NULL) {
+        msr_log(msr, 1, "Internal Error: Attempt to record NULL original variable.");
+        return -1;
+    }
+
+    var_name = orig_var->name;
+    table = (apr_table_t *)apr_table_get(msr->collections_original, col_name);
+
+    /* Does the collection exist already? */
+    if (table == NULL) {
+        table = apr_table_make(msr->mp, 24);
+        if (table == NULL) {
+            msr_log(msr, 1, "Failed to allocate space for original collection.");
+            return -1;
+        }
+        apr_table_setn(msr->collections_original, apr_pstrdup(msr->mp, col_name), (void *)table);
+    }
+    else {
+        /* Does the variable exist already? */
+        var = (msc_string *)apr_table_get(table, var_name);
+        if (var != NULL) {
+            if (msr->txcfg->debuglog_level >= 9) {
+                msr_log(msr, 9, "Original collection variable: %s.%s = \"%s\"", col_name, var_name, log_escape_ex(msr->mp, orig_var->value, orig_var->value_len));
+            }
+            return 1;
+        }
+    }
+
+    var = (msc_string *)apr_palloc(msr->mp, sizeof(msc_string));
+    if (var == NULL) {
+        msr_log(msr, 1, "Failed to allocate space for original collection variable.");
+        return -1;
+    }
+
+    /* Copy the original var and add to collection. */
+    var->name = orig_var->name ? apr_pstrmemdup(msr->mp, orig_var->name, orig_var->name_len) : NULL;
+    var->name_len = orig_var->name_len;
+    var->value = orig_var->value ? apr_pstrmemdup(msr->mp, orig_var->value, orig_var->value_len) : NULL;
+    var->value_len = orig_var->value_len;
+    apr_table_setn(table, apr_pstrmemdup(msr->mp, var->name, var->name_len), (void *)var);
+
+    if (msr->txcfg->debuglog_level >= 9) {
+        msr_log(msr, 9, "Recorded original collection variable: %s.%s = \"%s\"", col_name, var_name, log_escape_ex(msr->mp, var->value, var->value_len));
+    }
+
+    return 0;
+}
+
 /* id */
 
 static apr_status_t msre_action_id_init(msre_engine *engine, msre_actionset *actionset,
@@ -1186,11 +1244,16 @@ static apr_status_t msre_action_setvar_execute(modsec_rec *msr, apr_pool_t *mptm
                 rec->name = apr_pstrdup(msr->mp, var_name);
                 rec->name_len = strlen(rec->name);
                 value = 0;
+                rec->value = apr_psprintf(msr->mp, "%d", value);
+                rec->value_len = strlen(rec->value);
             }
             else {
                 value = atoi(rec->value);
             }
 
+            /* Record the original value before we change it */
+            collection_original_setvar(msr, col_name, rec);
+
             /* Expand values in value */
             val->value = var_value;
             val->value_len = strlen(val->value);
@@ -1463,6 +1526,7 @@ static apr_status_t init_collection(modsec_rec *msr, const char *real_col_name,
     const char *col_name, const char *col_key, unsigned int col_key_len)
 {
     apr_table_t *table = NULL;
+    msc_string *var = NULL;
 
     /* IMP1 Cannot initialise the built-in collections this way. */
 
@@ -1476,13 +1540,16 @@ static apr_status_t init_collection(modsec_rec *msr, const char *real_col_name,
     table = collection_retrieve(msr, real_col_name, col_key, col_key_len);
 
     if (table == NULL) {
-        msc_string *var = NULL;
 
         /* Does not exist yet - create new. */
         msr_log(msr, 4, "Creating collection (name \"%s\", key \"%s\").",
             real_col_name, col_key);
 
         table = apr_table_make(msr->mp, 24);
+        if (table == NULL) {
+            msr_log(msr, 1, "Failed to allocate space for collection.");
+            return -1;
+        }
 
         /* IMP1 Is the timeout hard-coded to 3600? */
 
@@ -1559,6 +1626,12 @@ static apr_status_t init_collection(modsec_rec *msr, const char *real_col_name,
         apr_table_setn(table, var->name, (void *)var);
     }
 
+    /* Record the original counter value before we change it */
+    var = (msc_string *)apr_table_get(table, "UPDATE_COUNTER");
+    if (var != NULL) {
+        collection_original_setvar(msr, col_name, var);
+    }
+
     /* Add the collection to the list. */
     apr_table_setn(msr->collections, apr_pstrdup(msr->mp, col_name), (void *)table);