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

[wiktorg]

Summary

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.

[vitalybuka]

LGTM
Would you be able to create a test from reproducer?

compiler-rt/lib/lsan/lsan_common.cpp

744

why do we need noinline? I guess to avoid "i" to move too far
With __builtin_frame_address it should not be needed.

799

__builtin_frame_address

[haowei]

We are seeing build failures after this change landed:

[1184/1522] Building CXX object compiler-rt/lib/lsan/CMakeFiles/RTLSanCommon.x86_64.dir/lsan_common.cpp.obj
FAILED: compiler-rt/lib/lsan/CMakeFiles/RTLSanCommon.x86_64.dir/lsan_common.cpp.obj 
/b/s/w/ir/cache/goma/client/gomacc /b/s/w/ir/x/w/staging/llvm_build/./bin/clang++ --target=x86_64-unknown-fuchsia --sysroot=/b/s/w/ir/x/w/cipd/sdk/arch/x64/sysroot -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/lib/lsan/.. --target=x86_64-unknown-fuchsia -I/b/s/w/ir/x/w/cipd/sdk/pkg/sync/include -I/b/s/w/ir/x/w/cipd/sdk/pkg/fdio/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -ffunction-sections -fdata-sections -ffile-prefix-map=/b/s/w/ir/x/w/staging/llvm_build/runtimes/runtimes-x86_64-unknown-fuchsia-bins=../staging/llvm_build/runtimes/runtimes-x86_64-unknown-fuchsia-bins -ffile-prefix-map=/b/s/w/ir/x/w/llvm-llvm-project/= -no-canonical-prefixes -Wall -Wno-unused-parameter -O2 -g -DNDEBUG -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -nostdinc++ -fno-rtti -Wno-format -std=c++17 -MD -MT compiler-rt/lib/lsan/CMakeFiles/RTLSanCommon.x86_64.dir/lsan_common.cpp.obj -MF compiler-rt/lib/lsan/CMakeFiles/RTLSanCommon.x86_64.dir/lsan_common.cpp.obj.d -o compiler-rt/lib/lsan/CMakeFiles/RTLSanCommon.x86_64.dir/lsan_common.cpp.obj -c /b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/lib/lsan/lsan_common.cpp
/b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/lib/lsan/lsan_common.cpp:615:3: error: no matching function for call to 'ProcessThreads'
  ProcessThreads(suspended_threads, frontier, caller_tid, caller_sp);
  ^~~~~~~~~~~~~~
/b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/lib/lsan/lsan_common.cpp:361:13: note: candidate function not viable: requires 2 arguments, but 4 were provided
static void ProcessThreads(SuspendedThreadsList const &, Frontier *) {}
            ^
1 error generated.

Looks like you forgot to change the declaration at L361. Could you revert your change if it takes a while to fix please?

compiler-rt/lib/lsan/lsan_common.h

 
 // LockStuffAndStopTheWorld can start to use Scan* calls to collect into
 // this Frontier vector before the StopTheWorldCallback actually runs.
 // This is used when the OS has a unified callback API for suspending
 // threads and enumerating roots.
 struct CheckForLeaksParam {
   Frontier frontier;
   LeakedChunks leaks;
+  tid_t caller_tid;
+  uptr caller_sp;
   bool success = false;
 };
 
 InternalMmapVectorNoCtor<RootRegion> const *GetRootRegions();
 void ScanRootRegion(Frontier *frontier, RootRegion const &region,
                     uptr region_begin, uptr region_end, bool is_readable);
 void ForEachExtraStackRangeCb(uptr begin, uptr end, void* arg);
 void GetAdditionalThreadContextPtrs(ThreadContextBase *tctx, void *ptrs);

compiler-rt/lib/lsan/lsan_common.cpp

for (uptr i = 0; i < ptrs.size(); ++i) {
     LOG_POINTERS("Treating pointer %p from ThreadContext as reachable\n", ptr);
     m.set_tag(kReachable);
     frontier->push_back(chunk);
   }
 }
 
 // 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));
     LOG_THREADS("Processing thread %llu.\n", os_id);
     uptr stack_begin, stack_end, tls_begin, tls_end, cache_begin, cache_end;
     DTLS *dtls;
     bool thread_found =
         GetThreadRangesLocked(os_id, &stack_begin, &stack_end, &tls_begin,
for (uptr i = 0; i < suspended_threads.ThreadCount(); i++) {
     if (have_registers != REGISTERS_AVAILABLE) {
       Report("Unable to get registers from thread %llu.\n", os_id);
       // If unable to get SP, consider the entire stack to be reachable unless
       // GetRegistersAndSP failed with ESRCH.
       if (have_registers == REGISTERS_UNAVAILABLE_FATAL)
         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());
       uptr registers_end =
           reinterpret_cast<uptr>(registers.data() + registers.size());
       ScanRangeForPointers(registers_begin, registers_end, frontier,
                            "REGISTERS", kReachable);
     }
if (m.allocated() && m.tag() == kIgnored) {
     LOG_POINTERS("Ignored: chunk %p-%p of size %zu.\n", (void *)chunk,
                  (void *)(chunk + m.requested_size()), m.requested_size());
     reinterpret_cast<Frontier *>(arg)->push_back(chunk);
   }
 }
 
 // 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()) {
     ForEachChunk(IgnoredSuppressedCb,
                  const_cast<InternalMmapVector<u32> *>(&suppressed_stacks));
   }
   ForEachChunk(CollectIgnoredCb, frontier);
   ProcessGlobalRegions(frontier);
-  ProcessThreads(suspended_threads, frontier);
+  ProcessThreads(suspended_threads, frontier, caller_tid, caller_sp);
   ProcessRootRegions(frontier);
   FloodFillTag(frontier, kReachable);
 
   // The check here is relatively expensive, so we do this in a separate flood
   // fill. That way we can skip the check for chunks that are reachable
   // otherwise.
   LOG_POINTERS("Processing platform-specific allocations.\n");
   ProcessPlatformSpecificAllocations(frontier);

 #  endif  // !SANITIZER_FUCHSIA
 
 static void CheckForLeaksCallback(const SuspendedThreadsList &suspended_threads,
                                   void *arg) {
   CheckForLeaksParam *param = reinterpret_cast<CheckForLeaksParam *>(arg);
   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.
   ForEachChunk(ResetTagsCb, nullptr);
   param->success = true;
 }
 
 static bool PrintResults(LeakReport &report) {
if (unsuppressed_count > 0) {
     report.PrintSummary();
     return true;
   }
   return false;
 }
 
 static bool CheckForLeaks() {
   if (&__lsan_is_turned_off && __lsan_is_turned_off())
     return false;
   // Inside LockStuffAndStopTheWorld we can't run symbolizer, so we can't match
   // suppressions. However if a stack id was previously suppressed, it should be
   // suppressed in future checks as well.
   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");
       Report(
           "HINT: For debugging, try setting environment variable "
           "LSAN_OPTIONS=verbosity=1:log_threads=1\n");
       Report(
           "HINT: LeakSanitizer does not work under ptrace (strace, gdb, "

 static bool has_reported_leaks = false;
 bool HasReportedLeaks() { return has_reported_leaks; }
 
 void DoLeakCheck() {
   Lock l(&global_mutex);
   static bool already_done;
   if (already_done)
     return;
   already_done = true;
   has_reported_leaks = CheckForLeaks();
   if (has_reported_leaks)
     HandleLeaks();
 }
 
 static int DoRecoverableLeakCheck() {
   Lock l(&global_mutex);
   bool have_leaks = CheckForLeaks();