[tsan] Fix sigaction implementation when it's called only to get handler

vitalybuka · vitalybuka · commit 7f6bb4ab1f35 · 2017-11-20T23:37:46.000Z
Reviewers: eugenis Subscribers: kubamracek, llvm-commits, krytarowski Differential Revision: https://reviews.llvm.org/D40272 llvm-svn: 318707
diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc b/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc
@@ -2287,33 +2287,35 @@ int sigaction_impl(int sig, const __sanitizer_sigaction *act,
   SCOPED_INTERCEPTOR_RAW(sigaction, sig, act, old);
   __sanitizer_sigaction *sigactions = interceptor_ctx()->sigactions;
   __sanitizer_sigaction old_stored;
-  internal_memcpy(&old_stored, &sigactions[sig], sizeof(old_stored));
-  if (act == 0) return 0;
-  // 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 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].handler =
-      *(volatile __sanitizer_sighandler_ptr const *)&act->handler;
-  sigactions[sig].sa_flags = *(volatile int const *)&act->sa_flags;
-  internal_memcpy(&sigactions[sig].sa_mask, &act->sa_mask,
-                  sizeof(sigactions[sig].sa_mask));
-#if !SANITIZER_FREEBSD && !SANITIZER_MAC && !SANITIZER_NETBSD
-  sigactions[sig].sa_restorer = act->sa_restorer;
-#endif
+  if (old) internal_memcpy(&old_stored, &sigactions[sig], sizeof(old_stored));
   __sanitizer_sigaction newact;
-  internal_memcpy(&newact, act, sizeof(newact));
-  internal_sigfillset(&newact.sa_mask);
-  if ((uptr)act->handler != sig_ign && (uptr)act->handler != sig_dfl) {
-    if (newact.sa_flags & SA_SIGINFO)
-      newact.sigaction = rtl_sigaction;
-    else
-      newact.handler = rtl_sighandler;
+  if (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 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].handler =
+        *(volatile __sanitizer_sighandler_ptr const *)&act->handler;
+    sigactions[sig].sa_flags = *(volatile int const *)&act->sa_flags;
+    internal_memcpy(&sigactions[sig].sa_mask, &act->sa_mask,
+                    sizeof(sigactions[sig].sa_mask));
+#if !SANITIZER_FREEBSD && !SANITIZER_MAC && !SANITIZER_NETBSD
+    sigactions[sig].sa_restorer = act->sa_restorer;
+#endif
+    internal_memcpy(&newact, act, sizeof(newact));
+    internal_sigfillset(&newact.sa_mask);
+    if ((uptr)act->handler != sig_ign && (uptr)act->handler != sig_dfl) {
+      if (newact.sa_flags & SA_SIGINFO)
+        newact.sigaction = rtl_sigaction;
+      else
+        newact.handler = rtl_sighandler;
+    }
+    ReleaseStore(thr, pc, (uptr)&sigactions[sig]);
+    act = &newact;
   }
-  ReleaseStore(thr, pc, (uptr)&sigactions[sig]);
-  int res = REAL(sigaction)(sig, &newact, old);
+  int res = REAL(sigaction)(sig, act, old);
   if (res == 0 && old) {
     uptr cb = (uptr)old->sigaction;
     if (cb == (uptr)rtl_sigaction || cb == (uptr)rtl_sighandler) {
diff --git a/compiler-rt/test/sanitizer_common/TestCases/Linux/signal_segv_handler.cc b/compiler-rt/test/sanitizer_common/TestCases/Linux/signal_segv_handler.cc
@@ -1,4 +1,6 @@
-// RUN: %clangxx -O1 %s -o %t && TSAN_OPTIONS="flush_memory_ms=1 memory_limit_mb=1" ASAN_OPTIONS="handle_segv=0" %run %t 2>&1 | FileCheck %s
+// clang-format off
+// RUN: %clangxx -O1 %s -o %t && TSAN_OPTIONS="flush_memory_ms=1 memory_limit_mb=1" %run %t 2>&1 | FileCheck %s
+// clang-format on
 
 // 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.
@@ -13,11 +15,12 @@
 // "benign" SEGVs that are handled by signal handler, and ensures that
 // the process survive.
 
+#include <assert.h>
+#include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
-#include <signal.h>
-#include <sys/mman.h>
 #include <string.h>
+#include <sys/mman.h>
 #include <unistd.h>
 
 unsigned long page_size;
@@ -35,6 +38,12 @@ int main() {
   a.sa_sigaction = handler;
   a.sa_flags = SA_SIGINFO;
   sigaction(SIGSEGV, &a, &old);
+
+  memset(&a, 0, sizeof(a));
+  sigaction(SIGSEGV, 0, &a);
+  assert(a.sa_sigaction == handler);
+  assert(a.sa_flags & SA_SIGINFO);
+
   guard = mmap(0, 3 * page_size, PROT_NONE, MAP_ANON | MAP_PRIVATE, -1, 0);
   guard = (char*)guard + page_size;  // work around a kernel bug 
   for (int i = 0; i < 1000000; i++) {
