Commit d4dde249 authored by Nick Mathewson's avatar Nick Mathewson 🎨
Browse files

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

parents 6965798a d1eab058
o Minor bugfixes (multithreading):
- Avoid some undefined behaviour when freeing mutexes.
Fixes bug 31736; bugfix on 0.0.7.
......@@ -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 */
......
......@@ -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);
......
......@@ -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_;
......
......@@ -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)
{
......
......@@ -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
......
......@@ -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)
{
......
......@@ -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)
{
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment