Skip to content

Commit cb1f1a7

Browse files
DanBlackwellDanBlackwell
authored
[TSan] Fix deadlocks during TSan error reporting on Apple platforms (#151495)
Changes: Apple only: * Move OutputReport (and thus symbolization) after the locked region when reporting. * ScopedReportBase constructor requires the thread_registry lock to be held, so the regions where ScopedReports get created must be inside the locked region. * Bail early from MemoryAccess if we are symbolizing. Other platforms should behave as before, outputting reports while the locks are held. This is a follow up to llvm/llvm-project#151120, which delayed symbolization of ScopedReports. rdar://152829396
1 parent 5d54a57 commit cb1f1a7

File tree

7 files changed

+252
-125
lines changed

7 files changed

+252
-125
lines changed

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

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include "sanitizer_common/sanitizer_internal_defs.h"
2323
#include "sanitizer_common/sanitizer_libc.h"
2424
#include "sanitizer_common/sanitizer_linux.h"
25+
#include "sanitizer_common/sanitizer_placement_new.h"
2526
#include "sanitizer_common/sanitizer_platform_interceptors.h"
2627
#include "sanitizer_common/sanitizer_platform_limits_netbsd.h"
2728
#include "sanitizer_common/sanitizer_platform_limits_posix.h"
@@ -2141,13 +2142,29 @@ static void ReportErrnoSpoiling(ThreadState *thr, uptr pc, int sig) {
21412142
// StackTrace::GetNestInstructionPc(pc) is used because return address is
21422143
// expected, OutputReport() will undo this.
21432144
ObtainCurrentStack(thr, StackTrace::GetNextInstructionPc(pc), &stack);
2144-
ThreadRegistryLock l(&ctx->thread_registry);
2145-
ScopedReport rep(ReportTypeErrnoInSignal);
2146-
rep.SetSigNum(sig);
2147-
if (!IsFiredSuppression(ctx, ReportTypeErrnoInSignal, stack)) {
2148-
rep.AddStack(stack, true);
2149-
OutputReport(thr, rep);
2145+
// Use alloca, because malloc during signal handling deadlocks
2146+
ScopedReport *rep = (ScopedReport *)__builtin_alloca(sizeof(ScopedReport));
2147+
bool suppressed;
2148+
// Take a new scope as Apple platforms require the below locks released
2149+
// before symbolizing in order to avoid a deadlock
2150+
{
2151+
ThreadRegistryLock l(&ctx->thread_registry);
2152+
new (rep) ScopedReport(ReportTypeErrnoInSignal);
2153+
rep->SetSigNum(sig);
2154+
suppressed = IsFiredSuppression(ctx, ReportTypeErrnoInSignal, stack);
2155+
if (!suppressed)
2156+
rep->AddStack(stack, true);
2157+
#if SANITIZER_APPLE
2158+
} // Close this scope to release the locks before writing report
2159+
#endif
2160+
if (!suppressed)
2161+
OutputReport(thr, *rep);
2162+
2163+
// Need to manually destroy this because we used placement new to allocate
2164+
rep->~ScopedReport();
2165+
#if !SANITIZER_APPLE
21502166
}
2167+
#endif
21512168
}
21522169

21532170
static void CallUserSignalHandler(ThreadState *thr, bool sync, bool acquire,

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

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -437,16 +437,30 @@ void __tsan_mutex_post_divert(void *addr, unsigned flagz) {
437437
}
438438

439439
static void ReportMutexHeldWrongContext(ThreadState *thr, uptr pc) {
440-
ThreadRegistryLock l(&ctx->thread_registry);
441-
ScopedReport rep(ReportTypeMutexHeldWrongContext);
442-
for (uptr i = 0; i < thr->mset.Size(); ++i) {
443-
MutexSet::Desc desc = thr->mset.Get(i);
444-
rep.AddMutex(desc.addr, desc.stack_id);
440+
// Use alloca, because malloc during signal handling deadlocks
441+
ScopedReport *rep = (ScopedReport *)__builtin_alloca(sizeof(ScopedReport));
442+
// Take a new scope as Apple platforms require the below locks released
443+
// before symbolizing in order to avoid a deadlock
444+
{
445+
ThreadRegistryLock l(&ctx->thread_registry);
446+
new (rep) ScopedReport(ReportTypeMutexHeldWrongContext);
447+
for (uptr i = 0; i < thr->mset.Size(); ++i) {
448+
MutexSet::Desc desc = thr->mset.Get(i);
449+
rep->AddMutex(desc.addr, desc.stack_id);
450+
}
451+
VarSizeStackTrace trace;
452+
ObtainCurrentStack(thr, pc, &trace);
453+
rep->AddStack(trace, true);
454+
#if SANITIZER_APPLE
455+
} // Close this scope to release the locks
456+
#endif
457+
OutputReport(thr, *rep);
458+
459+
// Need to manually destroy this because we used placement new to allocate
460+
rep->~ScopedReport();
461+
#if !SANITIZER_APPLE
445462
}
446-
VarSizeStackTrace trace;
447-
ObtainCurrentStack(thr, pc, &trace);
448-
rep.AddStack(trace, true);
449-
OutputReport(thr, rep);
463+
#endif
450464
}
451465

452466
INTERFACE_ATTRIBUTE

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

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -182,10 +182,24 @@ static void SignalUnsafeCall(ThreadState *thr, uptr pc) {
182182
ObtainCurrentStack(thr, pc, &stack);
183183
if (IsFiredSuppression(ctx, ReportTypeSignalUnsafe, stack))
184184
return;
185-
ThreadRegistryLock l(&ctx->thread_registry);
186-
ScopedReport rep(ReportTypeSignalUnsafe);
187-
rep.AddStack(stack, true);
188-
OutputReport(thr, rep);
185+
// Use alloca, because malloc during signal handling deadlocks
186+
ScopedReport *rep = (ScopedReport *)__builtin_alloca(sizeof(ScopedReport));
187+
// Take a new scope as Apple platforms require the below locks released
188+
// before symbolizing in order to avoid a deadlock
189+
{
190+
ThreadRegistryLock l(&ctx->thread_registry);
191+
new (rep) ScopedReport(ReportTypeSignalUnsafe);
192+
rep->AddStack(stack, true);
193+
#if SANITIZER_APPLE
194+
} // Close this scope to release the locks
195+
#endif
196+
OutputReport(thr, *rep);
197+
198+
// Need to manually destroy this because we used placement new to allocate
199+
rep->~ScopedReport();
200+
#if !SANITIZER_APPLE
201+
}
202+
#endif
189203
}
190204

191205

‎compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,11 @@ NOINLINE void TraceRestartMemoryAccess(ThreadState* thr, uptr pc, uptr addr,
419419

420420
ALWAYS_INLINE USED void MemoryAccess(ThreadState* thr, uptr pc, uptr addr,
421421
uptr size, AccessType typ) {
422+
#if SANITIZER_APPLE && !SANITIZER_GO
423+
// Swift symbolizer can be intercepted and deadlock without this
424+
if (thr->in_symbolizer)
425+
return;
426+
#endif
422427
RawShadow* shadow_mem = MemToShadow(addr);
423428
UNUSED char memBuf[4][64];
424429
DPrintf2("#%d: Access: %d@%d %p/%zd typ=0x%x {%s, %s, %s, %s}\n", thr->tid,

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

Lines changed: 94 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,15 @@
1111
//===----------------------------------------------------------------------===//
1212

1313
#include <sanitizer_common/sanitizer_deadlock_detector_interface.h>
14+
#include <sanitizer_common/sanitizer_placement_new.h>
1415
#include <sanitizer_common/sanitizer_stackdepot.h>
1516

16-
#include "tsan_rtl.h"
1717
#include "tsan_flags.h"
18-
#include "tsan_sync.h"
18+
#include "tsan_platform.h"
1919
#include "tsan_report.h"
20+
#include "tsan_rtl.h"
2021
#include "tsan_symbolize.h"
21-
#include "tsan_platform.h"
22+
#include "tsan_sync.h"
2223

2324
namespace __tsan {
2425

@@ -55,14 +56,28 @@ static void ReportMutexMisuse(ThreadState *thr, uptr pc, ReportType typ,
5556
return;
5657
if (!ShouldReport(thr, typ))
5758
return;
58-
ThreadRegistryLock l(&ctx->thread_registry);
59-
ScopedReport rep(typ);
60-
rep.AddMutex(addr, creation_stack_id);
61-
VarSizeStackTrace trace;
62-
ObtainCurrentStack(thr, pc, &trace);
63-
rep.AddStack(trace, true);
64-
rep.AddLocation(addr, 1);
65-
OutputReport(thr, rep);
59+
// Use alloca, because malloc during signal handling deadlocks
60+
ScopedReport *rep = (ScopedReport *)__builtin_alloca(sizeof(ScopedReport));
61+
// Take a new scope as Apple platforms require the below locks released
62+
// before symbolizing in order to avoid a deadlock
63+
{
64+
ThreadRegistryLock l(&ctx->thread_registry);
65+
new (rep) ScopedReport(typ);
66+
rep->AddMutex(addr, creation_stack_id);
67+
VarSizeStackTrace trace;
68+
ObtainCurrentStack(thr, pc, &trace);
69+
rep->AddStack(trace, true);
70+
rep->AddLocation(addr, 1);
71+
#if SANITIZER_APPLE
72+
} // Close this scope to release the locks
73+
#endif
74+
OutputReport(thr, *rep);
75+
76+
// Need to manually destroy this because we used placement new to allocate
77+
rep->~ScopedReport();
78+
#if !SANITIZER_APPLE
79+
}
80+
#endif
6681
}
6782

6883
static void RecordMutexLock(ThreadState *thr, uptr pc, uptr addr,
@@ -528,53 +543,81 @@ void AfterSleep(ThreadState *thr, uptr pc) {
528543
void ReportDeadlock(ThreadState *thr, uptr pc, DDReport *r) {
529544
if (r == 0 || !ShouldReport(thr, ReportTypeDeadlock))
530545
return;
531-
ThreadRegistryLock l(&ctx->thread_registry);
532-
ScopedReport rep(ReportTypeDeadlock);
533-
for (int i = 0; i < r->n; i++) {
534-
rep.AddMutex(r->loop[i].mtx_ctx0, r->loop[i].stk[0]);
535-
rep.AddUniqueTid((int)r->loop[i].thr_ctx);
536-
rep.AddThread((int)r->loop[i].thr_ctx);
537-
}
538-
uptr dummy_pc = 0x42;
539-
for (int i = 0; i < r->n; i++) {
540-
for (int j = 0; j < (flags()->second_deadlock_stack ? 2 : 1); j++) {
541-
u32 stk = r->loop[i].stk[j];
542-
StackTrace stack;
543-
if (stk && stk != kInvalidStackID) {
544-
stack = StackDepotGet(stk);
545-
} else {
546-
// Sometimes we fail to extract the stack trace (FIXME: investigate),
547-
// but we should still produce some stack trace in the report.
548-
stack = StackTrace(&dummy_pc, 1);
546+
// Use alloca, because malloc during signal handling deadlocks
547+
ScopedReport *rep = (ScopedReport *)__builtin_alloca(sizeof(ScopedReport));
548+
// Take a new scope as Apple platforms require the below locks released
549+
// before symbolizing in order to avoid a deadlock
550+
{
551+
ThreadRegistryLock l(&ctx->thread_registry);
552+
new (rep) ScopedReport(ReportTypeDeadlock);
553+
for (int i = 0; i < r->n; i++) {
554+
rep->AddMutex(r->loop[i].mtx_ctx0, r->loop[i].stk[0]);
555+
rep->AddUniqueTid((int)r->loop[i].thr_ctx);
556+
rep->AddThread((int)r->loop[i].thr_ctx);
557+
}
558+
uptr dummy_pc = 0x42;
559+
for (int i = 0; i < r->n; i++) {
560+
for (int j = 0; j < (flags()->second_deadlock_stack ? 2 : 1); j++) {
561+
u32 stk = r->loop[i].stk[j];
562+
StackTrace stack;
563+
if (stk && stk != kInvalidStackID) {
564+
stack = StackDepotGet(stk);
565+
} else {
566+
// Sometimes we fail to extract the stack trace (FIXME: investigate),
567+
// but we should still produce some stack trace in the report.
568+
stack = StackTrace(&dummy_pc, 1);
569+
}
570+
rep->AddStack(stack, true);
549571
}
550-
rep.AddStack(stack, true);
551572
}
573+
#if SANITIZER_APPLE
574+
} // Close this scope to release the locks
575+
#endif
576+
OutputReport(thr, *rep);
577+
578+
// Need to manually destroy this because we used placement new to allocate
579+
rep->~ScopedReport();
580+
#if !SANITIZER_APPLE
552581
}
553-
OutputReport(thr, rep);
582+
#endif
554583
}
555584

556585
void ReportDestroyLocked(ThreadState *thr, uptr pc, uptr addr,
557586
FastState last_lock, StackID creation_stack_id) {
558-
// We need to lock the slot during RestoreStack because it protects
559-
// the slot journal.
560-
Lock slot_lock(&ctx->slots[static_cast<uptr>(last_lock.sid())].mtx);
561-
ThreadRegistryLock l0(&ctx->thread_registry);
562-
Lock slots_lock(&ctx->slot_mtx);
563-
ScopedReport rep(ReportTypeMutexDestroyLocked);
564-
rep.AddMutex(addr, creation_stack_id);
565-
VarSizeStackTrace trace;
566-
ObtainCurrentStack(thr, pc, &trace);
567-
rep.AddStack(trace, true);
568-
569-
Tid tid;
570-
DynamicMutexSet mset;
571-
uptr tag;
572-
if (!RestoreStack(EventType::kLock, last_lock.sid(), last_lock.epoch(), addr,
573-
0, kAccessWrite, &tid, &trace, mset, &tag))
574-
return;
575-
rep.AddStack(trace, true);
576-
rep.AddLocation(addr, 1);
577-
OutputReport(thr, rep);
587+
// Use alloca, because malloc during signal handling deadlocks
588+
ScopedReport *rep = (ScopedReport *)__builtin_alloca(sizeof(ScopedReport));
589+
// Take a new scope as Apple platforms require the below locks released
590+
// before symbolizing in order to avoid a deadlock
591+
{
592+
// We need to lock the slot during RestoreStack because it protects
593+
// the slot journal.
594+
Lock slot_lock(&ctx->slots[static_cast<uptr>(last_lock.sid())].mtx);
595+
ThreadRegistryLock l0(&ctx->thread_registry);
596+
Lock slots_lock(&ctx->slot_mtx);
597+
new (rep) ScopedReport(ReportTypeMutexDestroyLocked);
598+
rep->AddMutex(addr, creation_stack_id);
599+
VarSizeStackTrace trace;
600+
ObtainCurrentStack(thr, pc, &trace);
601+
rep->AddStack(trace, true);
602+
603+
Tid tid;
604+
DynamicMutexSet mset;
605+
uptr tag;
606+
if (!RestoreStack(EventType::kLock, last_lock.sid(), last_lock.epoch(),
607+
addr, 0, kAccessWrite, &tid, &trace, mset, &tag))
608+
return;
609+
rep->AddStack(trace, true);
610+
rep->AddLocation(addr, 1);
611+
#if SANITIZER_APPLE
612+
} // Close this scope to release the locks
613+
#endif
614+
OutputReport(thr, *rep);
615+
616+
// Need to manually destroy this because we used placement new to allocate
617+
rep->~ScopedReport();
618+
#if !SANITIZER_APPLE
619+
}
620+
#endif
578621
}
579622

580623
} // namespace __tsan

0 commit comments

Comments
 (0)