[tsan] Fix deadlock with dyld during symbolization on darwin platforms

On darwin platforms, callbacks registered via `_dyld_register_func_for_add_image`
are invoked with a lock held by dyld. These callbacks, in turn, request locks in
the TSan runtime due to instrumented codes or interceptors. Previously, reporting
race issues involved holding TSan runtime locks and simultaneously attemping to
acquire the dyld lock during symbolization, which leads to deadlock.

This commit restructures the reporting process into 3 distinct steps: data
collection, symbolization and printing. Each step now only holds the necessary
locks to prevent the deadlock.

Author: pudge62

compiler-rt/lib/tsan/go/tsan_go.cpp

@@ -118,6 +118,26 @@ ReportLocation *SymbolizeData(uptr addr) {
   }
 }
 
+bool SymbolizeData(uptr addr, DataInfo *info) {
+  SymbolizeDataContext cbctx;
+  internal_memset(&cbctx, 0, sizeof(cbctx));
+  cbctx.addr = addr;
+  go_runtime_cb(CallbackSymbolizeData, &cbctx);
+  if (!cbctx.res)
+    return false;
+  if (cbctx.heap) {
+    return false;
+  } else {
+    info->Clear();
+    info->name = internal_strdup(cbctx.name ? cbctx.name : "??");
+    info->file = internal_strdup(cbctx.file ? cbctx.file : "??");
+    info->line = cbctx.line;
+    info->start = cbctx.start;
+    info->size = cbctx.size;
+    return true;
+  }
+}
+
 static ThreadState *main_thr;
 static bool inited;
 

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

@@ -2064,13 +2064,17 @@ static void ReportErrnoSpoiling(ThreadState *thr, uptr pc, int sig) {
   // 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);
-  rep.SetSigNum(sig);
-  if (!IsFiredSuppression(ctx, ReportTypeErrnoInSignal, stack)) {
+  if (IsFiredSuppression(ctx, ReportTypeErrnoInSignal, stack)) {
+    return;
+  }
+  ScopedReport rep(ReportTypeErrnoInSignal); 
+  {
+    // ThreadRegistryLock l(&ctx->thread_registry);
+    rep.SetSigNum(sig);
     rep.AddStack(stack, true);
-    OutputReport(thr, rep);
   }
+  rep.SymbolizeReport();
+  OutputReport(thr, rep);
 }
 
 static void CallUserSignalHandler(ThreadState *thr, bool sync, bool acquire,

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

@@ -437,15 +437,18 @@ void __tsan_mutex_post_divert(void *addr, unsigned flagz) {
 }
 
 static void ReportMutexHeldWrongContext(ThreadState *thr, uptr pc) {
-  ThreadRegistryLock l(&ctx->thread_registry);
   ScopedReport rep(ReportTypeMutexHeldWrongContext);
-  for (uptr i = 0; i < thr->mset.Size(); ++i) {
-    MutexSet::Desc desc = thr->mset.Get(i);
-    rep.AddMutex(desc.addr, desc.stack_id);
+  {
+    ThreadRegistryLock l(&ctx->thread_registry);
+    for (uptr i = 0; i < thr->mset.Size(); ++i) {
+      MutexSet::Desc desc = thr->mset.Get(i);
+      rep.AddMutex(desc.addr, desc.stack_id);
+    }
+    VarSizeStackTrace trace;
+    ObtainCurrentStack(thr, pc, &trace);
+    rep.AddStack(trace, true);
   }
-  VarSizeStackTrace trace;
-  ObtainCurrentStack(thr, pc, &trace);
-  rep.AddStack(trace, true);
+  rep.SymbolizeReport();
   OutputReport(thr, rep);
 }
 

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

@@ -182,9 +182,12 @@ static void SignalUnsafeCall(ThreadState *thr, uptr pc) {
   ObtainCurrentStack(thr, pc, &stack);
   if (IsFiredSuppression(ctx, ReportTypeSignalUnsafe, stack))
     return;
-  ThreadRegistryLock l(&ctx->thread_registry);
   ScopedReport rep(ReportTypeSignalUnsafe);
-  rep.AddStack(stack, true);
+  {
+    // ThreadRegistryLock l(&ctx->thread_registry);
+    rep.AddStack(stack, true);
+  }
+  rep.SymbolizeReport();
   OutputReport(thr, rep);
 }
 

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

@@ -16,6 +16,7 @@
 #include "sanitizer_common/sanitizer_thread_registry.h"
 #include "sanitizer_common/sanitizer_vector.h"
 #include "tsan_defs.h"
+#include "tsan_stack_trace.h"
 
 namespace __tsan {
 
@@ -39,6 +40,7 @@ enum ReportType {
 };
 
 struct ReportStack {
+  VarSizeStackTrace raw;
   SymbolizedStack *frames = nullptr;
   bool suppressable = false;
 };

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

@@ -406,6 +406,7 @@ uptr TagFromShadowStackFrame(uptr pc);
 
 class ScopedReportBase {
  public:
+  void SetKindAndTag(ReportType typ, uptr tag);
   void AddMemoryAccess(uptr addr, uptr external_tag, Shadow s, Tid tid,
                        StackTrace stack, const MutexSet *mset);
   void AddStack(StackTrace stack, bool suppressable = false);
@@ -418,29 +419,26 @@ class ScopedReportBase {
   void SetCount(int count);
   void SetSigNum(int sig);
 
+  void SymbolizeReport();
   const ReportDesc *GetReport() const;
 
  protected:
+  ScopedReportBase();
   ScopedReportBase(ReportType typ, uptr tag);
   ~ScopedReportBase();
 
  private:
   ReportDesc *rep_;
-  // Symbolizer makes lots of intercepted calls. If we try to process them,
-  // at best it will cause deadlocks on internal mutexes.
-  ScopedIgnoreInterceptors ignore_interceptors_;
 
   ScopedReportBase(const ScopedReportBase &) = delete;
   void operator=(const ScopedReportBase &) = delete;
 };
 
 class ScopedReport : public ScopedReportBase {
  public:
+  explicit ScopedReport();
   explicit ScopedReport(ReportType typ, uptr tag = kExternalTagNone);
   ~ScopedReport();
-
- private:
-  ScopedErrorReportLock lock_;
 };
 
 bool ShouldReport(ThreadState *thr, ReportType typ);

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

@@ -55,13 +55,16 @@ static void ReportMutexMisuse(ThreadState *thr, uptr pc, ReportType typ,
     return;
   if (!ShouldReport(thr, typ))
     return;
-  ThreadRegistryLock l(&ctx->thread_registry);
   ScopedReport rep(typ);
-  rep.AddMutex(addr, creation_stack_id);
-  VarSizeStackTrace trace;
-  ObtainCurrentStack(thr, pc, &trace);
-  rep.AddStack(trace, true);
-  rep.AddLocation(addr, 1);
+  {
+    ThreadRegistryLock l(&ctx->thread_registry);
+    rep.AddMutex(addr, creation_stack_id);
+    VarSizeStackTrace trace;
+    ObtainCurrentStack(thr, pc, &trace);
+    rep.AddStack(trace, true);
+    rep.AddLocation(addr, 1);
+  }
+  rep.SymbolizeReport();
   OutputReport(thr, rep);
 }
 
@@ -528,50 +531,56 @@ void AfterSleep(ThreadState *thr, uptr pc) {
 void ReportDeadlock(ThreadState *thr, uptr pc, DDReport *r) {
   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, r->loop[i].stk[0]);
-    rep.AddUniqueTid((int)r->loop[i].thr_ctx);
-    rep.AddThread((int)r->loop[i].thr_ctx);
-  }
-  uptr dummy_pc = 0x42;
-  for (int i = 0; i < r->n; i++) {
-    for (int j = 0; j < (flags()->second_deadlock_stack ? 2 : 1); j++) {
-      u32 stk = r->loop[i].stk[j];
-      if (stk && stk != kInvalidStackID) {
-        rep.AddStack(StackDepotGet(stk), true);
-      } else {
-        // Sometimes we fail to extract the stack trace (FIXME: investigate),
-        // but we should still produce some stack trace in the report.
-        rep.AddStack(StackTrace(&dummy_pc, 1), true);
+  {
+    ThreadRegistryLock l(&ctx->thread_registry);
+    for (int i = 0; i < r->n; i++) {
+      rep.AddMutex(r->loop[i].mtx_ctx0, r->loop[i].stk[0]);
+      rep.AddUniqueTid((int)r->loop[i].thr_ctx);
+      rep.AddThread((int)r->loop[i].thr_ctx);
+    }
+    uptr dummy_pc = 0x42;
+    for (int i = 0; i < r->n; i++) {
+      for (int j = 0; j < (flags()->second_deadlock_stack ? 2 : 1); j++) {
+        u32 stk = r->loop[i].stk[j];
+        if (stk && stk != kInvalidStackID) {
+          rep.AddStack(StackDepotGet(stk), true);
+        } else {
+          // Sometimes we fail to extract the stack trace (FIXME: investigate),
+          // but we should still produce some stack trace in the report.
+          rep.AddStack(StackTrace(&dummy_pc, 1), true);
+        }
       }
     }
   }
+  rep.SymbolizeReport();
   OutputReport(thr, rep);
 }
 
 void ReportDestroyLocked(ThreadState *thr, uptr pc, uptr addr,
                          FastState last_lock, StackID creation_stack_id) {
-  // We need to lock the slot during RestoreStack because it protects
-  // the slot journal.
-  Lock slot_lock(&ctx->slots[static_cast<uptr>(last_lock.sid())].mtx);
-  ThreadRegistryLock l0(&ctx->thread_registry);
-  Lock slots_lock(&ctx->slot_mtx);
   ScopedReport rep(ReportTypeMutexDestroyLocked);
-  rep.AddMutex(addr, creation_stack_id);
-  VarSizeStackTrace trace;
-  ObtainCurrentStack(thr, pc, &trace);
-  rep.AddStack(trace, true);
-
-  Tid tid;
-  DynamicMutexSet mset;
-  uptr tag;
-  if (!RestoreStack(EventType::kLock, last_lock.sid(), last_lock.epoch(), addr,
-                    0, kAccessWrite, &tid, &trace, mset, &tag))
-    return;
-  rep.AddStack(trace, true);
-  rep.AddLocation(addr, 1);
+  {
+    // We need to lock the slot during RestoreStack because it protects
+    // the slot journal.
+    Lock slot_lock(&ctx->slots[static_cast<uptr>(last_lock.sid())].mtx);
+    ThreadRegistryLock l0(&ctx->thread_registry);
+    Lock slots_lock(&ctx->slot_mtx);
+    rep.AddMutex(addr, creation_stack_id);
+    VarSizeStackTrace trace;
+    ObtainCurrentStack(thr, pc, &trace);
+    rep.AddStack(trace, true);
+
+    Tid tid;
+    DynamicMutexSet mset;
+    uptr tag;
+    if (!RestoreStack(EventType::kLock, last_lock.sid(), last_lock.epoch(), addr,
+                      0, kAccessWrite, &tid, &trace, mset, &tag))
+      return;
+    rep.AddStack(trace, true);
+    rep.AddLocation(addr, 1);
+  }
+  rep.SymbolizeReport();
   OutputReport(thr, rep);
 }