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

[vitalybuka]

Summary

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*.

[vitalybuka]

compiler-rt/lib/lsan/lsan_thread.cpp

61

I guess this is going to crash OSX:

In D146351#4207181, @yln wrote:

Hi, thanks Vitaly!

AFAIR, thread_local (and TLS in general) require macOS 10.12 / iOS 10 / watchOS 3 or greater.

Therefore, can we hold off on this a bit?

@thetruestblue is looking into bumping the minimal sanitizer deployment targets for Apple platforms. Blue, when you do that, can you circle back here and apply this patch? This change is a good example for a sanity check: it doesn't work right now but should work afterwards.

I'll update my patch, but is there any news on bumping min OSX version?

[grandinj]

compiler-rt/lib/lsan/lsan_thread.cpp

61

Just #ifdef this out on those platforms, and use the original code?

[MaskRay]

LGTM. The description can be made clearer. One thread may run ThreadRegistry::CreateThread and resize threads_. It's unsafe for another thread to read threads_ concurrently.

[vitalybuka]

store context in thread local

[vitalybuka]

rebase

[MaskRay]

compiler-rt/lib/lsan/lsan_linux.cpp

22–24

Doesn't clang-format prefer ThreadContextLsanBase * to ThreadContextLsanBase*?

[vitalybuka]

compiler-rt/lib/lsan/lsan_linux.cpp

22–24

lsan is configured as
BasedOnStyle: Google
AllowShortIfStatementsOnASingleLine: false
IndentPPDirectives: AfterHash

[MaskRay]

compiler-rt/lib/lsan/lsan_linux.cpp

22–24

OK. I see both A * and A* in the diff context :)

[zequanwu]

Hi, this test thread_context_crash.cpp failed on mac:
Log:

FAIL: LeakSanitizer-Standalone-x86_64 :: TestCases/thread_context_crash.cpp (66201 of 70710)
******************** TEST 'LeakSanitizer-Standalone-x86_64 :: TestCases/thread_context_crash.cpp' FAILED ********************
Script:
--
: 'RUN: at line 2';      /opt/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/./bin/clang  --driver-mode=g++ -O0  -arch x86_64 -stdlib=libc++ -mmacosx-version-min=10.12 -isysroot /opt/s/w/ir/cache/osx_sdk/XCode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk  -gline-tables-only -fsanitize=leak -I/opt/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/test/lsan/../ -O3 -pthread /opt/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/test/lsan/TestCases/thread_context_crash.cpp -o /opt/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/runtimes/runtimes-bins/compiler-rt/test/lsan/X86_64LsanConfig/TestCases/Output/thread_context_crash.cpp.tmp &&  /opt/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/runtimes/runtimes-bins/compiler-rt/test/lsan/X86_64LsanConfig/TestCases/Output/thread_context_crash.cpp.tmp 100
--
Exit Code: 1

Command Output (stderr):
--
Undefined symbols for architecture x86_64:
  "__ZN6__lsan16GetCurrentThreadEv", referenced from:
      __Z9null_funcPv in thread_context_crash-a63dee.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[vitalybuka]

In D148281#4278298, @zequanwu wrote:

Hi, this test thread_context_crash.cpp failed on mac:
Log:

FAIL: LeakSanitizer-Standalone-x86_64 :: TestCases/thread_context_crash.cpp (66201 of 70710)
******************** TEST 'LeakSanitizer-Standalone-x86_64 :: TestCases/thread_context_crash.cpp' FAILED ********************
Script:
--
: 'RUN: at line 2';      /opt/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/./bin/clang  --driver-mode=g++ -O0  -arch x86_64 -stdlib=libc++ -mmacosx-version-min=10.12 -isysroot /opt/s/w/ir/cache/osx_sdk/XCode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk  -gline-tables-only -fsanitize=leak -I/opt/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/test/lsan/../ -O3 -pthread /opt/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/test/lsan/TestCases/thread_context_crash.cpp -o /opt/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/runtimes/runtimes-bins/compiler-rt/test/lsan/X86_64LsanConfig/TestCases/Output/thread_context_crash.cpp.tmp &&  /opt/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/runtimes/runtimes-bins/compiler-rt/test/lsan/X86_64LsanConfig/TestCases/Output/thread_context_crash.cpp.tmp 100
--
Exit Code: 1

Command Output (stderr):
--
Undefined symbols for architecture x86_64:
  "__ZN6__lsan16GetCurrentThreadEv", referenced from:
      __Z9null_funcPv in thread_context_crash-a63dee.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

do you have a link to the builder?

[zequanwu]

In D148281#4278316, @vitalybuka wrote:

In D148281#4278298, @zequanwu wrote:

Hi, this test thread_context_crash.cpp failed on mac:
Log:

FAIL: LeakSanitizer-Standalone-x86_64 :: TestCases/thread_context_crash.cpp (66201 of 70710)
******************** TEST 'LeakSanitizer-Standalone-x86_64 :: TestCases/thread_context_crash.cpp' FAILED ********************
Script:
--
: 'RUN: at line 2';      /opt/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/./bin/clang  --driver-mode=g++ -O0  -arch x86_64 -stdlib=libc++ -mmacosx-version-min=10.12 -isysroot /opt/s/w/ir/cache/osx_sdk/XCode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk  -gline-tables-only -fsanitize=leak -I/opt/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/test/lsan/../ -O3 -pthread /opt/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/test/lsan/TestCases/thread_context_crash.cpp -o /opt/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/runtimes/runtimes-bins/compiler-rt/test/lsan/X86_64LsanConfig/TestCases/Output/thread_context_crash.cpp.tmp &&  /opt/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/runtimes/runtimes-bins/compiler-rt/test/lsan/X86_64LsanConfig/TestCases/Output/thread_context_crash.cpp.tmp 100
--
Exit Code: 1

Command Output (stderr):
--
Undefined symbols for architecture x86_64:
  "__ZN6__lsan16GetCurrentThreadEv", referenced from:
      __Z9null_funcPv in thread_context_crash-a63dee.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

do you have a link to the builder?

Yes, it's here: https://ci.chromium.org/ui/p/chromium/builders/try/mac_upload_clang/3593/overview

I haven't confirmed this patch is the cause, but it looks very likely. I'm trying to setup the environment for build llvm on my mac to confirm it.

[vitalybuka]

In D148281#4278339, @zequanwu wrote:

In D148281#4278316, @vitalybuka wrote:

In D148281#4278298, @zequanwu wrote:

Hi, this test thread_context_crash.cpp failed on mac:
Log:

FAIL: LeakSanitizer-Standalone-x86_64 :: TestCases/thread_context_crash.cpp (66201 of 70710)
******************** TEST 'LeakSanitizer-Standalone-x86_64 :: TestCases/thread_context_crash.cpp' FAILED ********************
Script:
--
: 'RUN: at line 2';      /opt/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/./bin/clang  --driver-mode=g++ -O0  -arch x86_64 -stdlib=libc++ -mmacosx-version-min=10.12 -isysroot /opt/s/w/ir/cache/osx_sdk/XCode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk  -gline-tables-only -fsanitize=leak -I/opt/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/test/lsan/../ -O3 -pthread /opt/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/test/lsan/TestCases/thread_context_crash.cpp -o /opt/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/runtimes/runtimes-bins/compiler-rt/test/lsan/X86_64LsanConfig/TestCases/Output/thread_context_crash.cpp.tmp &&  /opt/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/runtimes/runtimes-bins/compiler-rt/test/lsan/X86_64LsanConfig/TestCases/Output/thread_context_crash.cpp.tmp 100
--
Exit Code: 1

Command Output (stderr):
--
Undefined symbols for architecture x86_64:
  "__ZN6__lsan16GetCurrentThreadEv", referenced from:
      __Z9null_funcPv in thread_context_crash-a63dee.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

do you have a link to the builder?

Yes, it's here: https://ci.chromium.org/ui/p/chromium/builders/try/mac_upload_clang/3593/overview

I haven't confirmed this patch is the cause, but it looks very likely. I'm trying to setup the environment for build llvm on my mac to confirm it.

Yes. it's very likey, but seems strange, no other OSX bots complained.
Not sure why linker is unhappy. Could this be an issue with incremental build, e.g. it's linking agains older library?
I will disabled darwin for now.

[zequanwu]

Not sure why linker is unhappy. Could this be an issue with incremental build, e.g. it's linking agains older library?

The builder does full build on a clean building directory every time.

I will disabled darwin for now.

Thanks.

[zequanwu]

Just realized that my macbook is M1, but that only fails on x86_64.

[thetruestblue]

Yes, it's here: https://ci.chromium.org/ui/p/chromium/builders/try/mac_upload_clang/3593/overview

I haven't confirmed this patch is the cause, but it looks very likely. I'm trying to setup the environment for build llvm on my mac to confirm it.

Yes. it's very likey, but seems strange, no other OSX bots complained.
Not sure why linker is unhappy. Could this be an issue with incremental build, e.g. it's linking agains older library?
I will disabled darwin for now.

We are seeing this on our Apple internal CI bots as well. Thanks for disabling.

compiler-rt/lib/lsan/lsan.cpp

 }
 
 }  // namespace __lsan
 
 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();
   }
   if (SANITIZER_MIPS && !IsValidFrame(bp, stack_top, stack_bottom))
     return;
   bool fast = StackTrace::WillUseFastUnwind(request_fast);
   Unwind(max_depth, pc, bp, context, stack_top, stack_bottom, fast);
 }

compiler-rt/lib/lsan/lsan_common_mac.cpp

 
 struct RegionScanState {
   SeenRegion seen_regions = SeenRegion::None;
   bool in_libdispatch = false;
 };
 
 typedef struct {
   int disable_counter;
-  u32 current_thread_id;
+  ThreadContextLsanBase *current_thread;
   AllocatorCache cache;
 } thread_local_data_t;
 
 static pthread_key_t key;
 static pthread_once_t key_once = PTHREAD_ONCE_INIT;
 
 // The main thread destructor requires the current thread id,
 // so we can't destroy it until it's been used and reset to invalid tid

 void EnableInThisThread() {
   int *disable_counter = &get_tls_val(true)->disable_counter;
   if (*disable_counter == 0) {
     DisableCounterUnderflow();
   }
   --*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; }
 
 LoadedModule *GetLinker() { return nullptr; }
 
 // Required on Linux for initialization of TLS behavior, but should not be
 // required on Darwin.
 void InitializePlatformSpecificModules() {}

compiler-rt/lib/lsan/lsan_linux.cpp

 //=-- lsan_linux.cpp ------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of LeakSanitizer. Linux/NetBSD/Fuchsia-specific code.
 //
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_common/sanitizer_platform.h"
 
 #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; }
 
 void ReplaceSystemMalloc() {}
 
 } // namespace __lsan
 
 #endif  // SANITIZER_LINUX || SANITIZER_NETBSD || SANITIZER_FUCHSIA

compiler-rt/lib/lsan/lsan_thread.h

 void InitializeThreadRegistry();
 void InitializeMainThread();
 
 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
 
 #endif  // LSAN_THREAD_H

compiler-rt/lib/lsan/lsan_thread.cpp

void InitializeThreadRegistry() {
   static ALIGNED(64) char thread_registry_placeholder[sizeof(ThreadRegistry)];
   thread_registry =
       new (thread_registry_placeholder) ThreadRegistry(CreateThreadContext);
 }
 
 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) {
   return thread_registry->CreateThread(0, detached, parent_tid, arg);
 }
 
 void ThreadContextLsanBase::ThreadStart(u32 tid, tid_t os_id,
                                         ThreadType thread_type, void *arg) {
   thread_registry->StartThread(tid, os_id, thread_type, arg);
 }
 
 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. /////
 
 void GetThreadExtraStackRangesLocked(tid_t os_id,
                                      InternalMmapVector<Range> *ranges) {}
 void GetThreadExtraStackRangesLocked(InternalMmapVector<Range> *ranges) {}
 

compiler-rt/test/lsan/TestCases/thread_context_crash.cpp

+// 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;
+}

compiler-rt/test/lsan/lit.common.cfg.py

 
 # Choose between standalone and LSan+(ASan|HWAsan) modes.
 lsan_lit_test_mode = get_required_attr(config, 'lsan_lit_test_mode')
 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"]
   config.available_features.add('asan')
   if config.host_os == 'NetBSD':
     config.substitutions.insert(0, ('%run', config.netbsd_noaslr_prefix))
 elif lsan_lit_test_mode == "HWAddressSanitizer":
   config.name = "LeakSanitizer-HWAddressSanitizer"