Merge remote-tracking branch 'tor-github/pr/1346' into maint-0.4.1

8 files changed, 60 insertions, 5 deletions

diff --git a/changes/bug31736 b/changes/bug31736
new file mode 100644
index 0000000000..beb09e5069
--- /dev/null
+++ b/changes/bug31736
@@ -0,0 +1,3 @@
+  o Minor bugfixes (multithreading):
+    - Avoid some undefined behaviour when freeing mutexes.
+      Fixes bug 31736; bugfix on 0.0.7.
diff --git a/src/app/config/config.c b/src/app/config/config.c
index a061871748..8ccbac159a 100644
--- a/src/app/config/config.c
+++ b/src/app/config/config.c
@@ -1163,7 +1163,11 @@ init_protocol_warning_severity_level(void)
 static void
 cleanup_protocol_warning_severity_level(void)
 {
-   atomic_counter_destroy(&protocol_warning_severity_level);
+  /* Destroying a locked mutex is undefined behaviour. This mutex may be
+   * locked, because multiple threads can access it. But we need to destroy
+   * it, otherwise re-initialisation will trigger undefined behaviour.
+   * See #31735 for details. */
+  atomic_counter_destroy(&protocol_warning_severity_level);
 }
 
 /** List of default directory authorities */
diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c
index 25bb1835c2..a838b5b489 100644
--- a/src/feature/relay/router.c
+++ b/src/feature/relay/router.c
@@ -3355,6 +3355,10 @@ router_free_all(void)
   crypto_pk_free(server_identitykey);
   crypto_pk_free(client_identitykey);
 
+  /* Destroying a locked mutex is undefined behaviour. This mutex may be
+   * locked, because multiple threads can access it. But we need to destroy
+   * it, otherwise re-initialisation will trigger undefined behaviour.
+   * See #31735 for details. */
   tor_mutex_free(key_lock);
   routerinfo_free(desc_routerinfo);
   extrainfo_free(desc_extrainfo);
diff --git a/src/lib/crypt_ops/crypto_openssl_mgt.c b/src/lib/crypt_ops/crypto_openssl_mgt.c
index 9ec59e7c81..6d8364ebf8 100644
--- a/src/lib/crypt_ops/crypto_openssl_mgt.c
+++ b/src/lib/crypt_ops/crypto_openssl_mgt.c
@@ -176,6 +176,10 @@ crypto_openssl_free_all(void)
   tor_free(crypto_openssl_version_str);
   tor_free(crypto_openssl_header_version_str);
 
+  /* Destroying a locked mutex is undefined behaviour. This mutex may be
+   * locked, because multiple threads can access it. But we need to destroy
+   * it, otherwise re-initialisation will trigger undefined behaviour.
+   * See #31735 for details. */
 #ifndef NEW_THREAD_API
   if (n_openssl_mutexes_) {
     int n = n_openssl_mutexes_;
diff --git a/src/lib/lock/compat_mutex.c b/src/lib/lock/compat_mutex.c
index 4ad5929715..670bd0174c 100644
--- a/src/lib/lock/compat_mutex.c
+++ b/src/lib/lock/compat_mutex.c
@@ -29,7 +29,15 @@ tor_mutex_new_nonrecursive(void)
   tor_mutex_init_nonrecursive(m);
   return m;
 }
-/** Release all storage and system resources held by <b>m</b>. */
+/** Release all storage and system resources held by <b>m</b>.
+ *
+ * Destroying a locked mutex is undefined behaviour. Global mutexes may be
+ * locked when they are passed to this function, because multiple threads can
+ * still access them. So we can either:
+ *  - destroy on shutdown, and re-initialise when tor re-initialises, or
+ *  - skip destroying and re-initialisation, using a sentinel variable.
+ * See #31735 for details.
+ */
 void
 tor_mutex_free_(tor_mutex_t *m)
 {
diff --git a/src/lib/lock/compat_mutex_pthreads.c b/src/lib/lock/compat_mutex_pthreads.c
index ee5f520cd0..f82ad9f0e8 100644
--- a/src/lib/lock/compat_mutex_pthreads.c
+++ b/src/lib/lock/compat_mutex_pthreads.c
@@ -88,12 +88,26 @@ tor_mutex_release(tor_mutex_t *m)
 }
 /** Clean up the mutex <b>m</b> so that it no longer uses any system
  * resources.  Does not free <b>m</b>.  This function must only be called on
- * mutexes from tor_mutex_init(). */
+ * mutexes from tor_mutex_init().
+ *
+ * Destroying a locked mutex is undefined behaviour. Global mutexes may be
+ * locked when they are passed to this function, because multiple threads can
+ * still access them. So we can either:
+ *  - destroy on shutdown, and re-initialise when tor re-initialises, or
+ *  - skip destroying and re-initialisation, using a sentinel variable.
+ * See #31735 for details.
+ */
 void
 tor_mutex_uninit(tor_mutex_t *m)
 {
   int err;
   raw_assert(m);
+  /* If the mutex is already locked, wait until after it is unlocked to destroy
+   * it. Locking and releasing the mutex makes undefined behaviour less likely,
+   * but does not prevent it. Another thread can lock the mutex between release
+   * and destroy. */
+  tor_mutex_acquire(m);
+  tor_mutex_release(m);
   err = pthread_mutex_destroy(&m->mutex);
   if (PREDICT_UNLIKELY(err)) {
     // LCOV_EXCL_START
diff --git a/src/lib/thread/compat_threads.c b/src/lib/thread/compat_threads.c
index 35cfeba64c..8cf1967e53 100644
--- a/src/lib/thread/compat_threads.c
+++ b/src/lib/thread/compat_threads.c
@@ -67,7 +67,15 @@ atomic_counter_init(atomic_counter_t *counter)
   memset(counter, 0, sizeof(*counter));
   tor_mutex_init_nonrecursive(&counter->mutex);
 }
-/** Clean up all resources held by an atomic counter. */
+/** Clean up all resources held by an atomic counter.
+ *
+ * Destroying a locked mutex is undefined behaviour. Global mutexes may be
+ * locked when they are passed to this function, because multiple threads can
+ * still access them. So we can either:
+ *  - destroy on shutdown, and re-initialise when tor re-initialises, or
+ *  - skip destroying and re-initialisation, using a sentinel variable.
+ * See #31735 for details.
+ */
 void
 atomic_counter_destroy(atomic_counter_t *counter)
 {
diff --git a/src/lib/thread/threads.h b/src/lib/thread/threads.h
index ecf60641b5..de3da6a585 100644
--- a/src/lib/thread/threads.h
+++ b/src/lib/thread/threads.h
@@ -131,7 +131,17 @@ atomic_counter_init(atomic_counter_t *counter)
 {
   atomic_init(&counter->val, 0);
 }
-/** Clean up all resources held by an atomic counter. */
+/** Clean up all resources held by an atomic counter.
+ *
+ * This usage note applies to the compat_threads implementation of
+ * atomic_counter_destroy():
+ * Destroying a locked mutex is undefined behaviour. Global mutexes may be
+ * locked when they are passed to this function, because multiple threads can
+ * still access them. So we can either:
+ *  - destroy on shutdown, and re-initialise when tor re-initialises, or
+ *  - skip destroying and re-initialisation, using a sentinel variable.
+ * See #31735 for details.
+ */
 static inline void
 atomic_counter_destroy(atomic_counter_t *counter)
 {