Skip to content

Commit 090e4d3

Browse files
author
Marc Stern
authored
Merge pull request owasp-modsecurity#3257 from marcstern/v2/pr/msr_global_mutex_lock
msr_global_mutex_lock: handle errors from apr_global_mutex_lock
2 parents d6f1ebb + c99d931 commit 090e4d3

File tree

6 files changed

+74
-100
lines changed

6 files changed

+74
-100
lines changed

‎CHANGES‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,9 @@
1+
(to be released) - 2.9.x
2+
------------------------
3+
4+
* handle errors from apr_global_mutex_lock
5+
[PR #3257 - @marcstern]
6+
17
03 Sep 2024 - 2.9.8
28
-------------------
39

‎apache2/modsecurity.c‎

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ int acquire_global_lock(apr_global_mutex_t **lock, apr_pool_t *mp) {
126126
apr_status_t rc;
127127
apr_file_t *lock_name;
128128
const char *temp_dir;
129-
const char *filename;
129+
const char *filename = NULL;
130130

131131
// get platform temp dir
132132
rc = apr_temp_dir_get(&temp_dir, mp);
@@ -166,6 +166,41 @@ int acquire_global_lock(apr_global_mutex_t **lock, apr_pool_t *mp) {
166166
#endif /* MSC_TEST */
167167
return APR_SUCCESS;
168168
}
169+
170+
/**
171+
* handle errors from apr_global_mutex_lock
172+
*/
173+
int msr_global_mutex_lock(modsec_rec* msr, apr_global_mutex_t* lock, const char* fct) {
174+
assert(msr);
175+
assert(msr->modsecurity); // lock is msr->modsecurity->..._lock
176+
assert(msr->mp);
177+
if (!lock) {
178+
msr_log(msr, 1, "%s: Global mutex was not created", fct);
179+
return -1;
180+
}
181+
182+
int rc = apr_global_mutex_lock(lock);
183+
if (rc != APR_SUCCESS) msr_log(msr, 1, "Audit log: Failed to lock global mutex: %s", get_apr_error(msr->mp, rc));
184+
return rc;
185+
}
186+
/**
187+
* handle errors from apr_global_mutex_unlock
188+
*/
189+
int msr_global_mutex_unlock(modsec_rec* msr, apr_global_mutex_t* lock, const char* fct) {
190+
assert(msr);
191+
assert(msr->modsecurity); // lock is msr->modsecurity->..._lock
192+
assert(msr->mp);
193+
if (!lock) {
194+
msr_log(msr, 1, "%s: Global mutex was not created", fct);
195+
return -1;
196+
}
197+
198+
int rc = apr_global_mutex_unlock(lock);
199+
// We should have get the warning at lock time, so ignore it here
200+
// if (rc != APR_SUCCESS) msr_log(msr, 1, "Audit log: Failed to unlock global mutex: %s", get_apr_error(msr->mp, rc));
201+
return rc;
202+
}
203+
169204
/**
170205
* Initialise the modsecurity engine. This function must be invoked
171206
* after configuration processing is complete as Apache needs to know the

‎apache2/modsecurity.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -707,6 +707,8 @@ struct msc_parm {
707707

708708
/* Reusable functions */
709709
int acquire_global_lock(apr_global_mutex_t **lock, apr_pool_t *mp);
710+
int msr_global_mutex_lock(modsec_rec* msr, apr_global_mutex_t* lock, const char* fct);
711+
int msr_global_mutex_unlock(modsec_rec* msr, apr_global_mutex_t* lock, const char* fct);
710712

711713
/* Engine functions */
712714

‎apache2/msc_geo.c‎

Lines changed: 5 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -325,11 +325,7 @@ int geo_lookup(modsec_rec *msr, geo_rec *georec, const char *target, char **erro
325325
msr_log(msr, 9, "GEO: Using address \"%s\" (0x%08lx). %lu", targetip, ipnum, ipnum);
326326
}
327327

328-
ret = apr_global_mutex_lock(msr->modsecurity->geo_lock);
329-
if (ret != APR_SUCCESS) {
330-
msr_log(msr, 1, "Geo Lookup: Failed to lock proc mutex: %s",
331-
get_apr_error(msr->mp, ret));
332-
}
328+
msr_global_mutex_lock(msr, msr->modsecurity->geo_lock, "Geo lookup");
333329

334330
for (level = 31; level >= 0; level--) {
335331
/* Read the record */
@@ -361,13 +357,7 @@ int geo_lookup(modsec_rec *msr, geo_rec *georec, const char *target, char **erro
361357
if (rec_val == geo->ctry_offset) {
362358
*error_msg = apr_psprintf(msr->mp, "No geo data for \"%s\").", log_escape(msr->mp, target));
363359
msr_log(msr, 4, "%s", *error_msg);
364-
365-
ret = apr_global_mutex_unlock(msr->modsecurity->geo_lock);
366-
if (ret != APR_SUCCESS) {
367-
msr_log(msr, 1, "Geo Lookup: Failed to lock proc mutex: %s",
368-
get_apr_error(msr->mp, ret));
369-
}
370-
360+
msr_global_mutex_unlock(msr, msr->modsecurity->geo_lock, "Geo Lookup");
371361
return 0;
372362
}
373363

@@ -377,13 +367,7 @@ int geo_lookup(modsec_rec *msr, geo_rec *georec, const char *target, char **erro
377367
if ((country <= 0) || (country > GEO_COUNTRY_LAST)) {
378368
*error_msg = apr_psprintf(msr->mp, "No geo data for \"%s\" (country %d).", log_escape(msr->mp, target), country);
379369
msr_log(msr, 4, "%s", *error_msg);
380-
381-
ret = apr_global_mutex_unlock(msr->modsecurity->geo_lock);
382-
if (ret != APR_SUCCESS) {
383-
msr_log(msr, 1, "Geo Lookup: Failed to lock proc mutex: %s",
384-
get_apr_error(msr->mp, ret));
385-
}
386-
370+
msr_global_mutex_unlock(msr, msr->modsecurity->geo_lock, "Geo Lookup");
387371
return 0;
388372
}
389373

@@ -408,13 +392,7 @@ int geo_lookup(modsec_rec *msr, geo_rec *georec, const char *target, char **erro
408392
if ((country <= 0) || (country > GEO_COUNTRY_LAST)) {
409393
*error_msg = apr_psprintf(msr->mp, "No geo data for \"%s\" (country %d).", log_escape(msr->mp, target), country);
410394
msr_log(msr, 4, "%s", *error_msg);
411-
412-
ret = apr_global_mutex_unlock(msr->modsecurity->geo_lock);
413-
if (ret != APR_SUCCESS) {
414-
msr_log(msr, 1, "Geo Lookup: Failed to lock proc mutex: %s",
415-
get_apr_error(msr->mp, ret));
416-
}
417-
395+
msr_global_mutex_unlock(msr, msr->modsecurity->geo_lock, "Geo Lookup");
418396
return 0;
419397
}
420398
if (msr->txcfg->debuglog_level >= 9) {
@@ -503,13 +481,7 @@ int geo_lookup(modsec_rec *msr, geo_rec *georec, const char *target, char **erro
503481
}
504482

505483
*error_msg = apr_psprintf(msr->mp, "Geo lookup for \"%s\" succeeded.", log_escape(msr->mp, target));
506-
507-
ret = apr_global_mutex_unlock(msr->modsecurity->geo_lock);
508-
if (ret != APR_SUCCESS) {
509-
msr_log(msr, 1, "Geo Lookup: Failed to lock proc mutex: %s",
510-
get_apr_error(msr->mp, ret));
511-
}
512-
484+
msr_global_mutex_unlock(msr, msr->modsecurity->geo_lock, "Geo Lookup");
513485
return 1;
514486
}
515487

‎apache2/msc_logging.c‎

Lines changed: 4 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -757,14 +757,7 @@ void sec_audit_logger_json(modsec_rec *msr) {
757757

758758
/* Lock the mutex, but only if we are using serial format. */
759759
if (msr->txcfg->auditlog_type != AUDITLOG_CONCURRENT) {
760-
if (!msr->modsecurity->auditlog_lock) msr_log(msr, 1, "Audit log: Global mutex was not created");
761-
else {
762-
rc = apr_global_mutex_lock(msr->modsecurity->auditlog_lock);
763-
if (rc != APR_SUCCESS) {
764-
msr_log(msr, 1, "Audit log: Failed to lock global mutex: %s",
765-
get_apr_error(msr->mp, rc));
766-
}
767-
}
760+
msr_global_mutex_lock(msr, msr->modsecurity->auditlog_lock, "Audit log");
768761
}
769762

770763
/**
@@ -1471,15 +1464,8 @@ void sec_audit_logger_json(modsec_rec *msr) {
14711464
* as it does not need an index file.
14721465
*/
14731466
if (msr->txcfg->auditlog_type != AUDITLOG_CONCURRENT) {
1474-
14751467
/* Unlock the mutex we used to serialise access to the audit log file. */
1476-
rc = apr_global_mutex_unlock(msr->modsecurity->auditlog_lock);
1477-
if (rc != APR_SUCCESS) {
1478-
msr_log(msr, 1, "Audit log: Failed to unlock global mutex '%s': %s",
1479-
apr_global_mutex_lockfile(msr->modsecurity->auditlog_lock),
1480-
get_apr_error(msr->mp, rc));
1481-
}
1482-
1468+
msr_global_mutex_unlock(msr, msr->modsecurity->auditlog_lock, "Audit log");
14831469
return;
14841470
}
14851471

@@ -1650,11 +1636,7 @@ void sec_audit_logger_native(modsec_rec *msr) {
16501636

16511637
/* Lock the mutex, but only if we are using serial format. */
16521638
if (msr->txcfg->auditlog_type != AUDITLOG_CONCURRENT) {
1653-
rc = apr_global_mutex_lock(msr->modsecurity->auditlog_lock);
1654-
if (rc != APR_SUCCESS) {
1655-
msr_log(msr, 1, "Audit log: Failed to lock global mutex: %s",
1656-
get_apr_error(msr->mp, rc));
1657-
}
1639+
msr_global_mutex_lock(msr, msr->modsecurity->auditlog_lock, "Audit log");
16581640
}
16591641

16601642

@@ -2253,15 +2235,8 @@ void sec_audit_logger_native(modsec_rec *msr) {
22532235
*/
22542236
if (msr->txcfg->auditlog_type != AUDITLOG_CONCURRENT) {
22552237
sec_auditlog_write(msr, "\n", 1);
2256-
22572238
/* Unlock the mutex we used to serialise access to the audit log file. */
2258-
rc = apr_global_mutex_unlock(msr->modsecurity->auditlog_lock);
2259-
if (rc != APR_SUCCESS) {
2260-
msr_log(msr, 1, "Audit log: Failed to unlock global mutex '%s': %s",
2261-
apr_global_mutex_lockfile(msr->modsecurity->auditlog_lock),
2262-
get_apr_error(msr->mp, rc));
2263-
}
2264-
2239+
msr_global_mutex_unlock(msr, msr->modsecurity->auditlog_lock, "Audit log");
22652240
return;
22662241
}
22672242

‎apache2/persist_dbm.c‎

Lines changed: 21 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -125,19 +125,15 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
125125

126126
if (existing_dbm == NULL) {
127127
#ifdef GLOBAL_COLLECTION_LOCK
128-
rc = apr_global_mutex_lock(msr->modsecurity->dbm_lock);
129-
if (rc != APR_SUCCESS) {
130-
msr_log(msr, 1, "collection_retrieve_ex: Failed to lock proc mutex: %s",
131-
get_apr_error(msr->mp, rc));
132-
goto cleanup;
133-
}
128+
rc = msr_global_mutex_lock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex");
129+
if (rc != APR_SUCCESS) goto cleanup;
134130
#endif
135131
rc = apr_sdbm_open(&dbm, dbm_filename, APR_READ | APR_SHARELOCK,
136132
CREATEMODE, msr->mp);
137133
if (rc != APR_SUCCESS) {
138134
dbm = NULL;
139135
#ifdef GLOBAL_COLLECTION_LOCK
140-
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
136+
msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex");
141137
#endif
142138
goto cleanup;
143139
}
@@ -173,7 +169,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
173169
if (existing_dbm == NULL) {
174170
apr_sdbm_close(dbm);
175171
#ifdef GLOBAL_COLLECTION_LOCK
176-
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
172+
msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex");
177173
#endif
178174
dbm = NULL;
179175
}
@@ -222,12 +218,8 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
222218
if (apr_table_get(col, "KEY") == NULL) {
223219
if (existing_dbm == NULL) {
224220
#ifdef GLOBAL_COLLECTION_LOCK
225-
rc = apr_global_mutex_lock(msr->modsecurity->dbm_lock);
226-
if (rc != APR_SUCCESS) {
227-
msr_log(msr, 1, "collection_retrieve_ex: Failed to lock proc mutex: %s",
228-
get_apr_error(msr->mp, rc));
229-
goto cleanup;
230-
}
221+
rc = msr_global_mutex_lock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex");
222+
if (rc != APR_SUCCESS) goto cleanup;
231223
#endif
232224
rc = apr_sdbm_open(&dbm, dbm_filename, APR_CREATE | APR_WRITE | APR_SHARELOCK,
233225
CREATEMODE, msr->mp);
@@ -236,7 +228,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
236228
log_escape(msr->mp, dbm_filename), get_apr_error(msr->mp, rc));
237229
dbm = NULL;
238230
#ifdef GLOBAL_COLLECTION_LOCK
239-
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
231+
msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex");
240232
#endif
241233
goto cleanup;
242234
}
@@ -261,7 +253,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
261253
if (existing_dbm == NULL) {
262254
apr_sdbm_close(dbm);
263255
#ifdef GLOBAL_COLLECTION_LOCK
264-
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
256+
msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex");
265257
#endif
266258
dbm = NULL;
267259
}
@@ -326,7 +318,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
326318

327319
apr_sdbm_close(dbm);
328320
#ifdef GLOBAL_COLLECTION_LOCK
329-
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
321+
msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex");
330322
#endif
331323
}
332324

@@ -337,7 +329,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
337329
if ((existing_dbm == NULL) && dbm) {
338330
apr_sdbm_close(dbm);
339331
#ifdef GLOBAL_COLLECTION_LOCK
340-
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
332+
msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_retrieve_ex");
341333
#endif
342334
}
343335

@@ -408,12 +400,8 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
408400

409401
#ifdef GLOBAL_COLLECTION_LOCK
410402
/* Need to lock to pull in the stored data again and apply deltas. */
411-
rc = apr_global_mutex_lock(msr->modsecurity->dbm_lock);
412-
if (rc != APR_SUCCESS) {
413-
msr_log(msr, 1, "collection_store: Failed to lock proc mutex: %s",
414-
get_apr_error(msr->mp, rc));
415-
goto error;
416-
}
403+
int ret = msr_global_mutex_lock(msr, msr->modsecurity->dbm_lock, "collection_store");
404+
if (ret != APR_SUCCESS) goto error;
417405
#endif
418406

419407
/* Delete IS_NEW on store. */
@@ -473,7 +461,7 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
473461
CREATEMODE, msr->mp);
474462
if (rc != APR_SUCCESS) {
475463
#ifdef GLOBAL_COLLECTION_LOCK
476-
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
464+
msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_store");
477465
#endif
478466
msr_log(msr, 1, "collection_store: Failed to access DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
479467
get_apr_error(msr->mp, rc));
@@ -556,7 +544,7 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
556544
if (dbm != NULL) {
557545
#ifdef GLOBAL_COLLECTION_LOCK
558546
apr_sdbm_close(dbm);
559-
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
547+
msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_store");
560548
#else
561549
apr_sdbm_unlock(dbm);
562550
apr_sdbm_close(dbm);
@@ -619,7 +607,7 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
619607
if (dbm != NULL) {
620608
#ifdef GLOBAL_COLLECTION_LOCK
621609
apr_sdbm_close(dbm);
622-
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
610+
msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_store");
623611
#else
624612
apr_sdbm_unlock(dbm);
625613
apr_sdbm_close(dbm);
@@ -631,7 +619,7 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
631619

632620
#ifdef GLOBAL_COLLECTION_LOCK
633621
apr_sdbm_close(dbm);
634-
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
622+
msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collection_store");
635623
#else
636624
apr_sdbm_unlock(dbm);
637625
apr_sdbm_close(dbm);
@@ -684,19 +672,15 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
684672
}
685673

686674
#ifdef GLOBAL_COLLECTION_LOCK
687-
rc = apr_global_mutex_lock(msr->modsecurity->dbm_lock);
688-
if (rc != APR_SUCCESS) {
689-
msr_log(msr, 1, "collections_remove_stale: Failed to lock proc mutex: %s",
690-
get_apr_error(msr->mp, rc));
691-
goto error;
692-
}
675+
rc = msr_global_mutex_lock(msr, msr->modsecurity->dbm_lock, "collections_remove_stale");
676+
if (rc != APR_SUCCESS) goto error;
693677
#endif
694678

695679
rc = apr_sdbm_open(&dbm, dbm_filename, APR_CREATE | APR_WRITE | APR_SHARELOCK,
696680
CREATEMODE, msr->mp);
697681
if (rc != APR_SUCCESS) {
698682
#ifdef GLOBAL_COLLECTION_LOCK
699-
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
683+
msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collections_remove_stale");
700684
#endif
701685
msr_log(msr, 1, "collections_remove_stale: Failed to access DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
702686
get_apr_error(msr->mp, rc));
@@ -799,7 +783,7 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
799783

800784
apr_sdbm_close(dbm);
801785
#ifdef GLOBAL_COLLECTION_LOCK
802-
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
786+
msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collections_remove_stale");
803787
#endif
804788
return 1;
805789

@@ -808,7 +792,7 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
808792
if (dbm) {
809793
apr_sdbm_close(dbm);
810794
#ifdef GLOBAL_COLLECTION_LOCK
811-
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
795+
msr_global_mutex_unlock(msr, msr->modsecurity->dbm_lock, "collections_remove_stale");
812796
#endif
813797
}
814798

0 commit comments

Comments
 (0)