Skip to content

Commit

Permalink
[lsan] Don't crash on ThreadRegistry::threads_ data race
Browse files Browse the repository at this point in the history
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
  • Loading branch information
vitalybuka committed Apr 17, 2023
1 parent 67fed13 commit 7760272
Show file tree
Hide file tree
Showing 7 changed files with 55 additions and 26 deletions.
2 changes: 1 addition & 1 deletion compiler-rt/lib/lsan/lsan.cpp
Expand Up @@ -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();
}
Expand Down
10 changes: 6 additions & 4 deletions compiler-rt/lib/lsan/lsan_common_mac.cpp
Expand Up @@ -50,7 +50,7 @@ struct RegionScanState {

typedef struct {
int disable_counter;
u32 current_thread_id;
ThreadContextLsanBase *current_thread;
AllocatorCache cache;
} thread_local_data_t;

Expand Down Expand Up @@ -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; }

Expand Down
9 changes: 5 additions & 4 deletions compiler-rt/lib/lsan/lsan_linux.cpp
Expand Up @@ -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; }
Expand Down
16 changes: 3 additions & 13 deletions compiler-rt/lib/lsan/lsan_thread.cpp
Expand Up @@ -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) {
Expand All @@ -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. /////
Expand Down
10 changes: 6 additions & 4 deletions compiler-rt/lib/lsan/lsan_thread.h
Expand Up @@ -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
Expand Down
33 changes: 33 additions & 0 deletions 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;
}
1 change: 1 addition & 0 deletions compiler-rt/test/lsan/lit.common.cfg.py
Expand Up @@ -26,6 +26,7 @@ def get_required_attr(config, attr_name):
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"]
Expand Down

0 comments on commit 7760272

Please sign in to comment.