Skip to content

Commit 84d2f30

Browse files
Mladen TurkFelipe Zimmerle
Mladen Turk
authored and
Felipe Zimmerle
committed
Use global mutex instead sdbm file lock to fix issues with threaded mpm's
1 parent 2de5175 commit 84d2f30

File tree

3 files changed

+68
-21
lines changed

3 files changed

+68
-21
lines changed

‎apache2/modsecurity.c

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,22 @@ int modsecurity_init(msc_engine *msce, apr_pool_t *mp) {
169169
return -1;
170170
}
171171
#endif /* SET_MUTEX_PERMS */
172+
173+
rc = apr_global_mutex_create(&msce->dbm_lock, NULL, APR_LOCK_DEFAULT, mp);
174+
if (rc != APR_SUCCESS) {
175+
return -1;
176+
}
177+
178+
#ifdef __SET_MUTEX_PERMS
179+
#if AP_SERVER_MAJORVERSION_NUMBER > 1 && AP_SERVER_MINORVERSION_NUMBER > 2
180+
rc = ap_unixd_set_global_mutex_perms(msce->dbm_lock);
181+
#else
182+
rc = unixd_set_global_mutex_perms(msce->dbm_lock);
183+
#endif
184+
if (rc != APR_SUCCESS) {
185+
return -1;
186+
}
187+
#endif /* SET_MUTEX_PERMS */
172188
#endif
173189

174190
return 1;
@@ -195,6 +211,12 @@ void modsecurity_child_init(msc_engine *msce) {
195211
}
196212
}
197213

214+
if (msce->dbm_lock != NULL) {
215+
apr_status_t rc = apr_global_mutex_child_init(&msce->dbm_lock, NULL, msce->mp);
216+
if (rc != APR_SUCCESS) {
217+
// ap_log_error(APLOG_MARK, APLOG_ERR, rs, s, "Failed to child-init dbm mutex");
218+
}
219+
}
198220
}
199221

200222
/**

‎apache2/modsecurity.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -657,6 +657,7 @@ struct msc_engine {
657657
apr_pool_t *mp;
658658
apr_global_mutex_t *auditlog_lock;
659659
apr_global_mutex_t *geo_lock;
660+
apr_global_mutex_t *dbm_lock;
660661
msre_engine *msre;
661662
unsigned int processing_mode;
662663
};

‎apache2/persist_dbm.c

Lines changed: 45 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
101101
int expired = 0;
102102
int i;
103103

104+
104105
if (msr->txcfg->data_dir == NULL) {
105106
msr_log(msr, 1, "collection_retrieve_ex: Unable to retrieve collection (name \"%s\", key \"%s\"). Use "
106107
"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
119120
key.dsize = col_key_len + 1;
120121

121122
if (existing_dbm == NULL) {
123+
rc = apr_global_mutex_lock(msr->modsecurity->dbm_lock);
124+
if (rc != APR_SUCCESS) {
125+
msr_log(msr, 1, "collection_retrieve_ex: Failed to lock proc mutex: %s",
126+
get_apr_error(msr->mp, rc));
127+
goto cleanup;
128+
}
129+
122130
rc = apr_sdbm_open(&dbm, dbm_filename, APR_READ | APR_SHARELOCK,
123131
CREATEMODE, msr->mp);
124132
if (rc != APR_SUCCESS) {
125133
dbm = NULL;
134+
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
126135
goto cleanup;
127136
}
128137
}
@@ -156,6 +165,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
156165
/* Close after "value" used from fetch or memory may be overwritten. */
157166
if (existing_dbm == NULL) {
158167
apr_sdbm_close(dbm);
168+
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
159169
dbm = NULL;
160170
}
161171

@@ -202,12 +212,19 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
202212
*/
203213
if (apr_table_get(col, "KEY") == NULL) {
204214
if (existing_dbm == NULL) {
215+
rc = apr_global_mutex_lock(msr->modsecurity->dbm_lock);
216+
if (rc != APR_SUCCESS) {
217+
msr_log(msr, 1, "collection_retrieve_ex: Failed to lock proc mutex: %s",
218+
get_apr_error(msr->mp, rc));
219+
goto cleanup;
220+
}
205221
rc = apr_sdbm_open(&dbm, dbm_filename, APR_CREATE | APR_WRITE | APR_SHARELOCK,
206222
CREATEMODE, msr->mp);
207223
if (rc != APR_SUCCESS) {
208224
msr_log(msr, 1, "collection_retrieve_ex: Failed to access DBM file \"%s\": %s",
209225
log_escape(msr->mp, dbm_filename), get_apr_error(msr->mp, rc));
210226
dbm = NULL;
227+
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
211228
goto cleanup;
212229
}
213230
}
@@ -230,6 +247,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
230247

231248
if (existing_dbm == NULL) {
232249
apr_sdbm_close(dbm);
250+
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
233251
dbm = NULL;
234252
}
235253

@@ -292,14 +310,16 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
292310
log_escape(msr->mp, col_name), log_escape_ex(msr->mp, col_key, col_key_len));
293311

294312
apr_sdbm_close(dbm);
313+
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
295314
}
296-
315+
297316
return col;
298317

299318
cleanup:
300319

301320
if ((existing_dbm == NULL) && dbm) {
302321
apr_sdbm_close(dbm);
322+
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
303323
}
304324

305325
return NULL;
@@ -412,6 +432,14 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
412432
var->value_len = strlen(var->value);
413433
}
414434

435+
/* Need to lock to pull in the stored data again and apply deltas. */
436+
rc = apr_global_mutex_lock(msr->modsecurity->dbm_lock);
437+
if (rc != APR_SUCCESS) {
438+
msr_log(msr, 1, "collection_store: Failed to lock proc mutex: %s",
439+
get_apr_error(msr->mp, rc));
440+
goto error;
441+
}
442+
415443
/* ENH Make the expiration timestamp accessible in blob form so that
416444
* it is easier/faster to determine expiration without having to
417445
* convert back to table form
@@ -420,19 +448,13 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
420448
rc = apr_sdbm_open(&dbm, dbm_filename, APR_CREATE | APR_WRITE | APR_SHARELOCK,
421449
CREATEMODE, msr->mp);
422450
if (rc != APR_SUCCESS) {
451+
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
423452
msr_log(msr, 1, "collection_store: Failed to access DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
424453
get_apr_error(msr->mp, rc));
425454
dbm = NULL;
426455
goto error;
427456
}
428-
429-
/* Need to lock to pull in the stored data again and apply deltas. */
430-
rc = apr_sdbm_lock(dbm, APR_FLOCK_EXCLUSIVE);
431-
if (rc != APR_SUCCESS) {
432-
msr_log(msr, 1, "collection_store: Failed to exclusivly lock DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
433-
get_apr_error(msr->mp, rc));
434-
goto error;
435-
}
457+
436458

437459
/* If there is an original value, then create a delta and
438460
* apply the delta to the current value */
@@ -497,8 +519,8 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
497519
blob = apr_pcalloc(msr->mp, blob_size);
498520
if (blob == NULL) {
499521
if (dbm != NULL) {
500-
apr_sdbm_unlock(dbm);
501522
apr_sdbm_close(dbm);
523+
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
502524
}
503525

504526
return -1;
@@ -555,16 +577,16 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
555577
msr_log(msr, 1, "collection_store: Failed to write to DBM file \"%s\": %s", dbm_filename,
556578
get_apr_error(msr->mp, rc));
557579
if (dbm != NULL) {
558-
apr_sdbm_unlock(dbm);
559580
apr_sdbm_close(dbm);
581+
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
560582
}
561583

562584
return -1;
563585
}
564586

565-
apr_sdbm_unlock(dbm);
566587
apr_sdbm_close(dbm);
567-
588+
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
589+
568590
if (msr->txcfg->debuglog_level >= 4) {
569591
msr_log(msr, 4, "collection_store: Persisted collection (name \"%s\", key \"%s\").",
570592
log_escape_ex(msr->mp, var_name->value, var_name->value_len),
@@ -608,9 +630,17 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
608630
log_escape(msr->mp, dbm_filename));
609631
}
610632

633+
rc = apr_global_mutex_lock(msr->modsecurity->dbm_lock);
634+
if (rc != APR_SUCCESS) {
635+
msr_log(msr, 1, "collections_remove_stale: Failed to lock proc mutex: %s",
636+
get_apr_error(msr->mp, rc));
637+
goto error;
638+
}
639+
611640
rc = apr_sdbm_open(&dbm, dbm_filename, APR_CREATE | APR_WRITE | APR_SHARELOCK,
612641
CREATEMODE, msr->mp);
613642
if (rc != APR_SUCCESS) {
643+
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
614644
msr_log(msr, 1, "collections_remove_stale: Failed to access DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
615645
get_apr_error(msr->mp, rc));
616646
dbm = NULL;
@@ -619,12 +649,6 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
619649

620650
/* First get a list of all keys. */
621651
keys_arr = apr_array_make(msr->mp, 256, sizeof(char *));
622-
rc = apr_sdbm_lock(dbm, APR_FLOCK_SHARED);
623-
if (rc != APR_SUCCESS) {
624-
msr_log(msr, 1, "collections_remove_stale: Failed to lock DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
625-
get_apr_error(msr->mp, rc));
626-
goto error;
627-
}
628652

629653
/* No one can write to the file while doing this so
630654
* do it as fast as possible.
@@ -637,7 +661,6 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
637661
}
638662
rc = apr_sdbm_nextkey(dbm, &key);
639663
}
640-
apr_sdbm_unlock(dbm);
641664

642665
if (msr->txcfg->debuglog_level >= 9) {
643666
msr_log(msr, 9, "collections_remove_stale: Found %d record(s) in file \"%s\".", keys_arr->nelts,
@@ -706,13 +729,14 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
706729
}
707730

708731
apr_sdbm_close(dbm);
709-
732+
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
710733
return 1;
711734

712735
error:
713736

714737
if (dbm) {
715738
apr_sdbm_close(dbm);
739+
apr_global_mutex_unlock(msr->modsecurity->dbm_lock);
716740
}
717741

718742
return -1;

0 commit comments

Comments
 (0)