[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

64

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

64

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 ↗

(On Diff #513804)

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

[vitalybuka]

compiler-rt/lib/lsan/lsan_linux.cpp

22 ↗

(On Diff #513804)

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

[MaskRay]

compiler-rt/lib/lsan/lsan_linux.cpp

22 ↗

(On Diff #513804)

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_thread.cpp

 }
 
 void ThreadFinish() {
   thread_registry->FinishThread(GetCurrentThread());
   SetCurrentThread(kInvalidTid);
 }
 
 ThreadContext *CurrentThreadContext() {
+  static __attribute__((tls_model("initial-exec")))
+  THREADLOCAL ThreadContext *current_thread_context;
+  if (!current_thread_context) {
     if (!thread_registry)
       return nullptr;
     if (GetCurrentThread() == kInvalidTid)
       return nullptr;
-  // No lock needed when getting current thread.
-  return (ThreadContext *)thread_registry->GetThreadLocked(GetCurrentThread());
+    ThreadRegistryLock trl(thread_registry);
+    current_thread_context =
+        (ThreadContext *)thread_registry->GetThreadLocked(GetCurrentThread());
+  }
+  return current_thread_context;
 }
 
 void EnsureMainThreadIDIsCorrect() {
   if (GetCurrentThread() == kMainTid)
     CurrentThreadContext()->os_id = GetTid();
 }
 
 ///// Interface to the common LSan module. /////

compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.h

public:
   uptr GetMaxAliveThreads();
 
   void Lock() SANITIZER_ACQUIRE() { mtx_.Lock(); }
   void CheckLocked() const SANITIZER_CHECK_LOCKED() { mtx_.CheckLocked(); }
   void Unlock() SANITIZER_RELEASE() { mtx_.Unlock(); }
 
   // Should be guarded by ThreadRegistryLock.
   ThreadContextBase *GetThreadLocked(u32 tid) {
-    return threads_.empty() ? nullptr : threads_[tid];
+    return tid < threads_.size() ? threads_[tid] : nullptr;
   }
 
   u32 NumThreadsLocked() const { return threads_.size(); }
 
   u32 CreateThread(uptr user_id, bool detached, u32 parent_tid, void *arg);
 
   typedef void (*ThreadCallback)(ThreadContextBase *tctx, void *arg);
   // Invokes callback with a specified arg for each thread context.

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 ThreadContext *CurrentThreadContext();
+}
+
+void *null_func(void *args) {
+  for (int i = 0; i < 100000; ++i)
+    __lsan::CurrentThreadContext();
+  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"