commit 5f3aef660b534613df3b6ea5ab6b24a2dac5f24b
author Wiktor Garbacz <wiktorg@google.com> Wed Oct 12 16:36:37 2022 -0700
committer Copybara-Service <copybara-worker@google.com> Wed Oct 12 16:51:06 2022 -0700
tree 98155555ac4a8f7579fb61c00c09ed3e03e4a858
parent a74efd6a038404442d8886764f2d8a9b237cb7e5

[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

GitOrigin-RevId: 39db491957dcf095936d81bed89c2b4edae2a1e7

lib/lsan/lsan_common.cpp

diff --git a/lib/lsan/lsan_common.cpp b/lib/lsan/lsan_common.cpp
index ade0b80..5121877 100644
--- a/lib/lsan/lsan_common.cpp
+++ b/lib/lsan/lsan_common.cpp
@@ -391,7 +391,8 @@
 
 // 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 @@
         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 @@
 
 // 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 @@
   }
   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 @@
   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 @@
   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");

lib/lsan/lsan_common.h

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