Prevent Thread Exited/Joined events race

Summary:
Add atomic verification to ensure that Thread is Joined after marking it
Finished.

It is required for NetBSD in order to prevent Thread Exited/Joined race,
that may occur when native system libpthread(3) cannot be reliably traced
in a way to guarantee that the mentioned events happen one after another.

This change fixes at least TSan and LSan on NetBSD.

Sponsored by <The NetBSD Foundation>

Reviewers: joerg, dvyukov, vitalybuka

Reviewed By: dvyukov

Subscribers: llvm-commits, kubamracek, #sanitizers

Tags: #sanitizers

Differential Revision: https://reviews.llvm.org/D40294

git-svn-id: https://llvm.org/svn/llvm-project/compiler-rt/trunk@319004 91177308-0d34-0410-b5e6-96231b3b80d8

2 files changed, 35 insertions, 10 deletions

diff --git a/lib/sanitizer_common/sanitizer_thread_registry.cc b/lib/sanitizer_common/sanitizer_thread_registry.cc
index 79f3caad2..d9fd6549b 100644
--- a/lib/sanitizer_common/sanitizer_thread_registry.cc
+++ b/lib/sanitizer_common/sanitizer_thread_registry.cc
@@ -21,6 +21,7 @@ ThreadContextBase::ThreadContextBase(u32 tid)
       status(ThreadStatusInvalid),
       detached(false), workerthread(false), parent_tid(0), next(0) {
   name[0] = '\0';
+  atomic_store(&thread_destroyed, 0, memory_order_release);
 }
 
 ThreadContextBase::~ThreadContextBase() {
@@ -44,6 +45,14 @@ void ThreadContextBase::SetDead() {
   OnDead();
 }
 
+void ThreadContextBase::SetDestroyed() {
+  atomic_store(&thread_destroyed, 1, memory_order_release);
+}
+
+bool ThreadContextBase::GetDestroyed() {
+  return !!atomic_load(&thread_destroyed, memory_order_acquire);
+}
+
 void ThreadContextBase::SetJoined(void *arg) {
   // FIXME(dvyukov): print message and continue (it's user error).
   CHECK_EQ(false, detached);
@@ -85,6 +94,7 @@ void ThreadContextBase::SetCreated(uptr _user_id, u64 _unique_id,
 void ThreadContextBase::Reset() {
   status = ThreadStatusInvalid;
   SetName(0);
+  atomic_store(&thread_destroyed, 0, memory_order_release);
   OnReset();
 }
 
@@ -243,16 +253,25 @@ void ThreadRegistry::DetachThread(u32 tid, void *arg) {
 }
 
 void ThreadRegistry::JoinThread(u32 tid, void *arg) {
-  BlockingMutexLock l(&mtx_);
-  CHECK_LT(tid, n_contexts_);
-  ThreadContextBase *tctx = threads_[tid];
-  CHECK_NE(tctx, 0);
-  if (tctx->status == ThreadStatusInvalid) {
-    Report("%s: Join of non-existent thread\n", SanitizerToolName);
-    return;
-  }
-  tctx->SetJoined(arg);
-  QuarantinePush(tctx);
+  bool destroyed = false;
+  do {
+    {
+      BlockingMutexLock l(&mtx_);
+      CHECK_LT(tid, n_contexts_);
+      ThreadContextBase *tctx = threads_[tid];
+      CHECK_NE(tctx, 0);
+      if (tctx->status == ThreadStatusInvalid) {
+        Report("%s: Join of non-existent thread\n", SanitizerToolName);
+        return;
+      }
+      if ((destroyed = tctx->GetDestroyed())) {
+        tctx->SetJoined(arg);
+        QuarantinePush(tctx);
+      }
+    }
+    if (!destroyed)
+      internal_sched_yield();
+  } while (!destroyed);
 }
 
 // Normally this is called when the thread is about to exit.  If
@@ -281,6 +300,7 @@ void ThreadRegistry::FinishThread(u32 tid) {
     tctx->SetDead();
     QuarantinePush(tctx);
   }
+  tctx->SetDestroyed();
 }
 
 void ThreadRegistry::StartThread(u32 tid, tid_t os_id, bool workerthread,
diff --git a/lib/sanitizer_common/sanitizer_thread_registry.h b/lib/sanitizer_common/sanitizer_thread_registry.h
index 9aae875c7..b203be2f4 100644
--- a/lib/sanitizer_common/sanitizer_thread_registry.h
+++ b/lib/sanitizer_common/sanitizer_thread_registry.h
@@ -50,6 +50,8 @@ class ThreadContextBase {
   u32 parent_tid;
   ThreadContextBase *next;  // For storing thread contexts in a list.
 
+  atomic_uint32_t thread_destroyed; // To address race of Joined vs Finished
+
   void SetName(const char *new_name);
 
   void SetDead();
@@ -60,6 +62,9 @@ class ThreadContextBase {
                   u32 _parent_tid, void *arg);
   void Reset();
 
+  void SetDestroyed();
+  bool GetDestroyed();
+
   // The following methods may be overriden by subclasses.
   // Some of them take opaque arg that may be optionally be used
   // by subclasses.