[tsan] Fix signal chaining

[vitalybuka]

Summary

Return saved values only if installed sigaction is our wrapper.

[dvyukov]

LGTM with a nit

compiler-rt/lib/tsan/rtl/tsan_interceptors.cc

2309 ↗

(On Diff #122569)

I think we should at least zero old. Otherwise there is no way to distinguish when it's filled and when it's uninit garbage.

[vitalybuka]

compiler-rt/lib/tsan/rtl/tsan_interceptors.cc

2309 ↗

(On Diff #122569)

if (res != 0) it's OK to expect garbage there
if (res == 0) old contains whatever was installed without interceptor, and this is the point of the patch, to get Deadly Signal handler.
We install "Deadly Signal" handler using real sigaction to avoid "allow_user_segv_handler" flag check in interceptor.

[dvyukov]

compiler-rt/lib/tsan/rtl/tsan_interceptors.cc

2309 ↗

(On Diff #122569)

if (res == 0) old contains whatever was installed without interceptor

This is not the case when cb != rtl_sigaction. We return 0 and leave garbage in old. Does libc ever return res=0 and garbage in old? If not, we should not too.

[vitalybuka]

compiler-rt/lib/tsan/rtl/tsan_interceptors.cc

2309 ↗

(On Diff #122569)

if (res == 0 && cb != rtl_sigaction && cb != rtl_sighandler) "old" contains something that was set by REAL(sigaction)(sig, &newact, old) call above, which is not garbage.

[dvyukov]

compiler-rt/lib/tsan/rtl/tsan_interceptors.cc

2309 ↗

(On Diff #122569)

That's what I missed! We now pass old to sigaction.

compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors.cc

 
 int sigaction_impl(int sig, const __sanitizer_sigaction *act,
                    __sanitizer_sigaction *old) {
   // Note: if we call REAL(sigaction) directly for any reason without proxying
   // the signal handler through rtl_sigaction, very bad things will happen.
   // The handler will run synchronously and corrupt tsan per-thread state.
   SCOPED_INTERCEPTOR_RAW(sigaction, sig, act, old);
   __sanitizer_sigaction *sigactions = interceptor_ctx()->sigactions;
-  if (old) internal_memcpy(old, &sigactions[sig], sizeof(*old));
+  __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 =
#endif
   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]);
-  int res = REAL(sigaction)(sig, &newact, 0);
+  int res = REAL(sigaction)(sig, &newact, old);
+  if (res == 0 && old) {
+    uptr cb = (uptr)old->sigaction;
+    if (cb == (uptr)rtl_sigaction || cb == (uptr)rtl_sighandler) {
+      internal_memcpy(old, &old_stored, sizeof(*old));
+    }
+  }
   return res;
 }
 
 static __sanitizer_sighandler_ptr signal_impl(int sig,
                                               __sanitizer_sighandler_ptr h) {
   __sanitizer_sigaction act;
   act.handler = h;
   internal_memset(&act.sa_mask, -1, sizeof(act.sa_mask));

compiler-rt/trunk/test/sanitizer_common/TestCases/Linux/allow_user_segv.cc

 // RUN: %env_tool_opts=handle_segv=1:allow_user_segv_handler=0 not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK2
 // RUN: %env_tool_opts=handle_segv=2:allow_user_segv_handler=0 not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK2
 
 // RUN: %env_tool_opts=handle_segv=0:allow_user_segv_handler=1 not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK0
 // RUN: %env_tool_opts=handle_segv=1:allow_user_segv_handler=1 not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK1
 // RUN: %env_tool_opts=handle_segv=2:allow_user_segv_handler=1 not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK2
 // clang-format on
 
-// Remove when fixed: https://github.com/google/sanitizers/issues/637
-
-// XFAIL: tsan
-
 // Flaky errors in debuggerd with "waitpid returned unexpected pid (0)" in logcat.
 // UNSUPPORTED: android && i386-target-arch
 
 #include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
 
 struct sigaction original_sigaction_sigbus;