tsan: fix a race when assigning ThreadSignalContext

[ridiculousfish]

Summary

The SigCtx function lazily allocates a ThreadSignalContext, and stores it
in the ThreadState. This function may be called by various interceptors and
the signal handler itself.

If SigCtx itself is interrupted by a signal, then (prior to this fix) there
was a possibility of allocating two ThreadSignalContexts. This not only
leaks, it fails to deliver the signal to the program's signal handler, as
the recorded signal is overwritten by the new ThreadSignalContext.

Fix this by using a CAS to swap in the ThreadSignalContext, preventing the
race. Add a test for this case.

[melver]

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

264

A failed atomic_compare_exchange will load current value into ctx, so this additional atomic_load is unnecessary?

[ridiculousfish]

Address review feedback: we don't need to loop on CAS and we can elide the atomic_load.

[ridiculousfish]

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

264

Good call, fixed.

[melver]

LGTM, but please wait for Dmitry's review as well (he'll likely respond next week).

Thanks!

[melver]

@ Peter, are you able to commit this yourself, or should we commit it for you?

[Peter]

In D140582#4040428, @melver wrote:

@ Peter, are you able to commit this yourself, or should we commit it for you?

Sorry but I think you got the wrong Peter. I never worked on tsan before:)

[melver]

In D140582#4040513, @Peter wrote:

In D140582#4040428, @melver wrote:

@ Peter, are you able to commit this yourself, or should we commit it for you?

Sorry but I think you got the wrong Peter. I never worked on tsan before:)

Yes, I meant @ridiculousfish - sorry about that, I didn't realize @ auto-Ccs.

[ridiculousfish]

Thank you for the review! Please merge on my behalf.

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

 #else
 SANITIZER_WEAK_CXX_DEFAULT_IMPL void OnPotentiallyBlockingRegionBegin() {}
 SANITIZER_WEAK_CXX_DEFAULT_IMPL void OnPotentiallyBlockingRegionEnd() {}
 #endif
 
 }  // namespace __tsan
 
 static ThreadSignalContext *SigCtx(ThreadState *thr) {
-  ThreadSignalContext *ctx = (ThreadSignalContext*)thr->signal_ctx;
-  if (ctx == 0 && !thr->is_dead) {
-    ctx = (ThreadSignalContext*)MmapOrDie(sizeof(*ctx), "ThreadSignalContext");
-    MemoryResetRange(thr, (uptr)&SigCtx, (uptr)ctx, sizeof(*ctx));
-    thr->signal_ctx = ctx;
+  // This function may be called recursively if it is interrupted by a signal
+  // handler. Loop with CAS to handle the race.
+  uptr ctx = atomic_load(&thr->signal_ctx, memory_order_relaxed);
+  while (ctx == 0 && !thr->is_dead) {
+    uptr pctx =
+        (uptr)MmapOrDie(sizeof(ThreadSignalContext), "ThreadSignalContext");
+    MemoryResetRange(thr, (uptr)&SigCtx, pctx, sizeof(ThreadSignalContext));
+    if (atomic_compare_exchange_strong(&thr->signal_ctx, &ctx, pctx,
+                                       memory_order_relaxed)) {
+      ctx = pctx;
+    } else {
+      UnmapOrDie((ThreadSignalContext *)pctx, sizeof(ThreadSignalContext));
+      ctx = atomic_load(&thr->signal_ctx, memory_order_relaxed);
+    }
   }
-  return ctx;
+  return (ThreadSignalContext *)ctx;
 }
 
 ScopedInterceptor::ScopedInterceptor(ThreadState *thr, const char *fname,
                                      uptr pc)
     : thr_(thr) {
   LazyInitialize(thr);
   if (UNLIKELY(atomic_load(&thr->in_blocking_func, memory_order_relaxed))) {
     // pthread_join is marked as blocking, but it's also known to call other
void DestroyThreadState() {
   ThreadFinish(thr);
   ProcUnwire(proc, thr);
   ProcDestroy(proc);
   DTLS_Destroy();
   cur_thread_finalize();
 }
 
 void PlatformCleanUpThreadState(ThreadState *thr) {
-  ThreadSignalContext *sctx = thr->signal_ctx;
+  ThreadSignalContext *sctx = (ThreadSignalContext *)atomic_load(
+      &thr->signal_ctx, memory_order_relaxed);
   if (sctx) {
-    thr->signal_ctx = 0;
+    atomic_store(&thr->signal_ctx, 0, memory_order_relaxed);
     UnmapOrDie(sctx, sizeof(*sctx));
   }
 }
 }  // namespace __tsan
 
 #if !SANITIZER_APPLE && !SANITIZER_NETBSD && !SANITIZER_FREEBSD
 static void thread_finalize(void *v) {
   uptr iter = (uptr)v;

compiler-rt/lib/tsan/rtl/tsan_rtl.h

#endif
   Processor *proc1;
 #if !SANITIZER_GO
   Processor *proc() { return proc1; }
 #else
   Processor *proc();
 #endif
 
   atomic_uintptr_t in_signal_handler;
-  ThreadSignalContext *signal_ctx;
+  atomic_uintptr_t signal_ctx;
 
 #if !SANITIZER_GO
   StackID last_sleep_stack_id;
   VectorClock last_sleep_clock;
 #endif
 
   // Set in regions of runtime that must be signal-safe and fork-safe.
   // If set, malloc must not be called.

compiler-rt/test/tsan/signal_thread_sigctx_race.cpp

+// RUN: %clangxx_tsan %s -o %t && %run %t 2>&1 | FileCheck %s
+// UNSUPPORTED: darwin
+
+#include <errno.h>
+#include <limits.h>
+#include <pthread.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/select.h>
+#include <sys/time.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+// This attempts to exercise a race condition where both a thread and its signal
+// handler allocate the SigCtx. If the race is allowed, it leads to a leak and
+// the first signal being dropped.
+// Spawn threads in a loop and send it SIGUSR1 concurrently with the thread
+// doing a bogus kill() call. The signal handler writes to a self-pipe which the
+// thread detects and then exits. A dropped signal results in a timeout.
+int pipes[2];
+static void handler(int sig) { write(pipes[1], "x", 1); }
+
+static int do_select() {
+  struct timeval tvs {
+    0, 1000
+  };
+  fd_set fds;
+  FD_ZERO(&fds);
+  FD_SET(pipes[0], &fds);
+  return select(pipes[0] + 1, &fds, 0, 0, &tvs);
+}
+
+static void *thr(void *p) {
+  // This kill() is expected to fail; it exists only to trigger a call to SigCtx
+  // outside of the signal handler.
+  kill(INT_MIN, 0);
+  int success = 0;
+  for (int i = 0; i < 1024; i++) {
+    if (do_select() > 0) {
+      success = 1;
+      break;
+    }
+  }
+  if (success) {
+    char c;
+    read(pipes[0], &c, 1);
+  } else {
+    fprintf(stderr, "Failed to receive signal\n");
+    exit(1);
+  }
+  return p;
+}
+
+int main() {
+  if (pipe(pipes)) {
+    perror("pipe");
+    exit(1);
+  }
+
+  struct sigaction act = {};
+  act.sa_handler = &handler;
+  if (sigaction(SIGUSR1, &act, 0)) {
+    perror("sigaction");
+    exit(1);
+  }
+
+  for (int i = 0; i < (1 << 10); i++) {
+    pthread_t th{};
+    if (pthread_create(&th, 0, thr, 0)) {
+      perror("pthread_create");
+      exit(1);
+    }
+    pthread_kill(th, SIGUSR1);
+    pthread_join(th, 0);
+  }
+
+  fprintf(stderr, "DONE\n");
+  return 0;
+}
+
+// CHECK-NOT: WARNING: ThreadSanitizer:
+// CHECK: DONE
+// CHECK-NOT: WARNING: ThreadSanitizer: