tsan: fix signal handling during stop-the-world

[dvyukov]

Summary

Long story short: stop-the-world briefly resets SIGSEGV handler to SIG_DFL. This breaks programs that handle and continue after SIGSEGV (namely JVM). See the test and comments for details.

[earthdok]

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

253

Why did you remove this?

268

We shouldn't ever run TracerThreadDieCallback() from other threads. It wouldn't work anyway.

I think TracerThreadDieCallback() should check that internal_getpid() == stoptheworld_tracer_pid. If not, just call the old callback.

[earthdok]

The general approach LGTM

[dvyukov]

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

253

I see no reason for it to be here. We are going to die anyway, so why bother blocking signals?
If there is a reason for it to be here, then we need to comment this line.

268

Agree.
But this is a separate issue, let's fix it separately.

[dvyukov]

Committed revision 229678.

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

do {
     thread_lister.Reset();
   } while (added_threads);
   return true;
 }
 
 // Pointer to the ThreadSuspender instance for use in signal handler.
 static ThreadSuspender *thread_suspender_instance = NULL;
 
-// Signals that should not be blocked (this is used in the parent thread as well
-// as the tracer thread).
-static const int kUnblockedSignals[] = { SIGABRT, SIGILL, SIGFPE, SIGSEGV,
-                                         SIGBUS, SIGXCPU, SIGXFSZ };
+// Synchronous signals that should not be blocked.
+static const int kSyncSignals[] = { SIGABRT, SIGILL, SIGFPE, SIGSEGV, SIGBUS,
+                                    SIGXCPU, SIGXFSZ };
 
 // Structure for passing arguments into the tracer thread.
 struct TracerThreadArgument {
   StopTheWorldCallback callback;
   void *callback_argument;
   // The tracer thread waits on this mutex while the parent finishes its
   // preparations.
   BlockingMutex mutex;
   uptr parent_pid;
 };
 
 static DieCallbackType old_die_callback;
 
 // Signal handler to wake up suspended threads when the tracer thread dies.
-void TracerThreadSignalHandler(int signum, void *siginfo, void *) {
+static void TracerThreadSignalHandler(int signum, void *siginfo, void *) {
   if (thread_suspender_instance != NULL) {
     if (signum == SIGABRT)
       thread_suspender_instance->KillAllThreads();
     else
       thread_suspender_instance->ResumeAllThreads();
   }
   internal__exit((signum == SIGABRT) ? 1 : 2);
 }
static int TracerThread(void* argument) {
   // Check if parent is already dead.
   if (internal_getppid() != tracer_thread_argument->parent_pid)
     internal__exit(4);
 
   // Wait for the parent thread to finish preparations.
   tracer_thread_argument->mutex.Lock();
   tracer_thread_argument->mutex.Unlock();
 
+  old_die_callback = GetDieCallback();
   SetDieCallback(TracerThreadDieCallback);
 
   ThreadSuspender thread_suspender(internal_getppid());
   // Global pointer for the signal handler.
   thread_suspender_instance = &thread_suspender;
 
   // Alternate stack for signal handling.
   InternalScopedBuffer<char> handler_stack_memory(kHandlerStackSize);
   struct sigaltstack handler_stack;
   internal_memset(&handler_stack, 0, sizeof(handler_stack));
   handler_stack.ss_sp = handler_stack_memory.data();
   handler_stack.ss_size = kHandlerStackSize;
   internal_sigaltstack(&handler_stack, NULL);
 
-  // Install our handler for fatal signals. Other signals should be blocked by
-  // the mask we inherited from the caller thread.
-  for (uptr signal_index = 0; signal_index < ARRAY_SIZE(kUnblockedSignals);
-       signal_index++) {
-    __sanitizer_sigaction new_sigaction;
-    internal_memset(&new_sigaction, 0, sizeof(new_sigaction));
-    new_sigaction.sigaction = TracerThreadSignalHandler;
-    new_sigaction.sa_flags = SA_ONSTACK | SA_SIGINFO;
-    internal_sigfillset(&new_sigaction.sa_mask);
-    internal_sigaction_norestorer(kUnblockedSignals[signal_index],
-                                  &new_sigaction, NULL);
+  // Install our handler for synchronous signals. Other signals should be
+  // blocked by the mask we inherited from the parent thread.
+  for (uptr i = 0; i < ARRAY_SIZE(kSyncSignals); i++) {
+    __sanitizer_sigaction act;
+    internal_memset(&act, 0, sizeof(act));
+    act.sigaction = TracerThreadSignalHandler;
+    act.sa_flags = SA_ONSTACK | SA_SIGINFO;
+    internal_sigaction_norestorer(kSyncSignals[i], &act, 0);
   }
 
   int exit_code = 0;
   if (!thread_suspender.SuspendAllThreads()) {
     VReport(1, "Failed suspending threads.\n");
     exit_code = 3;
   } else {
     tracer_thread_argument->callback(thread_suspender.suspended_threads_list(),
                                      tracer_thread_argument->callback_argument);
     thread_suspender.ResumeAllThreads();
     exit_code = 0;
   }
+  // Note, this is a bad race. If TracerThreadDieCallback is already started
+  // in another thread and observed that thread_suspender_instance != 0,
+  // it can call KillAllThreads on the destroyed variable.
+  SetDieCallback(old_die_callback);
   thread_suspender_instance = NULL;
-  handler_stack.ss_flags = SS_DISABLE;
-  internal_sigaltstack(&handler_stack, NULL);
   return exit_code;
 }
 
 class ScopedStackSpaceWithGuard {
  public:
   explicit ScopedStackSpaceWithGuard(uptr stack_size) {
     stack_size_ = stack_size;
     guard_size_ = GetPageSizeCached();
private:
   uptr guard_size_;
   uptr guard_start_;
 };
 
 // We have a limitation on the stack frame size, so some stuff had to be moved
 // into globals.
 static __sanitizer_sigset_t blocked_sigset;
 static __sanitizer_sigset_t old_sigset;
-static __sanitizer_sigaction old_sigactions
-    [ARRAY_SIZE(kUnblockedSignals)];
 
 class StopTheWorldScope {
  public:
   StopTheWorldScope() {
-    // Block all signals that can be blocked safely, and install
-    // default handlers for the remaining signals.
-    // We cannot allow user-defined handlers to run while the ThreadSuspender
-    // thread is active, because they could conceivably call some libc functions
-    // which modify errno (which is shared between the two threads).
-    internal_sigfillset(&blocked_sigset);
-    for (uptr signal_index = 0; signal_index < ARRAY_SIZE(kUnblockedSignals);
-         signal_index++) {
-      // Remove the signal from the set of blocked signals.
-      internal_sigdelset(&blocked_sigset, kUnblockedSignals[signal_index]);
-      // Install the default handler.
-      __sanitizer_sigaction new_sigaction;
-      internal_memset(&new_sigaction, 0, sizeof(new_sigaction));
-      new_sigaction.handler = SIG_DFL;
-      internal_sigfillset(&new_sigaction.sa_mask);
-      internal_sigaction_norestorer(kUnblockedSignals[signal_index],
-          &new_sigaction, &old_sigactions[signal_index]);
-    }
-    int sigprocmask_status =
-        internal_sigprocmask(SIG_BLOCK, &blocked_sigset, &old_sigset);
-    CHECK_EQ(sigprocmask_status, 0); // sigprocmask should never fail
     // Make this process dumpable. Processes that are not dumpable cannot be
     // attached to.
     process_was_dumpable_ = internal_prctl(PR_GET_DUMPABLE, 0, 0, 0, 0);
     if (!process_was_dumpable_)
       internal_prctl(PR_SET_DUMPABLE, 1, 0, 0, 0);
-    old_die_callback = GetDieCallback();
   }
 
   ~StopTheWorldScope() {
-    SetDieCallback(old_die_callback);
     // Restore the dumpable flag.
     if (!process_was_dumpable_)
       internal_prctl(PR_SET_DUMPABLE, 0, 0, 0, 0);
-    // Restore the signal handlers.
-    for (uptr signal_index = 0; signal_index < ARRAY_SIZE(kUnblockedSignals);
-         signal_index++) {
-      internal_sigaction_norestorer(kUnblockedSignals[signal_index],
-                                    &old_sigactions[signal_index], NULL);
-    }
-    internal_sigprocmask(SIG_SETMASK, &old_sigset, &old_sigset);
   }
 
  private:
   int process_was_dumpable_;
 };
 
 // When sanitizer output is being redirected to file (i.e. by using log_path),
 // the tracer should write to the parent's log instead of trying to open a new
void StopTheWorld(StopTheWorldCallback callback, void *argument) {
   tracer_thread_argument.callback = callback;
   tracer_thread_argument.callback_argument = argument;
   tracer_thread_argument.parent_pid = internal_getpid();
   const uptr kTracerStackSize = 2 * 1024 * 1024;
   ScopedStackSpaceWithGuard tracer_stack(kTracerStackSize);
   // Block the execution of TracerThread until after we have set ptrace
   // permissions.
   tracer_thread_argument.mutex.Lock();
+  // Signal handling story.
+  // We don't want async signals to be delivered to the tracer thread,
+  // so we block all async signals before creating the thread. An async signal
+  // handler can temporary modify errno, which is shared with this thread.
+  // We ought to use pthread_sigmask here, because sigprocmask has undefined
+  // behavior in multithreaded programs. However, on linux sigprocmask is
+  // equivalent to pthread_sigmask with the exception that pthread_sigmask
+  // does not allow to block some signals used internally in pthread
+  // implementation. We are fine with blocking them here, we are really not
+  // going to pthread_cancel the thread.
+  // The tracer thread should not raise any synchronous signals. But in case it
+  // does, we setup a special handler for sync signals that properly kills the
+  // parent as well. Note: we don't pass CLONE_SIGHAND to clone, so handlers
+  // in the tracer thread won't interfere with user program. Double note: if a
+  // user does something along the lines of 'kill -11 pid', that can kill the
+  // process even if user setup own handler for SEGV.
+  // Thing to watch out for: this code should not change behavior of user code
+  // in any observable way. In particular it should not override user signal
+  // handlers.
+  internal_sigfillset(&blocked_sigset);
+  for (uptr i = 0; i < ARRAY_SIZE(kSyncSignals); i++)
+    internal_sigdelset(&blocked_sigset, kSyncSignals[i]);
+  int rv = internal_sigprocmask(SIG_BLOCK, &blocked_sigset, &old_sigset);
+  CHECK_EQ(rv, 0);
   uptr tracer_pid = internal_clone(
       TracerThread, tracer_stack.Bottom(),
       CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_UNTRACED,
       &tracer_thread_argument, 0 /* parent_tidptr */, 0 /* newtls */, 0
       /* child_tidptr */);
+  internal_sigprocmask(SIG_SETMASK, &old_sigset, 0);
   int local_errno = 0;
   if (internal_iserror(tracer_pid, &local_errno)) {
     VReport(1, "Failed spawning a tracer thread (errno %d).\n", local_errno);
     tracer_thread_argument.mutex.Unlock();
   } else {
     ScopedSetTracerPID scoped_set_tracer_pid(tracer_pid);
     // On some systems we have to explicitly declare that we want to be traced
     // by the tracer thread.

test/tsan/signal_segv_handler.cc

+// RUN: %clang_tsan -O1 %s -o %t && TSAN_OPTIONS="flush_memory_ms=1 memory_limit_mb=1" %run %t 2>&1 | FileCheck %s
+
+// JVM uses SEGV to preempt threads. All threads do a load from a known address
+// periodically. When runtime needs to preempt threads, it unmaps the page.
+// Threads start triggering SEGV one by one. The signal handler blocks
+// threads while runtime does its thing. Then runtime maps the page again
+// and resumes the threads.
+// Previously this pattern conflicted with stop-the-world machinery,
+// because it briefly reset SEGV handler to SIG_DFL.
+// As the consequence JVM just silently died.
+
+// This test sets memory flushing rate to maximum, then does series of
+// "benign" SEGVs that are handled by signal handler, and ensures that
+// the process survive.
+
+#include "test.h"
+#include <signal.h>
+#include <sys/mman.h>
+
+void *guard;
+
+void handler(int signo, siginfo_t *info, void *uctx) {
+  mprotect(guard, 4096, PROT_READ | PROT_WRITE);
+}
+
+int main() {
+  // Disabled for now, because there is another issue in ptrace.
+  // It will be addressed in a subsequent change.
+  goto done;
+  struct sigaction a;
+  a.sa_sigaction = handler;
+  a.sa_flags = SA_SIGINFO;
+  sigaction(SIGSEGV, &a, 0);
+  guard = mmap(0, 4096, PROT_NONE, MAP_ANON | MAP_PRIVATE, -1, 0);
+  for (int i = 0; i < 1000000; i++) {
+    mprotect(guard, 4096, PROT_NONE);
+    *(int*)guard = 1;
+  }
+done:
+  fprintf(stderr, "DONE\n");
+}
+
+// CHECK: DONE