https://github.com/vitalybuka updated https://github.com/llvm/llvm-project/pull/228614
>From ddf6b8a2fa2bb1c28d0f81736718c4e6c6b08e09 Mon Sep 17 00:00:00 2001 From: Vitaly Buka <[email protected]> Date: Fri, 2 Oct 2026 16:07:34 -0700 Subject: [PATCH] [TSan] Lock ScopedErrorReportLock before slot and thread_registry locks OutputReport runs while ScopedErrorReportLock is held after slot_mtx and thread_registry have been unlocked. Because code executed during OutputReport (symbolizer, callbacks, or signal handlers) can acquire slot_mtx or thread_registry, ScopedErrorReportLock must precede slot and thread_registry locks in the lock hierarchy to avoid AB-BA deadlocks between concurrent reports or fork(). - Move ScopedErrorReportLock::Lock() before slot.mtx, thread_registry, and slot_mtx in ForkBefore (and unlock in reverse order in ForkAfter). - Replace ctx->thread_registry.CheckLocked() in ScopedReportBase's constructor with CheckedMutex::CheckNoLocks(), and add CheckLocked() to AddThread(const ThreadContext *) and CheckNoLocks() to OutputReport. - Construct ScopedReport before acquiring ThreadRegistryLock across all reporting functions, and close the RestoreStack lock scope before constructing ScopedReport in ReportRace. Assisted-by: Gemini Pull Request: https://github.com/llvm/llvm-project/pull/228614 --- .../lib/tsan/rtl/tsan_interceptors_posix.cpp | 2 +- .../lib/tsan/rtl/tsan_interface_ann.cpp | 2 +- compiler-rt/lib/tsan/rtl/tsan_mman.cpp | 2 +- compiler-rt/lib/tsan/rtl/tsan_rtl.cpp | 4 +- compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp | 6 +-- compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp | 39 +++++++++++-------- compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp | 2 +- 7 files changed, 31 insertions(+), 26 deletions(-) diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp index b2ba666741e8918..a8d502966bccaf2 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp @@ -2182,8 +2182,8 @@ static void ReportErrnoSpoiling(ThreadState *thr, uptr pc, int sig) { // Release locks before symbolizing and outputting the report to avoid // deadlocks. { - ThreadRegistryLock l(&ctx->thread_registry); new (rep) ScopedReport(ReportTypeErrnoInSignal); + ThreadRegistryLock l(&ctx->thread_registry); rep->SetSigNum(sig); suppressed = IsFiredSuppression(ctx, ReportTypeErrnoInSignal, stack); if (!suppressed) diff --git a/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp b/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp index e520ead1d36aa4a..acd5066dee07bdd 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp @@ -448,8 +448,8 @@ static void ReportMutexHeldWrongContext(ThreadState *thr, uptr pc) { // Release locks before symbolizing and outputting the report to avoid // deadlocks. { - ThreadRegistryLock l(&ctx->thread_registry); new (rep) ScopedReport(ReportTypeMutexHeldWrongContext); + 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); diff --git a/compiler-rt/lib/tsan/rtl/tsan_mman.cpp b/compiler-rt/lib/tsan/rtl/tsan_mman.cpp index cea833054551d34..88cdf58af8d581f 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_mman.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_mman.cpp @@ -187,8 +187,8 @@ static void SignalUnsafeCall(ThreadState *thr, uptr pc) { // Release locks before symbolizing and outputting the report to avoid // deadlocks. { - ThreadRegistryLock l(&ctx->thread_registry); new (rep) ScopedReport(ReportTypeSignalUnsafe); + ThreadRegistryLock l(&ctx->thread_registry); rep->AddStack(stack, true); } OutputReport(thr, *rep); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp index c6e7dce5ecb1816..7328caab0a9186e 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp @@ -841,10 +841,10 @@ void ForkBefore(ThreadState* thr, uptr pc) SANITIZER_NO_THREAD_SAFETY_ANALYSIS { // Detaching from the slot makes OnUserFree skip writing to the shadow. // The slot will be locked so any attempts to use it will deadlock anyway. SlotDetach(thr); + ScopedErrorReportLock::Lock(); for (auto& slot : ctx->slots) slot.mtx.Lock(); ctx->thread_registry.Lock(); ctx->slot_mtx.Lock(); - ScopedErrorReportLock::Lock(); AllocatorLockBeforeFork(); // Suppress all reports in the pthread_atfork callbacks. // Reports may deadlock. @@ -871,10 +871,10 @@ static void ForkAfter(ThreadState* thr, thr->ignore_interceptors--; thr->ignore_reads_and_writes--; AllocatorUnlockAfterFork(child); - ScopedErrorReportLock::Unlock(); ctx->slot_mtx.Unlock(); ctx->thread_registry.Unlock(); for (auto& slot : ctx->slots) slot.mtx.Unlock(); + ScopedErrorReportLock::Unlock(); SlotAttachAndLock(thr); SlotUnlock(thr); GlobalProcessorUnlock(); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp index a09e3c11fd0da4e..a8c8e5cf44be409 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp @@ -63,8 +63,8 @@ static void ReportMutexMisuse(ThreadState *thr, uptr pc, ReportType typ, // Release locks before symbolizing and outputting the report to avoid // deadlocks. { - ThreadRegistryLock l(&ctx->thread_registry); new (rep) ScopedReport(typ); + ThreadRegistryLock l(&ctx->thread_registry); rep->AddMutex(addr, creation_stack_id); rep->AddStack(trace, true); rep->AddLocation(addr, 1); @@ -544,8 +544,8 @@ void ReportDeadlock(ThreadState *thr, uptr pc, DDReport *r) { // Release locks before symbolizing and outputting the report to avoid // deadlocks. { - ThreadRegistryLock l(&ctx->thread_registry); new (rep) ScopedReport(ReportTypeDeadlock); + 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); @@ -598,8 +598,8 @@ void ReportDestroyLocked(ThreadState *thr, uptr pc, uptr addr, // Release locks before symbolizing and outputting the report to avoid // deadlocks. { - ThreadRegistryLock l0(&ctx->thread_registry); new (rep) ScopedReport(ReportTypeMutexDestroyLocked); + ThreadRegistryLock l0(&ctx->thread_registry); rep->AddMutex(addr, creation_stack_id); rep->AddStack(trace, true); rep->AddStack(last_lock_stack, true); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp index be444eec3a4b785..7b86e955300de82 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp @@ -164,7 +164,7 @@ bool ShouldReport(ThreadState *thr, ReportType typ) { } ScopedReport::ScopedReport(ReportType typ, uptr tag) { - ctx->thread_registry.CheckLocked(); + CheckedMutex::CheckNoLocks(); rep_ = New<ReportDesc>(); rep_->typ = typ; rep_->tag = tag; @@ -256,6 +256,7 @@ void ScopedReport::AddUniqueTid(Tid unique_tid) { } void ScopedReport::AddThread(const ThreadContext* tctx, bool suppressable) { + ctx->thread_registry.CheckLocked(); for (uptr i = 0; i < rep_->threads.Size(); i++) { if ((u32)rep_->threads[i]->id == tctx->tid) return; @@ -671,6 +672,7 @@ static bool HandleRacyStacks(ThreadState *thr, VarSizeStackTrace traces[2]) { } bool OutputReport(ThreadState *thr, ScopedReport &srep) { + CheckedMutex::CheckNoLocks(); // These should have been checked in ShouldReport. // It's too late to check them here, we have already taken locks. CHECK(flags()->report_bugs); @@ -804,10 +806,6 @@ void ReportRace(ThreadState *thr, RawShadow *shadow_mem, Shadow cur, Shadow old, DynamicMutexSet mset1; MutexSet *mset[kMop] = {&thr->mset, mset1}; - // Use alloca, because malloc during signal handling deadlocks - ScopedReport *rep = (ScopedReport *)__builtin_alloca(sizeof(ScopedReport)); - // Release locks before symbolizing and outputting the report to avoid - // deadlocks. { // We need to lock the slot during RestoreStack because it protects // the slot journal. @@ -821,24 +819,31 @@ void ReportRace(ThreadState *thr, RawShadow *shadow_mem, Shadow cur, Shadow old, StoreShadow(&ctx->last_spurious_race, old.raw()); return; } + } - if (IsFiredSuppression(ctx, rep_typ, traces[1])) - return; + if (IsFiredSuppression(ctx, rep_typ, traces[1])) + return; - if (HandleRacyStacks(thr, traces)) - return; + if (HandleRacyStacks(thr, traces)) + return; - // If any of the accesses has a tag, treat this as an "external" race. - uptr tag = kExternalTagNone; - for (uptr i = 0; i < kMop; i++) { - if (tags[i] != kExternalTagNone) { - rep_typ = ReportTypeExternalRace; - tag = tags[i]; - break; - } + // If any of the accesses has a tag, treat this as an "external" race. + uptr tag = kExternalTagNone; + for (uptr i = 0; i < kMop; i++) { + if (tags[i] != kExternalTagNone) { + rep_typ = ReportTypeExternalRace; + tag = tags[i]; + break; } + } + // Use alloca, because malloc during signal handling deadlocks + ScopedReport* rep = (ScopedReport*)__builtin_alloca(sizeof(ScopedReport)); + // Release locks before symbolizing and outputting the report to avoid + // deadlocks. + { new (rep) ScopedReport(rep_typ, tag); + ThreadRegistryLock l0(&ctx->thread_registry); for (uptr i = 0; i < kMop; i++) rep->AddMemoryAccess(addr, tags[i], s[i], tids[i], traces[i], mset[i]); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp index afc6b8d4a0b1cdc..c408024e77e60d2 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp @@ -101,8 +101,8 @@ void ThreadFinalize(ThreadState *thr) { // Release locks before symbolizing and outputting the report to avoid // deadlocks. { - ThreadRegistryLock l(&ctx->thread_registry); new (rep) ScopedReport(ReportTypeThreadLeak); + ThreadRegistryLock l(&ctx->thread_registry); rep->AddThread(leaks[i].tctx, true); rep->SetCount(leaks[i].count); } _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
