From: Joe Orton <jorton@apache.org>
Date: Tue, 14 Apr 2020 12:37:17 +0000 (+0000)
Subject: * modules/core/mod_watchdog.c: Switch to simpler logic to avoid the
X-Git-Tag: 2.5.0-alpha2-ci-test-only~1526
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=452ae9a3c234295f7a8b4e94041f015919d9f5f4;p=thirdparty%2Fapache%2Fhttpd.git

* modules/core/mod_watchdog.c: Switch to simpler logic to avoid the
  thread cleanup running before the thread has started, avoiding
  mutex operations which both have undefined behaviour:

  a) double-locking an UNNESTED (non-recursive) mutex twice in the parent
  b) unlocking a mutex in the spawned thread which was locked by the parent

  (wd_startup, wd_worker_cleanup, wd_worker): Use a boolean to ensure
  the cleanup does nothing if the thread wasn't started, drop the mutex.


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1876511 13f79535-47bb-0310-9956-ffa450edef68
---

diff --git a/modules/core/mod_watchdog.c b/modules/core/mod_watchdog.c
index 51dea91c69e..bc05e852115 100644
--- a/modules/core/mod_watchdog.c
+++ b/modules/core/mod_watchdog.c
@@ -24,6 +24,8 @@
 #include "http_core.h"
 #include "util_mutex.h"
 
+#include "apr_atomic.h"
+
 #define AP_WATCHDOG_PGROUP    "watchdog"
 #define AP_WATCHDOG_PVERSION  "parent"
 #define AP_WATCHDOG_CVERSION  "child"
@@ -43,7 +45,7 @@ struct watchdog_list_t
 
 struct ap_watchdog_t
 {
-    apr_thread_mutex_t   *startup;
+    apr_uint32_t          thread_started; /* set to 1 once thread running */
     apr_proc_mutex_t     *mutex;
     const char           *name;
     watchdog_list_t      *callbacks;
@@ -74,6 +76,10 @@ static apr_status_t wd_worker_cleanup(void *data)
     apr_status_t rv;
     ap_watchdog_t *w = (ap_watchdog_t *)data;
 
+    /* Do nothing if the thread wasn't started. */
+    if (apr_atomic_read32(&w->thread_started) != 1)
+        return APR_SUCCESS;
+
     if (w->is_running) {
         watchdog_list_t *wl = w->callbacks;
         while (wl) {
@@ -110,7 +116,8 @@ static void* APR_THREAD_FUNC wd_worker(apr_thread_t *thread, void *data)
     w->pool = apr_thread_pool_get(thread);
     w->is_running = 1;
 
-    apr_thread_mutex_unlock(w->startup);
+    apr_atomic_set32(&w->thread_started, 1); /* thread started */
+
     if (w->mutex) {
         while (w->is_running) {
             if (ap_mpm_query(AP_MPMQ_MPM_STATE, &mpmq_s) != APR_SUCCESS) {
@@ -264,10 +271,7 @@ static apr_status_t wd_startup(ap_watchdog_t *w, apr_pool_t *p)
 {
     apr_status_t rc;
 
-    /* Create thread startup mutex */
-    rc = apr_thread_mutex_create(&w->startup, APR_THREAD_MUTEX_UNNESTED, p);
-    if (rc != APR_SUCCESS)
-        return rc;
+    apr_atomic_set32(&w->thread_started, 0);
 
     if (w->singleton) {
         /* Initialize singleton mutex in child */
@@ -277,22 +281,12 @@ static apr_status_t wd_startup(ap_watchdog_t *w, apr_pool_t *p)
             return rc;
     }
 
-    /* This mutex fixes problems with a fast start/fast end, where the pool
-     * cleanup was being invoked before the thread completely spawned.
-     */
-    apr_thread_mutex_lock(w->startup);
-    apr_pool_pre_cleanup_register(p, w, wd_worker_cleanup);
-
     /* Start the newly created watchdog */
     rc = apr_thread_create(&w->thread, NULL, wd_worker, w, p);
-    if (rc) {
-        apr_pool_cleanup_kill(p, w, wd_worker_cleanup);
+    if (rc == APR_SUCCESS) {
+        apr_pool_pre_cleanup_register(p, w, wd_worker_cleanup);
     }
 
-    apr_thread_mutex_lock(w->startup);
-    apr_thread_mutex_unlock(w->startup);
-    apr_thread_mutex_destroy(w->startup);
-
     return rc;
 }