[ASan, LSan] Improve tracking of thread creation.

[earthdok]

Summary

In the current scheme of things, the call to ThreadStart() in the child
thread is not synchronized with the parent thread. So, if a pointer is passed to
pthread_create, there may be a window of time during which this pointer will not
be discoverable by LSan. I.e. the pthread_create interceptor has already
returneed and thus the pointer is no longer on the parent stack, but we don't
yet know the location of the child stack. This has caused bogus leak reports
(see http://llvm.org/bugs/show_bug.cgi?id=21621/).

This patch makes the pthread_create interceptor wait until the child thread is
properly registered before returning.

[earthdok]

The problem with the solution I proposed in http://llvm.org/bugs/show_bug.cgi?id=21621#c6 is that we don't know the thread's OS PID until ThreadStart() has been called. So, instead of checking whether the thread that interests us is in "created" state, LSan would have to process all "created" threads separately. That's not super complicated, but I like this solution better. At least it brings us one step closer to a unified pthread_create interceptor in ASan/MSan/TSan.

[glider]

I don't think it's a good idea to stop the parent until the child is
created. Do we do that in other tools?

[earthdok]

TSan does this.

[earthdok]

• test

[samsonov]

lib/asan/asan_interceptors.cc

202

Reusing the same atomic_uintptr_t for different purpose looks kind of confusing. IIUC first we use it to publish the pointer to AsanThread*, and then use it to signal that the thread is registered (and you need one more argument to AsanThread::ThreadStart for this purpose).

Would it be cleaner to have

atomic_uintptr_t registered

as a field of AsanThread object? You can atomically set it inside ThreadStart() and call t->WaitUntilRegistered() here.

[earthdok]

lib/asan/asan_interceptors.cc

202

I agree that it's a bit confusing, but I'm averse to bloating AsanThread with a member that would only be used in this scope. How about two atomics in a struct passed as the void* argument to REAL(pthread_create)?

[samsonov]

lib/asan/asan_interceptors.cc

202

sizeof(AsanThread) == 117920. Bloating, huh? :)
IMO flag marking that thread is started qualifies for thread "state" and it would be easier to make it a class member.

[earthdok]

lib/asan/asan_interceptors.cc

202

It is possible for the child thread to set |registered|, run the callback and terminate while the parent thread is waiting in internal_sched_yield(). Then the parent thread will segfault when it attempts to read from |registered|, as the AsanThread object is now unmapped. This actually happens in about 20 tests.

[samsonov]

lib/asan/asan_interceptors.cc

202

Oh, that's funny :) Fine then, let's create a struct with two atomics - one to pass the pointer to AsanThread object, and the other one to signal that thread has started and we can return from pthread_create().

This is somewhat intrusive change, and I think we should test it thoroughly.

[earthdok]

• struct

[samsonov]

I'd appreciate a comment describing a motivation, in AsanThread::ThreadStart decl and/or in pthread_create() interceptor. Thanks!

lib/asan/asan_interceptors.cc

175

nullptr

lib/asan/asan_rtl.cc

684

nullptr

[earthdok]

• add comment and use nullptrs

lib/asan/asan_interceptors.cc

202

Done.

This is somewhat intrusive change, and I think we should test it thoroughly.

Any suggestions?

[earthdok]

BTW, I think the ThreadRegistry::CreateThread() call should be moved into AsanThread::Create(). This code is duplicated across 4 uses of AsanThread::Create(), and anyway most other ThreadRegistry access is done via AsanThread (including ThreadRegistry::FinishThread()).

[earthdok]

Committed r223419

[samsonov]

In D6441#24, @earthdok wrote:

BTW, I think the ThreadRegistry::CreateThread() call should be moved into AsanThread::Create(). This code is duplicated across 4 uses of AsanThread::Create(), and anyway most other ThreadRegistry access is done via AsanThread (including ThreadRegistry::FinishThread()).

I agree, that would be nice.

lib/asan/asan_interceptors.cc

#define COMMON_SYSCALL_POST_READ_RANGE(p, s) \
   } while (false)
 #define COMMON_SYSCALL_POST_WRITE_RANGE(p, s) \
   do {                                        \
     (void)(p);                                \
     (void)(s);                                \
   } while (false)
 #include "sanitizer_common/sanitizer_common_syscalls.inc"
 
+struct ThreadStartParam {
+  atomic_uintptr_t t;
+  atomic_uintptr_t is_registered;
+};
+
 static thread_return_t THREAD_CALLING_CONV asan_thread_start(void *arg) {
-  AsanThread *t = (AsanThread*)arg;
+  ThreadStartParam *param = reinterpret_cast<ThreadStartParam *>(arg);
+  AsanThread *t = nullptr;
+  while ((t = reinterpret_cast<AsanThread *>(
+              atomic_load(&param->t, memory_order_acquire))) == 0)
+    internal_sched_yield();
   SetCurrentThread(t);
-  return t->ThreadStart(GetTid());
+  return t->ThreadStart(GetTid(), &param->is_registered);
 }
 
 #if ASAN_INTERCEPT_PTHREAD_CREATE
 INTERCEPTOR(int, pthread_create, void *thread,
     void *attr, void *(*start_routine)(void*), void *arg) {
   EnsureMainThreadIDIsCorrect();
-  // Strict init-order checking in thread-hostile.
+  // Strict init-order checking is thread-hostile.
   if (flags()->strict_init_order)
     StopInitOrderChecking();
   GET_STACK_TRACE_THREAD;
   int detached = 0;
   if (attr != 0)
     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);
+  int result = REAL(pthread_create)(thread, attr, asan_thread_start, &param);
+  if (result == 0) {
     u32 current_tid = GetCurrentTidOrInvalid();
     AsanThread *t = AsanThread::Create(start_routine, arg);
     CreateThreadContextArgs args = { t, &stack };
-  asanThreadRegistry().CreateThread(*(uptr*)t, detached, current_tid, &args);
-  return REAL(pthread_create)(thread, attr, asan_thread_start, t);
+    asanThreadRegistry().CreateThread(*reinterpret_cast<uptr *>(t), detached,
+                                      current_tid, &args);
+    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();
+  }
+  return result;
 }
 #endif  // ASAN_INTERCEPT_PTHREAD_CREATE
 
 #if ASAN_INTERCEPT_SIGNAL_AND_SIGACTION
 
 #if SANITIZER_ANDROID
 INTERCEPTOR(void*, bsd_signal, int signum, void *handler) {
   if (!AsanInterceptsSignal(signum) ||

 }
 #endif  // ASAN_INTERCEPT_FORK
 
 #if SANITIZER_WINDOWS
 INTERCEPTOR_WINAPI(DWORD, CreateThread,
                    void* security, uptr stack_size,
                    DWORD (__stdcall *start_routine)(void*), void* arg,
                    DWORD thr_flags, void* tid) {
-  // Strict init-order checking in thread-hostile.
+  // Strict init-order checking is thread-hostile.
   if (flags()->strict_init_order)
     StopInitOrderChecking();
   GET_STACK_TRACE_THREAD;
   u32 current_tid = GetCurrentTidOrInvalid();
   AsanThread *t = AsanThread::Create(start_routine, arg);
   CreateThreadContextArgs args = { t, &stack };
   bool detached = false;  // FIXME: how can we determine it on Windows?
   asanThreadRegistry().CreateThread(*(uptr*)t, detached, current_tid, &args);

lib/asan/asan_rtl.cc

#endif
 
   // Create main thread.
   AsanThread *main_thread = AsanThread::Create(0, 0);
   CreateThreadContextArgs create_main_args = { main_thread, 0 };
   u32 main_tid = asanThreadRegistry().CreateThread(
       0, true, 0, &create_main_args);
   CHECK_EQ(0, main_tid);
   SetCurrentThread(main_thread);
-  main_thread->ThreadStart(internal_getpid());
+  main_thread->ThreadStart(internal_getpid(),
+                           /* signal_thread_is_registered */ nullptr);
   force_interface_symbols();  // no-op.
   SanitizerInitializeUnwinder();
 
 #if CAN_SANITIZE_LEAKS
   __lsan::InitCommonLsan(false);
   if (common_flags()->detect_leaks && common_flags()->leak_check_at_exit) {
     Atexit(__lsan::DoLeakCheck);
   }

lib/asan/asan_thread.h

 // AsanThread are stored in TSD and destroyed when the thread dies.
 class AsanThread {
  public:
   static AsanThread *Create(thread_callback_t start_routine, void *arg);
   static void TSDDtor(void *tsd);
   void Destroy();
 
   void Init();  // Should be called from the thread itself.
-  thread_return_t ThreadStart(uptr os_id);
+  thread_return_t ThreadStart(uptr os_id,
+                              atomic_uintptr_t *signal_thread_is_registered);
 
   uptr stack_top() { return stack_top_; }
   uptr stack_bottom() { return stack_bottom_; }
   uptr stack_size() { return stack_size_; }
   uptr tls_begin() { return tls_begin_; }
   uptr tls_end() { return tls_end_; }
   u32 tid() { return context_->tid; }
   AsanThreadContext *context() { return context_; }

lib/asan/asan_thread.cc

void AsanThread::Init() {
   ClearShadowForThreadStackAndTLS();
   int local = 0;
   VReport(1, "T%d: stack [%p,%p) size 0x%zx; local=%p\n", tid(),
           (void *)stack_bottom_, (void *)stack_top_, stack_top_ - stack_bottom_,
           &local);
   AsanPlatformThreadInit();
 }
 
-thread_return_t AsanThread::ThreadStart(uptr os_id) {
+thread_return_t AsanThread::ThreadStart(
+    uptr os_id, atomic_uintptr_t *signal_thread_is_registered) {
   Init();
   asanThreadRegistry().StartThread(tid(), os_id, 0);
+  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);
     return 0;

lib/lsan/lsan_interceptors.cc

extern "C" void *__lsan_thread_start_func(void *arg) {
   if (pthread_setspecific(g_thread_finalize_key,
                           (void*)kPthreadDestructorIterations)) {
     Report("LeakSanitizer: failed to set thread key.\n");
     Die();
   }
   int tid = 0;
   while ((tid = atomic_load(&p->tid, memory_order_acquire)) == 0)
     internal_sched_yield();
-  atomic_store(&p->tid, 0, memory_order_release);
   SetCurrentThread(tid);
   ThreadStart(tid, GetTid());
+  atomic_store(&p->tid, 0, memory_order_release);
   return callback(param);
 }
 
 INTERCEPTOR(int, pthread_create, void *th, void *attr,
             void *(*callback)(void *), void *param) {
   ENSURE_LSAN_INITED;
   EnsureMainThreadIDIsCorrect();
   __sanitizer_pthread_attr_t myattr;

test/lsan/TestCases/leak_check_before_thread_started.cc

+// Regression test for http://llvm.org/bugs/show_bug.cgi?id=21621
+// This test relies on timing between threads, so any failures will be flaky.
+// RUN: LSAN_BASE="use_stacks=0:use_registers=0"
+// RUN: %clangxx_lsan %s -std=c++11 -o %t
+// RUN: %run %t
+#include <thread>
+#include <chrono>
+
+void func() {
+      std::this_thread::sleep_for(std::chrono::milliseconds(500));
+}
+
+int main() {
+      std::thread(func).detach();
+}