Revert "[ASAN] Use ThreadArgRetval in ASAN"

https://bugs.chromium.org/p/chromium/issues/detail?id=1445676

This reverts commit 1030bd181eb74b67b7ea51631ce4becca410c406.

6 files changed, 44 insertions, 65 deletions

diff --git a/compiler-rt/lib/asan/asan_interceptors.cpp b/compiler-rt/lib/asan/asan_interceptors.cpp
index 2595a6adda06..b8585c4df5b0 100644
--- a/compiler-rt/lib/asan/asan_interceptors.cpp
+++ b/compiler-rt/lib/asan/asan_interceptors.cpp
@@ -199,16 +199,11 @@ DECLARE_REAL_AND_INTERCEPTOR(void, free, void *)
 static thread_return_t THREAD_CALLING_CONV asan_thread_start(void *arg) {
   AsanThread *t = (AsanThread *)arg;
   SetCurrentThread(t);
-  auto self = GetThreadSelf();
-  auto args = asanThreadArgRetval().GetArgs(self);
-  thread_return_t retval = t->ThreadStart(GetTid());
-  asanThreadArgRetval().Finish(self, retval);
-  CHECK_EQ(args.arg_retval, t->get_arg());
-  return retval;
+  return t->ThreadStart(GetTid());
 }
 
-INTERCEPTOR(int, pthread_create, void *thread, void *attr,
-            void *(*start_routine)(void *), void *arg) {
+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)
@@ -228,13 +223,10 @@ INTERCEPTOR(int, pthread_create, void *thread, void *attr,
     // 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
+#if CAN_SANITIZE_LEAKS
     __lsan::ScopedInterceptorDisabler disabler;
-#    endif
-    asanThreadArgRetval().Create(detached, {start_routine, arg}, [&]() -> uptr {
-      result = REAL(pthread_create)(thread, attr, asan_thread_start, t);
-      return result ? 0 : *(uptr *)(thread);
-    });
+#endif
+    result = REAL(pthread_create)(thread, attr, asan_thread_start, t);
   }
   if (result != 0) {
     // If the thread didn't start delete the AsanThread to avoid leaking it.
@@ -246,48 +238,27 @@ INTERCEPTOR(int, pthread_create, void *thread, void *attr,
 }
 
 INTERCEPTOR(int, pthread_join, void *thread, void **retval) {
-  int result;
-  asanThreadArgRetval().Join((uptr)thread, [&]() {
-    result = REAL(pthread_join)(thread, retval);
-    return !result;
-  });
-  return result;
+  return REAL(pthread_join)(thread, retval);
 }
 
 INTERCEPTOR(int, pthread_detach, void *thread) {
-  int result;
-  asanThreadArgRetval().Detach((uptr)thread, [&](){
-    result = REAL(pthread_detach)(thread);
-    return !result;
-  });
-  return result;
+  return REAL(pthread_detach)(thread);
 }
 
 INTERCEPTOR(int, pthread_exit, void *retval) {
-  asanThreadArgRetval().Finish(GetThreadSelf(), retval);
   return REAL(pthread_exit)(retval);
 }
 
 #    if ASAN_INTERCEPT_TRYJOIN
 INTERCEPTOR(int, pthread_tryjoin_np, void *thread, void **ret) {
-  int result;
-  asanThreadArgRetval().Join((uptr)thread, [&]() {
-    result = REAL(pthread_tryjoin_np)(thread, ret);
-    return !result;
-  });
-  return result;
+  return REAL(pthread_tryjoin_np)(thread, ret);
 }
 #    endif
 
 #    if ASAN_INTERCEPT_TIMEDJOIN
 INTERCEPTOR(int, pthread_timedjoin_np, void *thread, void **ret,
             const struct timespec *abstime) {
-  int result;
-  asanThreadArgRetval().Join((uptr)thread, [&]() {
-    result = REAL(pthread_timedjoin_np)(thread, ret, abstime);
-    return !result;
-  });
-  return result;
+  return REAL(pthread_timedjoin_np)(thread, ret, abstime);
 }
 #    endif
 
diff --git a/compiler-rt/lib/asan/asan_thread.cpp b/compiler-rt/lib/asan/asan_thread.cpp
index 343fd079fa02..a1734a6f896d 100644
--- a/compiler-rt/lib/asan/asan_thread.cpp
+++ b/compiler-rt/lib/asan/asan_thread.cpp
@@ -41,8 +41,6 @@ void AsanThreadContext::OnFinished() {
 }
 
 static ThreadRegistry *asan_thread_registry;
-static ThreadArgRetval *thread_data;
-
 static Mutex mu_for_thread_context;
 static LowLevelAllocator allocator_for_thread_context;
 
@@ -65,12 +63,9 @@ static void InitThreads() {
   // MIPS requires aligned address
   static ALIGNED(alignof(
       ThreadRegistry)) char thread_registry_placeholder[sizeof(ThreadRegistry)];
-  static ALIGNED(alignof(
-      ThreadArgRetval)) char thread_data_placeholder[sizeof(ThreadArgRetval)];
 
   asan_thread_registry =
       new (thread_registry_placeholder) ThreadRegistry(GetAsanThreadContext);
-  thread_data = new (thread_data_placeholder) ThreadArgRetval();
   initialized = true;
 }
 
@@ -79,11 +74,6 @@ ThreadRegistry &asanThreadRegistry() {
   return *asan_thread_registry;
 }
 
-ThreadArgRetval &asanThreadArgRetval() {
-  InitThreads();
-  return *thread_data;
-}
-
 AsanThreadContext *GetThreadContextByTidLocked(u32 tid) {
   return static_cast<AsanThreadContext *>(
       asanThreadRegistry().GetThreadLocked(tid));
@@ -494,15 +484,9 @@ __asan::AsanThread *GetAsanThreadByOsIDLocked(tid_t os_id) {
 
 // --- Implementation of LSan-specific functions --- {{{1
 namespace __lsan {
-void LockThreadRegistry() {
-  __asan::asanThreadRegistry().Lock();
-  __asan::asanThreadArgRetval().Lock();
-}
+void LockThreadRegistry() { __asan::asanThreadRegistry().Lock(); }
 
-void UnlockThreadRegistry() {
-  __asan::asanThreadArgRetval().Unlock();
-  __asan::asanThreadRegistry().Unlock();
-}
+void UnlockThreadRegistry() { __asan::asanThreadRegistry().Unlock(); }
 
 static ThreadRegistry *GetAsanThreadRegistryLocked() {
   __asan::asanThreadRegistry().CheckLocked();
@@ -557,7 +541,33 @@ void GetThreadExtraStackRangesLocked(InternalMmapVector<Range> *ranges) {
 }
 
 void GetAdditionalThreadContextPtrsLocked(InternalMmapVector<uptr> *ptrs) {
-  __asan::asanThreadArgRetval().GetAllPtrsLocked(ptrs);
+  GetAsanThreadRegistryLocked()->RunCallbackForEachThreadLocked(
+      [](ThreadContextBase *tctx, void *ptrs) {
+        // 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 =
+            static_cast<__asan::AsanThreadContext *>(tctx);
+
+        // 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>(atctx->thread->get_arg());
+        if (!thread_arg)
+          return;
+
+        auto ptrsVec = reinterpret_cast<InternalMmapVector<uptr> *>(ptrs);
+        ptrsVec->push_back(thread_arg);
+      },
+      ptrs);
 }
 
 void GetRunningThreadsLocked(InternalMmapVector<tid_t> *threads) {
diff --git a/compiler-rt/lib/asan/asan_thread.h b/compiler-rt/lib/asan/asan_thread.h
index c131dd40d864..dff1a0fd85d5 100644
--- a/compiler-rt/lib/asan/asan_thread.h
+++ b/compiler-rt/lib/asan/asan_thread.h
@@ -20,7 +20,6 @@
 #include "asan_stats.h"
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_libc.h"
-#include "sanitizer_common/sanitizer_thread_arg_retval.h"
 #include "sanitizer_common/sanitizer_thread_registry.h"
 
 namespace __sanitizer {
@@ -172,7 +171,6 @@ class AsanThread {
 
 // Returns a single instance of registry.
 ThreadRegistry &asanThreadRegistry();
-ThreadArgRetval &asanThreadArgRetval();
 
 // Must be called under ThreadRegistryLock.
 AsanThreadContext *GetThreadContextByTidLocked(u32 tid);
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp
index bddb28521408..69b19e8f10c9 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp
@@ -75,7 +75,7 @@ void ThreadArgRetval::DetachLocked(uptr thread) {
   CHECK(t);
   CHECK(!t->second.detached);
   if (t->second.done) {
-    // We can't retrive retval after detached thread finished.
+    // Detached thread has no use after it started and returned args.
     data_.erase(t);
     return;
   }
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.h b/compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.h
index c77021beb67d..f2e5af208474 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.h
@@ -69,8 +69,8 @@ class SANITIZER_MUTEX ThreadArgRetval {
   template <typename JoinFn /* returns true on success */>
   void Join(uptr thread, const JoinFn& fn) {
     // Remember internal id of the thread to prevent re-use of the thread
-    // between fn() and AfterJoin() calls. Locking JoinFn, like in
-    // Detach(), implementation can cause deadlock.
+    // between fn() and DetachLocked() calls. We can't just lock JoinFn
+    // like in Detach() implementation.
     auto gen = BeforeJoin(thread);
     if (fn())
       AfterJoin(thread, gen);
diff --git a/compiler-rt/test/lsan/TestCases/create_thread_leak.cpp b/compiler-rt/test/lsan/TestCases/create_thread_leak.cpp
index 4074cd4e540e..10c377658e61 100644
--- a/compiler-rt/test/lsan/TestCases/create_thread_leak.cpp
+++ b/compiler-rt/test/lsan/TestCases/create_thread_leak.cpp
@@ -6,8 +6,8 @@
 // RUN: %run not %t 10 0 0 1 2>&1 | FileCheck %s --check-prefixes=LEAK,LEAK234
 
 // FIXME: Remove "not". There is no leak.
-// False LEAK234 is broken for LSAN.
-// RUN: %run %if lsan-standalone %{ not %} %t 10 0 0 0
+// False LEAK234 is broken for ASAN, LSAN.
+// RUN: %run %if asan %{ not %} %if lsan-standalone %{ not %} %t 10 0 0 0
 
 #include <pthread.h>
 #include <stdlib.h>