tsan: fix deadlock/crash in signal handling

[dvyukov]

Summary

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

[melver]

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

179

typo -> synchronously

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

595

synchronously

[dvyukov]

fixed typos in comments

[dvyukov]

rebase

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

public:
   }
   void EnableIgnores() {
     if (UNLIKELY(ignoring_))
       EnableIgnoresImpl();
   }
 
  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();
 };
 
 LibIgnore *libignore();
 
 #if !SANITIZER_GO

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

 struct SignalDesc {
   bool armed;
   __sanitizer_siginfo siginfo;
   ucontext_t ctx;
 };
 
 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 synchnously 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 {
   void (*f)();
   void *arg;
   uptr pc;
 };
 
if (ctx == 0 && !thr->is_dead) {
     MemoryResetRange(thr, (uptr)&SigCtx, (uptr)ctx, sizeof(*ctx));
     thr->signal_ctx = ctx;
   }
   return ctx;
 }
 
 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);
   ignoring_ =
       !thr_->in_ignored_lib && (flags()->ignore_interceptors_accesses ||
                                 libignore()->IsIgnored(pc, &in_ignored_lib_));
   EnableIgnores();
 }
 
 ScopedInterceptor::~ScopedInterceptor() {
   if (!thr_->is_inited) return;
   DisableIgnores();
+  if (UNLIKELY(in_blocking_func_))
+    EnterBlockingFunc(thr_);
   if (!thr_->ignore_interceptors) {
     ProcessPendingSignals(thr_);
     FuncExit(thr_);
     CheckedMutex::CheckNoLocks();
   }
 }
 
 NOINLINE

 
 #define READ_STRING(thr, pc, s, n)                             \
     READ_STRING_OF_LEN((thr), (pc), (s), internal_strlen(s), (n))
 
 #define BLOCK_REAL(name) (BlockingCall(thr), REAL(name))
 
 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
     // this is not true is: pthread_join -> munmap(stack). It's fine
     // to ignore munmap in this case -- we handle stack shadow separately.
     thr->ignore_interceptors++;
   }
 
   ~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) {
   SCOPED_TSAN_INTERCEPTOR(sleep, sec);
   unsigned res = BLOCK_REAL(sleep)(sec);
   AfterSleep(thr, pc);
   return res;
 }
static void SetJmp(ThreadState *thr, uptr sp) {
   // Cleanup old bufs.
   JmpBufGarbageCollect(thr, sp);
   // Remember the buf.
   JmpBuf *buf = thr->jmp_bufs.PushBack();
   buf->sp = 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);
 }
 
 static void LongJmp(ThreadState *thr, uptr *env) {
   uptr sp = ExtractLongJmpSp(env);
   // Find the saved buf with matching sp.
   for (uptr i = 0; i < thr->jmp_bufs.Size(); i++) {
     JmpBuf *buf = &thr->jmp_bufs[i];
     if (buf->sp == sp) {
       CHECK_GE(thr->shadow_stack_pos, buf->shadow_stack_pos);
       // Unwind the stack.
       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,
       atomic_store(&thr->in_signal_handler, buf->in_signal_handler,
           memory_order_relaxed);
       JmpBufGarbageCollect(thr, buf->sp - 1);  // do not collect buf->sp
       return;
     }
   }
   Printf("ThreadSanitizer: can't find longjmp buf\n");
   CHECK(0);

 
 template <class Fn>
 void CondMutexUnlockCtx<Fn>::Unlock() const {
   // pthread_cond_wait interceptor has enabled async signal delivery
   // (see BlockingCall below). Disable async signals since we are running
   // 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--;
   si->~ScopedInterceptor();
 }
 }  // namespace
 
 INTERCEPTOR(int, pthread_cond_init, void *c, void *a) {
if (sig < 0 || sig >= kSigCount) {
     return;
   }
   // Don't mess with synchronous signals.
   const bool sync = is_sync_signal(sctx, sig);
   if (sync ||
       // 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
       // can be in inconsistent state.
       // SIGSYS looks relatively safe -- it's synchronous and can actually
       // need some global state.
       bool acq = (sig == SIGSYS);
       CallUserSignalHandler(thr, sync, acq, sig, info, ctx);

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

#if !SANITIZER_GO
   IgnoreSet mop_ignore_set;
   IgnoreSet sync_ignore_set;
 #endif
   uptr *shadow_stack;
   uptr *shadow_stack_end;
 #if !SANITIZER_GO
   Vector<JmpBuf> jmp_bufs;
   int in_symbolizer;
+  atomic_uintptr_t in_blocking_func;
   bool in_ignored_lib;
   bool is_inited;
 #endif
   MutexSet mset;
   bool is_dead;
   const Tid tid;
   uptr stk_addr;
   uptr stk_size;
enum FiberSwitchFlags {
   FiberSwitchFlagNoSync = 1 << 0, // __tsan_switch_to_fiber_no_sync
 };
 
 class SlotLocker {
  public:
   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 syncronously,
+    // 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_);
   }
 
   ALWAYS_INLINE
   ~SlotLocker() {
     if (!locked_)
       SlotUnlock(thr_);

compiler-rt/test/tsan/signal_thread2.cpp

+// 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: