diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors.h b/compiler-rt/lib/tsan/rtl/tsan_interceptors.h --- a/compiler-rt/lib/tsan/rtl/tsan_interceptors.h +++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors.h @@ -21,8 +21,9 @@ 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(); diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp --- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp @@ -165,13 +165,26 @@ 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 { @@ -245,8 +258,18 @@ 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() { 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 @@ 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 @@ ~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 @@ 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 @@ 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 @@ // 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 @@ // 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 diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.h b/compiler-rt/lib/tsan/rtl/tsan_rtl.h --- a/compiler-rt/lib/tsan/rtl/tsan_rtl.h +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.h @@ -191,6 +191,7 @@ #if !SANITIZER_GO Vector jmp_bufs; int in_symbolizer; + atomic_uintptr_t in_blocking_func; bool in_ignored_lib; bool is_inited; #endif @@ -589,6 +590,13 @@ 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_); } diff --git a/compiler-rt/test/tsan/signal_thread2.cpp b/compiler-rt/test/tsan/signal_thread2.cpp new file mode 100644 --- /dev/null +++ b/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 +#include +#include +#include +#include +#include +#include +#include + +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: