September 2019 - tor-commits - lists.torproject.org

[tor/master] lock: Avoid some undefined behaviour when freeing mutexes.
by asn＠torproject.org 30 Sep '19

30 Sep '19

commit d1eab05834566f998721d3a16107767885711c57 Author: teor <teor(a)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 m. */ +/** Release all storage and system resources held by m. + * + * 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 m so that it no longer uses any system * resources. Does not free m. 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) {

1 0

[tor/master] Merge branch 'tor-github/pr/1346'
by asn＠torproject.org 30 Sep '19

30 Sep '19

commit 9318682109c5b8742bc868f3d30cb5cd39095f98 Merge: ae8d36db3 d1eab0583 Author: George Kadianakis <desnacked(a)riseup.net> Date: Mon Sep 30 13:56:51 2019 +0300 Merge branch 'tor-github/pr/1346' 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 --cc src/app/config/config.c index 6ee818ab0,2416da2b2..451593a5f --- a/src/app/config/config.c +++ b/src/app/config/config.c @@@ -1197,9 -1187,28 +1197,13 @@@ init_protocol_warning_severity_level(vo 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 */ - -static const char *default_authorities[] = { -#include "auth_dirs.inc" - NULL -}; - -/** List of fallback directory authorities. The list is generated by opt-in of - * relays that meet certain stability criteria. - */ -static const char *default_fallbacks[] = { -#include "fallback_dirs.inc" - NULL -}; - /** Add the default directory authorities directly into the trusted dir list, * but only add them insofar as they share bits with type. * Each authority's bits are restricted to the bits shared with type.

1 0

[gettor/master] Update check service script
by hiro＠torproject.org 30 Sep '19

30 Sep '19

commit 38a16893aa2d1089b460ee90860e7710160e3b7d Author: hiro <hiro(a)torproject.org> Date: Mon Sep 30 12:55:46 2019 +0200 Update check service script --- bin/gettor_check | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/gettor_check b/bin/gettor_check index 4c9cca6..75d3148 100755 --- a/bin/gettor_check +++ b/bin/gettor_check @@ -9,7 +9,7 @@ # # :license: This is Free Software. See LICENSE for license information. -cd ; +cd /home/gettor/gettor/; virtualenv /home/gettor/gettor/venv source /home/gettor/gettor/venv/bin/activate

1 0

[tor/master] backtrace: Disable signal handlers in remove_bt_handler()
by asn＠torproject.org 30 Sep '19

30 Sep '19

commit 6b97a5a843341b5ea2a9039bdd5290d524a44cd4 Author: teor <teor(a)torproject.org> Date: Wed Sep 4 14:50:09 2019 +1000 backtrace: Disable signal handlers in remove_bt_handler() Fixes bug 31614; bugfix on 0.2.5.2-alpha. --- src/lib/err/backtrace.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/lib/err/backtrace.c b/src/lib/err/backtrace.c index 1d1b3bcfa..77dc6b8cb 100644 --- a/src/lib/err/backtrace.c +++ b/src/lib/err/backtrace.c @@ -190,13 +190,15 @@ dump_stack_symbols_to_error_fds(void) backtrace_symbols_fd(cb_buf, (int)depth, fds[i]); } +/* The signals that we want our backtrace handler to trap */ +static int trap_signals[] = { SIGSEGV, SIGILL, SIGFPE, SIGBUS, SIGSYS, + SIGIO, -1 }; + /** Install signal handlers as needed so that when we crash, we produce a * useful stack trace. Return 0 on success, -errno on failure. */ static int install_bt_handler(const char *software) { - int trap_signals[] = { SIGSEGV, SIGILL, SIGFPE, SIGBUS, SIGSYS, - SIGIO, -1 }; int i, rv=0; strncpy(bt_version, software, sizeof(bt_version) - 1); @@ -235,6 +237,19 @@ install_bt_handler(const char *software) static void remove_bt_handler(void) { + int i; + + struct sigaction sa; + + memset(&sa, 0, sizeof(sa)); + sa.sa_handler = SIG_DFL; + sigfillset(&sa.sa_mask); + + for (i = 0; trap_signals[i] >= 0; ++i) { + /* remove_bt_handler() is called on shutdown, from low-level code. + * It's not a fatal error, so we just ignore it. */ + (void)sigaction(trap_signals[i], &sa, NULL); + } } #endif /* defined(USE_BACKTRACE) */

1 0

[tor/master] backtrace: Add a missing check for HAVE_PTHREAD_H before using mutexes
by asn＠torproject.org 30 Sep '19

30 Sep '19

commit ab7bfdf40402449d6a2214e1dd5bc3fe5bc9a2e7 Author: teor <teor(a)torproject.org> Date: Mon Sep 9 11:21:15 2019 +1000 backtrace: Add a missing check for HAVE_PTHREAD_H before using mutexes Fixes bug 31614; bugfix on 0.2.5.2-alpha. --- src/lib/err/backtrace.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lib/err/backtrace.c b/src/lib/err/backtrace.c index 77dc6b8cb..bf833c162 100644 --- a/src/lib/err/backtrace.c +++ b/src/lib/err/backtrace.c @@ -57,7 +57,8 @@ #include "lib/err/torerr.h" #if defined(HAVE_EXECINFO_H) && defined(HAVE_BACKTRACE) && \ - defined(HAVE_BACKTRACE_SYMBOLS_FD) && defined(HAVE_SIGACTION) + defined(HAVE_BACKTRACE_SYMBOLS_FD) && defined(HAVE_SIGACTION) && \ + defined(HAVE_PTHREAD_H) #define USE_BACKTRACE #endif

1 0

[tor/master] sandbox: Allow backtrace signals to be disabled
by asn＠torproject.org 30 Sep '19

30 Sep '19

commit 2f8a9a2db692a101b7e6241cbcdf9ed87841310b Author: teor <teor(a)torproject.org> Date: Tue Sep 24 13:51:38 2019 +1000 sandbox: Allow backtrace signals to be disabled Part of 31614. --- src/lib/sandbox/sandbox.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/lib/sandbox/sandbox.c b/src/lib/sandbox/sandbox.c index e2356a172..5c02c012e 100644 --- a/src/lib/sandbox/sandbox.c +++ b/src/lib/sandbox/sandbox.c @@ -295,6 +295,7 @@ sb_rt_sigaction(scmp_filter_ctx ctx, sandbox_cfg_t *filter) unsigned i; int rc; int param[] = { SIGINT, SIGTERM, SIGPIPE, SIGUSR1, SIGUSR2, SIGHUP, SIGCHLD, + SIGSEGV, SIGILL, SIGFPE, SIGBUS, SIGSYS, SIGIO, #ifdef SIGXFSZ SIGXFSZ #endif

1 0

[tor/master] backtrace: avoid undefined behaviour on re-initialisation
by asn＠torproject.org 30 Sep '19

30 Sep '19

commit 315f14c709d019c55731faedb19a97b771f9c42f Author: teor <teor(a)torproject.org> Date: Fri Sep 20 11:40:05 2019 +1000 backtrace: avoid undefined behaviour on re-initialisation cb_buf_mutex is statically initialised, so we can not destroy it when we are shutting down the err subsystem. If we destroy it, and then re-initialise tor, all our backtraces will fail. Part of 31736, but committed in this branch to avoid merge conflicts. --- src/lib/err/backtrace.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/lib/err/backtrace.c b/src/lib/err/backtrace.c index bf833c162..2a956e611 100644 --- a/src/lib/err/backtrace.c +++ b/src/lib/err/backtrace.c @@ -251,6 +251,10 @@ remove_bt_handler(void) * It's not a fatal error, so we just ignore it. */ (void)sigaction(trap_signals[i], &sa, NULL); } + + /* cb_buf_mutex is statically initialised, so we can not destroy it. + * If we destroy it, and then re-initialise tor, all our backtraces will + * fail. */ } #endif /* defined(USE_BACKTRACE) */

1 0

[tor/master] changes: file for 31614
by asn＠torproject.org 30 Sep '19

30 Sep '19

commit c9c046c365f10f5cbffada921931413edc690dbe Author: teor <teor(a)torproject.org> Date: Thu Sep 26 12:09:56 2019 +1000 changes: file for 31614 --- changes/bug31614 | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/changes/bug31614 b/changes/bug31614 new file mode 100644 index 000000000..c425a9fcd --- /dev/null +++ b/changes/bug31614 @@ -0,0 +1,9 @@ + o Minor bugfixes (logging): + - Disable backtrace signal handlers when shutting down tor. + Fixes bug 31614; bugfix on 0.2.5.2-alpha. + - Add a missing check for HAVE_PTHREAD_H, because the backtrace code uses + mutexes. Fixes bug 31614; bugfix on 0.2.5.2-alpha. + o Documentation: + - Explain why we can't destroy the backtrace buffer mutex. Explain why + we don't need to destroy the log mutex. + Closes ticket 31736.

1 0

[tor/master] Merge branch 'tor-github/pr/1302'
by asn＠torproject.org 30 Sep '19

30 Sep '19

commit ae8d36db313a548d9828384f2131f481640c6173 Merge: fc760c508 749c2e176 Author: George Kadianakis <desnacked(a)riseup.net> Date: Mon Sep 30 13:47:53 2019 +0300 Merge branch 'tor-github/pr/1302' changes/bug31614 | 9 +++++++++ src/lib/err/backtrace.c | 26 +++++++++++++++++++++++--- src/lib/log/log.c | 5 ++++- src/lib/sandbox/sandbox.c | 1 + 4 files changed, 37 insertions(+), 4 deletions(-) diff --cc src/lib/err/backtrace.c index c2011285c,2a956e611..8bc7e6965 --- a/src/lib/err/backtrace.c +++ b/src/lib/err/backtrace.c @@@ -193,12 -198,13 +198,10 @@@ static int trap_signals[] = { SIGSEGV, /** Install signal handlers as needed so that when we crash, we produce a * useful stack trace. Return 0 on success, -errno on failure. */ static int -install_bt_handler(const char *software) +install_bt_handler(void) { - int trap_signals[] = { SIGSEGV, SIGILL, SIGFPE, SIGBUS, SIGSYS, - SIGIO, -1 }; int i, rv=0; - strncpy(bt_version, software, sizeof(bt_version) - 1); - bt_version[sizeof(bt_version) - 1] = 0; - struct sigaction sa; memset(&sa, 0, sizeof(sa)); diff --cc src/lib/log/log.c index 4463bff61,eacd413a5..d2002f6ea --- a/src/lib/log/log.c +++ b/src/lib/log/log.c @@@ -832,33 -804,12 +832,36 @@@ logs_free_all(void } /* We _could_ destroy the log mutex here, but that would screw up any logs - * that happened between here and the end of execution. */ + * that happened between here and the end of execution. + * If tor is re-initialized, log_mutex_initialized will still be 1. So we + * won't trigger any undefined behaviour by trying to re-initialize the + * log mutex. */ } +/** Close signal-safe log files. + * Closing the log files makes the process and OS flush log buffers. + * + * This function is safe to call from a signal handler. It should only be + * called when shutting down the log or err modules. It is currenly called + * by the err module, when terminating the process on an abnormal condition. + */ +void +logs_close_sigsafe(void) +{ + logfile_t *victim, *next; + /* We can't LOCK_LOGS() in a signal handler, because it may call + * signal-unsafe functions. And we can't deallocate memory, either. */ + next = logfiles; + logfiles = NULL; + while (next) { + victim = next; + next = next->next; + if (victim->needs_close) { + close_log_sigsafe(victim); + } + } +} + /** Remove and free the log entry victim from the linked-list * logfiles (it is probably present, but it might not be due to thread * racing issues). After this function is called, the caller shouldn't

1 0

[tor/master] log: explain why it is safe to leave the log mutex initialized
by asn＠torproject.org 30 Sep '19

30 Sep '19

commit 749c2e1761c753992fb2549e7ee912e568f563d6 Author: teor <teor(a)torproject.org> Date: Thu Sep 26 12:18:23 2019 +1000 log: explain why it is safe to leave the log mutex initialized The log mutex is dynamically initialized, guarded by log_mutex_initialized. We don't want to destroy it, because after it is destroyed, we won't see any more logs. If tor is re-initialized, log_mutex_initialized will still be 1. So we won't trigger any undefined behaviour by trying to re-initialize the log mutex. Part of 31736, but committed in this branch to avoid merge conflicts. --- src/lib/log/log.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/lib/log/log.c b/src/lib/log/log.c index a9ad38fb2..eacd413a5 100644 --- a/src/lib/log/log.c +++ b/src/lib/log/log.c @@ -804,7 +804,10 @@ logs_free_all(void) } /* We _could_ destroy the log mutex here, but that would screw up any logs - * that happened between here and the end of execution. */ + * that happened between here and the end of execution. + * If tor is re-initialized, log_mutex_initialized will still be 1. So we + * won't trigger any undefined behaviour by trying to re-initialize the + * log mutex. */ } /** Remove and free the log entry victim from the linked-list

1 0