[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

1141

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.

1152–1167

I would prefer we keep this logic in lsan

compiler-rt/lib/asan/asan_interceptors.cpp

224

if (result != 0) {

// destroy t;

}

[delcypher]

compiler-rt/lib/asan/asan_allocator.cpp

1141

Good catch.

1152–1167

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.

1155

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

224

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

224

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

226

@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

1239

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

1244–1246

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

compiler-rt/lib/asan/asan_interceptors.cpp

226

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

1239

Okay. I'll change the style.

1244–1246

Sure. I'll rewrite it that way.

compiler-rt/lib/asan/asan_interceptors.cpp

226

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

uptr PointsIntoChunk(void *p) {
   if (m->AddrIsInside(addr))
     return chunk;
   if (IsSpecialCaseOfOperatorNew0(chunk, m->UsedSize(), addr))
     return chunk;
   return 0;
 }
 
 void ForEachRegisteredThreadContextCb(ThreadContextBase *tctx, void *arg) {
-  // 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.
+  // The code here looks for created but not yet started threads and treats the
+  // thread's `arg` pointer as reachable if it is a heap object. This is
+  // necessary to prevent leak false positives if nothing else holds a live
+  // reference to the heap object. This can happen because the ASan
+  // `pthread_create()` interceptor doesn't wait for the child thread to
+  // start before exiting.
+
+  __asan::AsanThreadContext *atctx =
+      reinterpret_cast<__asan::AsanThreadContext *>(tctx);
+  __asan::AsanThread *asanThread = atctx->thread;
+  // The AsanThread will non null if `AsanThreadContext::OnFinished()`
+  // hasn't been called.
+  if (!asanThread)
+    return;
+  // We only need to deal with threads that have been created but not
+  // yet started.
+  if (atctx->status != ThreadStatusCreated)
+    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.
+  void *threadArg = asanThread->get_arg();
+  if (!threadArg)
+    return;
+
+  uptr chunk = PointsIntoChunk(threadArg);
+  if (!chunk)
+    return;
+
+  // Add chunk to frontier and mark it as reachable.
+  Frontier *frontier = reinterpret_cast<Frontier *>(arg);
+  frontier->push_back(chunk);
+  LsanMetadata m(chunk);
+  m.set_tag(kReachable);
+
+  // __lsan::flags() has to be guarded by this macro because it uses data that
+  // is not defined when !CAN_SANITIZE_LEAKS.
+#if CAN_SANITIZE_LEAKS
+  if (flags()->log_pointers)
+    Report(
+        "Treating arg pointer of created but not started thread T%d as live: "
+        "%p\n",
+        atctx->tid, threadArg);
+#endif
 }
 
 uptr GetUserBegin(uptr chunk) {
   __asan::AsanChunk *m = __asan::instance.GetAsanChunkByAddrFastLocked(chunk);
   return m ? m->Beg() : 0;
 }
 
 LsanMetadata::LsanMetadata(uptr chunk) {

 }  // 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) {
   return size;
 }
 
 int __sanitizer_get_ownership(const void *p) {
   uptr ptr = reinterpret_cast<uptr>(p);
   return instance.AllocationSize(ptr) > 0;
 }
 
 uptr __sanitizer_get_allocated_size(const void *p) {
   if (!p) return 0;
   uptr ptr = reinterpret_cast<uptr>(p);
   uptr allocated_size = instance.AllocationSize(ptr);
   // Die if p is not malloced or if it is already freed.
   if (allocated_size == 0) {
     GET_STACK_TRACE_FATAL_HERE;
     ReportSanitizerGetAllocatedSizeNotOwned(ptr, &stack);

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);
-  }
-  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();
+    result = REAL(pthread_create)(thread, attr, asan_thread_start, t);
   }
   return result;
 }
 
 INTERCEPTOR(int, pthread_join, void *t, void **arg) {
   return real_pthread_join(t, arg);
 }
 
 DEFINE_REAL_PTHREAD_FUNCTIONS
 #endif  // ASAN_INTERCEPT_PTHREAD_CREATE
 

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);