[LSAN] More LSAN interface tweaking.

[kstoimenov]

Summary

Main goal is to remove thread registry dependency from the interface because HWASAN is using its own code to manage threads.

[vitalybuka]

compiler-rt/lib/asan/asan_thread.cpp

553

I am not sure I like that not suspended/unsuspended logic is exposed to asan

[vitalybuka]

I am not sure what is the problem here for HWASAN
It's going to use ThreadContextBase anyway?

[fmayer]

nit: move second sentence of commit message into separate paragraph.

[kstoimenov]

In D140039#3995965, @vitalybuka wrote:

I am not sure what is the problem here for HWASAN
It's going to use ThreadContextBase anyway?

Currently HWASAN is using HwasanThreadList to manage the threads. I am thinking that I could adapt this class to do what the ThreadRegistry does, but for that I will have to clean up the interface. So I am trying to get rid of anything that is ThreadRegistry related including ThreadContextBase.

[vitalybuka]

compiler-rt/lib/asan/asan_thread.cpp

553

Probably it would be nicer if you have
oid RunCallbackForEachRunningThreadLocked(SomeCBWithoutThreadContext

[kstoimenov]

PTAL. This patch should go in before the rest of the code.

[vitalybuka]

compiler-rt/lib/asan/asan_thread.cpp

553

Probably it would be nicer if you have
oid RunCallbackForEachRunningThreadLocked(SomeCBWithoutThreadContext

Why you don't want to go this way?

[kstoimenov]

compiler-rt/lib/asan/asan_thread.cpp

553

In some cases like for example GetAdditionalThreadContextPtrs for the LSAN implementation the actual callback is empty. In the current implementation we would still enumerate all threads and call an empty callback which is less efficient.

[vitalybuka]

compiler-rt/lib/asan/asan_thread.cpp

553

Which implementations?

Also even in extremely thread apps, number of threads is a relatively small number

[kstoimenov]

compiler-rt/lib/asan/asan_thread.cpp

553

What I mean is that if we use RunCallbackForEachThreadLocked and the callback is empty we will still run a loop over all threads and call an empty callback. Also it is not only about the loop, but we also lock the allocator when we run the loop.

Let me try to do it your way.

[kstoimenov]

After pull.

[kstoimenov]

compiler-rt/lib/asan/asan_thread.cpp

553

I tried using exposing generic RunCallbackForEachRunningThreadLocked, but it is very kludgy. In this case RunCallbackForEachRunningThreadLocked and each callback have to be implemented by individual client (hwasan, asan, lsan). This leads to a problem with the callback because to implement RunCallbackForEachRunningThreadLocked I need to cast the generic callback that is passed into the concrete type, but this is not possible. I tried using a lambda, but I need to capture the original callback and according to the C++ standard this cannot be use and function pointer. See D141621 for details.

[vitalybuka]

update

[vitalybuka]

How about this one? https://reviews.llvm.org/D140039?vs=488417&id=488756#toc

[vitalybuka]

if you agree with the last version then, please "arc patch D140039" and submit

[thakis]

Looks like this doesn't build on Windows: http://45.33.8.238/win/73274/step_4.txt

Please take a look and revert for now if it takes a while to fix.

[abrachet]

In D140039#4049719, @thakis wrote:

Looks like this doesn't build on Windows: http://45.33.8.238/win/73274/step_4.txt

Please take a look and revert for now if it takes a while to fix.

If you revert instead of a fix forward would you mind reverting rGf001e50f955c3cdf2deb79e38a9fd19c9a781882 too?

[vitalybuka]

In D140039#4049818, @abrachet wrote:

In D140039#4049719, @thakis wrote:

Looks like this doesn't build on Windows: http://45.33.8.238/win/73274/step_4.txt

Please take a look and revert for now if it takes a while to fix.

If you revert instead of a fix forward would you mind reverting rGf001e50f955c3cdf2deb79e38a9fd19c9a781882 too?

I'll revert

[vitalybuka]

reupload correct snapshot

[vitalybuka]

lambda

[abrachet]

Hmm, the update I did to lsan_common_fuchsia.cpp is fine for clang_rt.lsan, but unfortunately it breaks clang_rt.asan. I'm pretty sure adding lsan_thread.cpp to LSAN_COMMON_SOURCES is wrong, but it isn't even tenable because then clang_rt.asan gets multiple definition errors between lsan_thread.cpp and asan_thread.cpp. I'm wondering what the best way to go about this is? In lsan we want to use GetLsanThreadRegistryLocked but in asan we want GetAsanThreadRegistryLocked instead. Though this seems erroneous. Do folks have any other suggestions?

[kstoimenov]

In D140039#4052367, @abrachet wrote:

Hmm, the update I did to lsan_common_fuchsia.cpp is fine for clang_rt.lsan, but unfortunately it breaks clang_rt.asan. I'm pretty sure adding lsan_thread.cpp to LSAN_COMMON_SOURCES is wrong, but it isn't even tenable because then clang_rt.asan gets multiple definition errors between lsan_thread.cpp and asan_thread.cpp. I'm wondering what the best way to go about this is? In lsan we want to use GetLsanThreadRegistryLocked but in asan we want GetAsanThreadRegistryLocked instead. Though this seems erroneous. Do folks have any other suggestions?

Do you have a link with the error you are getting?

compiler-rt/lib/asan/asan_allocator.cpp

if (!m ||
     return kIgnoreObjectInvalid;
   }
   if (m->lsan_tag == kIgnored)
     return kIgnoreObjectAlreadyIgnored;
   m->lsan_tag = __lsan::kIgnored;
   return kIgnoreObjectSuccess;
 }
 
-void GetAdditionalThreadContextPtrs(ThreadContextBase *tctx, void *ptrs) {
-  // Look for the arg pointer of threads that have been created or are running.
-  // This is necessary to prevent false positive leaks due to the AsanThread
-  // holding the only live reference to a heap object.  This can happen because
-  // the `pthread_create()` interceptor doesn't wait for the child thread to
-  // start before returning and thus loosing the the only live reference to the
-  // heap object on the stack.
-
-  __asan::AsanThreadContext *atctx =
-      reinterpret_cast<__asan::AsanThreadContext *>(tctx);
-  __asan::AsanThread *asan_thread = atctx->thread;
-
-  // Note ThreadStatusRunning is required because there is a small window where
-  // the thread status switches to `ThreadStatusRunning` but the `arg` pointer
-  // still isn't on the stack yet.
-  if (atctx->status != ThreadStatusCreated &&
-      atctx->status != ThreadStatusRunning)
-    return;
-
-  uptr thread_arg = reinterpret_cast<uptr>(asan_thread->get_arg());
-  if (!thread_arg)
-    return;
-
-  auto ptrsVec = reinterpret_cast<InternalMmapVector<uptr> *>(ptrs);
-  ptrsVec->push_back(thread_arg);
-}
-
 }  // namespace __lsan
 
 // ---------------------- Interface ---------------- {{{1
 using namespace __asan;
 
 // ASan allocator doesn't reserve extra bytes, so normally we would
 // just return "size". We don't want to expose our redzone sizes, etc here.
 uptr __sanitizer_get_estimated_allocated_size(uptr size) {

compiler-rt/lib/asan/asan_thread.cpp

void ForEachExtraStackRange(tid_t os_id, RangeIteratorCallback callback,
   if (!t)
     return;
   __asan::FakeStack *fake_stack = t->get_fake_stack();
   if (!fake_stack)
     return;
   fake_stack->ForEachFakeFrame(callback, arg);
 }
 
-void RunCallbackForEachThreadLocked(__sanitizer::ThreadRegistry::ThreadCallback cb,
-                                    void *arg) {
-  GetAsanThreadRegistryLocked()->RunCallbackForEachThreadLocked(cb, arg);
+void GetAdditionalThreadContextPtrsLocked(InternalMmapVector<uptr> *ptrs) {
+  GetAsanThreadRegistryLocked()->RunCallbackForEachThreadLocked(
+      [](ThreadContextBase *tctx, void *ptrs) {
+        // Look for the arg pointer of threads that have been created or are
+        // running. This is necessary to prevent false positive leaks due to the
+        // AsanThread holding the only live reference to a heap object.  This
+        // can happen because the `pthread_create()` interceptor doesn't wait
+        // for the child thread to start before returning and thus loosing the
+        // the only live reference to the heap object on the stack.
+
+        __asan::AsanThreadContext *atctx =
+            static_cast<__asan::AsanThreadContext *>(tctx);
+
+        // Note ThreadStatusRunning is required because there is a small window
+        // where the thread status switches to `ThreadStatusRunning` but the
+        // `arg` pointer still isn't on the stack yet.
+        if (atctx->status != ThreadStatusCreated &&
+            atctx->status != ThreadStatusRunning)
+          return;
+
+        uptr thread_arg = reinterpret_cast<uptr>(atctx->thread->get_arg());
+        if (!thread_arg)
+          return;
+
+        auto ptrsVec = reinterpret_cast<InternalMmapVector<uptr> *>(ptrs);
+        ptrsVec->push_back(thread_arg);
+      },
+      ptrs);
+}
+
+void GetRunningThreadsLocked(InternalMmapVector<tid_t> *threads) {
+  GetAsanThreadRegistryLocked()->RunCallbackForEachThreadLocked(
+      [](ThreadContextBase *tctx, void *threads) {
+        if (tctx->status == ThreadStatusRunning)
+          reinterpret_cast<InternalMmapVector<tid_t> *>(threads)->push_back(
+              tctx->os_id);
+      },
+      threads);
 }
 
 void FinishThreadLocked(u32 tid) {
   GetAsanThreadRegistryLocked()->FinishThread(tid);
 }
 
 } // namespace __lsan
 

compiler-rt/lib/hwasan/hwasan_thread.cpp

 }
 
 void GetAllThreadAllocatorCachesLocked(InternalMmapVector<uptr> *caches) {}
 
 void ForEachExtraStackRange(tid_t os_id, RangeIteratorCallback callback,
                             void *arg) {}
 
 void GetAdditionalThreadContextPtrsLocked(InternalMmapVector<uptr> *ptrs) {}
-void ReportUnsuspendedThreadsLocked(InternalMmapVector<tid_t> *threads) {}
+void GetRunningThreadsLocked(InternalMmapVector<tid_t> *threads) {}
 
 }  // namespace __lsan

compiler-rt/lib/lsan/lsan_allocator.cpp

if (m->tag == kIgnored)
       return kIgnoreObjectAlreadyIgnored;
     m->tag = kIgnored;
     return kIgnoreObjectSuccess;
   } else {
     return kIgnoreObjectInvalid;
   }
 }
 
-void GetAdditionalThreadContextPtrs(ThreadContextBase *tctx, void *ptrs) {
+void GetAdditionalThreadContextPtrsLocked(InternalMmapVector<uptr> *ptrs) {
   // This function can be used to treat memory reachable from `tctx` as live.
   // This is useful for threads that have been created but not yet started.
 
   // This is currently a no-op because the LSan `pthread_create()` interceptor
   // blocks until the child thread starts which keeps the thread's `arg` pointer
   // live.
 }
 

compiler-rt/lib/lsan/lsan_common.h

 void EnsureMainThreadIDIsCorrect();
 
 bool GetThreadRangesLocked(tid_t os_id, uptr *stack_begin, uptr *stack_end,
                            uptr *tls_begin, uptr *tls_end, uptr *cache_begin,
                            uptr *cache_end, DTLS **dtls);
 void GetAllThreadAllocatorCachesLocked(InternalMmapVector<uptr> *caches);
 void ForEachExtraStackRange(tid_t os_id, RangeIteratorCallback callback,
                             void *arg);
-
-void RunCallbackForEachThreadLocked(__sanitizer::ThreadRegistry::ThreadCallback cb,
-                                    void *arg);
+void GetAdditionalThreadContextPtrsLocked(InternalMmapVector<uptr> *ptrs);
+void GetRunningThreadsLocked(InternalMmapVector<tid_t> *threads);
 
 //// --------------------------------------------------------------------------
 //// Allocator prototypes.
 //// --------------------------------------------------------------------------
 
 // Wrappers for allocator's ForceLock()/ForceUnlock().
 void LockAllocator();
 void UnlockAllocator();

 };
 
 // Iterate over all existing chunks. Allocator must be locked.
 void ForEachChunk(ForEachChunkCallback callback, void *arg);
 
 // Helper for __lsan_ignore_object().
 IgnoreObjectResult IgnoreObjectLocked(const void *p);
 
-void GetAdditionalThreadContextPtrs(ThreadContextBase *tctx, void *ptrs);
-
 // The rest of the LSan interface which is implemented by library.
 
 struct ScopedStopTheWorldLock {
   ScopedStopTheWorldLock() {
     LockThreadRegistry();
     LockAllocator();
   }
 

compiler-rt/lib/lsan/lsan_common.cpp

 #    if SANITIZER_ANDROID
 // FIXME: Move this out into *libcdep.cpp
 extern "C" SANITIZER_WEAK_ATTRIBUTE void __libc_iterate_dynamic_tls(
     pid_t, void (*cb)(void *, void *, uptr, void *), void *);
 #    endif
 
 static void ProcessThreadRegistry(Frontier *frontier) {
   InternalMmapVector<uptr> ptrs;
-  RunCallbackForEachThreadLocked(GetAdditionalThreadContextPtrs, &ptrs);
+  GetAdditionalThreadContextPtrsLocked(&ptrs);
 
   for (uptr i = 0; i < ptrs.size(); ++i) {
     void *ptr = reinterpret_cast<void *>(ptrs[i]);
     uptr chunk = PointsIntoChunk(ptr);
     if (!chunk)
       continue;
     LsanMetadata m(chunk);
     if (!m.allocated())
void LeakSuppressionContext::PrintMatchedSuppressions() {
   for (uptr i = 0; i < matched.size(); i++) {
     Printf("%7zu %10zu %s\n",
            static_cast<uptr>(atomic_load_relaxed(&matched[i]->hit_count)),
            matched[i]->weight, matched[i]->templ);
   }
   Printf("%s\n\n", line);
 }
 
-static void ReportIfNotSuspended(ThreadContextBase *tctx, void *arg) {
-  const InternalMmapVector<tid_t> &suspended_threads =
-      *(const InternalMmapVector<tid_t> *)arg;
-  if (tctx->status == ThreadStatusRunning) {
-    uptr i = InternalLowerBound(suspended_threads, tctx->os_id);
-    if (i >= suspended_threads.size() || suspended_threads[i] != tctx->os_id)
-      Report(
-          "Running thread %llu was not suspended. False leaks are possible.\n",
-          tctx->os_id);
-  }
-}
-
 #  if SANITIZER_FUCHSIA
 
 // Fuchsia provides a libc interface that guarantees all threads are
 // covered, and SuspendedThreadList is never really used.
 static void ReportUnsuspendedThreads(const SuspendedThreadsList &) {}
 
 #  else  // !SANITIZER_FUCHSIA
 
 static void ReportUnsuspendedThreads(
     const SuspendedThreadsList &suspended_threads) {
   InternalMmapVector<tid_t> threads(suspended_threads.ThreadCount());
   for (uptr i = 0; i < suspended_threads.ThreadCount(); ++i)
     threads[i] = suspended_threads.GetThreadID(i);
 
   Sort(threads.data(), threads.size());
 
-  RunCallbackForEachThreadLocked(&ReportIfNotSuspended, &threads);
+  InternalMmapVector<tid_t> unsuspended;
+  GetRunningThreadsLocked(&unsuspended);
+
+  for (auto os_id : unsuspended) {
+    uptr i = InternalLowerBound(threads, os_id);
+    if (i >= threads.size() || threads[i] != os_id)
+      Report(
+          "Running thread %zu was not suspended. False leaks are possible.\n",
+          os_id);
+  }
 }
 
 #  endif  // !SANITIZER_FUCHSIA
 
 static void CheckForLeaksCallback(const SuspendedThreadsList &suspended_threads,
                                   void *arg) {
   CheckForLeaksParam *param = reinterpret_cast<CheckForLeaksParam *>(arg);
   CHECK(param);

compiler-rt/lib/lsan/lsan_common_fuchsia.cpp

 //=-- lsan_common_fuchsia.cpp --------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===---------------------------------------------------------------------===//
 //
 // This file is a part of LeakSanitizer.
 // Implementation of common leak checking functionality. Fuchsia-specific code.
 //
 //===---------------------------------------------------------------------===//
 
 #include "lsan_common.h"
+#include "lsan_thread.h"
 #include "sanitizer_common/sanitizer_platform.h"
 
 #if CAN_SANITIZE_LEAKS && SANITIZER_FUCHSIA
 #include <zircon/sanitizer.h>
 
 #include "lsan_allocator.h"
 #include "sanitizer_common/sanitizer_flags.h"
 #include "sanitizer_common/sanitizer_stoptheworld_fuchsia.h"
__sanitizer_memory_snapshot(
       [](zx_status_t, void *data) {
         auto params = static_cast<const Params *>(data);
 
         // We don't use the thread registry at all for enumerating the threads
         // and their stacks, registers, and TLS regions.  So use it separately
         // just for the allocator cache, and to call ForEachExtraStackRange,
         // which ASan needs.
         if (flags()->use_stacks) {
-          RunCallbackForEachThreadLocked(
+          GetLsanThreadRegistryLocked()->RunCallbackForEachThreadLocked(
               [](ThreadContextBase *tctx, void *arg) {
                 ForEachExtraStackRange(tctx->os_id, ForEachExtraStackRangeCb,
                                        arg);
               },
               &params->argument->frontier);
         }
 
         params->callback(SuspendedThreadsListFuchsia(), params->argument);

compiler-rt/lib/lsan/lsan_fuchsia.cpp

void InitializeMainThread() {
   __sanitizer::GetThreadStackTopAndBottom(true, &args.stack_end,
                                           &args.stack_begin);
   u32 tid = ThreadCreate(kMainTid, true, &args);
   CHECK_EQ(tid, 0);
   ThreadStart(tid);
 }
 
 void GetAllThreadAllocatorCachesLocked(InternalMmapVector<uptr> *caches) {
-  RunCallbackForEachThreadLocked(
+  GetLsanThreadRegistryLocked()->RunCallbackForEachThreadLocked(
       [](ThreadContextBase *tctx, void *arg) {
         auto ctx = static_cast<ThreadContext *>(tctx);
         static_cast<decltype(caches)>(arg)->push_back(ctx->cache_begin());
       },
       caches);
 }
 
 // On Fuchsia, leak detection is done by a special hook after atexit hooks.

compiler-rt/lib/lsan/lsan_thread.cpp

 
 void UnlockThreadRegistry() { thread_registry->Unlock(); }
 
 ThreadRegistry *GetLsanThreadRegistryLocked() {
   thread_registry->CheckLocked();
   return thread_registry;
 }
 
-void RunCallbackForEachThreadLocked(
-    __sanitizer::ThreadRegistry::ThreadCallback cb, void *arg) {
-  GetLsanThreadRegistryLocked()->RunCallbackForEachThreadLocked(cb, arg);
+void GetRunningThreadsLocked(InternalMmapVector<tid_t> *threads) {
+  GetLsanThreadRegistryLocked()->RunCallbackForEachThreadLocked(
+      [](ThreadContextBase *tctx, void *threads) {
+        if (tctx->status == ThreadStatusRunning) {
+          reinterpret_cast<InternalMmapVector<tid_t> *>(threads)->push_back(
+              tctx->os_id);
+        }
+      },
+      threads);
 }
 
 }  // namespace __lsan