[compiler-rt] 3056ccd - tsan: fix deadlock/crash in signal handling
Dmitry Vyukov via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 30 05:23:55 PDT 2022
Author: Dmitry Vyukov
Date: 2022-09-30T14:23:46+02:00
New Revision: 3056ccdbae7c36e4eaa2ac89bc82d9c94bddd77a
URL: https://github.com/llvm/llvm-project/commit/3056ccdbae7c36e4eaa2ac89bc82d9c94bddd77a
DIFF: https://github.com/llvm/llvm-project/commit/3056ccdbae7c36e4eaa2ac89bc82d9c94bddd77a.diff
LOG: 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
Added:
compiler-rt/test/tsan/signal_thread2.cpp
Modified:
compiler-rt/lib/tsan/rtl/tsan_interceptors.h
compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
compiler-rt/lib/tsan/rtl/tsan_rtl.h
Removed:
################################################################################
diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors.h b/compiler-rt/lib/tsan/rtl/tsan_interceptors.h
index 3091ad809c40f..60fbc58f988a2 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_interceptors.h
+++ b/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();
diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
index f2eaaf443e4ae..c557d5ddc6abc 100644
--- 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 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
diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.h b/compiler-rt/lib/tsan/rtl/tsan_rtl.h
index e1e121e2ee073..90164c893ee16 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_rtl.h
+++ b/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_);
}
diff --git a/compiler-rt/test/tsan/signal_thread2.cpp b/compiler-rt/test/tsan/signal_thread2.cpp
new file mode 100644
index 0000000000000..a773058b2cf95
--- /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 <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:
More information about the llvm-commits
mailing list