Skip to content

Commit

Permalink
Merge pull request #3257 from marcstern/v2/pr/msr_global_mutex_lock
Browse files Browse the repository at this point in the history 
msr_global_mutex_lock: handle errors from apr_global_mutex_lock
  • Loading branch information
@marcstern
marcstern authored Oct 2, 2024
2 parents d6f1ebb + c99d931 commit 090e4d3
Show file tree
Hide file tree
Showing 6 changed files with 74 additions and 100 deletions.
6 changes: 6 additions & 0 deletions CHANGES
View file
Original file line number Diff line number Diff line change
@@ -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
-------------------

Expand Down
37 changes: 36 additions & 1 deletion apache2/modsecurity.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ 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;
const char *filename = NULL;

// get platform temp dir
rc = apr_temp_dir_get(&temp_dir, mp);
Expand Down Expand Up @@ -166,6 +166,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
Expand Down
2 changes: 2 additions & 0 deletions apache2/modsecurity.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,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 */

Expand Down
38 changes: 5 additions & 33 deletions apache2/msc_geo.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand All @@ -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) {
Expand Down Expand Up @@ -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;
}

Expand Down
33 changes: 4 additions & 29 deletions apache2/msc_logging.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}

/**
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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");
}


Expand Down Expand Up @@ -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;
}

Expand Down
58 changes: 21 additions & 37 deletions apache2/persist_dbm.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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
}

Expand All @@ -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
}

Expand Down Expand Up @@ -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. */
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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;

Expand All @@ -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
}

Expand Down

0 comments on commit 090e4d3

Please sign in to comment.