From dc69c72db3613da6a0af5c9abc14c72c40e2c7dc Mon Sep 17 00:00:00 2001 From: Mladen Turk Date: Wed, 28 Sep 2016 18:34:32 +0200 Subject: [PATCH 1/2] Use global mutex instead sdbm file lock to fix issues with threaded mpm's --- apache2/modsecurity.c | 22 +++++++++++++++ apache2/modsecurity.h | 1 + apache2/persist_dbm.c | 66 +++++++++++++++++++++++++++++-------------- 3 files changed, 68 insertions(+), 21 deletions(-) diff --git a/apache2/modsecurity.c b/apache2/modsecurity.c index 5bda4cff82..88c599d523 100644 --- a/apache2/modsecurity.c +++ b/apache2/modsecurity.c @@ -169,6 +169,22 @@ int modsecurity_init(msc_engine *msce, apr_pool_t *mp) { return -1; } #endif /* SET_MUTEX_PERMS */ + + rc = apr_global_mutex_create(&msce->dbm_lock, NULL, APR_LOCK_DEFAULT, mp); + if (rc != APR_SUCCESS) { + return -1; + } + +#ifdef __SET_MUTEX_PERMS +#if AP_SERVER_MAJORVERSION_NUMBER > 1 && AP_SERVER_MINORVERSION_NUMBER > 2 + rc = ap_unixd_set_global_mutex_perms(msce->dbm_lock); +#else + rc = unixd_set_global_mutex_perms(msce->dbm_lock); +#endif + if (rc != APR_SUCCESS) { + return -1; + } +#endif /* SET_MUTEX_PERMS */ #endif return 1; @@ -195,6 +211,12 @@ void modsecurity_child_init(msc_engine *msce) { } } + 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"); + } + } } /** diff --git a/apache2/modsecurity.h b/apache2/modsecurity.h index 228bea0b22..7bdc12de7a 100644 --- a/apache2/modsecurity.h +++ b/apache2/modsecurity.h @@ -657,6 +657,7 @@ struct msc_engine { apr_pool_t *mp; apr_global_mutex_t *auditlog_lock; apr_global_mutex_t *geo_lock; + apr_global_mutex_t *dbm_lock; msre_engine *msre; unsigned int processing_mode; }; diff --git a/apache2/persist_dbm.c b/apache2/persist_dbm.c index 76c3820baf..ccacabcfd9 100644 --- a/apache2/persist_dbm.c +++ b/apache2/persist_dbm.c @@ -101,6 +101,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec int expired = 0; int i; + if (msr->txcfg->data_dir == NULL) { msr_log(msr, 1, "collection_retrieve_ex: Unable to retrieve collection (name \"%s\", key \"%s\"). Use " "SecDataDir to define data directory first.", log_escape(msr->mp, col_name), @@ -119,10 +120,18 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec key.dsize = col_key_len + 1; if (existing_dbm == NULL) { + 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; + } + rc = apr_sdbm_open(&dbm, dbm_filename, APR_READ | APR_SHARELOCK, CREATEMODE, msr->mp); if (rc != APR_SUCCESS) { dbm = NULL; + apr_global_mutex_unlock(msr->modsecurity->dbm_lock); goto cleanup; } } @@ -156,6 +165,7 @@ 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); + apr_global_mutex_unlock(msr->modsecurity->dbm_lock); dbm = NULL; } @@ -202,12 +212,19 @@ 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) { + 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; + } 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; + apr_global_mutex_unlock(msr->modsecurity->dbm_lock); goto cleanup; } } @@ -227,6 +244,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec if (existing_dbm == NULL) { apr_sdbm_close(dbm); + apr_global_mutex_unlock(msr->modsecurity->dbm_lock); dbm = NULL; } @@ -289,14 +307,16 @@ 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); } - + return col; cleanup: if ((existing_dbm == NULL) && dbm) { apr_sdbm_close(dbm); + apr_global_mutex_unlock(msr->modsecurity->dbm_lock); } return NULL; @@ -409,6 +429,14 @@ int collection_store(modsec_rec *msr, apr_table_t *col) { var->value_len = strlen(var->value); } + /* 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; + } + /* 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 @@ -417,19 +445,13 @@ 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) { + apr_global_mutex_unlock(msr->modsecurity->dbm_lock); 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; } - - /* 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; - } + /* If there is an original value, then create a delta and * apply the delta to the current value */ @@ -492,8 +514,8 @@ int collection_store(modsec_rec *msr, apr_table_t *col) { blob = apr_pcalloc(msr->mp, blob_size); if (blob == NULL) { if (dbm != NULL) { - apr_sdbm_unlock(dbm); apr_sdbm_close(dbm); + apr_global_mutex_unlock(msr->modsecurity->dbm_lock); } return -1; @@ -550,16 +572,16 @@ 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) { - apr_sdbm_unlock(dbm); apr_sdbm_close(dbm); + apr_global_mutex_unlock(msr->modsecurity->dbm_lock); } return -1; } - apr_sdbm_unlock(dbm); apr_sdbm_close(dbm); - + apr_global_mutex_unlock(msr->modsecurity->dbm_lock); + 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), @@ -603,9 +625,17 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) { log_escape(msr->mp, dbm_filename)); } + 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; + } + rc = apr_sdbm_open(&dbm, dbm_filename, APR_CREATE | APR_WRITE | APR_SHARELOCK, CREATEMODE, msr->mp); if (rc != APR_SUCCESS) { + apr_global_mutex_unlock(msr->modsecurity->dbm_lock); 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; @@ -614,12 +644,6 @@ 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 *)); - 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; - } /* No one can write to the file while doing this so * do it as fast as possible. @@ -632,7 +656,6 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) { } rc = apr_sdbm_nextkey(dbm, &key); } - apr_sdbm_unlock(dbm); if (msr->txcfg->debuglog_level >= 9) { msr_log(msr, 9, "collections_remove_stale: Found %d record(s) in file \"%s\".", keys_arr->nelts, @@ -698,13 +721,14 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) { } apr_sdbm_close(dbm); - + apr_global_mutex_unlock(msr->modsecurity->dbm_lock); return 1; error: if (dbm) { apr_sdbm_close(dbm); + apr_global_mutex_unlock(msr->modsecurity->dbm_lock); } return -1; From deaa65a2ccb0867c7d5a1dafb1b3fa4b23fbfb22 Mon Sep 17 00:00:00 2001 From: Mladen Turk Date: Fri, 4 Nov 2016 10:06:33 +0100 Subject: [PATCH 2/2] Move locking before table update --- apache2/persist_dbm.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/apache2/persist_dbm.c b/apache2/persist_dbm.c index ccacabcfd9..5d225e50da 100644 --- a/apache2/persist_dbm.c +++ b/apache2/persist_dbm.c @@ -381,6 +381,14 @@ int collection_store(modsec_rec *msr, apr_table_t *col) { log_escape(msr->mp, dbm_filename)); } + /* 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; + } + /* Delete IS_NEW on store. */ apr_table_unset(col, "IS_NEW"); @@ -428,14 +436,6 @@ 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); } - - /* 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; - } /* ENH Make the expiration timestamp accessible in blob form so that * it is easier/faster to determine expiration without having to