[tor-commits] [tor/master] err: Always lock the backtrace buffer before it is used

[nickm at torproject.org]

commit c23986246b970bd01d887fa151a5312a6dc7db04
Author: teor <teor at torproject.org>
Date:   Mon Sep 30 23:12:54 2019 +1000

    err: Always lock the backtrace buffer before it is used
    
    Fixes bug 31734; bugfix on 0.2.5.3-alpha.
---
 changes/bug31734        |  3 +++
 src/lib/err/backtrace.c | 47 ++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/changes/bug31734 b/changes/bug31734
new file mode 100644
index 000000000..ce989ea5d
--- /dev/null
+++ b/changes/bug31734
@@ -0,0 +1,3 @@
+  o Minor bugfixes (error handling):
+    - Always lock the backtrace buffer before it is used.
+      Fixes bug 31734; bugfix on 0.2.5.3-alpha.
diff --git a/src/lib/err/backtrace.c b/src/lib/err/backtrace.c
index 8bc7e6965..4e881b979 100644
--- a/src/lib/err/backtrace.c
+++ b/src/lib/err/backtrace.c
@@ -52,6 +52,8 @@
 #include <pthread.h>
 #endif
 
+#include "lib/cc/ctassert.h"
+
 #define EXPOSE_CLEAN_BACKTRACE
 #include "lib/err/backtrace.h"
 #include "lib/err/torerr.h"
@@ -73,15 +75,40 @@
 static char bt_version[128] = "";
 
 #ifdef USE_BACKTRACE
+
 /** Largest stack depth to try to dump. */
 #define MAX_DEPTH 256
-/** Static allocation of stack to dump. This is static so we avoid stack
- * pressure. */
-static void *cb_buf[MAX_DEPTH];
+/** The size of the callback buffer, so we can clear it in unlock_cb_buf(). */
+#define SIZEOF_CB_BUF (MAX_DEPTH * sizeof(void *))
 /** Protects cb_buf from concurrent access. Pthreads, since this code
  * is Unix-only, and since this code needs to be lowest-level. */
 static pthread_mutex_t cb_buf_mutex = PTHREAD_MUTEX_INITIALIZER;
 
+/** Lock and return a static stack pointer buffer that can hold up to
+ *  MAX_DEPTH function pointers. */
+static void *
+lock_cb_buf(void)
+{
+  /* Lock the mutex first, before even declaring the buffer. */
+  pthread_mutex_lock(&cb_buf_mutex);
+
+  /** Static allocation of stack to dump. This is static so we avoid stack
+   * pressure. */
+  static void *cb_buf[MAX_DEPTH];
+  CTASSERT(SIZEOF_CB_BUF == sizeof(cb_buf));
+  memset(cb_buf, 0, SIZEOF_CB_BUF);
+
+  return cb_buf;
+}
+
+/** Unlock the static stack pointer buffer. */
+static void
+unlock_cb_buf(void *cb_buf)
+{
+  memset(cb_buf, 0, SIZEOF_CB_BUF);
+  pthread_mutex_unlock(&cb_buf_mutex);
+}
+
 /** Change a stacktrace in <b>stack</b> of depth <b>depth</b> so that it will
  * log the correct function from which a signal was received with context
  * <b>ctx</b>.  (When we get a signal, the current function will not have
@@ -123,7 +150,7 @@ log_backtrace_impl(int severity, log_domain_mask_t domain, const char *msg,
   char **symbols;
   size_t i;
 
-  pthread_mutex_lock(&cb_buf_mutex);
+  void *cb_buf = lock_cb_buf();
 
   depth = backtrace(cb_buf, MAX_DEPTH);
   symbols = backtrace_symbols(cb_buf, (int)depth);
@@ -141,7 +168,7 @@ log_backtrace_impl(int severity, log_domain_mask_t domain, const char *msg,
   raw_free(symbols);
 
  done:
-  pthread_mutex_unlock(&cb_buf_mutex);
+  unlock_cb_buf(cb_buf);
 }
 
 static void crash_handler(int sig, siginfo_t *si, void *ctx_)
@@ -157,6 +184,8 @@ crash_handler(int sig, siginfo_t *si, void *ctx_)
   int n_fds, i;
   const int *fds = NULL;
 
+  void *cb_buf = lock_cb_buf();
+
   (void) si;
 
   depth = backtrace(cb_buf, MAX_DEPTH);
@@ -173,6 +202,8 @@ crash_handler(int sig, siginfo_t *si, void *ctx_)
   for (i=0; i < n_fds; ++i)
     backtrace_symbols_fd(cb_buf, (int)depth, fds[i]);
 
+  unlock_cb_buf(cb_buf);
+
   tor_raw_abort_();
 }
 
@@ -184,11 +215,15 @@ dump_stack_symbols_to_error_fds(void)
   const int *fds = NULL;
   size_t depth;
 
+  void *cb_buf = lock_cb_buf();
+
   depth = backtrace(cb_buf, MAX_DEPTH);
 
   n_fds = tor_log_get_sigsafe_err_fds(&fds);
   for (i=0; i < n_fds; ++i)
     backtrace_symbols_fd(cb_buf, (int)depth, fds[i]);
+
+  unlock_cb_buf(cb_buf);
 }
 
 /* The signals that we want our backtrace handler to trap */
@@ -222,10 +257,12 @@ install_bt_handler(void)
      * libc has pre-loaded the symbols we need to dump things, so that later
      * reads won't be denied by the sandbox code */
     char **symbols;
+    void *cb_buf = lock_cb_buf();
     size_t depth = backtrace(cb_buf, MAX_DEPTH);
     symbols = backtrace_symbols(cb_buf, (int) depth);
     if (symbols)
       raw_free(symbols);
+    unlock_cb_buf(cb_buf);
   }
 
   return rv;