[compiler-rt] 39db491 - [LeakSanitizer] Capture calling thread SP early to avoid false negatives.

[Vitaly Buka via llvm-commits]

Author: Wiktor Garbacz
Date: 2022-10-12T16:46:32-07:00
New Revision: 39db491957dcf095936d81bed89c2b4edae2a1e7

URL: https://github.com/llvm/llvm-project/commit/39db491957dcf095936d81bed89c2b4edae2a1e7
DIFF: https://github.com/llvm/llvm-project/commit/39db491957dcf095936d81bed89c2b4edae2a1e7.diff

LOG: [LeakSanitizer] Capture calling thread SP early to avoid false negatives.

As shown in https://github.com/llvm/llvm-project/issues/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

Added: 
    

Modified: 
    compiler-rt/lib/lsan/lsan_common.cpp
    compiler-rt/lib/lsan/lsan_common.h

Removed: 
    


################################################################################
diff  --git a/compiler-rt/lib/lsan/lsan_common.cpp b/compiler-rt/lib/lsan/lsan_common.cpp
index ade0b8081f276..51218770d6dcc 100644
--- a/compiler-rt/lib/lsan/lsan_common.cpp
+++ b/compiler-rt/lib/lsan/lsan_common.cpp
@@ -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));
@@ -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());
@@ -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()) {
@@ -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);
 
@@ -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.
@@ -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");

diff  --git a/compiler-rt/lib/lsan/lsan_common.h b/compiler-rt/lib/lsan/lsan_common.h
index d7153751faee0..20ef7c458b42a 100644
--- a/compiler-rt/lib/lsan/lsan_common.h
+++ b/compiler-rt/lib/lsan/lsan_common.h
@@ -145,6 +145,8 @@ struct RootRegion {
 struct CheckForLeaksParam {
   Frontier frontier;
   LeakedChunks leaks;
+  tid_t caller_tid;
+  uptr caller_sp;
   bool success = false;
 };