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.

This is reincarnation of reverted r229678 (http://reviews.llvm.org/D7722).
Changed:

• execute TracerThreadDieCallback only on tracer thread
• reset global data in TracerThreadSignalHandler/TracerThreadDieCallback
• handle EINTR from waitpid

Add 3 new test:

• SIGSEGV during leak checking
• StopTheWorld operation during signal storm from an external process
• StopTheWorld operation when the program generates and handles SIGSEGVs

[earthdok]

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

416

Wrong. You're assuming that internal_waitpid() will not touch errno. This is true only on x86_64 Linux where we have our custom wrappers implemented. Other platforms (MIPS, ARM) use the libc implementation (see sanitizer_syscall_generic.inc).

test/asan/TestCases/Linux/leak_check_segv.cc

2

-pthread is unnecessary with asan
if you want to use it at least be consistent (-pthread/-lpthread)

test/asan/TestCases/Linux/signal_during_stop_the_world.cc

5

see above

37

Why these two signals in particular?

43

This barely makes a difference, the time spent attaching to threads will probably be >> this.

If you want to slow down leak detection you could make a few large allocs and fill the memory with pointers to them.

54

There's zero guarantee that it will crash...

[dvyukov]

PTAL

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

416

I am assuming nothing. This is existing code. I just properly handle EINTR.
Is there a simple way to fix other platforms?

test/asan/TestCases/Linux/leak_check_segv.cc

2

Done

test/asan/TestCases/Linux/signal_during_stop_the_world.cc

5

Done

37

no strong reason
it is reasonable if SIGCHLD unblocks waitpid
but on my system it seems that SIGCHLD does not, but SIGPROF does
added comment

43

Removed this part

54

Episodic crashes are fine with me.
Still much better than the current absence of interesting tests.

[earthdok]

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

416

As discussed offline: existing code only works correctly (on other platforms) under the assumption that signals are blocked => waitpid does not return until tracer is dead.

test/asan/TestCases/Linux/signal_during_stop_the_world.cc

54

As discussed offline, it should actually crash as soon as the ScopedStackSpaceWithGuard object is destroyed. I retract my statement. May be worth a comment.

[dvyukov]

PTAL

[dvyukov]

Fixed the errno issue by spin-waiting on completion of the tracer thread.

[earthdok]

We need a comment in sanitizer_stoptheworld.h documenting the fact that the callback must exit by returning and not by calling exit() or _exit().

For the record, this is what currently happens when the tracer dies for any reason:

If  the  tracer  dies,  all  tracees  are  automatically  detached  and
restarted,  unless  they  were in group-stop.  Handling of restart from
group-stop is currently buggy, but the  "as  planned"  behavior  is  to
leave  tracee  stopped  and  waiting  for  SIGCONT.   If  the tracee is
restarted from signal-delivery-stop, the pending signal is injected.

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

208

what is the purpose of these changes?

229–231

same question

414

internal_waitpid() may call syscall() which can access/spoil...

[dvyukov]

We need a comment in sanitizer_stoptheworld.h documenting the fact that the callback must exit by returning and not by calling exit() or _exit().

done

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

208

I want to set inst->arg->done=1 as the last action before internal__exit. But I also need to set thread_suspender_instance to NULL. This means that I need a copy of thread_suspender_instance in a local var.

229–231

Just to reduce length of overly lengthy identifiers.

414

done

[earthdok]

Still kinda wary about this approach, but let's give it a try. LGTM

lib/sanitizer_common/sanitizer_stoptheworld.h

63 ↗

(On Diff #21273)

nit: could you please be consistent in "function" vs "function()" in comments

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

414

same

[dvyukov]

fixed in rev 231457

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 *uctx) {
+static void TracerThreadSignalHandler(int signum, void *siginfo, void *uctx) {
   SignalContext ctx = SignalContext::Create(siginfo, uctx);
   VPrintf(1, "Tracer caught signal %d: addr=0x%zx pc=0x%zx sp=0x%zx\n",
       signum, ctx.addr, ctx.pc, ctx.sp);
   if (thread_suspender_instance != NULL) {
     if (signum == SIGABRT)
       thread_suspender_instance->KillAllThreads();
     else
       thread_suspender_instance->ResumeAllThreads();
+    thread_suspender_instance = NULL;
+    SetDieCallback(old_die_callback);
+    old_die_callback = NULL;
   }
   internal__exit((signum == SIGABRT) ? 1 : 2);
 }
 
 static void TracerThreadDieCallback() {
   // Generally a call to Die() in the tracer thread should be fatal to the
   // parent process as well, because they share the address space.
   // This really only works correctly if all the threads are suspended at this
   // point. So we correctly handle calls to Die() from within the callback, but
   // not those that happen before or after the callback. Hopefully there aren't
   // a lot of opportunities for that to happen...
-  if (thread_suspender_instance)
+  if (thread_suspender_instance != NULL &&
+      stoptheworld_tracer_pid == internal_getpid()) {
     thread_suspender_instance->KillAllThreads();
+    thread_suspender_instance = NULL;
+  }
   if (old_die_callback)
     old_die_callback();
+  SetDieCallback(old_die_callback);
+  old_die_callback = NULL;
 }
 
 // Size of alternative stack for signal handlers in the tracer thread.
 static const int kHandlerStackSize = 4096;
 
 // This function will be run as a cloned task.
 static int TracerThread(void* argument) {
   TracerThreadArgument *tracer_thread_argument =
       (TracerThreadArgument *)argument;
 
   internal_prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0);
   // 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;
   }
+  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.
 #ifdef PR_SET_PTRACER
     internal_prctl(PR_SET_PTRACER, tracer_pid, 0, 0, 0);
 #endif
     // Allow the tracer thread to start.
     tracer_thread_argument.mutex.Unlock();
-    // Since errno is shared between this thread and the tracer thread, we
-    // must avoid using errno while the tracer thread is running.
-    // At this point, any signal will either be blocked or kill us, so waitpid
-    // should never return (and set errno) while the tracer thread is alive.
+    // NOTE: errno is shared between this thread and the tracer thread.
+    for (;;) {
       uptr waitpid_status = internal_waitpid(tracer_pid, NULL, __WALL);
-    if (internal_iserror(waitpid_status, &local_errno))
+      if (!internal_iserror(waitpid_status, &local_errno))
+        break;
+      if (local_errno == EINTR)
+        continue;
       VReport(1, "Waiting on the tracer thread failed (errno %d).\n",
               local_errno);
+      break;
+    }
   }
 }
 
 // Platform-specific methods from SuspendedThreadsList.
 #if SANITIZER_ANDROID && defined(__arm__)
 typedef pt_regs regs_struct;
 #define REG_SP ARM_sp
 

test/asan/TestCases/Linux/leak_check_segv.cc

+// Test that SIGSEGV during leak checking does not crash the process.
+// RUN: %clangxx_asan -O1 %s -o %t -pthread && LSAN_OPTIONS="verbosity=1" not %run %t 2>&1
+// REQUIRES: asan-64-bits
+#include <stdlib.h>
+#include <stdio.h>
+#include <sys/mman.h>
+#include <sanitizer/lsan_interface.h>
+
+char data[10 * 1024 * 1024];
+
+int main() {
+  void *p = malloc(10 * 1024 * 1024);
+  // surprise-surprise!
+  mprotect((void*)(((unsigned long)p + 4095) & ~4095), 16 * 1024, PROT_NONE); 
+  mprotect((void*)(((unsigned long)data + 4095) & ~4095), 16 * 1024, PROT_NONE);
+  __lsan_do_leak_check();
+  fprintf(stderr, "DONE\n");
+}
+
+// CHECK: Tracer caught signal 11
+// CHECK: LeakSanitizer has encountered a fatal error
+// CHECK-NOT: DONE
+

test/asan/TestCases/Linux/signal_during_stop_the_world.cc

+// Test StopTheWorld behavior during signal storm.
+// Historically StopTheWorld crashed because did not handle EINTR properly.
+// The test is somewhat convoluted, but that's what caused crashes previously.
+
+// RUN: %clangxx_asan -O1 -lpthread %s -o %t && %run %t 2>&1 | FileCheck %s
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <signal.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/prctl.h>
+#include <sys/wait.h>
+#include <time.h>
+#include <pthread.h>
+#include <sanitizer/lsan_interface.h>
+
+static void handler(int signo);
+static void *thr(void *arg);
+
+int main() {
+  struct sigaction act = {};
+  act.sa_handler = handler;
+  sigaction(SIGPROF, &act, 0);
+
+  pid_t pid = fork();
+  if (pid < 0) {
+    fprintf(stderr, "failed to fork\n");
+    exit(1);
+  }
+  if (pid == 0) {
+    // Child constantly sends signals to parent to cause spurious return from
+    // waitpid in StopTheWorld.
+    prctl(PR_SET_PDEATHSIG, SIGTERM, 0, 0, 0);
+    pid_t parent = getppid();
+    for (;;) {
+      kill(parent, SIGCHLD);
+      kill(parent, SIGPROF);
+    }
+  }
+  // Allocate some memory and create some threads to make leak checking longer.
+  void *mem[1000];
+  for (int i = 0; i < 1000; i++)
+    mem[i] = malloc(1000);
+  for (int i = 0; i < 100; i++) {
+    pthread_t th;
+    pthread_create(&th, 0, thr, mem[i]);
+  }
+  usleep(5000);  // Let the child start.
+  __lsan_do_leak_check();
+  // Kill and join the child.
+  kill(pid, SIGTERM);
+  waitpid(pid, 0, 0);
+  sleep(1);  // If the tracer thread still runs, give it time to crash.
+  for (int i = 0; i < 1000; i++)
+    free(mem[i]);
+  fprintf(stderr, "DONE\n");
+// CHECK: DONE
+}
+
+static void handler(int signo) {
+}
+
+static void *thr(void *arg) {
+  for (;;)
+    sleep(1);
+  return 0;
+}

test/sanitizer_common/TestCases/Linux/signal_segv_handler.cc

+// RUN: %clangxx -O1 %s -o %t && TSAN_OPTIONS="flush_memory_ms=1 memory_limit_mb=1" ASAN_OPTIONS="handle_segv=0 allow_user_segv_handler=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 <stdio.h>
+#include <stdlib.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() {
+  struct sigaction a, old;
+  a.sa_sigaction = handler;
+  a.sa_flags = SA_SIGINFO;
+  sigaction(SIGSEGV, &a, &old);
+  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;
+  }
+  sigaction(SIGSEGV, &old, 0);
+  fprintf(stderr, "DONE\n");
+}
+
+// CHECK: DONE