Merge pull request #11706 from DanBlackwell/pick-tsan-lock-during-write

[🍒][TSan] Add in lock_during_write flag on Apple platforms to avoid deadlock

Author: DanBlackwell

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

@@ -105,6 +105,12 @@ extern "C" {
     mach_msg_type_number_t *infoCnt);
 }
 
+#  if !SANITIZER_GO
+// Weak symbol no-op when TSan is not linked
+SANITIZER_WEAK_ATTRIBUTE extern void __tsan_set_in_internal_write_call(
+    bool value) {}
+#  endif
+
 namespace __sanitizer {
 
 #include "sanitizer_syscall_generic.inc"
@@ -175,7 +181,15 @@ uptr internal_read(fd_t fd, void *buf, uptr count) {
 }
 
 uptr internal_write(fd_t fd, const void *buf, uptr count) {
+#  if SANITIZER_GO
   return write(fd, buf, count);
+#  else
+  // We need to disable interceptors when writing in TSan
+  __tsan_set_in_internal_write_call(true);
+  uptr res = write(fd, buf, count);
+  __tsan_set_in_internal_write_call(false);
+  return res;
+#  endif
 }
 
 uptr internal_stat(const char *path, void *buf) {

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

@@ -20,6 +20,43 @@
 #include "tsan_rtl.h"
 #include "ubsan/ubsan_flags.h"
 
+#if SANITIZER_APPLE && !SANITIZER_GO
+namespace __sanitizer {
+
+template <>
+inline bool FlagHandler<LockDuringWriteSetting>::Parse(const char *value) {
+  if (internal_strcmp(value, "on") == 0) {
+    *t_ = kLockDuringAllWrites;
+    return true;
+  }
+  if (internal_strcmp(value, "disable_for_current_process") == 0) {
+    *t_ = kNoLockDuringWritesCurrentProcess;
+    return true;
+  }
+  if (internal_strcmp(value, "disable_for_all_processes") == 0) {
+    *t_ = kNoLockDuringWritesAllProcesses;
+    return true;
+  }
+  Printf("ERROR: Invalid value for signal handler option: '%s'\n", value);
+  return false;
+}
+
+template <>
+inline bool FlagHandler<LockDuringWriteSetting>::Format(char *buffer,
+                                                        uptr size) {
+  switch (*t_) {
+    case kLockDuringAllWrites:
+      return FormatString(buffer, size, "on");
+    case kNoLockDuringWritesCurrentProcess:
+      return FormatString(buffer, size, "disable_for_current_process");
+    case kNoLockDuringWritesAllProcesses:
+      return FormatString(buffer, size, "disable_for_all_processes");
+  }
+}
+
+}  // namespace __sanitizer
+#endif  // SANITIZER_APPLE && !SANITIZER_GO
+
 namespace __tsan {
 
 // Can be overriden in frontend.

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

@@ -16,6 +16,14 @@
 #include "sanitizer_common/sanitizer_flags.h"
 #include "sanitizer_common/sanitizer_deadlock_detector_interface.h"
 
+#if SANITIZER_APPLE && !SANITIZER_GO
+enum LockDuringWriteSetting {
+  kLockDuringAllWrites,
+  kNoLockDuringWritesCurrentProcess,
+  kNoLockDuringWritesAllProcesses,
+};
+#endif
+
 namespace __tsan {
 
 struct Flags : DDFlags {

compiler-rt/lib/tsan/rtl/tsan_flags.inc

@@ -80,3 +80,15 @@ TSAN_FLAG(bool, shared_ptr_interceptor, true,
 TSAN_FLAG(bool, print_full_thread_history, false,
           "If set, prints thread creation stacks for the threads involved in "
           "the report and their ancestors up to the main thread.")
+
+#if SANITIZER_APPLE && !SANITIZER_GO
+TSAN_FLAG(LockDuringWriteSetting, lock_during_write, kLockDuringAllWrites,
+          "Determines whether to obtain a lock while writing logs or error "
+          "reports. "
+          "\"on\" - [default] lock during all writes. "
+          "\"disable_for_current_process\" - don't lock during all writes in "
+          "the current process, but do lock for all writes in child "
+          "processes."
+          "\"disable_for_all_processes\" - don't lock during all writes in "
+          "the current process and it's children processes.")
+#endif

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

@@ -1,6 +1,9 @@
 #ifndef TSAN_INTERCEPTORS_H
 #define TSAN_INTERCEPTORS_H
 
+#if SANITIZER_APPLE && !SANITIZER_GO
+#  include "sanitizer_common/sanitizer_mac.h"
+#endif
 #include "sanitizer_common/sanitizer_stacktrace.h"
 #include "tsan_rtl.h"
 
@@ -43,7 +46,12 @@ inline bool in_symbolizer() {
 #endif
 
 inline bool MustIgnoreInterceptor(ThreadState *thr) {
-  return !thr->is_inited || thr->ignore_interceptors || thr->in_ignored_lib;
+  return !thr->is_inited || thr->ignore_interceptors || thr->in_ignored_lib
+#if SANITIZER_APPLE && !SANITIZER_GO
+         || (flags()->lock_during_write != kLockDuringAllWrites &&
+             thr->in_internal_write_call)
+#endif
+      ;
 }
 
 }  // namespace __tsan

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

@@ -31,6 +31,9 @@
 #include "sanitizer_common/sanitizer_tls_get_addr.h"
 #include "sanitizer_common/sanitizer_vector.h"
 #include "tsan_fd.h"
+#if SANITIZER_APPLE && !SANITIZER_GO
+#  include "tsan_flags.h"
+#endif
 #include "tsan_interceptors.h"
 #include "tsan_interface.h"
 #include "tsan_mman.h"
@@ -1665,6 +1668,14 @@ TSAN_INTERCEPTOR(int, pthread_barrier_wait, void *b) {
 
 TSAN_INTERCEPTOR(int, pthread_once, void *o, void (*f)()) {
   SCOPED_INTERCEPTOR_RAW(pthread_once, o, f);
+#if SANITIZER_APPLE && !SANITIZER_GO
+  if (flags()->lock_during_write != kLockDuringAllWrites &&
+      cur_thread_init()->in_internal_write_call) {
+    // This is needed to make it through process launch without hanging
+    f();
+    return 0;
+  }
+#endif
   if (o == 0 || f == 0)
     return errno_EINVAL;
   atomic_uint32_t *a;

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

@@ -40,6 +40,13 @@ SANITIZER_WEAK_DEFAULT_IMPL
 void __tsan_test_only_on_fork() {}
 #endif
 
+#if SANITIZER_APPLE && !SANITIZER_GO
+// Override weak symbol from sanitizer_common
+extern void __tsan_set_in_internal_write_call(bool value) {
+  __tsan::cur_thread_init()->in_internal_write_call = value;
+}
+#endif
+
 namespace __tsan {
 
 #if !SANITIZER_GO
@@ -893,6 +900,13 @@ void ForkChildAfter(ThreadState* thr, uptr pc, bool start_thread) {
     ThreadIgnoreBegin(thr, pc);
     ThreadIgnoreSyncBegin(thr, pc);
   }
+
+#  if SANITIZER_APPLE && !SANITIZER_GO
+  // This flag can have inheritance disabled - we are the child so act
+  // accordingly
+  if (flags()->lock_during_write == kNoLockDuringWritesCurrentProcess)
+    flags()->lock_during_write = kLockDuringAllWrites;
+#  endif
 }
 #endif
 

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

@@ -236,6 +236,10 @@ struct alignas(SANITIZER_CACHE_LINE_SIZE) ThreadState {
 
   const ReportDesc *current_report;
 
+#if SANITIZER_APPLE && !SANITIZER_GO
+  bool in_internal_write_call;
+#endif
+
   explicit ThreadState(Tid tid);
 };
 

compiler-rt/test/tsan/Darwin/write-interpose.c

@@ -0,0 +1,52 @@
+// Test that dylibs interposing write, and then calling functions intercepted
+// by TSan don't deadlock (self-lock)
+
+// RUN: %clang_tsan %s -o %t
+// RUN: %clang_tsan %s -o %t.dylib -fno-sanitize=thread -dynamiclib -DSHARED_LIB
+
+// Note that running the below command with out `lock_during_write` should
+// deadlock (self-lock)
+// RUN: env DYLD_INSERT_LIBRARIES=%t.dylib TSAN_OPTIONS=verbosity=2:lock_during_write=disable_for_current_process %run %t 2>&1 | FileCheck %s
+//
+// UNSUPPORTED: ios
+
+#include <stdio.h>
+
+#if defined(SHARED_LIB)
+
+// dylib implementation - interposes write() calls
+#  include <os/lock.h>
+#  include <unistd.h>
+
+struct interpose_substitution {
+  const void *replacement;
+  const void *original;
+};
+
+#  define INTERPOSE(replacement, original)                                     \
+    __attribute__((used)) static const struct interpose_substitution           \
+        substitution_##original[]                                              \
+        __attribute__((section("__DATA, __interpose"))) = {                    \
+            {(const void *)(replacement), (const void *)(original)}}
+
+static ssize_t my_write(int fd, const void *buf, size_t count) {
+  struct os_unfair_lock_s lock = OS_UNFAIR_LOCK_INIT;
+  os_unfair_lock_lock(&lock);
+  printf("Interposed write called: fd=%d, count=%zu\n", fd, count);
+  ssize_t res = write(fd, buf, count);
+  os_unfair_lock_unlock(&lock);
+  return res;
+}
+INTERPOSE(my_write, write);
+
+#else // defined(SHARED_LIB)
+
+int main() {
+  printf("Write test completed\n");
+  return 0;
+}
+
+#endif // defined(SHARED_LIB)
+
+// CHECK: Interposed write called: fd={{[0-9]+}}, count={{[0-9]+}}
+// CHECK: Write test completed