We don't have to generate a temp name ourselves, it'll be done in apr_global_mutex_create().

Marc Stern · Marc Stern · commit dfacf88ec932 · 2024-10-03T09:00:23.000+02:00
We don't have to provide a filename, apr_global_mutex_create() generates one automatically.
Moreover, under Unix &amp; Windows, the preferred mechanism won't use a file at all =&gt; don't waste time.
apr_file_mktemp() cannot be used as it creates the file (at least on FreeBSD).
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
