[compiler-rt] [tsan] Fix nested signal handling (PR #138599)
via llvm-commits
llvm-commits at lists.llvm.org
Mon May 5 15:11:02 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-compiler-rt-sanitizer
Author: Jakob Widauer (jwidauer)
<details>
<summary>Changes</summary>
This PR fixes the bug reported in #<!-- -->134358.
In the current implementation of the tsan posix interceptors, the signal set does not get restored to the correct original set, if a signal handler gets called, while already inside of a signal handler. This leads to the wrong signal set being set for the thread in which the signal handler was called.
To fix this I introduced a stack of `__sanitizer_sigset_t` to keep all the correct old signal sets and restore them in the correct order.
There was also already an existing test that tested nested / recursive signal handlers, but it was disabled.
I therefore reenabled it, made it more robust by waiting for the second thread to have been properly started and added checks for the signal sets.
This test then failed before the introduction of the interceptor fix and didn't fail with the fix.
@<!-- -->dvyukov What are your thoughts?
---
Full diff: https://github.com/llvm/llvm-project/pull/138599.diff
2 Files Affected:
- (modified) compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp (+13-9)
- (modified) compiler-rt/test/tsan/signal_recursive.cpp (+62-19)
``````````diff
diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
index 6d79b80593379..7b91c54e91007 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
@@ -12,6 +12,9 @@
// sanitizer_common/sanitizer_common_interceptors.inc
//===----------------------------------------------------------------------===//
+#include <stdarg.h>
+
+#include "interception/interception.h"
#include "sanitizer_common/sanitizer_allocator_dlsym.h"
#include "sanitizer_common/sanitizer_atomic.h"
#include "sanitizer_common/sanitizer_errno.h"
@@ -24,16 +27,14 @@
#include "sanitizer_common/sanitizer_posix.h"
#include "sanitizer_common/sanitizer_stacktrace.h"
#include "sanitizer_common/sanitizer_tls_get_addr.h"
-#include "interception/interception.h"
+#include "sanitizer_common/sanitizer_vector.h"
+#include "tsan_fd.h"
#include "tsan_interceptors.h"
#include "tsan_interface.h"
+#include "tsan_mman.h"
#include "tsan_platform.h"
-#include "tsan_suppressions.h"
#include "tsan_rtl.h"
-#include "tsan_mman.h"
-#include "tsan_fd.h"
-
-#include <stdarg.h>
+#include "tsan_suppressions.h"
using namespace __tsan;
@@ -177,7 +178,7 @@ struct ThreadSignalContext {
SignalDesc pending_signals[kSigCount];
// emptyset and oldset are too big for stack.
__sanitizer_sigset_t emptyset;
- __sanitizer_sigset_t oldset;
+ __sanitizer::Vector<__sanitizer_sigset_t> oldset;
};
void EnterBlockingFunc(ThreadState *thr) {
@@ -980,6 +981,7 @@ void PlatformCleanUpThreadState(ThreadState *thr) {
&thr->signal_ctx, memory_order_relaxed);
if (sctx) {
atomic_store(&thr->signal_ctx, 0, memory_order_relaxed);
+ sctx->oldset.Reset();
UnmapOrDie(sctx, sizeof(*sctx));
}
}
@@ -2176,7 +2178,8 @@ void ProcessPendingSignalsImpl(ThreadState *thr) {
return;
atomic_fetch_add(&thr->in_signal_handler, 1, memory_order_relaxed);
internal_sigfillset(&sctx->emptyset);
- int res = REAL(pthread_sigmask)(SIG_SETMASK, &sctx->emptyset, &sctx->oldset);
+ __sanitizer_sigset_t *oldset = sctx->oldset.PushBack();
+ int res = REAL(pthread_sigmask)(SIG_SETMASK, &sctx->emptyset, oldset);
CHECK_EQ(res, 0);
for (int sig = 0; sig < kSigCount; sig++) {
SignalDesc *signal = &sctx->pending_signals[sig];
@@ -2186,8 +2189,9 @@ void ProcessPendingSignalsImpl(ThreadState *thr) {
&signal->ctx);
}
}
- res = REAL(pthread_sigmask)(SIG_SETMASK, &sctx->oldset, 0);
+ res = REAL(pthread_sigmask)(SIG_SETMASK, oldset, 0);
CHECK_EQ(res, 0);
+ sctx->oldset.PopBack();
atomic_fetch_add(&thr->in_signal_handler, -1, memory_order_relaxed);
}
diff --git a/compiler-rt/test/tsan/signal_recursive.cpp b/compiler-rt/test/tsan/signal_recursive.cpp
index 40be2d01502b6..fca8757d2a952 100644
--- a/compiler-rt/test/tsan/signal_recursive.cpp
+++ b/compiler-rt/test/tsan/signal_recursive.cpp
@@ -3,8 +3,6 @@
// Test case for recursive signal handlers, adopted from:
// https://github.com/google/sanitizers/issues/478
-// REQUIRES: disabled
-
#include "test.h"
#include <semaphore.h>
#include <signal.h>
@@ -15,8 +13,6 @@ static const int kSigRestart = SIGUSR2;
static sem_t g_thread_suspend_ack_sem;
-static bool g_busy_thread_received_restart;
-
static volatile bool g_busy_thread_garbage_collected;
static void SaveRegistersInStack() {
@@ -30,22 +26,51 @@ static void fail(const char *what) {
exit(1);
}
+static void CheckSigBlocked(const sigset_t &oldset, const sigset_t &newset,
+ int sig) {
+ const int is_old_member = sigismember(&oldset, sig);
+ const int is_new_member = sigismember(&newset, sig);
+
+ if (is_old_member == -1 || is_new_member == -1)
+ fail("sigismember failed");
+
+ if (is_old_member != is_new_member)
+ fail("restoring signals failed");
+}
+
+sigset_t GetCurrentSigSet() {
+ sigset_t set;
+ if (sigemptyset(&set) != 0)
+ fail("sigemptyset failed");
+
+ if (pthread_sigmask(SIG_BLOCK, NULL, &set) != 0)
+ fail("pthread_sigmask failed");
+
+ return set;
+}
+
static void SuspendHandler(int sig) {
int old_errno = errno;
SaveRegistersInStack();
// Enable kSigRestart handling, tsan disables signals around signal handlers.
- sigset_t sigset;
- sigemptyset(&sigset);
- pthread_sigmask(SIG_SETMASK, &sigset, 0);
+ const auto oldset = GetCurrentSigSet();
// Acknowledge that thread is saved and suspended
if (sem_post(&g_thread_suspend_ack_sem) != 0)
fail("sem_post failed");
// Wait for wakeup signal.
- while (!g_busy_thread_received_restart)
- usleep(100); // wait for kSigRestart signal
+ sigset_t sigset;
+ sigemptyset(&sigset);
+ if (sigsuspend(&sigset) != 0 && errno != EINTR)
+ fail("sigsuspend failed");
+
+ const auto newset = GetCurrentSigSet();
+
+ // Check that the same signals are blocked as before
+ CheckSigBlocked(oldset, newset, kSigSuspend);
+ CheckSigBlocked(oldset, newset, kSigRestart);
// Acknowledge that thread restarted
if (sem_post(&g_thread_suspend_ack_sem) != 0)
@@ -56,31 +81,33 @@ static void SuspendHandler(int sig) {
errno = old_errno;
}
-static void RestartHandler(int sig) {
- g_busy_thread_received_restart = true;
+static void RestartHandler(int sig) {}
+
+static void WaitSem() {
+ while (sem_wait(&g_thread_suspend_ack_sem) != 0) {
+ if (errno != EINTR)
+ fail("sem_wait failed");
+ }
}
static void StopWorld(pthread_t thread) {
if (pthread_kill(thread, kSigSuspend) != 0)
fail("pthread_kill failed");
- while (sem_wait(&g_thread_suspend_ack_sem) != 0) {
- if (errno != EINTR)
- fail("sem_wait failed");
- }
+ WaitSem();
}
static void StartWorld(pthread_t thread) {
if (pthread_kill(thread, kSigRestart) != 0)
fail("pthread_kill failed");
- while (sem_wait(&g_thread_suspend_ack_sem) != 0) {
- if (errno != EINTR)
- fail("sem_wait failed");
- }
+ WaitSem();
}
static void CollectGarbage(pthread_t thread) {
+ // Wait for the thread to start
+ WaitSem();
+
StopWorld(thread);
// Walk stacks
StartWorld(thread);
@@ -102,21 +129,37 @@ static void Init() {
void* BusyThread(void *arg) {
(void)arg;
+ const auto oldset = GetCurrentSigSet();
+
+ if (sem_post(&g_thread_suspend_ack_sem) != 0)
+ fail("sem_post failed");
+
while (!g_busy_thread_garbage_collected) {
usleep(100); // Tsan deadlocks without these sleeps
}
+
+ const auto newset = GetCurrentSigSet();
+
+ // Check that we have the same signals blocked as before
+ CheckSigBlocked(oldset, newset, kSigSuspend);
+ CheckSigBlocked(oldset, newset, kSigRestart);
+
return NULL;
}
int main(int argc, const char *argv[]) {
Init();
+
pthread_t busy_thread;
if (pthread_create(&busy_thread, NULL, &BusyThread, NULL) != 0)
fail("pthread_create failed");
+
CollectGarbage(busy_thread);
if (pthread_join(busy_thread, 0) != 0)
fail("pthread_join failed");
+
fprintf(stderr, "DONE\n");
+
return 0;
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/138599
More information about the llvm-commits
mailing list