[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[tor-commits] [tor/maint-0.4.1] lock: Avoid some undefined behaviour when freeing mutexes.



commit d1eab05834566f998721d3a16107767885711c57
Author: teor <teor@xxxxxxxxxxxxxx>
Date:   Fri Sep 20 11:27:05 2019 +1000

    lock: Avoid some undefined behaviour when freeing mutexes.
    
    Fixes bug 31736; bugfix on 0.0.7.
---
 changes/bug31736                       |  3 +++
 src/app/config/config.c                |  6 +++++-
 src/feature/relay/router.c             |  4 ++++
 src/lib/crypt_ops/crypto_openssl_mgt.c |  4 ++++
 src/lib/lock/compat_mutex.c            | 10 +++++++++-
 src/lib/lock/compat_mutex_pthreads.c   | 16 +++++++++++++++-
 src/lib/thread/compat_threads.c        | 10 +++++++++-
 src/lib/thread/threads.h               | 12 +++++++++++-
 8 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/changes/bug31736 b/changes/bug31736
new file mode 100644
index 000000000..beb09e506
--- /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 0b1b758d9..2416da2b2 100644
--- a/src/app/config/config.c
+++ b/src/app/config/config.c
@@ -1187,7 +1187,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 dad2c6a50..af47e79d2 100644
--- a/src/feature/relay/router.c
+++ b/src/feature/relay/router.c
@@ -3072,6 +3072,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 c97815f9a..a245ef381 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 4ad592971..670bd0174 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 ee5f520cd..f82ad9f0e 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 94ab021c5..16cece612 100644
--- a/src/lib/thread/compat_threads.c
+++ b/src/lib/thread/compat_threads.c
@@ -65,7 +65,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 ecf60641b..de3da6a58 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)
 {



_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits