Merge branch 'tor-github/pr/1346'

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.

src/app/config/config.c

@@ -1197,7 +1197,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);
 }
 
 /** Add the default directory authorities directly into the trusted dir list,

src/feature/relay/router.c

@@ -3463,6 +3463,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);

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_;

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)
 {

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

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)
 {

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)
 {