tsan: fix deadlock/crash in signal handling

We set in_blocking_func around some blocking C functions so that we don't
delay signal infinitely (if in_blocking_func is set we deliver signals
synchronously).

However, pthread_join is blocking but also call munmap/free to free thread
resources. If we are inside the munmap/free interceptors called from
pthread_join and deliver a signal synchronously, it can lead to deadlocks
and crashes since we re-enter runtime and try to lock the same mutexes
or use the same per-thread data structures.

If we re-enter runtime via an interceptor when in_blocking_func is set,
temporary reset in_blocking_func around the interceptor and restore it back
when we return from the recursive interceptor.

Also move in_blocking_func from ThreadSignalContext to ThreadContext
so that we can CHECK that it's not set in SlotLocker ctor.

Fixes https://github.com/google/sanitizers/issues/1540

Reviewed By: melver

Differential Revision: https://reviews.llvm.org/D127845

compiler-rt/lib/tsan/rtl/tsan_interceptors.h

@@ -21,8 +21,9 @@ class ScopedInterceptor {
 
  private:
   ThreadState *const thr_;
-  bool in_ignored_lib_;
-  bool ignoring_;
+  bool in_ignored_lib_ = false;
+  bool in_blocking_func_ = false;
+  bool ignoring_ = false;
 
   void DisableIgnoresImpl();
   void EnableIgnoresImpl();

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

@@ -165,13 +165,26 @@ struct SignalDesc {
 
 struct ThreadSignalContext {
   int int_signal_send;
-  atomic_uintptr_t in_blocking_func;
   SignalDesc pending_signals[kSigCount];
   // emptyset and oldset are too big for stack.
   __sanitizer_sigset_t emptyset;
   __sanitizer_sigset_t oldset;
 };
 
+void EnterBlockingFunc(ThreadState *thr) {
+  for (;;) {
+    // The order is important to not delay a signal infinitely if it's
+    // delivered right before we set in_blocking_func. Note: we can't call
+    // ProcessPendingSignals when in_blocking_func is set, or we can handle
+    // a signal synchronously when we are already handling a signal.
+    atomic_store(&thr->in_blocking_func, 1, memory_order_relaxed);
+    if (atomic_load(&thr->pending_signals, memory_order_relaxed) == 0)
+      break;
+    atomic_store(&thr->in_blocking_func, 0, memory_order_relaxed);
+    ProcessPendingSignals(thr);
+  }
+}
+
 // The sole reason tsan wraps atexit callbacks is to establish synchronization
 // between callback setup and callback execution.
 struct AtExitCtx {
@@ -245,8 +258,18 @@ static ThreadSignalContext *SigCtx(ThreadState *thr) {
 
 ScopedInterceptor::ScopedInterceptor(ThreadState *thr, const char *fname,
                                      uptr pc)
-    : thr_(thr), in_ignored_lib_(false), ignoring_(false) {
+    : thr_(thr) {
   LazyInitialize(thr);
+  if (UNLIKELY(atomic_load(&thr->in_blocking_func, memory_order_relaxed))) {
+    // pthread_join is marked as blocking, but it's also known to call other
+    // intercepted functions (mmap, free). If we don't reset in_blocking_func
+    // we can get deadlocks and memory corruptions if we deliver a synchronous
+    // signal inside of an mmap/free interceptor.
+    // So reset it and restore it back in the destructor.
+    // See https://github.com/google/sanitizers/issues/1540
+    atomic_store(&thr->in_blocking_func, 0, memory_order_relaxed);
+    in_blocking_func_ = true;
+  }
   if (!thr_->is_inited) return;
   if (!thr_->ignore_interceptors) FuncEntry(thr, pc);
   DPrintf("#%d: intercept %s()\n", thr_->tid, fname);
@@ -259,6 +282,8 @@ ScopedInterceptor::ScopedInterceptor(ThreadState *thr, const char *fname,
 ScopedInterceptor::~ScopedInterceptor() {
   if (!thr_->is_inited) return;
   DisableIgnores();
+  if (UNLIKELY(in_blocking_func_))
+    EnterBlockingFunc(thr_);
   if (!thr_->ignore_interceptors) {
     ProcessPendingSignals(thr_);
     FuncExit(thr_);
@@ -321,15 +346,8 @@ void ScopedInterceptor::DisableIgnoresImpl() {
 
 struct BlockingCall {
   explicit BlockingCall(ThreadState *thr)
-      : thr(thr)
-      , ctx(SigCtx(thr)) {
-    for (;;) {
-      atomic_store(&ctx->in_blocking_func, 1, memory_order_relaxed);
-      if (atomic_load(&thr->pending_signals, memory_order_relaxed) == 0)
-        break;
-      atomic_store(&ctx->in_blocking_func, 0, memory_order_relaxed);
-      ProcessPendingSignals(thr);
-    }
+      : thr(thr) {
+    EnterBlockingFunc(thr);
     // When we are in a "blocking call", we process signals asynchronously
     // (right when they arrive). In this context we do not expect to be
     // executing any user/runtime code. The known interceptor sequence when
@@ -340,11 +358,10 @@ struct BlockingCall {
 
   ~BlockingCall() {
     thr->ignore_interceptors--;
-    atomic_store(&ctx->in_blocking_func, 0, memory_order_relaxed);
+    atomic_store(&thr->in_blocking_func, 0, memory_order_relaxed);
   }
 
   ThreadState *thr;
-  ThreadSignalContext *ctx;
 };
 
 TSAN_INTERCEPTOR(unsigned, sleep, unsigned sec) {
@@ -517,9 +534,7 @@ static void SetJmp(ThreadState *thr, uptr sp) {
   buf->shadow_stack_pos = thr->shadow_stack_pos;
   ThreadSignalContext *sctx = SigCtx(thr);
   buf->int_signal_send = sctx ? sctx->int_signal_send : 0;
-  buf->in_blocking_func = sctx ?
-      atomic_load(&sctx->in_blocking_func, memory_order_relaxed) :
-      false;
+  buf->in_blocking_func = atomic_load(&thr->in_blocking_func, memory_order_relaxed);
   buf->in_signal_handler = atomic_load(&thr->in_signal_handler,
       memory_order_relaxed);
 }
@@ -535,11 +550,10 @@ static void LongJmp(ThreadState *thr, uptr *env) {
       while (thr->shadow_stack_pos > buf->shadow_stack_pos)
         FuncExit(thr);
       ThreadSignalContext *sctx = SigCtx(thr);
-      if (sctx) {
+      if (sctx)
         sctx->int_signal_send = buf->int_signal_send;
-        atomic_store(&sctx->in_blocking_func, buf->in_blocking_func,
-            memory_order_relaxed);
-      }
+      atomic_store(&thr->in_blocking_func, buf->in_blocking_func,
+          memory_order_relaxed);
       atomic_store(&thr->in_signal_handler, buf->in_signal_handler,
           memory_order_relaxed);
       JmpBufGarbageCollect(thr, buf->sp - 1);  // do not collect buf->sp
@@ -1198,9 +1212,8 @@ void CondMutexUnlockCtx<Fn>::Unlock() const {
   // tsan code. Also ScopedInterceptor and BlockingCall destructors won't run
   // since the thread is cancelled, so we have to manually execute them
   // (the thread still can run some user code due to pthread_cleanup_push).
-  ThreadSignalContext *ctx = SigCtx(thr);
-  CHECK_EQ(atomic_load(&ctx->in_blocking_func, memory_order_relaxed), 1);
-  atomic_store(&ctx->in_blocking_func, 0, memory_order_relaxed);
+  CHECK_EQ(atomic_load(&thr->in_blocking_func, memory_order_relaxed), 1);
+  atomic_store(&thr->in_blocking_func, 0, memory_order_relaxed);
   MutexPostLock(thr, pc, (uptr)m, MutexFlagDoPreLockOnPostLock);
   // Undo BlockingCall ctor effects.
   thr->ignore_interceptors--;
@@ -2089,12 +2102,12 @@ void sighandler(int sig, __sanitizer_siginfo *info, void *ctx) {
       // If we are in blocking function, we can safely process it now
       // (but check if we are in a recursive interceptor,
       // i.e. pthread_join()->munmap()).
-      (sctx && atomic_load(&sctx->in_blocking_func, memory_order_relaxed))) {
+      atomic_load(&thr->in_blocking_func, memory_order_relaxed)) {
     atomic_fetch_add(&thr->in_signal_handler, 1, memory_order_relaxed);
-    if (sctx && atomic_load(&sctx->in_blocking_func, memory_order_relaxed)) {
-      atomic_store(&sctx->in_blocking_func, 0, memory_order_relaxed);
+    if (atomic_load(&thr->in_blocking_func, memory_order_relaxed)) {
+      atomic_store(&thr->in_blocking_func, 0, memory_order_relaxed);
       CallUserSignalHandler(thr, sync, true, sig, info, ctx);
-      atomic_store(&sctx->in_blocking_func, 1, memory_order_relaxed);
+      atomic_store(&thr->in_blocking_func, 1, memory_order_relaxed);
     } else {
       // Be very conservative with when we do acquire in this case.
       // It's unsafe to do acquire in async handlers, because ThreadState

compiler-rt/lib/tsan/rtl/tsan_rtl.h

@@ -191,6 +191,7 @@ struct ThreadState {
 #if !SANITIZER_GO
   Vector<JmpBuf> jmp_bufs;
   int in_symbolizer;
+  atomic_uintptr_t in_blocking_func;
   bool in_ignored_lib;
   bool is_inited;
 #endif
@@ -627,6 +628,13 @@ class SlotLocker {
   ALWAYS_INLINE
   SlotLocker(ThreadState *thr, bool recursive = false)
       : thr_(thr), locked_(recursive ? thr->slot_locked : false) {
+#if !SANITIZER_GO
+    // We are in trouble if we are here with in_blocking_func set.
+    // If in_blocking_func is set, all signals will be delivered synchronously,
+    // which means we can't lock slots since the signal handler will try
+    // to lock it recursively and deadlock.
+    DCHECK(!atomic_load(&thr->in_blocking_func, memory_order_relaxed));
+#endif
     if (!locked_)
       SlotLock(thr_);
   }

compiler-rt/test/tsan/signal_thread2.cpp

@@ -0,0 +1,66 @@
+// RUN: %clangxx_tsan %s -o %t && %run %t 2>&1 | FileCheck %s
+// UNSUPPORTED: darwin
+
+// Test case for https://github.com/google/sanitizers/issues/1540
+
+#include <errno.h>
+#include <pthread.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/time.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+volatile int X;
+
+static void handler(int sig) {
+  (void)sig;
+  if (X != 0)
+    printf("bad");
+}
+
+static void *thr1(void *p) {
+  sleep(1);
+  return 0;
+}
+
+static void *thr(void *p) {
+  pthread_t th[10];
+  for (int i = 0; i < sizeof(th) / sizeof(th[0]); i++)
+    pthread_create(&th[i], 0, thr1, 0);
+  for (int i = 0; i < sizeof(th) / sizeof(th[0]); i++)
+    pthread_join(th[i], 0);
+  return 0;
+}
+
+int main() {
+  struct sigaction act = {};
+  act.sa_handler = &handler;
+  if (sigaction(SIGPROF, &act, 0)) {
+    perror("sigaction");
+    exit(1);
+  }
+
+  itimerval t;
+  t.it_value.tv_sec = 0;
+  t.it_value.tv_usec = 10;
+  t.it_interval = t.it_value;
+  if (setitimer(ITIMER_PROF, &t, 0)) {
+    perror("setitimer");
+    exit(1);
+  }
+
+  pthread_t th[100];
+  for (int i = 0; i < sizeof(th) / sizeof(th[0]); i++)
+    pthread_create(&th[i], 0, thr, 0);
+  for (int i = 0; i < sizeof(th) / sizeof(th[0]); i++)
+    pthread_join(th[i], 0);
+
+  fprintf(stderr, "DONE\n");
+  return 0;
+}
+
+// CHECK-NOT: WARNING: ThreadSanitizer:
+// CHECK: DONE
+// CHECK-NOT: WARNING: ThreadSanitizer: