diff --git a/CHANGES b/CHANGES index 8a35b6752..7bc8cbd27 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,9 @@ +(to be released) - 2.9.x +------------------------ + + * handle errors from apr_global_mutex_lock + [PR #3257 - @marcstern] + 03 Sep 2024 - 2.9.8 ------------------- diff --git a/apache2/modsecurity.c b/apache2/modsecurity.c index 55150afe2..e21f17766 100644 --- a/apache2/modsecurity.c +++ b/apache2/modsecurity.c @@ -123,30 +123,7 @@ msc_engine *modsecurity_create(apr_pool_t *mp, int processing_mode) { } int acquire_global_lock(apr_global_mutex_t **lock, apr_pool_t *mp) { - apr_status_t rc; - apr_file_t *lock_name; - const char *temp_dir; - const char *filename; - - // get platform temp dir - rc = apr_temp_dir_get(&temp_dir, mp); - if (rc != APR_SUCCESS) { - ap_log_perror(APLOG_MARK, APLOG_ERR, 0, mp, "ModSecurity: Could not get temp dir"); - return -1; - } - - // use temp path template for lock files - char *path = apr_pstrcat(mp, temp_dir, GLOBAL_LOCK_TEMPLATE, NULL); - - rc = apr_file_mktemp(&lock_name, path, 0, mp); - if (rc != APR_SUCCESS) { - ap_log_perror(APLOG_MARK, APLOG_ERR, 0, mp, " ModSecurity: Could not create temporary file for global lock"); - return -1; - } - // below func always return APR_SUCCESS - apr_file_name_get(&filename, lock_name); - - rc = apr_global_mutex_create(lock, filename, APR_LOCK_DEFAULT, mp); + apr_status_t rc = apr_global_mutex_create(lock, NULL, APR_LOCK_DEFAULT, mp); if (rc != APR_SUCCESS) { ap_log_perror(APLOG_MARK, APLOG_ERR, 0, mp, " ModSecurity: Could not create global mutex"); return -1; @@ -166,6 +143,41 @@ int acquire_global_lock(apr_global_mutex_t **lock, apr_pool_t *mp) { #endif /* MSC_TEST */ return APR_SUCCESS; } + +/** + * handle errors from apr_global_mutex_lock + */ +int msr_global_mutex_lock(modsec_rec* msr, apr_global_mutex_t* lock, const char* fct) { + assert(msr); + assert(msr->modsecurity); // lock is msr->modsecurity->..._lock + assert(msr->mp); + if (!lock) { + msr_log(msr, 1, "%s: Global mutex was not created", fct); + return -1; + } + + int rc = apr_global_mutex_lock(lock); + if (rc != APR_SUCCESS) msr_log(msr, 1, "Audit log: Failed to lock global mutex: %s", get_apr_error(msr->mp, rc)); + return rc; +} +/** + * handle errors from apr_global_mutex_unlock + */ +int msr_global_mutex_unlock(modsec_rec* msr, apr_global_mutex_t* lock, const char* fct) { + assert(msr); + assert(msr->modsecurity); // lock is msr->modsecurity->..._lock + assert(msr->mp); + if (!lock) { + msr_log(msr, 1, "%s: Global mutex was not created", fct); + return -1; + } + + int rc = apr_global_mutex_unlock(lock); + // We should have get the warning at lock time, so ignore it here + // if (rc != APR_SUCCESS) msr_log(msr, 1, "Audit log: Failed to unlock global mutex: %s", get_apr_error(msr->mp, rc)); + return rc; +} + /** * Initialise the modsecurity engine. This function must be invoked * after configuration processing is complete as Apache needs to know the diff --git a/apache2/modsecurity.h b/apache2/modsecurity.h index d1aa1d834..b3976f936 100644 --- a/apache2/modsecurity.h +++ b/apache2/modsecurity.h @@ -135,8 +135,6 @@ typedef struct msc_parm msc_parm; #define FATAL_ERROR "ModSecurity: Fatal error (memory allocation or unexpected internal error)!" -#define GLOBAL_LOCK_TEMPLATE "/modsec-lock-tmp.XXXXXX" - extern DSOLOCAL char *new_server_signature; extern DSOLOCAL char *real_server_signature; extern DSOLOCAL char *chroot_dir; @@ -707,6 +705,8 @@ struct msc_parm { /* Reusable functions */ int acquire_global_lock(apr_global_mutex_t **lock, apr_pool_t *mp); +int msr_global_mutex_lock(modsec_rec* msr, apr_global_mutex_t* lock, const char* fct); +int msr_global_mutex_unlock(modsec_rec* msr, apr_global_mutex_t* lock, const char* fct); /* Engine functions */ diff --git a/apache2/msc_geo.c b/apache2/msc_geo.c index 6f60b0032..7d3a8e402 100644 --- a/apache2/msc_geo.c +++ b/apache2/msc_geo.c @@ -325,11 +325,7 @@ int geo_lookup(modsec_rec *msr, geo_rec *georec, const char *target, char **erro msr_log(msr, 9, "GEO: Using address \"%s\" (0x%08lx). %lu", targetip, ipnum, ipnum); } - ret = apr_global_mutex_lock(msr->modsecurity->geo_lock); - if (ret != APR_SUCCESS) { - msr_log(msr, 1, "Geo Lookup: Failed to lock proc mutex: %s", - get_apr_error(msr->mp, ret)); - } + msr_global_mutex_lock(msr, msr->modsecurity->geo_lock, "Geo lookup"); for (level = 31; level >= 0; level--) { /* Read the record */ @@ -361,13 +357,7 @@ int geo_lookup(modsec_rec *msr, geo_rec *georec, const char *target, char **erro if (rec_val == geo->ctry_offset) { *error_msg = apr_psprintf(msr->mp, "No geo data for \"%s\").", log_escape(msr->mp, target)); msr_log(msr, 4, "%s", *error_msg); - - ret = apr_global_mutex_unlock(msr->modsecurity->geo_lock); - if (ret != APR_SUCCESS) { - msr_log(msr, 1, "Geo Lookup: Failed to lock proc mutex: %s", - get_apr_error(msr->mp, ret)); - } - + msr_global_mutex_unlock(msr, msr->modsecurity->geo_lock, "Geo Lookup"); return 0; } @@ -377,13 +367,7 @@ int geo_lookup(modsec_rec *msr, geo_rec *georec, const char *target, char **erro if ((country <= 0) || (country > GEO_COUNTRY_LAST)) { *error_msg = apr_psprintf(msr->mp, "No geo data for \"%s\" (country %d).", log_escape(msr->mp, target), country); msr_log(msr, 4, "%s", *error_msg); - - ret = apr_global_mutex_unlock(msr->modsecurity->geo_lock); - if (ret != APR_SUCCESS) { - msr_log(msr, 1, "Geo Lookup: Failed to lock proc mutex: %s", - get_apr_error(msr->mp, ret)); - } - + msr_global_mutex_unlock(msr, msr->modsecurity->geo_lock, "Geo Lookup"); return 0; } @@ -408,13 +392,7 @@ int geo_lookup(modsec_rec *msr, geo_rec *georec, const char *target, char **erro if ((country <= 0) || (country > GEO_COUNTRY_LAST)) { *error_msg = apr_psprintf(msr->mp, "No geo data for \"%s\" (country %d).", log_escape(msr->mp, target), country); msr_log(msr, 4, "%s", *error_msg); - - ret = apr_global_mutex_unlock(msr->modsecurity->geo_lock); - if (ret != APR_SUCCESS) { - msr_log(msr, 1, "Geo Lookup: Failed to lock proc mutex: %s", - get_apr_error(msr->mp, ret)); - } - + msr_global_mutex_unlock(msr, msr->modsecurity->geo_lock, "Geo Lookup"); return 0; } if (msr->txcfg->debuglog_level >= 9) { @@ -503,13 +481,7 @@ int geo_lookup(modsec_rec *msr, geo_rec *georec, const char *target, char **erro } *error_msg = apr_psprintf(msr->mp, "Geo lookup for \"%s\" succeeded.", log_escape(msr->mp, target)); - - ret = apr_global_mutex_unlock(msr->modsecurity->geo_lock); - if (ret != APR_SUCCESS) { - msr_log(msr, 1, "Geo Lookup: Failed to lock proc mutex: %s", - get_apr_error(msr->mp, ret)); - } - + msr_global_mutex_unlock(msr, msr->modsecurity->geo_lock, "Geo Lookup"); return 1; } diff --git a/apache2/msc_logging.c b/apache2/msc_logging.c index 46c088262..39588b10f 100644 --- a/apache2/msc_logging.c +++ b/apache2/msc_logging.c @@ -757,14 +757,7 @@ void sec_audit_logger_json(modsec_rec *msr) { /* Lock the mutex, but only if we are using serial format. */ if (msr->txcfg->auditlog_type != AUDITLOG_CONCURRENT) { - if (!msr->modsecurity->auditlog_lock) msr_log(msr, 1, "Audit log: Global mutex was not created"); - else { - rc = apr_global_mutex_lock(msr->modsecurity->auditlog_lock); - if (rc != APR_SUCCESS) { - msr_log(msr, 1, "Audit log: Failed to lock global mutex: %s", - get_apr_error(msr->mp, rc)); - } - } + msr_global_mutex_lock(msr, msr->modsecurity->auditlog_lock, "Audit log"); } /** @@ -1471,15 +1464,8 @@ void sec_audit_logger_json(modsec_rec *msr) { * as it does not need an index file. */ if (msr->txcfg->auditlog_type != AUDITLOG_CONCURRENT) { - /* Unlock the mutex we used to serialise access to the audit log file. */ - rc = apr_global_mutex_unlock(msr->modsecurity->auditlog_lock); - if (rc != APR_SUCCESS) { - msr_log(msr, 1, "Audit log: Failed to unlock global mutex '%s': %s", - apr_global_mutex_lockfile(msr->modsecurity->auditlog_lock), - get_apr_error(msr->mp, rc)); - } - + msr_global_mutex_unlock(msr, msr->modsecurity->auditlog_lock, "Audit log"); return; } @@ -1650,11 +1636,7 @@ void sec_audit_logger_native(modsec_rec *msr) { /* Lock the mutex, but only if we are using serial format. */ if (msr->txcfg->auditlog_type != AUDITLOG_CONCURRENT) { - rc = apr_global_mutex_lock(msr->modsecurity->auditlog_lock); - if (rc != APR_SUCCESS) { - msr_log(msr, 1, "Audit log: Failed to lock global mutex: %s", - get_apr_error(msr->mp, rc)); - } + msr_global_mutex_lock(msr, msr->modsecurity->auditlog_lock, "Audit log"); } @@ -2253,15 +2235,8 @@ void sec_audit_logger_native(modsec_rec *msr) { */ if (msr->txcfg->auditlog_type != AUDITLOG_CONCURRENT) { sec_auditlog_write(msr, "\n", 1); - /* Unlock the mutex we used to serialise access to the audit log file. */ - rc = apr_global_mutex_unlock(msr->modsecurity->auditlog_lock); - if (rc != APR_SUCCESS) { - msr_log(msr, 1, "Audit log: Failed to unlock global mutex '%s': %s", - apr_global_mutex_lockfile(msr->modsecurity->auditlog_lock), - get_apr_error(msr->mp, rc)); - } - + msr_global_mutex_unlock(msr, msr->modsecurity->auditlog_lock, "Audit log"); return; } diff --git a/apache2/persist_dbm.c b/apache2/persist_dbm.c index c685e1de3..ba8475cc5 100644 --- a/apache2/persist_dbm.c +++ b/apache2/persist_dbm.c @@ -125,19 +125,15 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec 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; - } + rc = msr_global_mutex_lock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex"); + if (rc != APR_SUCCESS) 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); + msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex"); #endif goto cleanup; } @@ -173,7 +169,7 @@ 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); + msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex"); #endif dbm = NULL; } @@ -222,12 +218,8 @@ 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; - } + rc = msr_global_mutex_lock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex"); + if (rc != APR_SUCCESS) goto cleanup; #endif rc = apr_sdbm_open(&dbm, dbm_filename, APR_CREATE | APR_WRITE | APR_SHARELOCK, CREATEMODE, msr->mp); @@ -236,7 +228,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec 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); + msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex"); #endif goto cleanup; } @@ -261,7 +253,7 @@ 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); + msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex"); #endif dbm = NULL; } @@ -326,7 +318,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec apr_sdbm_close(dbm); #ifdef GLOBAL_COLLECTION_LOCK - apr_global_mutex_unlock(msr->modsecurity->dbm_lock); + msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex"); #endif } @@ -337,7 +329,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec if ((existing_dbm == NULL) && dbm) { apr_sdbm_close(dbm); #ifdef GLOBAL_COLLECTION_LOCK - apr_global_mutex_unlock(msr->modsecurity->dbm_lock); + msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex"); #endif } @@ -408,12 +400,8 @@ int collection_store(modsec_rec *msr, apr_table_t *col) { #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; - } + int ret = msr_global_mutex_lock(msr, msr->modsecurity->dbm_lock, "collection_store"); + if (ret != APR_SUCCESS) goto error; #endif /* Delete IS_NEW on store. */ @@ -473,7 +461,7 @@ int collection_store(modsec_rec *msr, apr_table_t *col) { CREATEMODE, msr->mp); if (rc != APR_SUCCESS) { #ifdef GLOBAL_COLLECTION_LOCK - apr_global_mutex_unlock(msr->modsecurity->dbm_lock); + msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_store"); #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)); @@ -556,7 +544,7 @@ int collection_store(modsec_rec *msr, apr_table_t *col) { if (dbm != NULL) { #ifdef GLOBAL_COLLECTION_LOCK apr_sdbm_close(dbm); - apr_global_mutex_unlock(msr->modsecurity->dbm_lock); + msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_store"); #else apr_sdbm_unlock(dbm); apr_sdbm_close(dbm); @@ -619,7 +607,7 @@ int collection_store(modsec_rec *msr, apr_table_t *col) { if (dbm != NULL) { #ifdef GLOBAL_COLLECTION_LOCK apr_sdbm_close(dbm); - apr_global_mutex_unlock(msr->modsecurity->dbm_lock); + msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_store"); #else apr_sdbm_unlock(dbm); apr_sdbm_close(dbm); @@ -631,7 +619,7 @@ int collection_store(modsec_rec *msr, apr_table_t *col) { #ifdef GLOBAL_COLLECTION_LOCK apr_sdbm_close(dbm); - apr_global_mutex_unlock(msr->modsecurity->dbm_lock); + msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_store"); #else apr_sdbm_unlock(dbm); apr_sdbm_close(dbm); @@ -684,19 +672,15 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) { } #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; - } + rc = msr_global_mutex_lock(msr, msr->modsecurity->dbm_lock, "collections_remove_stale"); + if (rc != APR_SUCCESS) 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); + msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collections_remove_stale"); #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)); @@ -799,7 +783,7 @@ 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); + msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collections_remove_stale"); #endif return 1; @@ -808,7 +792,7 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) { if (dbm) { apr_sdbm_close(dbm); #ifdef GLOBAL_COLLECTION_LOCK - apr_global_mutex_unlock(msr->modsecurity->dbm_lock); + msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collections_remove_stale"); #endif }