[lsan] Don't crash on ThreadRegistry::threads_ data race

Comment "No lock needed" in CurrentThreadContext was wrong.
Concurent ThreadRegistry::CreateThread can resize and relocate
ThreadRegistry::threads_ the same time CurrentThreadContext reads it.

To mitigate lock cost we store ThreadContext* instead of tid in
THREADLOCAL cache, we can tid from the ThreadContext*.

Reviewed By: kstoimenov, MaskRay

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

diff --git a/compiler-rt/lib/lsan/lsan.cpp b/compiler-rt/lib/lsan/lsan.cpp
index 489c5ca..319f399 100644 (file)
--- a/compiler-rt/lib/lsan/lsan.cpp
+++ b/compiler-rt/lib/lsan/lsan.cpp
@@ -36,7 +36,7 @@ void __sanitizer::BufferedStackTrace::UnwindImpl(
     uptr pc, uptr bp, void *context, bool request_fast, u32 max_depth) {
   using namespace __lsan;
   uptr stack_top = 0, stack_bottom = 0;
-  if (ThreadContext *t = CurrentThreadContext()) {
+  if (ThreadContextLsanBase *t = GetCurrentThread()) {
     stack_top = t->stack_end();
     stack_bottom = t->stack_begin();
   }

diff --git a/compiler-rt/lib/lsan/lsan_common_mac.cpp b/compiler-rt/lib/lsan/lsan_common_mac.cpp
index b6b1509..70ff6d5 100644 (file)
--- a/compiler-rt/lib/lsan/lsan_common_mac.cpp
+++ b/compiler-rt/lib/lsan/lsan_common_mac.cpp
@@ -50,7 +50,7 @@ struct RegionScanState {
 
 typedef struct {
   int disable_counter;
-  u32 current_thread_id;
+  ThreadContextLsanBase *current_thread;
   AllocatorCache cache;
 } thread_local_data_t;
 
@@ -99,12 +99,14 @@ void EnableInThisThread() {
   --*disable_counter;
 }
 
-u32 GetCurrentThread() {
+ThreadContextLsanBase *GetCurrentThread() {
   thread_local_data_t *data = get_tls_val(false);
-  return data ? data->current_thread_id : kInvalidTid;
+  return data ? data->current_thread : nullptr;
 }
 
-void SetCurrentThread(u32 tid) { get_tls_val(true)->current_thread_id = tid; }
+void SetCurrentThread(ThreadContextLsanBase *tctx) {
+  get_tls_val(true)->current_thread = tctx;
+}
 
 AllocatorCache *GetAllocatorCache() { return &get_tls_val(true)->cache; }
 

diff --git a/compiler-rt/lib/lsan/lsan_linux.cpp b/compiler-rt/lib/lsan/lsan_linux.cpp
index 47c2f21..5074cee 100644 (file)
--- a/compiler-rt/lib/lsan/lsan_linux.cpp
+++ b/compiler-rt/lib/lsan/lsan_linux.cpp
@@ -14,13 +14,14 @@
 
 #if SANITIZER_LINUX || SANITIZER_NETBSD || SANITIZER_FUCHSIA
 
-#include "lsan_allocator.h"
+#  include "lsan_allocator.h"
+#  include "lsan_thread.h"
 
 namespace __lsan {
 
-static THREADLOCAL u32 current_thread_tid = kInvalidTid;
-u32 GetCurrentThread() { return current_thread_tid; }
-void SetCurrentThread(u32 tid) { current_thread_tid = tid; }
+static THREADLOCAL ThreadContextLsanBase *current_thread = nullptr;
+ThreadContextLsanBase *GetCurrentThread() { return current_thread; }
+void SetCurrentThread(ThreadContextLsanBase *tctx) { current_thread = tctx; }
 
 static THREADLOCAL AllocatorCache allocator_cache;
 AllocatorCache *GetAllocatorCache() { return &allocator_cache; }

diff --git a/compiler-rt/lib/lsan/lsan_thread.cpp b/compiler-rt/lib/lsan/lsan_thread.cpp
index bc05021..f1075af 100644 (file)
--- a/compiler-rt/lib/lsan/lsan_thread.cpp
+++ b/compiler-rt/lib/lsan/lsan_thread.cpp
@@ -39,12 +39,12 @@ void InitializeThreadRegistry() {
 ThreadContextLsanBase::ThreadContextLsanBase(int tid)
     : ThreadContextBase(tid) {}
 
-void ThreadContextLsanBase::OnStarted(void *arg) { SetCurrentThread(tid); }
+void ThreadContextLsanBase::OnStarted(void *arg) { SetCurrentThread(this); }
 
 void ThreadContextLsanBase::OnFinished() {
   AllocatorThreadFinish();
   DTLS_Destroy();
-  SetCurrentThread(kInvalidTid);
+  SetCurrentThread(nullptr);
 }
 
 u32 ThreadCreate(u32 parent_tid, bool detached, void *arg) {
@@ -58,19 +58,9 @@ void ThreadContextLsanBase::ThreadStart(u32 tid, tid_t os_id,
 
 void ThreadFinish() { thread_registry->FinishThread(GetCurrentThreadId()); }
 
-ThreadContext *CurrentThreadContext() {
-  if (!thread_registry)
-    return nullptr;
-  if (GetCurrentThreadId() == kInvalidTid)
-    return nullptr;
-  // No lock needed when getting current thread.
-  return (ThreadContext *)thread_registry->GetThreadLocked(
-      GetCurrentThreadId());
-}
-
 void EnsureMainThreadIDIsCorrect() {
   if (GetCurrentThreadId() == kMainTid)
-    CurrentThreadContext()->os_id = GetTid();
+    GetCurrentThread()->os_id = GetTid();
 }
 
 ///// Interface to the common LSan module. /////

diff --git a/compiler-rt/lib/lsan/lsan_thread.h b/compiler-rt/lib/lsan/lsan_thread.h
index 5abbaff..709a029 100644 (file)
--- a/compiler-rt/lib/lsan/lsan_thread.h
+++ b/compiler-rt/lib/lsan/lsan_thread.h
@@ -51,10 +51,12 @@ ThreadRegistry *GetLsanThreadRegistryLocked();
 u32 ThreadCreate(u32 tid, bool detached, void *arg = nullptr);
 void ThreadFinish();
 
-u32 GetCurrentThread();
-inline u32 GetCurrentThreadId() { return GetCurrentThread(); }
-void SetCurrentThread(u32 tid);
-ThreadContext *CurrentThreadContext();
+ThreadContextLsanBase *GetCurrentThread();
+inline u32 GetCurrentThreadId() {
+  ThreadContextLsanBase *ctx = GetCurrentThread();
+  return ctx ? ctx->tid : kInvalidTid;
+}
+void SetCurrentThread(ThreadContextLsanBase *tctx);
 void EnsureMainThreadIDIsCorrect();
 
 }  // namespace __lsan

diff --git a/compiler-rt/test/lsan/TestCases/thread_context_crash.cpp b/compiler-rt/test/lsan/TestCases/thread_context_crash.cpp
new file mode 100644 (file)
index 0000000..cef6d58
--- /dev/null
+++ b/compiler-rt/test/lsan/TestCases/thread_context_crash.cpp
@@ -0,0 +1,33 @@
+// Check that concurent CurrentThreadContext does not crash.
+// RUN: %clangxx_lsan -O3 -pthread %s -o %t && %run %t 100
+
+// REQUIRES: lsan-standalone
+
+#include <pthread.h>
+#include <stdlib.h>
+#include <vector>
+
+#include <sanitizer/common_interface_defs.h>
+
+namespace __lsan {
+class ThreadContextLsanBase *GetCurrentThread();
+}
+
+void *null_func(void *args) {
+  for (int i = 0; i < 100000; ++i)
+    __lsan::GetCurrentThread();
+  return nullptr;
+}
+
+int main(int argc, char **argv) {
+  std::vector<pthread_t> threads;
+  for (int i = 0; i < atoi(argv[1]); ++i) {
+    threads.resize(10);
+    for (auto &thread : threads)
+      pthread_create(&thread, 0, null_func, NULL);
+
+    for (auto &thread : threads)
+      pthread_join(thread, nullptr);
+  }
+  return 0;
+}

diff --git a/compiler-rt/test/lsan/lit.common.cfg.py b/compiler-rt/test/lsan/lit.common.cfg.py
index c3df68f..cec5664 100644 (file)
--- a/compiler-rt/test/lsan/lit.common.cfg.py
+++ b/compiler-rt/test/lsan/lit.common.cfg.py
@@ -26,6 +26,7 @@ target_arch = getattr(config, 'target_arch', None)
 if lsan_lit_test_mode == "Standalone":
   config.name = "LeakSanitizer-Standalone"
   lsan_cflags = ["-fsanitize=leak"]
+  config.available_features.add('lsan-standalone')
 elif lsan_lit_test_mode == "AddressSanitizer":
   config.name = "LeakSanitizer-AddressSanitizer"
   lsan_cflags = ["-fsanitize=address"]