[tsan] Fix nested signal handling (#138599)
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?
GitOrigin-RevId: 1aa746d300d72042aaa48e7982f76a823aed8cb3
diff --git a/lib/tsan/rtl/tsan_interceptors_posix.cpp b/lib/tsan/rtl/tsan_interceptors_posix.cpp
index 6d79b80..7c4d23a 100644
--- a/lib/tsan/rtl/tsan_interceptors_posix.cpp
+++ b/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_platform.h"
-#include "tsan_suppressions.h"
-#include "tsan_rtl.h"
#include "tsan_mman.h"
-#include "tsan_fd.h"
-
-#include <stdarg.h>
+#include "tsan_platform.h"
+#include "tsan_rtl.h"
+#include "tsan_suppressions.h"
using namespace __tsan;
@@ -177,7 +178,7 @@
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) {
@@ -558,6 +559,7 @@
buf->shadow_stack_pos = thr->shadow_stack_pos;
ThreadSignalContext *sctx = SigCtx(thr);
buf->int_signal_send = sctx ? sctx->int_signal_send : 0;
+ buf->oldset_stack_size = sctx ? sctx->oldset.Size() : 0;
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);
@@ -574,8 +576,11 @@
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;
+ while (sctx->oldset.Size() > buf->oldset_stack_size)
+ sctx->oldset.PopBack();
+ }
atomic_store(&thr->in_blocking_func, buf->in_blocking_func,
memory_order_relaxed);
atomic_store(&thr->in_signal_handler, buf->in_signal_handler,
@@ -980,6 +985,7 @@
&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 +2182,8 @@
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 +2193,9 @@
&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/lib/tsan/rtl/tsan_rtl.h b/lib/tsan/rtl/tsan_rtl.h
index 49bee9c..4dc5e63 100644
--- a/lib/tsan/rtl/tsan_rtl.h
+++ b/lib/tsan/rtl/tsan_rtl.h
@@ -98,6 +98,7 @@
uptr sp;
int int_signal_send;
bool in_blocking_func;
+ uptr oldset_stack_size;
uptr in_signal_handler;
uptr *shadow_stack_pos;
};
diff --git a/test/tsan/signal_recursive.cpp b/test/tsan/signal_recursive.cpp
index 40be2d0..fca8757 100644
--- a/test/tsan/signal_recursive.cpp
+++ b/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 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 @@
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 @@
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 @@
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;
}
