tsan: don't start background thread after clone

[dvyukov]

Summary

Start the background thread only after fork, but not after clone.
For fork we did this always and it's known to work (or user code has adopted).
But if we do this for the new clone interceptor some code (sandbox2) fails.
So model we used to do for years and don't start the background thread after clone.

[melver]

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

522–523

This comment block seems more appropriate above the call to this function in the clone interceptor.

[dvyukov]

move the comment to clone interceptor

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

void atfork_parent() {
   ForkParentAfter(thr, pc);
 }
 
 void atfork_child() {
   if (in_symbolizer())
     return;
   ThreadState *thr = cur_thread();
   const uptr pc = StackTrace::GetCurrentPc();
-  ForkChildAfter(thr, pc);
+  ForkChildAfter(thr, pc, true);
   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).
SCOPED_INTERCEPTOR_RAW(clone, fn, stack, flags, arg, parent_tid, tls,
                          child_tid);
   struct Arg {
     int (*fn)(void *);
     void *arg;
   };
   auto wrapper = +[](void *p) -> int {
     auto *thr = cur_thread();
     uptr pc = GET_CURRENT_PC();
-    ForkChildAfter(thr, pc);
+    // Start the background thread for fork, but not for clone.
+    // For fork we did this always and it's known to work (or user code has
+    // adopted). But if we do this for the new clone interceptor some code
+    // (sandbox2) fails. So model we used to do for years and don't start the
+    // background thread after clone.
+    ForkChildAfter(thr, pc, false);
     FdOnFork(thr, pc);
     auto *arg = static_cast<Arg *>(p);
     return arg->fn(arg->arg);
   };
   ForkBefore(thr, pc);
   Arg arg_wrapper = {fn, arg};
   int pid = REAL(clone)(wrapper, stack, flags, &arg_wrapper, parent_tid, tls,
                         child_tid);

 }
 
 static void syscall_pre_fork(uptr pc) { ForkBefore(cur_thread(), pc); }
 
 static void syscall_post_fork(uptr pc, int pid) {
   ThreadState *thr = cur_thread();
   if (pid == 0) {
     // child
-    ForkChildAfter(thr, pc);
+    ForkChildAfter(thr, pc, true);
     FdOnFork(thr, pc);
   } else if (pid > 0) {
     // parent
     ForkParentAfter(thr, pc);
   } else {
     // error
     ForkParentAfter(thr, pc);
   }

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

 void UnmapShadow(ThreadState *thr, uptr addr, uptr size);
 void InitializeShadowMemory();
 void InitializeInterceptors();
 void InitializeLibIgnore();
 void InitializeDynamicAnnotations();
 
 void ForkBefore(ThreadState *thr, uptr pc);
 void ForkParentAfter(ThreadState *thr, uptr pc);
-void ForkChildAfter(ThreadState *thr, uptr pc);
+void ForkChildAfter(ThreadState *thr, uptr pc, bool start_thread);
 
 void ReportRace(ThreadState *thr);
 bool OutputReport(ThreadState *thr, const ScopedReport &srep);
 bool IsFiredSuppression(Context *ctx, ReportType type, StackTrace trace);
 bool IsExpectedReport(uptr addr, uptr size);
 
 #if defined(TSAN_DEBUG_OUTPUT) && TSAN_DEBUG_OUTPUT >= 1
 # define DPrintf Printf

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

 void ForkParentAfter(ThreadState *thr, uptr pc) NO_THREAD_SAFETY_ANALYSIS {
   thr->suppress_reports--;  // Enabled in ForkBefore.
   thr->ignore_interceptors--;
   ScopedErrorReportLock::Unlock();
   ctx->report_mtx.Unlock();
   ctx->thread_registry.Unlock();
 }
 
-void ForkChildAfter(ThreadState *thr, uptr pc) NO_THREAD_SAFETY_ANALYSIS {
+void ForkChildAfter(ThreadState *thr, uptr pc,
+                    bool start_thread) NO_THREAD_SAFETY_ANALYSIS {
   thr->suppress_reports--;  // Enabled in ForkBefore.
   thr->ignore_interceptors--;
   ScopedErrorReportLock::Unlock();
   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) {
+    if (start_thread)
       StartBackgroundThread();
   } else {
     // We've just forked a multi-threaded process. We cannot reasonably function
     // after that (some mutexes may be locked before fork). So just enable
     // ignores for everything in the hope that we will exec soon.
     ctx->after_multithreaded_fork = true;
     thr->ignore_interceptors++;
     ThreadIgnoreBegin(thr, pc);
     ThreadIgnoreSyncBegin(thr, pc);

compiler-rt/test/tsan/Linux/clone_setns.cpp

+// RUN: %clangxx_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s
+
+// The test models how sandbox2 unshares user namespace after clone:
+// https://github.com/google/sandboxed-api/blob/c95837a6c131fbdf820db352a97d54fcbcbde6c0/sandboxed_api/sandbox2/forkserver.cc#L249
+// which works only in sigle-threaded processes.
+
+#include "../test.h"
+#include <errno.h>
+#include <sched.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+static int cloned(void *arg) {
+  if (unshare(CLONE_NEWUSER)) {
+    fprintf(stderr, "unshare failed: %d\n", errno);
+    exit(1);
+  }
+  exit(0);
+  return 0;
+}
+
+int main() {
+  char stack[64 << 10] __attribute__((aligned(64)));
+  int pid = clone(cloned, stack + sizeof(stack), SIGCHLD, 0);
+  if (pid == -1) {
+    fprintf(stderr, "failed to clone: %d\n", errno);
+    exit(1);
+  }
+  int status = 0;
+  while (wait(&status) != pid) {
+  }
+  if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
+    fprintf(stderr, "child failed: %d\n", status);
+    exit(1);
+  }
+  fprintf(stderr, "DONE\n");
+}
+
+// CHECK: DONE