Makes global mutex for collections optional

owasp-modsecurity · May 21, 2017 · 112ba45 · 112ba45
1 parent c6f6dff
commit 112ba45
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 11 deletions.
diff --git a/CHANGES b/CHANGES
@@ -1,6 +1,8 @@
 DD MMM YYYY - 2.9.2 - To be released
 ------------------------------------
 
+ * Uses an optional global lock while manipulating collections.
+   [Issues #1224 - @mturk and @zimmerle]
  * Fix collection naming problem while merging collections.
    [Issue #1274 - Coty Sutherland and @zimmerle]
  * Fix --enable-docs adding missing Makefile, modifying autoconf and filenames

diff --git a/apache2/modsecurity.c b/apache2/modsecurity.c
@@ -170,6 +170,7 @@ int modsecurity_init(msc_engine *msce, apr_pool_t *mp) {
     }
 #endif /* SET_MUTEX_PERMS */
 
+#ifdef GLOBAL_COLLECTION_LOCK
     rc = apr_global_mutex_create(&msce->dbm_lock, NULL, APR_LOCK_DEFAULT, mp);
     if (rc != APR_SUCCESS) {
         return -1;
@@ -185,6 +186,7 @@ int modsecurity_init(msc_engine *msce, apr_pool_t *mp) {
         return -1;
     }
 #endif /* SET_MUTEX_PERMS */
+#endif
 #endif
 
     return 1;
@@ -211,12 +213,15 @@ void modsecurity_child_init(msc_engine *msce) {
         }
     }
 
+#ifdef GLOBAL_COLLECTION_LOCK
     if (msce->dbm_lock != NULL) {
         apr_status_t rc = apr_global_mutex_child_init(&msce->dbm_lock, NULL, msce->mp);
         if (rc != APR_SUCCESS) {
             // ap_log_error(APLOG_MARK, APLOG_ERR, rs, s, "Failed to child-init dbm mutex");
         }
     }
+#endif
+
 }
 
 /**

diff --git a/apache2/modsecurity.h b/apache2/modsecurity.h
@@ -657,7 +657,9 @@ struct msc_engine {
     apr_pool_t              *mp;
     apr_global_mutex_t      *auditlog_lock;
     apr_global_mutex_t      *geo_lock;
+#ifdef GLOBAL_COLLECTION_LOCK
     apr_global_mutex_t      *dbm_lock;
+#endif
     msre_engine             *msre;
     unsigned int             processing_mode;
 };

diff --git a/apache2/persist_dbm.c b/apache2/persist_dbm.c
@@ -120,18 +120,21 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
     key.dsize = col_key_len + 1;
 
     if (existing_dbm == NULL) {
+#ifdef GLOBAL_COLLECTION_LOCK
         rc = apr_global_mutex_lock(msr->modsecurity->dbm_lock);
         if (rc != APR_SUCCESS) {
             msr_log(msr, 1, "collection_retrieve_ex: Failed to lock proc mutex: %s",
                     get_apr_error(msr->mp, rc));
             goto cleanup;
         }
-
+#endif
         rc = apr_sdbm_open(&dbm, dbm_filename, APR_READ | APR_SHARELOCK,
             CREATEMODE, msr->mp);
         if (rc != APR_SUCCESS) {
             dbm = NULL;
+#ifdef GLOBAL_COLLECTION_LOCK
             apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
+#endif
             goto cleanup;
         }
     }
@@ -165,7 +168,9 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
     /* Close after "value" used from fetch or memory may be overwritten. */
     if (existing_dbm == NULL) {
         apr_sdbm_close(dbm);
+#ifdef GLOBAL_COLLECTION_LOCK
         apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
+#endif
         dbm = NULL;
     }
 
@@ -212,19 +217,23 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
      */
     if (apr_table_get(col, "KEY") == NULL) {
         if (existing_dbm == NULL) {
+#ifdef GLOBAL_COLLECTION_LOCK
             rc = apr_global_mutex_lock(msr->modsecurity->dbm_lock);
             if (rc != APR_SUCCESS) {
                 msr_log(msr, 1, "collection_retrieve_ex: Failed to lock proc mutex: %s",
                         get_apr_error(msr->mp, rc));
                 goto cleanup;
-            }            
+            }
+#endif
             rc = apr_sdbm_open(&dbm, dbm_filename, APR_CREATE | APR_WRITE | APR_SHARELOCK,
                 CREATEMODE, msr->mp);
             if (rc != APR_SUCCESS) {
                 msr_log(msr, 1, "collection_retrieve_ex: Failed to access DBM file \"%s\": %s",
                     log_escape(msr->mp, dbm_filename), get_apr_error(msr->mp, rc));
                 dbm = NULL;
+#ifdef GLOBAL_COLLECTION_LOCK
                 apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
+#endif
                 goto cleanup;
             }
         }
@@ -247,7 +256,9 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
 
         if (existing_dbm == NULL) {
             apr_sdbm_close(dbm);
+#ifdef GLOBAL_COLLECTION_LOCK
             apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
+#endif
             dbm = NULL;
         }
 
@@ -310,16 +321,20 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
             log_escape(msr->mp, col_name), log_escape_ex(msr->mp, col_key, col_key_len));
 
         apr_sdbm_close(dbm);
-        apr_global_mutex_unlock(msr->modsecurity->dbm_lock);    
+#ifdef GLOBAL_COLLECTION_LOCK
+        apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
+#endif
     }
-    
+
     return col;
 
 cleanup:
 
     if ((existing_dbm == NULL) && dbm) {
         apr_sdbm_close(dbm);
-        apr_global_mutex_unlock(msr->modsecurity->dbm_lock);    
+#ifdef GLOBAL_COLLECTION_LOCK
+        apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
+#endif
     }
 
     return NULL;
@@ -384,13 +399,15 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
                 log_escape(msr->mp, dbm_filename));
     }
 
+#ifdef GLOBAL_COLLECTION_LOCK
     /* Need to lock to pull in the stored data again and apply deltas. */
     rc = apr_global_mutex_lock(msr->modsecurity->dbm_lock);
     if (rc != APR_SUCCESS) {
         msr_log(msr, 1, "collection_store: Failed to lock proc mutex: %s",
                 get_apr_error(msr->mp, rc));
         goto error;
     }
+#endif
 
     /* Delete IS_NEW on store. */
     apr_table_unset(col, "IS_NEW");
@@ -439,7 +456,7 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
         var->value = apr_psprintf(msr->mp, "%d", counter + 1);
         var->value_len = strlen(var->value);
     }
-    
+
     /* ENH Make the expiration timestamp accessible in blob form so that
      * it is easier/faster to determine expiration without having to
      * convert back to table form
@@ -448,13 +465,24 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
     rc = apr_sdbm_open(&dbm, dbm_filename, APR_CREATE | APR_WRITE | APR_SHARELOCK,
         CREATEMODE, msr->mp);
     if (rc != APR_SUCCESS) {
+#ifdef GLOBAL_COLLECTION_LOCK
         apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
+#endif
         msr_log(msr, 1, "collection_store: Failed to access DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
             get_apr_error(msr->mp, rc));
         dbm = NULL;
         goto error;
     }
-
+
+#ifndef GLOBAL_COLLECTION_LOCK
+    /* Need to lock to pull in the stored data again and apply deltas. */
+    rc = apr_sdbm_lock(dbm, APR_FLOCK_EXCLUSIVE);
+    if (rc != APR_SUCCESS) {
+        msr_log(msr, 1, "collection_store: Failed to exclusivly lock DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
+            get_apr_error(msr->mp, rc));
+        goto error;
+    }
+#endif
 
     /* If there is an original value, then create a delta and
      * apply the delta to the current value */
@@ -519,8 +547,13 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
     blob = apr_pcalloc(msr->mp, blob_size);
     if (blob == NULL) {
         if (dbm != NULL) {
+#ifdef GLOBAL_COLLECTION_LOCK
             apr_sdbm_close(dbm);
             apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
+#else
+            apr_sdbm_unlock(dbm);
+            apr_sdbm_close(dbm);
+#endif
         }
 
         return -1;
@@ -577,16 +610,26 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
         msr_log(msr, 1, "collection_store: Failed to write to DBM file \"%s\": %s", dbm_filename,
                 get_apr_error(msr->mp, rc));
         if (dbm != NULL) {
+#ifdef GLOBAL_COLLECTION_LOCK
+            apr_sdbm_close(dbm);
+            apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
+#else
+            apr_sdbm_unlock(dbm);
             apr_sdbm_close(dbm);
-            apr_global_mutex_unlock(msr->modsecurity->dbm_lock);            
+#endif
         }
 
         return -1;
     }
 
+#ifdef GLOBAL_COLLECTION_LOCK
     apr_sdbm_close(dbm);
     apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
-
+#else
+    apr_sdbm_unlock(dbm);
+    apr_sdbm_close(dbm);
+#endif
+
     if (msr->txcfg->debuglog_level >= 4) {
         msr_log(msr, 4, "collection_store: Persisted collection (name \"%s\", key \"%s\").",
             log_escape_ex(msr->mp, var_name->value, var_name->value_len),
@@ -630,17 +673,21 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
                 log_escape(msr->mp, dbm_filename));
     }
 
+#ifdef GLOBAL_COLLECTION_LOCK
     rc = apr_global_mutex_lock(msr->modsecurity->dbm_lock);
     if (rc != APR_SUCCESS) {
         msr_log(msr, 1, "collections_remove_stale: Failed to lock proc mutex: %s",
                 get_apr_error(msr->mp, rc));
         goto error;
     }
-
+#endif
+
     rc = apr_sdbm_open(&dbm, dbm_filename, APR_CREATE | APR_WRITE | APR_SHARELOCK,
             CREATEMODE, msr->mp);
     if (rc != APR_SUCCESS) {
+#ifdef GLOBAL_COLLECTION_LOCK
         apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
+#endif
         msr_log(msr, 1, "collections_remove_stale: Failed to access DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
                 get_apr_error(msr->mp, rc));
         dbm = NULL;
@@ -650,6 +697,15 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
     /* First get a list of all keys. */
     keys_arr = apr_array_make(msr->mp, 256, sizeof(char *));
 
+#ifndef GLOBAL_COLLECTION_LOCK
+    rc = apr_sdbm_lock(dbm, APR_FLOCK_SHARED);
+    if (rc != APR_SUCCESS) {
+        msr_log(msr, 1, "collections_remove_stale: Failed to lock DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
+            get_apr_error(msr->mp, rc));
+        goto error;
+    }
+#endif
+
     /* No one can write to the file while doing this so
      * do it as fast as possible.
      */
@@ -661,6 +717,9 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
         }
         rc = apr_sdbm_nextkey(dbm, &key);
     }
+#ifndef GLOBAL_COLLECTION_LOCK
+    apr_sdbm_unlock(dbm);
+#endif
 
     if (msr->txcfg->debuglog_level >= 9) {
         msr_log(msr, 9, "collections_remove_stale: Found %d record(s) in file \"%s\".", keys_arr->nelts,
@@ -729,14 +788,18 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
     }
 
     apr_sdbm_close(dbm);
+#ifdef GLOBAL_COLLECTION_LOCK
     apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
+#endif
     return 1;
 
 error:
 
     if (dbm) {
         apr_sdbm_close(dbm);
+#ifdef GLOBAL_COLLECTION_LOCK
         apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
+#endif
     }
 
     return -1;

diff --git a/configure.ac b/configure.ac
@@ -545,6 +545,23 @@ AC_ARG_ENABLE(server-context-logging,
   log_server_context=''
 ])
 
+
+# Enable collection's global lock
+AC_ARG_ENABLE(collection-global-lock,
+              AS_HELP_STRING([--enable-collection-global-lock],
+                             [Enable collection correctness by using a global lock. May reduce performance significatively. This is disabled by default]),
+[
+  if test "$enableval" != "yes"; then
+    collection_global_lock=""
+  else
+    collection_global_lock="-DGLOBAL_COLLECTION_LOCK"
+  fi
+],
+[
+  collection_global_lock=''
+])
+
+
 # Ignore configure errors
 AC_ARG_ENABLE(errors,
               AS_HELP_STRING([--disable-errors],
@@ -795,7 +812,7 @@ else
   fi
 fi
 
-MODSEC_EXTRA_CFLAGS="$pcre_study $pcre_match_limit $pcre_match_limit_recursion $pcre_jit $request_early $htaccess_config $lua_cache $debug_conf $debug_cache $debug_acmp $debug_mem $perf_meas $modsec_api $cpu_type $unique_id $log_filename $log_server $log_collection_delete_problem $log_dechunk $log_stopwatch $log_handler $log_server_contex"
+MODSEC_EXTRA_CFLAGS="$pcre_study $pcre_match_limit $pcre_match_limit_recursion $pcre_jit $request_early $htaccess_config $lua_cache $debug_conf $debug_cache $debug_acmp $debug_mem $perf_meas $modsec_api $cpu_type $unique_id $log_filename $log_server $log_collection_delete_problem $log_dechunk $log_stopwatch $log_handler $log_server_contex $collection_global_lock"
 
 APXS_WRAPPER=build/apxs-wrapper
 APXS_EXTRA_CFLAGS=""