[ASan] Stop blocking child thread progress from parent thread in `pthread_create` interceptor.

[delcypher]

Summary

Previously in ASan's pthread_create interceptor we would block in the
pthread_create interceptor waiting for the child thread to start.

Unfortunately this has bad performance characteristics because the OS
scheduler doesn't know the relationship between the parent and child
thread (i.e. the parent thread cannot make progress until the child
thread makes progress) and may make the wrong scheduling decision which
stalls progress.

It turns out that ASan didn't use to block in this interceptor but was
changed to do so to try to address
http://llvm.org/bugs/show_bug.cgi?id=21621/.

In that bug the problem being addressed was a LeakSanitizer false
positive. That bug concerns a heap object being passed
as arg to pthread_create. If:

• The calling thread loses a live reference to the object (e.g. pthread_create finishes and the thread no longer has a live reference to the object).
• Leak checking is triggered.
• The child thread has not yet started (once it starts it will have a live reference).

then the heap object will incorrectly appear to be leaked.

This bug is covered by the lsan/TestCases/leak_check_before_thread_started.cpp test case.

In b029c5101fb49b3577a1c322f42ef9fc616f25bf ASan was changed to block
in pthread_create() until the child thread starts so that arg is
kept alive for the purposes of leaking check.

While this change "works" its problematic due to the performance
problems it causes. The change is also completely unnecessary if leak
checking is disabled (via detect_leaks runtime option or
CAN_SANITIZE_LEAKS compile time config).

This patch does two things:

1. Takes a different approach to solving the leak false positive by making LSan's leak checking mechanism treat the arg pointer of created but not started threads as reachable. This is done by implementing the ForEachRegisteredThreadContextCb callback for ASan.

2. Removes the blocking behaviour in the ASan pthread_create interceptor.

rdar://problem/63537240

[delcypher]

@vitalybuka This patch implements the "update the frontier" approach you outlined in the https://reviews.llvm.org/D94210 . The patch depends on https://reviews.llvm.org/D95183 .

[delcypher]

Tweak condition to make it easier to read.

[vitalybuka]

compiler-rt/lib/asan/asan_allocator.cpp

1139

if we at asan_thread.cpp:260 arg is still invisible to lsan, and we can report false leak. So ThreadStatusRunning should be covered as well.

1150–1165

I would prefer we keep this logic in lsan

compiler-rt/lib/asan/asan_interceptors.cpp

230

if (result != 0) {

// destroy t;

}

[delcypher]

compiler-rt/lib/asan/asan_allocator.cpp

1139

Good catch.

1150–1165

Yep. Your suggestion in https://reviews.llvm.org/D95183 should resolve this. One downside though is when producing log output is that we won't be able to print the thread ID. That's a pretty small downside so I'm okay with that.

1153

Hmm. It just occurred to me we aren't checking m.allocated() is true before changing the tag and adding it to the frontier. This could lead to bad behaviour if the chunk gets freed at some point (e.g. the created thread frees it). Adding a freed chunk to the frontier could be especially bad because later on when doing the flood fill in LSan we'll scan the freed memory for pointers.

I'll add a check here to prevent this.

compiler-rt/lib/asan/asan_interceptors.cpp

230

Good catch. Isn't this a separate bug though? It looks like the old code didn't handle this either. I'm happy to fix it but I think it should be in a separate patch.

[delcypher]

compiler-rt/lib/asan/asan_interceptors.cpp

230

Wait.. no the bug isn't the old code because I've reserved the creation order so I've introduced a new bug. I'll fix this in this patch.

[delcypher]

Address first round of feedback.

[delcypher]

compiler-rt/lib/asan/asan_interceptors.cpp

228

@vitalybuka Is AsanThread::Destroy() safe to call from this thread and in this context where the corresponding thread was never started.? I wasn't really sure.

I see that calling this will call DTLS_Destroy() and I'm not sure if that needs to be called from a different thread (i.e. called from the thread before its destroyed). It looks like this function is a no-op on macOS which is where I'm testing.

[vitalybuka]

compiler-rt/lib/asan/asan_allocator.cpp

1197

historically sanitizers use snake case for local variable, from Google coding style

1202–1204

if (atctx->status != ThreadStatusCreated && atctx->status != ThreadStatusRunning)
  return;

compiler-rt/lib/asan/asan_interceptors.cpp

228

I think DTLS_Destroy should be fine here. I assume this thread will never showup in suspended_threads so lsan will not try to scan it.

As I understand Ctx is not leaked but pushed into quarantine by ThreadRegistry::FinishThread

[delcypher]

Address second round of feedback.

[delcypher]

compiler-rt/lib/asan/asan_allocator.cpp

1197

Okay. I'll change the style.

1202–1204

Sure. I'll rewrite it that way.

compiler-rt/lib/asan/asan_interceptors.cpp

228

Sorry "leaks" is a poor choice of words. What I meant is we've wasted an AsanThreadContext because created it but then never used it. Because in ASan we never re-use AsanThreadContexts its wasted.

I'll update the comment.

[clemenswasser]

You should also change compiler-rt/lib/asan/asan_win.cpp like this:

diff --git a/compiler-rt/lib/asan/asan_win.cpp b/compiler-rt/lib/asan/asan_win.cpp
index 8044ae16ff9b..1577c83cf994 100644
--- a/compiler-rt/lib/asan/asan_win.cpp
+++ b/compiler-rt/lib/asan/asan_win.cpp
@@ -134,7 +134,7 @@ INTERCEPTOR(int, _except_handler4, void *a, void *b, void *c, void *d) {
 static thread_return_t THREAD_CALLING_CONV asan_thread_start(void *arg) {
   AsanThread *t = (AsanThread *)arg;
   SetCurrentThread(t);
-  return t->ThreadStart(GetTid(), /* signal_thread_is_registered */ nullptr);
+  return t->ThreadStart(GetTid());
 }

 INTERCEPTOR_WINAPI(HANDLE, CreateThread, LPSECURITY_ATTRIBUTES security,

[delcypher]

In D95184#2517508, @clemenswasser wrote:

You should also change compiler-rt/lib/asan/asan_win.cpp like this:

diff --git a/compiler-rt/lib/asan/asan_win.cpp b/compiler-rt/lib/asan/asan_win.cpp
index 8044ae16ff9b..1577c83cf994 100644
--- a/compiler-rt/lib/asan/asan_win.cpp
+++ b/compiler-rt/lib/asan/asan_win.cpp
@@ -134,7 +134,7 @@ INTERCEPTOR(int, _except_handler4, void *a, void *b, void *c, void *d) {
 static thread_return_t THREAD_CALLING_CONV asan_thread_start(void *arg) {
   AsanThread *t = (AsanThread *)arg;
   SetCurrentThread(t);
-  return t->ThreadStart(GetTid(), /* signal_thread_is_registered */ nullptr);
+  return t->ThreadStart(GetTid());
 }

 INTERCEPTOR_WINAPI(HANDLE, CreateThread, LPSECURITY_ATTRIBUTES security,

Thanks. Sorry for breaking the Windows build. This should be resolved by 757b93bb7b384038a8dec35433f78f5c7c2ef8b0

compiler-rt/lib/asan/asan_allocator.cpp

 
 LsanMetadata::LsanMetadata(uptr chunk) {
   metadata_ = chunk ? reinterpret_cast<void *>(chunk - __asan::kChunkHeaderSize)
                     : nullptr;
 }
 
 bool LsanMetadata::allocated() const {
   if (!metadata_)
     return false;
   __asan::AsanChunk *m = reinterpret_cast<__asan::AsanChunk *>(metadata_);
   return atomic_load(&m->chunk_state, memory_order_relaxed) ==
          __asan::CHUNK_ALLOCATED;
 }
 
 ChunkTag LsanMetadata::tag() const {
   __asan::AsanChunk *m = reinterpret_cast<__asan::AsanChunk *>(metadata_);
   return static_cast<ChunkTag>(m->lsan_tag);
 }
 
 void LsanMetadata::set_tag(ChunkTag value) {
   __asan::AsanChunk *m = reinterpret_cast<__asan::AsanChunk *>(metadata_);
   m->lsan_tag = value;
 }
 
 uptr LsanMetadata::requested_size() const {
   __asan::AsanChunk *m = reinterpret_cast<__asan::AsanChunk *>(metadata_);
   return m->UsedSize();
 }
 
 u32 LsanMetadata::stack_trace_id() const {
   __asan::AsanChunk *m = reinterpret_cast<__asan::AsanChunk *>(metadata_);
   u32 tid = 0;
   u32 stack = 0;
   m->GetAllocContext(tid, stack);
   return stack;
 }
 
 void ForEachChunk(ForEachChunkCallback callback, void *arg) {
   __asan::get_allocator().ForEachChunk(callback, arg);
 }
 
 IgnoreObjectResult IgnoreObjectLocked(const void *p) {
   uptr addr = reinterpret_cast<uptr>(p);
   __asan::AsanChunk *m = __asan::instance.GetAsanChunkByAddr(addr);
   if (!m ||
       (atomic_load(&m->chunk_state, memory_order_acquire) !=
        __asan::CHUNK_ALLOCATED) ||
       !m->AddrIsInside(addr)) {
     return kIgnoreObjectInvalid;
   }
   if (m->lsan_tag == kIgnored)
     return kIgnoreObjectAlreadyIgnored;
   m->lsan_tag = __lsan::kIgnored;
   return kIgnoreObjectSuccess;
 }
 
 void GetAdditionalThreadContextPtrs(ThreadContextBase *tctx, void *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.
+  // 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;
 
-  // This is currently a no-op because the ASan `pthread_create()` interceptor
-  // blocks until the child thread starts which keeps the thread's `arg` pointer
-  // live.
+  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

compiler-rt/lib/asan/asan_interceptors.cpp

 #define COMMON_SYSCALL_POST_WRITE_RANGE(p, s) \
   do {                                        \
     (void)(p);                                \
     (void)(s);                                \
   } while (false)
 #include "sanitizer_common/sanitizer_common_syscalls.inc"
 #include "sanitizer_common/sanitizer_syscalls_netbsd.inc"
 
-struct ThreadStartParam {
-  atomic_uintptr_t t;
-  atomic_uintptr_t is_registered;
-};
-
 #if ASAN_INTERCEPT_PTHREAD_CREATE
 static thread_return_t THREAD_CALLING_CONV asan_thread_start(void *arg) {
-  ThreadStartParam *param = reinterpret_cast<ThreadStartParam *>(arg);
-  AsanThread *t = nullptr;
-  while ((t = reinterpret_cast<AsanThread *>(
-              atomic_load(&param->t, memory_order_acquire))) == nullptr)
-    internal_sched_yield();
+  AsanThread *t = (AsanThread *)arg;
   SetCurrentThread(t);
-  return t->ThreadStart(GetTid(), &param->is_registered);
+  return t->ThreadStart(GetTid());
 }
 
 INTERCEPTOR(int, pthread_create, void *thread,
     void *attr, void *(*start_routine)(void*), void *arg) {
   EnsureMainThreadIDIsCorrect();
   // Strict init-order checking is thread-hostile.
   if (flags()->strict_init_order)
     StopInitOrderChecking();
   GET_STACK_TRACE_THREAD;
   int detached = 0;
   if (attr)
     REAL(pthread_attr_getdetachstate)(attr, &detached);
-  ThreadStartParam param;
-  atomic_store(&param.t, 0, memory_order_relaxed);
-  atomic_store(&param.is_registered, 0, memory_order_relaxed);
+
+  u32 current_tid = GetCurrentTidOrInvalid();
+  AsanThread *t =
+      AsanThread::Create(start_routine, arg, current_tid, &stack, detached);
+
   int result;
   {
     // Ignore all allocations made by pthread_create: thread stack/TLS may be
     // stored by pthread for future reuse even after thread destruction, and
     // the linked list it's stored in doesn't even hold valid pointers to the
     // objects, the latter are calculated by obscure pointer arithmetic.
 #if CAN_SANITIZE_LEAKS
     __lsan::ScopedInterceptorDisabler disabler;
 #endif
-    result = REAL(pthread_create)(thread, attr, asan_thread_start, &param);
+    result = REAL(pthread_create)(thread, attr, asan_thread_start, t);
   }
-  if (result == 0) {
-    u32 current_tid = GetCurrentTidOrInvalid();
-    AsanThread *t =
-        AsanThread::Create(start_routine, arg, current_tid, &stack, detached);
-    atomic_store(&param.t, reinterpret_cast<uptr>(t), memory_order_release);
-    // Wait until the AsanThread object is initialized and the ThreadRegistry
-    // entry is in "started" state. One reason for this is that after this
-    // interceptor exits, the child thread's stack may be the only thing holding
-    // the |arg| pointer. This may cause LSan to report a leak if leak checking
-    // happens at a point when the interceptor has already exited, but the stack
-    // range for the child thread is not yet known.
-    while (atomic_load(&param.is_registered, memory_order_acquire) == 0)
-      internal_sched_yield();
+  if (result != 0) {
+    // If the thread didn't start delete the AsanThread to avoid leaking it.
+    // Note AsanThreadContexts never get destroyed so the AsanThreadContext
+    // that was just created for the AsanThread is wasted.
+    t->Destroy();
   }
   return result;
 }
 
 INTERCEPTOR(int, pthread_join, void *t, void **arg) {
   return real_pthread_join(t, arg);
 }
 
 DEFINE_REAL_PTHREAD_FUNCTIONS

compiler-rt/lib/asan/asan_thread.h

public:
   static AsanThread *Create(thread_callback_t start_routine, void *arg,
                             u32 parent_tid, StackTrace *stack, bool detached);
   static void TSDDtor(void *tsd);
   void Destroy();
 
   struct InitOptions;
   void Init(const InitOptions *options = nullptr);
 
-  thread_return_t ThreadStart(tid_t os_id,
-                              atomic_uintptr_t *signal_thread_is_registered);
+  thread_return_t ThreadStart(tid_t os_id);
 
   uptr stack_top();
   uptr stack_bottom();
   uptr stack_size();
   uptr tls_begin() { return tls_begin_; }
   uptr tls_end() { return tls_end_; }
   DTLS *dtls() { return dtls_; }
   u32 tid() { return context_->tid; }
public:
   bool isUnwinding() const { return unwinding_; }
   void setUnwinding(bool b) { unwinding_ = b; }
 
   AsanThreadLocalMallocStorage &malloc_storage() { return malloc_storage_; }
   AsanStats &stats() { return stats_; }
 
   void *extra_spill_area() { return &extra_spill_area_; }
 
+  void *get_arg() { return arg_; }
+
  private:
   // NOTE: There is no AsanThread constructor. It is allocated
   // via mmap() and *must* be valid in zero-initialized state.
 
   void SetThreadStackAndTls(const InitOptions *options);
 
   void ClearShadowForThreadStackAndTLS();
   FakeStack *AsyncSignalSafeLazyInitFakeStack();

compiler-rt/lib/asan/asan_thread.cpp

VReport(1, "T%d: stack [%p,%p) size 0x%zx; local=%p\n", tid(),
           &local);
 }
 
 // Fuchsia and RTEMS don't use ThreadStart.
 // asan_fuchsia.c/asan_rtems.c define CreateMainThread and
 // SetThreadStackAndTls.
 #if !SANITIZER_FUCHSIA && !SANITIZER_RTEMS
 
-thread_return_t AsanThread::ThreadStart(
-    tid_t os_id, atomic_uintptr_t *signal_thread_is_registered) {
+thread_return_t AsanThread::ThreadStart(tid_t os_id) {
   Init();
   asanThreadRegistry().StartThread(tid(), os_id, ThreadType::Regular, nullptr);
-  if (signal_thread_is_registered)
-    atomic_store(signal_thread_is_registered, 1, memory_order_release);
 
   if (common_flags()->use_sigaltstack) SetAlternateSignalStack();
 
   if (!start_routine_) {
     // start_routine_ == 0 if we're on the main thread or on one of the
     // OS X libdispatch worker threads. But nobody is supposed to call
     // ThreadStart() for the worker threads.
     CHECK_EQ(tid(), 0);
thread_return_t AsanThread::ThreadStart(tid_t os_id) {
   return res;
 }
 
 AsanThread *CreateMainThread() {
   AsanThread *main_thread = AsanThread::Create(
       /* start_routine */ nullptr, /* arg */ nullptr, /* parent_tid */ 0,
       /* stack */ nullptr, /* detached */ true);
   SetCurrentThread(main_thread);
-  main_thread->ThreadStart(internal_getpid(),
-                           /* signal_thread_is_registered */ nullptr);
+  main_thread->ThreadStart(internal_getpid());
   return main_thread;
 }
 
 // This implementation doesn't use the argument, which is just passed down
 // from the caller of Init (which see, above).  It's only there to support
 // OS-specific implementations that need more information passed through.
 void AsanThread::SetThreadStackAndTls(const InitOptions *options) {
   DCHECK_EQ(options, nullptr);