[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?

Author: jwidauer

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) {
@@ -558,6 +559,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->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 @@ 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;
+        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 @@ 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 +2182,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 +2193,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);
 }
 

compiler-rt/lib/tsan/rtl/tsan_rtl.h

@@ -98,6 +98,7 @@ struct JmpBuf {
   uptr sp;
   int int_signal_send;
   bool in_blocking_func;
+  uptr oldset_stack_size;
   uptr in_signal_handler;
   uptr *shadow_stack_pos;
 };

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;
 }