tsan: refactor fork handling

[dvyukov]

Summary

Commit efd254b6362 ("tsan: fix deadlock in pthread_atfork callbacks")
fixed another deadlock related to atfork handling.
But builders with DCHECKs enabled reported failures of
pthread_atfork_deadlock2.c and pthread_atfork_deadlock3.c tests
related to the fact that we hold runtime locks on interceptor exit:
https://lab.llvm.org/buildbot/#/builders/70/builds/6727
This issue is somewhat inherent to the current approach,
we indeed execute user code (atfork callbacks) with runtime lock held.

Refactor fork handling to not run user code (atfork callbacks)
with runtime locks held. This change does this by installing
own atfork callbacks during runtime initialization.
Atfork callbacks run in LIFO order, so the expectation is that
our callbacks run last, right before the actual fork.
This way we lock runtime mutexes around fork, but not around
user callbacks.

Extend tests to also install after fork callbacks just to cover
more scenarios. Some tests also started reporting real races
that we previously suppressed.

[dvyukov]

cherry pick the previous fix as it was reverted

[dvyukov]

I've squashed this with the original fix as it was reverted. I run both release and debug tests now.

[dvyukov]

We won't know if it breaks something before we merge :) fingers crossed

[tpopp]

I'm reverting this due to test failures and emailing you a more in depth reproducer for the failures. It seems like you expected the possibility of failures, so I hope this is okay.

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

 #elif defined(__aarch64__) || SANITIZER_PPC64V2
 #define PTHREAD_ABI_BASE  "GLIBC_2.17"
 #endif
 
 extern "C" int pthread_attr_init(void *attr);
 extern "C" int pthread_attr_destroy(void *attr);
 DECLARE_REAL(int, pthread_attr_getdetachstate, void *, void *)
 extern "C" int pthread_attr_setstacksize(void *attr, uptr stacksize);
+extern "C" int pthread_atfork(void (*prepare)(void), void (*parent)(void),
+                              void (*child)(void));
 extern "C" int pthread_key_create(unsigned *key, void (*destructor)(void* v));
 extern "C" int pthread_setspecific(unsigned key, const void *v);
 DECLARE_REAL(int, pthread_mutexattr_gettype, void *, void *)
 DECLARE_REAL(int, fflush, __sanitizer_FILE *fp)
 DECLARE_REAL_AND_INTERCEPTOR(void *, malloc, uptr size)
 DECLARE_REAL_AND_INTERCEPTOR(void, free, void *ptr)
 extern "C" void *pthread_self();
 extern "C" void _exit(int status);
static void CallUserSignalHandler(ThreadState *thr, bool sync, bool acquire,
   }
   // 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.
   // It's difficult to properly detect this situation (reraise),
   // because in async signal processing case (when handler is called directly
   // from rtl_generic_sighandler) we have not yet received the reraised
   // signal; and it looks too fragile to intercept all ways to reraise a signal.
-  if (flags()->report_bugs && !sync && sig != SIGTERM && errno != 99) {
+  if (ShouldReport(thr, ReportTypeErrnoInSignal) && !sync && sig != SIGTERM &&
+      errno != 99) {
     VarSizeStackTrace stack;
     // StackTrace::GetNestInstructionPc(pc) is used because return address is
     // expected, OutputReport() will undo this.
     ObtainCurrentStack(thr, StackTrace::GetNextInstructionPc(pc), &stack);
     ThreadRegistryLock l(ctx->thread_registry);
     ScopedReport rep(ReportTypeErrnoInSignal);
     if (!IsFiredSuppression(ctx, ReportTypeErrnoInSignal, stack)) {
       rep.AddStack(stack, true);
TSAN_INTERCEPTOR(int, getaddrinfo, void *node, void *service,
   ThreadIgnoreEnd(thr, pc);
   return res;
 }
 
 TSAN_INTERCEPTOR(int, fork, int fake) {
   if (in_symbolizer())
     return REAL(fork)(fake);
   SCOPED_INTERCEPTOR_RAW(fork, fake);
+  return REAL(fork)(fake);
+}
+
+void atfork_prepare() {
+  if (in_symbolizer())
+    return;
+  ThreadState *thr = cur_thread();
+  const uptr pc = StackTrace::GetCurrentPc();
   ForkBefore(thr, pc);
-  int pid;
-  {
-    // On OS X, REAL(fork) can call intercepted functions (OSSpinLockLock), and
-    // we'll assert in CheckNoLocks() unless we ignore interceptors.
-    ScopedIgnoreInterceptors ignore;
-    pid = REAL(fork)(fake);
-  }
-  if (pid == 0) {
-    // child
-    ForkChildAfter(thr, pc);
-    FdOnFork(thr, pc);
-  } else if (pid > 0) {
-    // parent
-    ForkParentAfter(thr, pc);
-  } else {
-    // error
-    ForkParentAfter(thr, pc);
-  }
-  return pid;
+
+void atfork_parent() {
+  if (in_symbolizer())
+    return;
+  ThreadState *thr = cur_thread();
+  const uptr pc = StackTrace::GetCurrentPc();
+
+void atfork_child() {
+  if (in_symbolizer())
+    return;
+  ThreadState *thr = cur_thread();
+  const uptr pc = StackTrace::GetCurrentPc();
+  ForkChildAfter(thr, pc);
+  FdOnFork(thr, pc);
 }
 
 TSAN_INTERCEPTOR(int, vfork, int fake) {
   // Some programs (e.g. openjdk) call close for all file descriptors
   // in the child process. Under tsan it leads to false positives, because
   // address space is shared, so the parent process also thinks that
   // the descriptors are closed (while they are actually not).
   // This leads to false positives due to missed synchronization.
#if !SANITIZER_MAC && !SANITIZER_ANDROID
   // But atexit is emitted directly into the module, so can't be resolved.
   REAL(atexit) = (int(*)(void(*)()))unreachable;
 #endif
 
   if (REAL(__cxa_atexit)(&finalize, 0, 0)) {
     Printf("ThreadSanitizer: failed to setup atexit callback\n");
     Die();
   }
+  if (pthread_atfork(atfork_prepare, atfork_parent, atfork_child)) {
+    Printf("ThreadSanitizer: failed to setup atfork callbacks\n");
+    Die();
+  }
 
 #if !SANITIZER_MAC && !SANITIZER_NETBSD && !SANITIZER_FREEBSD
   if (pthread_key_create(&interceptor_ctx()->finalize_key, &thread_finalize)) {
     Printf("ThreadSanitizer: failed to create thread key\n");
     Die();
   }
 #endif
 

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

 }
 
 void AllocatorPrintStats() {
   allocator()->PrintStats();
 }
 
 static void SignalUnsafeCall(ThreadState *thr, uptr pc) {
   if (atomic_load_relaxed(&thr->in_signal_handler) == 0 ||
-      !flags()->report_signal_unsafe)
+      !ShouldReport(thr, ReportTypeSignalUnsafe))
     return;
   VarSizeStackTrace stack;
   ObtainCurrentStack(thr, pc, &stack);
   if (IsFiredSuppression(ctx, ReportTypeSignalUnsafe, stack))
     return;
   ThreadRegistryLock l(ctx->thread_registry);
   ScopedReport rep(ReportTypeSignalUnsafe);
   rep.AddStack(stack, true);

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

  public:
   explicit ScopedReport(ReportType typ, uptr tag = kExternalTagNone);
   ~ScopedReport();
 
  private:
   ScopedErrorReportLock lock_;
 };
 
+bool ShouldReport(ThreadState *thr, ReportType typ);
 ThreadContext *IsThreadStackOrTls(uptr addr, bool *is_stack);
 void RestoreStack(int tid, const u64 epoch, VarSizeStackTrace *stk,
                   MutexSet *mset, uptr *tag = nullptr);
 
 // The stack could look like:
 //   <start> | <main> | <foo> | tag | <bar>
 // This will extract the tag and keep:
 //   <start> | <main> | <foo> | <bar>

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

#endif
 
   return failed ? common_flags()->exitcode : 0;
 }
 
 #if !SANITIZER_GO
 void ForkBefore(ThreadState *thr, uptr pc) {
   ctx->thread_registry->Lock();
   ctx->report_mtx.Lock();
-  // Ignore memory accesses in the pthread_atfork callbacks.
-  // If any of them triggers a data race we will deadlock
-  // on the report_mtx.
-  // We could ignore interceptors and sync operations as well,
+  // Suppress all reports in the pthread_atfork callbacks.
+  // Reports will deadlock on the report_mtx.
+  // We could ignore sync operations as well,
   // but so far it's unclear if it will do more good or harm.
   // Unnecessarily ignoring things can lead to false positives later.
-  ThreadIgnoreBegin(thr, pc);
+  thr->suppress_reports++;
+  // On OS X, REAL(fork) can call intercepted functions (OSSpinLockLock), and
+  // we'll assert in CheckNoLocks() unless we ignore interceptors.
+  thr->ignore_interceptors++;
 }
 
 void ForkParentAfter(ThreadState *thr, uptr pc) {
-  ThreadIgnoreEnd(thr, pc);  // Begin is in ForkBefore.
+  thr->suppress_reports--;  // Enabled in ForkBefore.
+  thr->ignore_interceptors--;
   ctx->report_mtx.Unlock();
   ctx->thread_registry->Unlock();
 }
 
 void ForkChildAfter(ThreadState *thr, uptr pc) {
-  ThreadIgnoreEnd(thr, pc);  // Begin is in ForkBefore.
+  thr->suppress_reports--;  // Enabled in ForkBefore.
+  thr->ignore_interceptors--;
   ctx->report_mtx.Unlock();
   ctx->thread_registry->Unlock();
 
   uptr nthread = 0;
   ctx->thread_registry->GetNumberOfThreads(0, 0, &nthread /* alive threads */);
   VPrintf(1, "ThreadSanitizer: forked new process with pid %d,"
       " parent had %d threads\n", (int)internal_getpid(), (int)nthread);
   if (nthread == 1) {

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

 }
 
 static void ReportMutexMisuse(ThreadState *thr, uptr pc, ReportType typ,
     uptr addr, u64 mid) {
   // In Go, these misuses are either impossible, or detected by std lib,
   // or false positives (e.g. unlock in a different thread).
   if (SANITIZER_GO)
     return;
+  if (!ShouldReport(thr, typ))
+    return;
   ThreadRegistryLock l(ctx->thread_registry);
   ScopedReport rep(typ);
   rep.AddMutex(mid);
   VarSizeStackTrace trace;
   ObtainCurrentStack(thr, pc, &trace);
   rep.AddStack(trace, true);
   rep.AddLocation(addr, 1);
   OutputReport(thr, rep);
if (flags()->report_destroy_locked
     s->SetFlags(MutexFlagBroken);
     unlock_locked = true;
   }
   u64 mid = s->GetId();
   u64 last_lock = s->last_lock;
   if (!unlock_locked)
     s->Reset(thr->proc());  // must not reset it before the report is printed
   s->mtx.Unlock();
-  if (unlock_locked) {
+  if (unlock_locked && ShouldReport(thr, ReportTypeMutexDestroyLocked)) {
     ThreadRegistryLock l(ctx->thread_registry);
     ScopedReport rep(ReportTypeMutexDestroyLocked);
     rep.AddMutex(mid);
     VarSizeStackTrace trace;
     ObtainCurrentStack(thr, pc, &trace);
     rep.AddStack(trace, true);
     FastState last(last_lock);
     RestoreStack(last.tid(), last.epoch(), &trace, 0);
void AcquireReleaseImpl(ThreadState *thr, uptr pc, SyncClock *c) {
   thr->clock.set(thr->fast_state.epoch());
   thr->fast_synch_epoch = thr->fast_state.epoch();
   thr->clock.acq_rel(&thr->proc()->clock_cache, c);
   StatInc(thr, StatSyncAcquire);
   StatInc(thr, StatSyncRelease);
 }
 
 void ReportDeadlock(ThreadState *thr, uptr pc, DDReport *r) {
-  if (r == 0)
+  if (r == 0 || !ShouldReport(thr, ReportTypeDeadlock))
     return;
   ThreadRegistryLock l(ctx->thread_registry);
   ScopedReport rep(ReportTypeDeadlock);
   for (int i = 0; i < r->n; i++) {
     rep.AddMutex(r->loop[i].mtx_ctx0);
     rep.AddUniqueTid((int)r->loop[i].thr_ctx);
     rep.AddThread((int)r->loop[i].thr_ctx);
   }

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

static ReportStack *SymbolizeStack(StackTrace trace) {
   }
   StackStripMain(top);
 
   ReportStack *stack = ReportStack::New();
   stack->frames = top;
   return stack;
 }
 
+bool ShouldReport(ThreadState *thr, ReportType typ) {
+  // We set thr->suppress_reports in the fork context.
+  // Taking any locking in the fork context can lead to deadlocks.
+  // If any locks are already taken, it's too late to do this check.
+  CheckNoLocks(thr);
+  // For the same reason check we didn't lock thread_registry yet.
+  if (SANITIZER_DEBUG)
+    ThreadRegistryLock l(ctx->thread_registry);
+  if (!flags()->report_bugs || thr->suppress_reports)
+    return false;
+  switch (typ) {
+    case ReportTypeSignalUnsafe:
+      return flags()->report_signal_unsafe;
+    case ReportTypeThreadLeak:
+#if !SANITIZER_GO
+      // It's impossible to join phantom threads
+      // in the child after fork.
+      if (ctx->after_multithreaded_fork)
+        return false;
+#endif
+      return flags()->report_thread_leaks;
+    case ReportTypeMutexDestroyLocked:
+      return flags()->report_destroy_locked;
+    default:
+      return true;
+  }
+}
+
 ScopedReportBase::ScopedReportBase(ReportType typ, uptr tag) {
   ctx->thread_registry->CheckLocked();
   void *mem = internal_alloc(MBlockReport, sizeof(ReportDesc));
   rep_ = new(mem) ReportDesc;
   rep_->typ = typ;
   rep_->tag = tag;
   ctx->report_mtx.Lock();
 }
static bool HandleRacyAddress(ThreadState *thr, uptr addr_min, uptr addr_max) {
   Lock lock(&ctx->racy_mtx);
   if (FindRacyAddress(ra0))
     return true;
   ctx->racy_addresses.PushBack(ra0);
   return false;
 }
 
 bool OutputReport(ThreadState *thr, const ScopedReport &srep) {
-  if (!flags()->report_bugs || thr->suppress_reports)
-    return false;
+  // These should have been checked in ShouldReport.
+  // It's too late to check them here, we have already taken locks.
+  CHECK(flags()->report_bugs);
+  CHECK(!thr->suppress_reports);
   atomic_store_relaxed(&ctx->last_symbolize_time_ns, NanoTime());
   const ReportDesc *rep = srep.GetReport();
   CHECK_EQ(thr->current_report, nullptr);
   thr->current_report = rep;
   Suppression *supp = 0;
   uptr pc_or_addr = 0;
   for (uptr i = 0; pc_or_addr == 0 && i < rep->mops.Size(); i++)
     pc_or_addr = IsSuppressed(rep->typ, rep->mops[i]->stack, &supp);

 
 void ReportRace(ThreadState *thr) {
   CheckNoLocks(thr);
 
   // Symbolizer makes lots of intercepted calls. If we try to process them,
   // at best it will cause deadlocks on internal mutexes.
   ScopedIgnoreInterceptors ignore;
 
-  if (!flags()->report_bugs)
+  if (!ShouldReport(thr, ReportTypeRace))
     return;
   if (!flags()->report_atomic_races && !RaceBetweenAtomicAndFree(thr))
     return;
 
   bool freed = false;
   {
     Shadow s(thr->racy_state[1]);
     freed = s.GetFreedAndReset();

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

 }
 #else
 static void ThreadCheckIgnore(ThreadState *thr) {}
 #endif
 
 void ThreadFinalize(ThreadState *thr) {
   ThreadCheckIgnore(thr);
 #if !SANITIZER_GO
-  if (!flags()->report_thread_leaks)
+  if (!ShouldReport(thr, ReportTypeThreadLeak))
     return;
   ThreadRegistryLock l(ctx->thread_registry);
   Vector<ThreadLeak> leaks;
   ctx->thread_registry->RunCallbackForEachThreadLocked(
       MaybeReportThreadLeak, &leaks);
   for (uptr i = 0; i < leaks.Size(); i++) {
     ScopedReport rep(ReportTypeThreadLeak);
     rep.AddThread(leaks[i].tctx, true);

compiler-rt/test/tsan/pthread_atfork_deadlock.c

 
 void atfork() {
   fprintf(stderr, "ATFORK\n");
   glob++;
 }
 
 int main() {
   barrier_init(&barrier, 2);
-  pthread_atfork(atfork, NULL, NULL);
+  pthread_atfork(atfork, atfork, atfork);
   pthread_t t;
   pthread_create(&t, NULL, worker, NULL);
   glob++;
   barrier_wait(&barrier);
   pthread_join(t, NULL);
   // CHECK: ThreadSanitizer: data race
   // CHECK-NOT: ATFORK
   return 0;
 }

compiler-rt/test/tsan/pthread_atfork_deadlock2.c

-// RUN: %clang_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s
+// RUN: %clang_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s
 // Regression test for
 // https://groups.google.com/d/msg/thread-sanitizer/e_zB9gYqFHM/DmAiTsrLAwAJ
 // pthread_atfork() callback triggers a data race and we deadlocked
 // on the report_mtx as we lock it around fork.
 #include "test.h"
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <errno.h>
 
 int glob = 0;
 
 void *worker(void *unused) {
   glob++;
   barrier_wait(&barrier);
   return NULL;
 }
 
 void atfork() {
   glob++;
 }
 
 int main() {
   barrier_init(&barrier, 2);
-  pthread_atfork(atfork, NULL, NULL);
+  pthread_atfork(atfork, atfork, atfork);
   pthread_t t;
   pthread_create(&t, NULL, worker, NULL);
   barrier_wait(&barrier);
   pid_t pid = fork();
   if (pid < 0) {
     fprintf(stderr, "fork failed: %d\n", errno);
     return 1;
   }
   if (pid == 0) {
     fprintf(stderr, "CHILD\n");
     return 0;
   }
   if (pid != waitpid(pid, NULL, 0)) {
     fprintf(stderr, "waitpid failed: %d\n", errno);
     return 1;
   }
   pthread_join(t, NULL);
   fprintf(stderr, "PARENT\n");
   return 0;
 }
 
-// CHECK-NOT: ThreadSanitizer: data race
+// CHECK: ThreadSanitizer: data race
+// CHECK:   Write of size 4
+// CHECK:     #0 atfork
+// CHECK:   Previous write of size 4
+// CHECK:     #0 worker
 // CHECK: CHILD
 // CHECK: PARENT

compiler-rt/test/tsan/pthread_atfork_deadlock3.c

+// RUN: %clang_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s
+// Regression test for
+// https://groups.google.com/g/thread-sanitizer/c/TQrr4-9PRYo/m/HFR4FMi6AQAJ
+#include "test.h"
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <errno.h>
+#include <string.h>
+#include <signal.h>
+
+long glob = 0;
+
+void *worker(void *main) {
+  glob++;
+  // synchronize with main
+  barrier_wait(&barrier);
+  // synchronize with atfork
+  barrier_wait(&barrier);
+  pthread_kill((pthread_t)main, SIGPROF);
+  barrier_wait(&barrier);
+  // synchronize with afterfork
+  barrier_wait(&barrier);
+  pthread_kill((pthread_t)main, SIGPROF);
+  barrier_wait(&barrier);
+  return NULL;
+}
+
+void atfork() {
+  barrier_wait(&barrier);
+  barrier_wait(&barrier);
+  write(2, "in atfork\n", strlen("in atfork\n"));
+  static volatile long a;
+  __atomic_fetch_add(&a, 1, __ATOMIC_RELEASE);
+}
+
+void afterfork() {
+  barrier_wait(&barrier);
+  barrier_wait(&barrier);
+  write(2, "in afterfork\n", strlen("in afterfork\n"));
+  static volatile long a;
+  __atomic_fetch_add(&a, 1, __ATOMIC_RELEASE);
+}
+
+void afterfork_child() {
+  // Can't synchronize with barriers because we are
+  // in the new process, but want consistent output.
+  sleep(1);
+  write(2, "in afterfork_child\n", strlen("in afterfork_child\n"));
+  glob++;
+}
+
+void handler(int sig) {
+  write(2, "in handler\n", strlen("in handler\n"));
+  glob++;
+}
+
+int main() {
+  barrier_init(&barrier, 2);
+  struct sigaction act = {};
+  act.sa_handler = &handler;
+  if (sigaction(SIGPROF, &act, 0)) {
+    perror("sigaction");
+    exit(1);
+  }
+  pthread_atfork(atfork, afterfork, afterfork_child);
+  pthread_t t;
+  pthread_create(&t, NULL, worker, (void*)pthread_self());
+  barrier_wait(&barrier);
+  pid_t pid = fork();
+  if (pid < 0) {
+    fprintf(stderr, "fork failed: %d\n", errno);
+    return 1;
+  }
+  if (pid == 0) {
+    fprintf(stderr, "CHILD\n");
+    return 0;
+  }
+  if (pid != waitpid(pid, NULL, 0)) {
+    fprintf(stderr, "waitpid failed: %d\n", errno);
+    return 1;
+  }
+  pthread_join(t, NULL);
+  fprintf(stderr, "PARENT\n");
+  return 0;
+}
+
+// CHECK: in atfork
+// CHECK: in handler
+// CHECK: ThreadSanitizer: data race
+// CHECK:   Write of size 8
+// CHECK:     #0 handler
+// CHECK:   Previous write of size 8
+// CHECK:     #0 worker
+// CHECK: afterfork
+// CHECK: in handler
+// CHECK: afterfork_child
+// CHECK: CHILD
+// CHECK: PARENT