[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 takes a different approach to solving the leak false positive
telling LSan to ignore the arg object in between the time the child
thread is created and started. Because the object is being ignored its
no longer necessary to wait for the child thread to start and so the
blocking behaviour can be removed.

In more detail:

1. Change pthread_create interceptor and asan_thread_start to no longer have a blocking relationship. This is similar to reverting b029c5101fb49b3577a1c322f42ef9fc616f25bf except we
   • Keep the test case added.
   • Leave the __lsan_thread_start_func changes in place because we haven't changed the blocking behaviour in standalone LSan. We could but I think its best we start with making this change to ASan first.
   • Adapt to source code changes made since the patch was landed.

2. In the pthread_create interceptor call __lsan::IgnoreObjectLocked(arg) to cause the object to be ignored for the purposes of leak detection. This is only done if leak checking is enabled. The interceptor then calls a new method (set_unignore_arg_on_start) to tell the AsanThread that it should unignore the object when it starts.

3. In AsanThread::ThreadStart call __lsan::UnIgnoredObjectLocked(arg) if unignore_arg_on_start_ is true.

rdar://problem/63537240

[delcypher]

compiler-rt/lib/asan/asan_thread.cpp

264

The lint warning here can be ignored. The function is introduced by https://reviews.llvm.org/D94209

[vitalybuka]

compiler-rt/lib/asan/asan_interceptors.cpp

220

probably too artificial but looks like will be broken

p = new char;
pthread_create(... p)
delete p;  // we know that thread is not going to use it
... 

p = new char; // unrelated allocation
Ingore(p)

could be simple and more reliable to change leak sanitizer to walk through all alive threads and include thread->arg_ into frontier?

[vitalybuka]

compiler-rt/lib/asan/asan_interceptors.cpp

220

more practical

shared = new char;
pthread_create(... shared)
pthread_create(... shared)

if the first thread unignores and exists and the second thread are not yet started we may see false leak.
should be fine with proposed lsan change.

[delcypher]

compiler-rt/lib/asan/asan_interceptors.cpp

220

For the benefit of readers, here's what can go wrong in the "For the "we know that thread is not going to use it" example.

Note we are assuming that the second new char gets the same address as the first new char. This wouldn't happen with the quarantine in place which is not the normal runtime configuration so this example is a bit artificial but it is still worth considering.

If the call to Ingore(p) (i.e. __lsan_ignore_object()) happens in between the call to __lsan::IgnoreObjectLocked() in the pthread_create interceptor and the call to __lsan::UnIgnoreObjectLocked() in the child thread then it will be as if that call never happened because the child thread will unignore the object regardless of the Ingore(p) call.

220

Thanks for the examples. They illustrate a significant problem with my approach that I hadn't considered.

Your proposal to have "leak sanitizer to walk through all alive threads and include thread->arg_ into frontier" would work if we could "walk through all alive threads". Unfortunately we can't do that with the removal of the blocking behaviour that this patch tries to achieve.

This is because although we create the AsanThread object in the parent thread (in the pthread_create interceptor) we don't actually register this thread with the thread registry until the child thread starts (registration happens in the call to t->ThreadStart(GetTid()); in the child thread). Because the thread isn't registered with the thread registry in between the exiting of the pthread_create() interceptor and the start of the child thread LSan won't be able to find this unstarted child thread in the thread registry and thus can't ignore its arg.

However, I have two alternative ideas that might work...

Solution 1. Introduce internal APIs that can be used to add/remove pointers from the frontier. Then this patch would call __lsan::AddPtrToFrontier() and __lsan::RemoveAddedPtrFromFrontier() in the pthread_create interceptor (parent thread) and in asan_thread_start() (child thread). The idea here being if leak detection is triggered in between __lsan::AddPtrToFrontier() and __lsan::RemoveAddedPtrFromFrontier() the arg of the not started child thread will be added to the frontier when the frontier is created and thus reporting the arg leak will be suppressed.

We need to do __lsan::RemoveAddedPtrFromFrontier() because that pointer might not be valid forever and the address could get reused for something which would mess up reporting leaks.
It looks like the Frontier object stores LsanMetadata chunks so these new APIs would need to ignore pointers that are not allocated chunks.

Solution 2.

Change the code to basically not do the blocking behaviour when leak checking is off. This technically gets me to my end goal because the situation I care about is where leak checking is off. I don't really like this solution though because it

• Make the code complicated.
• Creates a divergence in behaviour between leaks on vs leaks off which we probably don't have good test coverage for today (most tests probably run with leak checking on).
• Doesn't fix the performance problem when leak checking is on.

What do you think? Do you like either of these solutions or do you have another you'd like to propose?

[vitalybuka]

compiler-rt/lib/asan/asan_interceptors.cpp

211

previously we kept AsanThread only if pthread_create succeed.
now if pthread_create fails we still keep it. We probably should destroy it.

220

Your proposal to have "leak sanitizer to walk through all alive threads and include thread->arg_ into frontier" would work if we could "walk through all alive threads". Unfortunately we can't do that with the removal of the blocking behaviour that this patch tries to achieve.

Your patch still calls AsanThread::Create -> asanThreadRegistry().CreateThread -> adds args into threads_
We don't rely care if it's alive or not. Leak sanitizer can add all args from there into frontier. When thread exists we can set args to null, just in case.

Solution 1.

LGTM, if the one above is not good enough.
However lets call it something like lsan::IgnorePermanently or AddToIgnoreList avoid exposing "Frontier" which is internal lsan term and to show somehow that it's different from IgnoreObjectLocked that ignore does not goes away when object is deallocated.

[delcypher]

compiler-rt/lib/asan/asan_interceptors.cpp

220

Your patch still calls AsanThread::Create -> asanThreadRegistry().CreateThread -> adds args into threads_
We don't rely care if it's alive or not. Leak sanitizer can add all args from there into frontier. When thread exists we can set args to null, just in case.

Oops. You're right, I misread the code. It might be possible to do your original suggestion. I'll give it a try and report back.

[delcypher]

This is superseded by https://reviews.llvm.org/D95183 and https://reviews.llvm.org/D95184 .

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);
+
+#if CAN_SANITIZE_LEAKS
+  if (common_flags()->detect_leaks && arg) {
+    // If `arg` is the only live pointer to a heap object then we need to
+    // ignore it (during leak detection) until the child thread has started.
+    // Otherwise we might falsely report that the object leaked.
+    //
+    // We set a flag to tell the child thread if it should unignore the object.
+    // We should not try to unignore the object if `kIgnoreObjectAlreadyIgnored`
+    // or `kIgnoreObjectInvalid`.
+    if (__lsan::IgnoreObjectLocked(arg) == __lsan::kIgnoreObjectSuccess)
+      t->set_unignore_arg_on_start(true);
+  }
+#endif
+
   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);
 }
 

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 set_unignore_arg_on_start(bool value) { unignore_arg_on_start_ = value; }
+
  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();
private:
   uptr tls_end_;
   DTLS *dtls_;
 
   FakeStack *fake_stack_;
   AsanThreadLocalMallocStorage malloc_storage_;
   AsanStats stats_;
   bool unwinding_;
   uptr extra_spill_area_;
+  bool unignore_arg_on_start_;
 };
 
 // Returns a single instance of registry.
 ThreadRegistry &asanThreadRegistry();
 
 // Must be called under ThreadRegistryLock.
 AsanThreadContext *GetThreadContextByTidLocked(u32 tid);
 

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 CAN_SANITIZE_LEAKS
+  if (common_flags()->detect_leaks && arg_ && unignore_arg_on_start_)
+    (void)__lsan::UnIgnoreObjectLocked(arg_);
+#endif
+
   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;
   }
 
#endif
   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);