From 2dcbdba85406e498b885570c05e9573ddc3e4a81 Mon Sep 17 00:00:00 2001 From: Dmitry Vyukov Date: Wed, 19 Feb 2020 14:18:53 +0100 Subject: [PATCH] tsan: fix pthread_detach with called_from_lib suppressions Generally we ignore interceptors coming from called_from_lib-suppressed libraries. However, we must not ignore critical interceptors like e.g. pthread_create, otherwise runtime will lost track of threads. pthread_detach is one of these interceptors we should not ignore as it affects thread states and behavior of pthread_join which we don't ignore as well. Currently we can produce very obscure false positives. For more context see: https://groups.google.com/forum/#!topic/thread-sanitizer/ecH2P0QUqPs The added test captures this pattern. While we are here rename ThreadTid to ThreadConsumeTid to make it clear that it's not just a "getter", it resets user_id to 0. This lead to confusion recently. Reviewed in https://reviews.llvm.org/D74828 --- .../lib/tsan/rtl/tsan_interceptors_posix.cpp | 14 ++-- compiler-rt/lib/tsan/rtl/tsan_rtl.h | 2 +- compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp | 31 ++++++-- compiler-rt/test/tsan/ignore_lib6.cpp | 74 +++++++++++++++++++ compiler-rt/test/tsan/ignore_lib6.cpp.supp | 1 + 5 files changed, 106 insertions(+), 16 deletions(-) create mode 100644 compiler-rt/test/tsan/ignore_lib6.cpp create mode 100644 compiler-rt/test/tsan/ignore_lib6.cpp.supp diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp index 8aea1e4ec051..a623f4fe589d 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp @@ -1016,7 +1016,7 @@ TSAN_INTERCEPTOR(int, pthread_create, TSAN_INTERCEPTOR(int, pthread_join, void *th, void **ret) { SCOPED_INTERCEPTOR_RAW(pthread_join, th, ret); - int tid = ThreadTid(thr, pc, (uptr)th); + int tid = ThreadConsumeTid(thr, pc, (uptr)th); ThreadIgnoreBegin(thr, pc); int res = BLOCK_REAL(pthread_join)(th, ret); ThreadIgnoreEnd(thr, pc); @@ -1029,8 +1029,8 @@ TSAN_INTERCEPTOR(int, pthread_join, void *th, void **ret) { DEFINE_REAL_PTHREAD_FUNCTIONS TSAN_INTERCEPTOR(int, pthread_detach, void *th) { - SCOPED_TSAN_INTERCEPTOR(pthread_detach, th); - int tid = ThreadTid(thr, pc, (uptr)th); + SCOPED_INTERCEPTOR_RAW(pthread_detach, th); + int tid = ThreadConsumeTid(thr, pc, (uptr)th); int res = REAL(pthread_detach)(th); if (res == 0) { ThreadDetach(thr, pc, tid); @@ -1050,8 +1050,8 @@ TSAN_INTERCEPTOR(void, pthread_exit, void *retval) { #if SANITIZER_LINUX TSAN_INTERCEPTOR(int, pthread_tryjoin_np, void *th, void **ret) { - SCOPED_TSAN_INTERCEPTOR(pthread_tryjoin_np, th, ret); - int tid = ThreadTid(thr, pc, (uptr)th); + SCOPED_INTERCEPTOR_RAW(pthread_tryjoin_np, th, ret); + int tid = ThreadConsumeTid(thr, pc, (uptr)th); ThreadIgnoreBegin(thr, pc); int res = REAL(pthread_tryjoin_np)(th, ret); ThreadIgnoreEnd(thr, pc); @@ -1064,8 +1064,8 @@ TSAN_INTERCEPTOR(int, pthread_tryjoin_np, void *th, void **ret) { TSAN_INTERCEPTOR(int, pthread_timedjoin_np, void *th, void **ret, const struct timespec *abstime) { - SCOPED_TSAN_INTERCEPTOR(pthread_timedjoin_np, th, ret, abstime); - int tid = ThreadTid(thr, pc, (uptr)th); + SCOPED_INTERCEPTOR_RAW(pthread_timedjoin_np, th, ret, abstime); + int tid = ThreadConsumeTid(thr, pc, (uptr)th); ThreadIgnoreBegin(thr, pc); int res = BLOCK_REAL(pthread_timedjoin_np)(th, ret, abstime); ThreadIgnoreEnd(thr, pc); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.h b/compiler-rt/lib/tsan/rtl/tsan_rtl.h index c38fc43a9f84..20f7a99157ab 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl.h +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.h @@ -775,7 +775,7 @@ int ThreadCreate(ThreadState *thr, uptr pc, uptr uid, bool detached); void ThreadStart(ThreadState *thr, int tid, tid_t os_id, ThreadType thread_type); void ThreadFinish(ThreadState *thr); -int ThreadTid(ThreadState *thr, uptr pc, uptr uid); +int ThreadConsumeTid(ThreadState *thr, uptr pc, uptr uid); void ThreadJoin(ThreadState *thr, uptr pc, int tid); void ThreadDetach(ThreadState *thr, uptr pc, int tid); void ThreadFinalize(ThreadState *thr); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp index 0ac1ee99c470..98beb5aad644 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp @@ -285,19 +285,34 @@ void ThreadFinish(ThreadState *thr) { ctx->thread_registry->FinishThread(thr->tid); } -static bool FindThreadByUid(ThreadContextBase *tctx, void *arg) { - uptr uid = (uptr)arg; - if (tctx->user_id == uid && tctx->status != ThreadStatusInvalid) { +struct ConsumeThreadContext { + uptr uid; + ThreadContextBase *tctx; +}; + +static bool ConsumeThreadByUid(ThreadContextBase *tctx, void *arg) { + ConsumeThreadContext *findCtx = (ConsumeThreadContext *)arg; + if (tctx->user_id == findCtx->uid && tctx->status != ThreadStatusInvalid) { + if (findCtx->tctx) { + // Ensure that user_id is unique. If it's not the case we are screwed. + // Something went wrong before, but now there is no way to recover. + // Returning a wrong thread is not an option, it may lead to very hard + // to debug false positives (e.g. if we join a wrong thread). + Report("ThreadSanitizer: dup thread with used id 0x%zx\n", findCtx->uid); + Die(); + } + findCtx->tctx = tctx; tctx->user_id = 0; - return true; } return false; } -int ThreadTid(ThreadState *thr, uptr pc, uptr uid) { - int res = ctx->thread_registry->FindThread(FindThreadByUid, (void*)uid); - DPrintf("#%d: ThreadTid uid=%zu tid=%d\n", thr->tid, uid, res); - return res; +int ThreadConsumeTid(ThreadState *thr, uptr pc, uptr uid) { + ConsumeThreadContext findCtx = {uid, nullptr}; + ctx->thread_registry->FindThread(ConsumeThreadByUid, &findCtx); + int tid = findCtx.tctx ? findCtx.tctx->tid : ThreadRegistry::kUnknownTid; + DPrintf("#%d: ThreadTid uid=%zu tid=%d\n", thr->tid, uid, tid); + return tid; } void ThreadJoin(ThreadState *thr, uptr pc, int tid) { diff --git a/compiler-rt/test/tsan/ignore_lib6.cpp b/compiler-rt/test/tsan/ignore_lib6.cpp new file mode 100644 index 000000000000..b03bde3765e3 --- /dev/null +++ b/compiler-rt/test/tsan/ignore_lib6.cpp @@ -0,0 +1,74 @@ +// RUN: rm -rf %t-dir +// RUN: mkdir %t-dir +// RUN: %clangxx_tsan -O1 %s -DLIB -fPIC -fno-sanitize=thread -shared -o %t-dir/libignore_lib.so +// RUN: %clangxx_tsan -O1 %s %link_libcxx_tsan -o %t-dir/executable +// RUN: %env_tsan_opts=suppressions='%s.supp' %run %t-dir/executable 2>&1 | FileCheck %s + +// Copied from ignore_lib5.cpp: +// REQUIRES: stable-runtime +// UNSUPPORTED: powerpc64le +// UNSUPPORTED: netbsd + +// Test that pthread_detach works in libraries ignored by called_from_lib. +// For more context see: +// https://groups.google.com/forum/#!topic/thread-sanitizer/ecH2P0QUqPs + +#include "test.h" +#include +#include +#include +#include +#include +#include +#include + +#ifndef LIB + +void *thr(void *arg) { + *(volatile long long *)arg = 1; + return 0; +} + +int main(int argc, char **argv) { + std::string lib = std::string(dirname(argv[0])) + "/libignore_lib.so"; + void *h = dlopen(lib.c_str(), RTLD_GLOBAL | RTLD_NOW); + if (h == 0) + exit(printf("failed to load the library (%d)\n", errno)); + void (*libfunc)() = (void (*)())dlsym(h, "libfunc"); + if (libfunc == 0) + exit(printf("failed to find the func (%d)\n", errno)); + libfunc(); + + const int kThreads = 10; + pthread_t t[kThreads]; + volatile long long data[kThreads]; + for (int i = 0; i < kThreads; i++) + pthread_create(&t[i], 0, thr, (void *)&data[i]); + for (int i = 0; i < kThreads; i++) { + pthread_join(t[i], 0); + data[i] = 2; + } + fprintf(stderr, "DONE\n"); +} + +// CHECK-NOT: WARNING: ThreadSanitizer: +// CHECK: DONE +// CHECK-NOT: WARNING: ThreadSanitizer: + +#else // #ifdef LIB + +void *thr(void *p) { + sleep(1); + pthread_detach(pthread_self()); + return 0; +} + +extern "C" void libfunc() { + const int kThreads = 10; + pthread_t t[kThreads]; + for (int i = 0; i < kThreads; i++) + pthread_create(&t[i], 0, thr, 0); + sleep(2); +} + +#endif // #ifdef LIB diff --git a/compiler-rt/test/tsan/ignore_lib6.cpp.supp b/compiler-rt/test/tsan/ignore_lib6.cpp.supp new file mode 100644 index 000000000000..78ec27c94acd --- /dev/null +++ b/compiler-rt/test/tsan/ignore_lib6.cpp.supp @@ -0,0 +1 @@ +called_from_lib:/libignore_lib.so$