tsan: fix data races between signal handler and sigaction

signal handler reads sa_sigaction when a concurrent sigaction call can modify it
as the result in could try to call SIG_DFL or a partially overwritten function pointer

llvm-svn: 224530

2 files changed, 106 insertions, 17 deletions

diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc b/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc
index d80809dc689..fc742a56ace 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc
+++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc
@@ -101,14 +101,15 @@ typedef long long_t;  // NOLINT
 # define F_TLOCK 2      /* Test and lock a region for exclusive use.  */
 # define F_TEST  3      /* Test a region for other processes locks.  */
 
-typedef void (*sighandler_t)(int sig);
-
 #define errno (*__errno_location())
 
+typedef void (*sighandler_t)(int sig);
+typedef void (*sigactionhandler_t)(int sig, my_siginfo_t *siginfo, void *uctx);
+
 struct sigaction_t {
   union {
     sighandler_t sa_handler;
-    void (*sa_sigaction)(int sig, my_siginfo_t *siginfo, void *uctx);
+    sigactionhandler_t sa_sigaction;
   };
 #if SANITIZER_FREEBSD
   int sa_flags;
@@ -1873,15 +1874,18 @@ static void CallUserSignalHandler(ThreadState *thr, bool sync, bool acquire,
   // Ensure that the handler does not spoil errno.
   const int saved_errno = errno;
   errno = 99;
-  // Need to remember pc before the call, because the handler can reset it.
-  uptr pc = sigact ?
+  // This code races with sigaction. Be careful to not read sa_sigaction twice.
+  // Also need to remember pc for reporting before the call,
+  // because the handler can reset it.
+  volatile uptr pc = sigact ?
      (uptr)sigactions[sig].sa_sigaction :
      (uptr)sigactions[sig].sa_handler;
-  pc += 1;  // return address is expected, OutputReport() will undo this
-  if (sigact)
-    sigactions[sig].sa_sigaction(sig, info, uctx);
-  else
-    sigactions[sig].sa_handler(sig);
+  if (pc != (uptr)SIG_DFL && pc != (uptr)SIG_IGN) {
+    if (sigact)
+      ((sigactionhandler_t)pc)(sig, info, uctx);
+    else
+      ((sighandler_t)pc)(sig);
+  }
   // We do not detect errno spoiling for SIGTERM,
   // because some SIGTERM handlers do spoil errno but reraise SIGTERM,
   // tsan reports false positive in such case.
@@ -1891,7 +1895,9 @@ static void CallUserSignalHandler(ThreadState *thr, bool sync, bool acquire,
   // signal; and it looks too fragile to intercept all ways to reraise a signal.
   if (flags()->report_bugs && !sync && sig != SIGTERM && errno != 99) {
     VarSizeStackTrace stack;
-    ObtainCurrentStack(thr, pc, &stack);
+    // Add 1 to pc because return address is expected,
+    // OutputReport() will undo this.
+    ObtainCurrentStack(thr, pc + 1, &stack);
     ThreadRegistryLock l(ctx->thread_registry);
     ScopedReport rep(ReportTypeErrnoInSignal);
     if (!IsFiredSuppression(ctx, rep, stack)) {
@@ -1917,11 +1923,8 @@ void ProcessPendingSignals(ThreadState *thr) {
     SignalDesc *signal = &sctx->pending_signals[sig];
     if (signal->armed) {
       signal->armed = false;
-      if (sigactions[sig].sa_handler != SIG_DFL
-          && sigactions[sig].sa_handler != SIG_IGN) {
-        CallUserSignalHandler(thr, false, true, signal->sigaction,
-            sig, &signal->siginfo, &signal->ctx);
-      }
+      CallUserSignalHandler(thr, false, true, signal->sigaction, sig,
+          &signal->siginfo, &signal->ctx);
     }
   }
   pthread_sigmask(SIG_SETMASK, &oldset, 0);
@@ -2003,7 +2006,19 @@ TSAN_INTERCEPTOR(int, sigaction, int sig, sigaction_t *act, sigaction_t *old) {
     internal_memcpy(old, &sigactions[sig], sizeof(*old));
   if (act == 0)
     return 0;
-  internal_memcpy(&sigactions[sig], act, sizeof(*act));
+  // Copy act into sigactions[sig].
+  // Can't use struct copy, because compiler can emit call to memcpy.
+  // Can't use internal_memcpy, because it copies byte-by-byte,
+  // and signal handler reads the sa_handler concurrently. It it can read
+  // some bytes from old value and some bytes from new value.
+  // Use volatile to prevent insertion of memcpy.
+  sigactions[sig].sa_handler = *(volatile sighandler_t*)&act->sa_handler;
+  sigactions[sig].sa_flags = *(volatile int*)&act->sa_flags;
+  internal_memcpy(&sigactions[sig].sa_mask, &act->sa_mask,
+      sizeof(sigactions[sig].sa_mask));
+#if !SANITIZER_FREEBSD
+  sigactions[sig].sa_restorer = act->sa_restorer;
+#endif
   sigaction_t newact;
   internal_memcpy(&newact, act, sizeof(newact));
   REAL(sigfillset)(&newact.sa_mask);
diff --git a/compiler-rt/test/tsan/signal_reset.cc b/compiler-rt/test/tsan/signal_reset.cc
new file mode 100644
index 00000000000..aec98dc399e
--- /dev/null
+++ b/compiler-rt/test/tsan/signal_reset.cc
@@ -0,0 +1,74 @@
+// RUN: %clangxx_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <signal.h>
+#include <sys/types.h>
+#include <sys/time.h>
+#include <unistd.h>
+#include <errno.h>
+
+volatile int X;
+int stop;
+
+static void handler(int sig) {
+  (void)sig;
+  if (X != 0)
+    printf("bad");
+}
+
+static void* busy(void *p) {
+  while (__atomic_load_n(&stop, __ATOMIC_RELAXED) == 0) {
+  }
+  return 0;
+}
+
+static void* reset(void *p) {
+  struct sigaction act = {};
+  for (int i = 0; i < 1000000; i++) {
+    act.sa_handler = &handler;
+    if (sigaction(SIGPROF, &act, 0)) {
+      perror("sigaction");
+      exit(1);
+    }
+    act.sa_handler = SIG_IGN;
+    if (sigaction(SIGPROF, &act, 0)) {
+      perror("sigaction");
+      exit(1);
+    }
+  }
+  return 0;
+}
+
+int main() {
+  struct sigaction act = {};
+  act.sa_handler = SIG_IGN;
+  if (sigaction(SIGPROF, &act, 0)) {
+    perror("sigaction");
+    exit(1);
+  }
+
+  itimerval t;
+  t.it_value.tv_sec = 0;
+  t.it_value.tv_usec = 10;
+  t.it_interval = t.it_value;
+  if (setitimer(ITIMER_PROF, &t, 0)) {
+    perror("setitimer");
+    exit(1);
+  }
+
+  pthread_t th[2];
+  pthread_create(&th[0], 0, busy, 0);
+  pthread_create(&th[1], 0, reset, 0);
+
+  pthread_join(th[1], 0);
+  __atomic_store_n(&stop, 1, __ATOMIC_RELAXED);
+  pthread_join(th[0], 0);
+
+  fprintf(stderr, "DONE\n");
+  return 0;
+}
+
+// CHECK-NOT: WARNING: ThreadSanitizer:
+// CHECK: DONE
+// CHECK-NOT: WARNING: ThreadSanitizer: