Skip to content

Commit

Permalink
[LeakSanitizer] Capture calling thread SP early to avoid false negati…
Browse files Browse the repository at this point in the history
…ves.

As shown in #42932 dead
pointers might be overlapped by a new stack frame inside CheckForLeaks,
which does not use bytes with pointers. This leads to false negatives.

It's not a full solution for the problem as it does not solve
"overlapping" new/old frames for frames below the CheckForLeaks and in
other threads. It should improve leaks found in direct callers of
__lsan_do_leak_check.

Differential Revision: https://reviews.llvm.org/D130237
  • Loading branch information
happyCoder92 authored and vitalybuka committed Oct 12, 2022
1 parent 2498964 commit 39db491
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 4 deletions.
20 changes: 16 additions & 4 deletions compiler-rt/lib/lsan/lsan_common.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,8 @@ static void ProcessThreadRegistry(Frontier *frontier) {

// Scans thread data (stacks and TLS) for heap pointers.
static void ProcessThreads(SuspendedThreadsList const &suspended_threads,
Frontier *frontier) {
Frontier *frontier, tid_t caller_tid,
uptr caller_sp) {
InternalMmapVector<uptr> registers;
for (uptr i = 0; i < suspended_threads.ThreadCount(); i++) {
tid_t os_id = static_cast<tid_t>(suspended_threads.GetThreadID(i));
Expand All @@ -418,6 +419,9 @@ static void ProcessThreads(SuspendedThreadsList const &suspended_threads,
continue;
sp = stack_begin;
}
if (suspended_threads.GetThreadID(i) == caller_tid) {
sp = caller_sp;
}

if (flags()->use_registers && have_registers) {
uptr registers_begin = reinterpret_cast<uptr>(registers.data());
Expand Down Expand Up @@ -598,7 +602,8 @@ static void CollectIgnoredCb(uptr chunk, void *arg) {

// Sets the appropriate tag on each chunk.
static void ClassifyAllChunks(SuspendedThreadsList const &suspended_threads,
Frontier *frontier) {
Frontier *frontier, tid_t caller_tid,
uptr caller_sp) {
const InternalMmapVector<u32> &suppressed_stacks =
GetSuppressionContext()->GetSortedSuppressedStacks();
if (!suppressed_stacks.empty()) {
Expand All @@ -607,7 +612,7 @@ static void ClassifyAllChunks(SuspendedThreadsList const &suspended_threads,
}
ForEachChunk(CollectIgnoredCb, frontier);
ProcessGlobalRegions(frontier);
ProcessThreads(suspended_threads, frontier);
ProcessThreads(suspended_threads, frontier, caller_tid, caller_sp);
ProcessRootRegions(frontier);
FloodFillTag(frontier, kReachable);

Expand Down Expand Up @@ -703,7 +708,8 @@ static void CheckForLeaksCallback(const SuspendedThreadsList &suspended_threads,
CHECK(param);
CHECK(!param->success);
ReportUnsuspendedThreads(suspended_threads);
ClassifyAllChunks(suspended_threads, &param->frontier);
ClassifyAllChunks(suspended_threads, &param->frontier, param->caller_tid,
param->caller_sp);
ForEachChunk(CollectLeaksCb, &param->leaks);
// Clean up for subsequent leak checks. This assumes we did not overwrite any
// kIgnored tags.
Expand Down Expand Up @@ -742,6 +748,12 @@ static bool CheckForLeaks() {
for (int i = 0;; ++i) {
EnsureMainThreadIDIsCorrect();
CheckForLeaksParam param;
// Capture calling thread's stack pointer early, to avoid false negatives.
// Old frame with dead pointers might be overlapped by new frame inside
// CheckForLeaks which does not use bytes with pointers before the
// threads are suspended and stack pointers captured.
param.caller_tid = GetTid();
param.caller_sp = reinterpret_cast<uptr>(__builtin_frame_address(0));
LockStuffAndStopTheWorld(CheckForLeaksCallback, &param);
if (!param.success) {
Report("LeakSanitizer has encountered a fatal error.\n");
Expand Down
2 changes: 2 additions & 0 deletions compiler-rt/lib/lsan/lsan_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ struct RootRegion {
struct CheckForLeaksParam {
Frontier frontier;
LeakedChunks leaks;
tid_t caller_tid;
uptr caller_sp;
bool success = false;
};

Expand Down

0 comments on commit 39db491

Please sign in to comment.