[tor-commits] [tor/master] lock: Avoid some undefined behaviour when freeing mutexes.
asn at torproject.org
asn at torproject.org
Mon Sep 30 10:57:32 UTC 2019
commit d1eab05834566f998721d3a16107767885711c57
Author: teor <teor at torproject.org>
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)
{
More information about the tor-commits
mailing list