From 35acc32b3e6535213d3ec651c9b7870cf74fe0ec Mon Sep 17 00:00:00 2001 From: Peter Ammon Date: Tue, 10 Jan 2023 19:21:41 +0100 Subject: [PATCH] tsan: fix a race when assigning ThreadSignalContext The SigCtx function lazily allocates a ThreadSignalContext, and stores it in the ThreadState. This function may be called by various interceptors and the signal handler itself. If SigCtx itself is interrupted by a signal, then (prior to this fix) there was a possibility of allocating two ThreadSignalContexts. This not only leaks, it fails to deliver the signal to the program's signal handler, as the recorded signal is overwritten by the new ThreadSignalContext. Fix this by using a CAS to swap in the ThreadSignalContext, preventing the race. Add a test for this case. Reviewed By: dvyukov, melver Differential Revision: https://reviews.llvm.org/D140582 --- .../lib/tsan/rtl/tsan_interceptors_posix.cpp | 23 ++++-- compiler-rt/lib/tsan/rtl/tsan_rtl.h | 2 +- .../test/tsan/signal_thread_sigctx_race.cpp | 84 ++++++++++++++++++++++ 3 files changed, 101 insertions(+), 8 deletions(-) create mode 100644 compiler-rt/test/tsan/signal_thread_sigctx_race.cpp diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp index cb28e74..becaba4 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp @@ -249,13 +249,21 @@ SANITIZER_WEAK_CXX_DEFAULT_IMPL void OnPotentiallyBlockingRegionEnd() {} } // namespace __tsan static ThreadSignalContext *SigCtx(ThreadState *thr) { - ThreadSignalContext *ctx = (ThreadSignalContext*)thr->signal_ctx; + // This function may be called reentrantly if it is interrupted by a signal + // handler. Use CAS to handle the race. + uptr ctx = atomic_load(&thr->signal_ctx, memory_order_relaxed); if (ctx == 0 && !thr->is_dead) { - ctx = (ThreadSignalContext*)MmapOrDie(sizeof(*ctx), "ThreadSignalContext"); - MemoryResetRange(thr, (uptr)&SigCtx, (uptr)ctx, sizeof(*ctx)); - thr->signal_ctx = ctx; + uptr pctx = + (uptr)MmapOrDie(sizeof(ThreadSignalContext), "ThreadSignalContext"); + MemoryResetRange(thr, (uptr)&SigCtx, pctx, sizeof(ThreadSignalContext)); + if (atomic_compare_exchange_strong(&thr->signal_ctx, &ctx, pctx, + memory_order_relaxed)) { + ctx = pctx; + } else { + UnmapOrDie((ThreadSignalContext *)pctx, sizeof(ThreadSignalContext)); + } } - return ctx; + return (ThreadSignalContext *)ctx; } ScopedInterceptor::ScopedInterceptor(ThreadState *thr, const char *fname, @@ -970,9 +978,10 @@ void DestroyThreadState() { } void PlatformCleanUpThreadState(ThreadState *thr) { - ThreadSignalContext *sctx = thr->signal_ctx; + ThreadSignalContext *sctx = (ThreadSignalContext *)atomic_load( + &thr->signal_ctx, memory_order_relaxed); if (sctx) { - thr->signal_ctx = 0; + atomic_store(&thr->signal_ctx, 0, memory_order_relaxed); UnmapOrDie(sctx, sizeof(*sctx)); } } diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.h b/compiler-rt/lib/tsan/rtl/tsan_rtl.h index 298b4b3..7f5de6b 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl.h +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.h @@ -220,7 +220,7 @@ struct ThreadState { #endif atomic_uintptr_t in_signal_handler; - ThreadSignalContext *signal_ctx; + atomic_uintptr_t signal_ctx; #if !SANITIZER_GO StackID last_sleep_stack_id; diff --git a/compiler-rt/test/tsan/signal_thread_sigctx_race.cpp b/compiler-rt/test/tsan/signal_thread_sigctx_race.cpp new file mode 100644 index 0000000..a3a206f --- /dev/null +++ b/compiler-rt/test/tsan/signal_thread_sigctx_race.cpp @@ -0,0 +1,84 @@ +// RUN: %clangxx_tsan %s -o %t && %run %t 2>&1 | FileCheck %s +// UNSUPPORTED: darwin + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +// This attempts to exercise a race condition where both a thread and its signal +// handler allocate the SigCtx. If the race is allowed, it leads to a leak and +// the first signal being dropped. +// Spawn threads in a loop and send it SIGUSR1 concurrently with the thread +// doing a bogus kill() call. The signal handler writes to a self-pipe which the +// thread detects and then exits. A dropped signal results in a timeout. +int pipes[2]; +static void handler(int sig) { write(pipes[1], "x", 1); } + +static int do_select() { + struct timeval tvs { + 0, 1000 + }; + fd_set fds; + FD_ZERO(&fds); + FD_SET(pipes[0], &fds); + return select(pipes[0] + 1, &fds, 0, 0, &tvs); +} + +static void *thr(void *p) { + // This kill() is expected to fail; it exists only to trigger a call to SigCtx + // outside of the signal handler. + kill(INT_MIN, 0); + int success = 0; + for (int i = 0; i < 1024; i++) { + if (do_select() > 0) { + success = 1; + break; + } + } + if (success) { + char c; + read(pipes[0], &c, 1); + } else { + fprintf(stderr, "Failed to receive signal\n"); + exit(1); + } + return p; +} + +int main() { + if (pipe(pipes)) { + perror("pipe"); + exit(1); + } + + struct sigaction act = {}; + act.sa_handler = &handler; + if (sigaction(SIGUSR1, &act, 0)) { + perror("sigaction"); + exit(1); + } + + for (int i = 0; i < (1 << 10); i++) { + pthread_t th{}; + if (pthread_create(&th, 0, thr, 0)) { + perror("pthread_create"); + exit(1); + } + pthread_kill(th, SIGUSR1); + pthread_join(th, 0); + } + + fprintf(stderr, "DONE\n"); + return 0; +} + +// CHECK-NOT: WARNING: ThreadSanitizer: +// CHECK: DONE +// CHECK-NOT: WARNING: ThreadSanitizer: -- 2.7.4