diff --git a/compiler-rt/lib/asan/asan_allocator.cpp b/compiler-rt/lib/asan/asan_allocator.cpp --- a/compiler-rt/lib/asan/asan_allocator.cpp +++ b/compiler-rt/lib/asan/asan_allocator.cpp @@ -1185,12 +1185,30 @@ } 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. - - // 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. + // 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(asan_thread->get_arg()); + if (!thread_arg) + return; + + auto ptrsVec = reinterpret_cast *>(ptrs); + ptrsVec->push_back(thread_arg); } } // namespace __lsan diff --git a/compiler-rt/lib/asan/asan_interceptors.cpp b/compiler-rt/lib/asan/asan_interceptors.cpp --- a/compiler-rt/lib/asan/asan_interceptors.cpp +++ b/compiler-rt/lib/asan/asan_interceptors.cpp @@ -189,20 +189,11 @@ #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(arg); - AsanThread *t = nullptr; - while ((t = reinterpret_cast( - atomic_load(¶m->t, memory_order_acquire))) == nullptr) - internal_sched_yield(); + AsanThread *t = (AsanThread *)arg; SetCurrentThread(t); - return t->ThreadStart(GetTid(), ¶m->is_registered); + return t->ThreadStart(GetTid()); } INTERCEPTOR(int, pthread_create, void *thread, @@ -215,9 +206,11 @@ int detached = 0; if (attr) REAL(pthread_attr_getdetachstate)(attr, &detached); - ThreadStartParam param; - atomic_store(¶m.t, 0, memory_order_relaxed); - atomic_store(¶m.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 @@ -227,21 +220,13 @@ #if CAN_SANITIZE_LEAKS __lsan::ScopedInterceptorDisabler disabler; #endif - result = REAL(pthread_create)(thread, attr, asan_thread_start, ¶m); + 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(¶m.t, reinterpret_cast(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(¶m.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; } diff --git a/compiler-rt/lib/asan/asan_thread.h b/compiler-rt/lib/asan/asan_thread.h --- a/compiler-rt/lib/asan/asan_thread.h +++ b/compiler-rt/lib/asan/asan_thread.h @@ -69,8 +69,7 @@ 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(); @@ -132,6 +131,8 @@ 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. diff --git a/compiler-rt/lib/asan/asan_thread.cpp b/compiler-rt/lib/asan/asan_thread.cpp --- a/compiler-rt/lib/asan/asan_thread.cpp +++ b/compiler-rt/lib/asan/asan_thread.cpp @@ -253,12 +253,9 @@ // 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(); @@ -288,8 +285,7 @@ /* 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; }