[compiler-rt] e91595b - tsan: don't start background thread after clone

[Dmitry Vyukov via llvm-commits]

Author: Dmitry Vyukov
Date: 2021-11-12T12:58:49+01:00
New Revision: e91595bf948a23bb44a47854321aa80307a48fd1

URL: https://github.com/llvm/llvm-project/commit/e91595bf948a23bb44a47854321aa80307a48fd1
DIFF: https://github.com/llvm/llvm-project/commit/e91595bf948a23bb44a47854321aa80307a48fd1.diff

LOG: tsan: don't start background thread after clone

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.

Reviewed By: melver

Differential Revision: https://reviews.llvm.org/D113744

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

Modified: 
    compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
    compiler-rt/lib/tsan/rtl/tsan_rtl.cpp
    compiler-rt/lib/tsan/rtl/tsan_rtl.h

Removed: 
    


################################################################################
diff  --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
index c7dcd07b8529..9a85ee00d2d7 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
@@ -2189,7 +2189,7 @@ void atfork_child() {
     return;
   ThreadState *thr = cur_thread();
   const uptr pc = StackTrace::GetCurrentPc();
-  ForkChildAfter(thr, pc);
+  ForkChildAfter(thr, pc, true);
   FdOnFork(thr, pc);
 }
 
@@ -2222,7 +2222,12 @@ TSAN_INTERCEPTOR(int, clone, int (*fn)(void *), void *stack, int flags,
   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);
@@ -2570,7 +2575,7 @@ 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

diff  --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp
index 6e57d4aeb093..46dec04b8759 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp
@@ -506,7 +506,8 @@ void ForkParentAfter(ThreadState *thr, uptr pc) NO_THREAD_SAFETY_ANALYSIS {
   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();
@@ -518,7 +519,8 @@ void ForkChildAfter(ThreadState *thr, uptr pc) NO_THREAD_SAFETY_ANALYSIS {
   VPrintf(1, "ThreadSanitizer: forked new process with pid %d,"
       " parent had %d threads\n", (int)internal_getpid(), (int)nthread);
   if (nthread == 1) {
-    StartBackgroundThread();
+    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

diff  --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.h b/compiler-rt/lib/tsan/rtl/tsan_rtl.h
index 089144c17ff0..eab83704240d 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_rtl.h
+++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.h
@@ -440,7 +440,7 @@ 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);

diff  --git a/compiler-rt/test/tsan/Linux/clone_setns.cpp b/compiler-rt/test/tsan/Linux/clone_setns.cpp
new file mode 100644
index 000000000000..25e30345cce9
--- /dev/null
+++ b/compiler-rt/test/tsan/Linux/clone_setns.cpp
@@ -0,0 +1,39 @@
+// 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