From d8a0f76de7bd98dc7a271bc15b39a4cdbfdf6ecb Mon Sep 17 00:00:00 2001 From: Dmitry Vyukov Date: Wed, 25 Mar 2020 17:03:29 +0100 Subject: [PATCH] tsan: fix leak of ThreadSignalContext for fibers When creating and destroying fibers in tsan a thread state is created and destroyed. Currently, a memory mapping is leaked with each fiber (in __tsan_destroy_fiber). This causes applications with many short running fibers to crash or hang because of linux vm.max_map_count. The root of this is that ThreadState holds a pointer to ThreadSignalContext for handling signals. The initialization and destruction of it is tied to platform specific events in tsan_interceptors_posix and missed when destroying a fiber (specifically, SigCtx is used to lazily create the ThreadSignalContext in tsan_interceptors_posix). This patch cleans up the memory by inverting the control from the platform specific code calling the generic ThreadFinish to ThreadFinish calling a platform specific clean-up routine after finishing a thread. The relevant code causing the leak with fibers is the fiber destruction: void FiberDestroy(ThreadState *thr, uptr pc, ThreadState *fiber) { FiberSwitchImpl(thr, fiber); ThreadFinish(fiber); FiberSwitchImpl(fiber, thr); internal_free(fiber); } I would appreciate feedback if this way of fixing the leak is ok. Also, I think it would be worthwhile to more closely look at the lifecycle of ThreadState (i.e. it uses no constructor/destructor, thus requiring manual callbacks for cleanup) and how OS-Threads/user level fibers are differentiated in the codebase. I would be happy to contribute more if someone could point me at the right place to discuss this issue. Reviewed-in: https://reviews.llvm.org/D76073 Author: Florian (Florian) --- .../lib/tsan/rtl/tsan_interceptors_posix.cpp | 26 ++++--- compiler-rt/lib/tsan/rtl/tsan_platform.h | 2 +- compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp | 2 +- compiler-rt/lib/tsan/rtl/tsan_platform_mac.cpp | 2 +- compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp | 1 + compiler-rt/test/tsan/fiber_cleanup.cpp | 88 ++++++++++++++++++++++ 6 files changed, 106 insertions(+), 15 deletions(-) create mode 100644 compiler-rt/test/tsan/fiber_cleanup.cpp diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp index a623f4f..ad5db79 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp @@ -885,19 +885,21 @@ STDCXX_INTERCEPTOR(void, __cxa_guard_abort, atomic_uint32_t *g) { } namespace __tsan { -void DestroyThreadState() { - ThreadState *thr = cur_thread(); - Processor *proc = thr->proc(); - ThreadFinish(thr); - ProcUnwire(proc, thr); - ProcDestroy(proc); +void PlatformThreadFinished(ThreadState *thr) { ThreadSignalContext *sctx = thr->signal_ctx; if (sctx) { - thr->signal_ctx = 0; + thr->signal_ctx = nullptr; UnmapOrDie(sctx, sizeof(*sctx)); } - DTLS_Destroy(); - cur_thread_finalize(); + + if (thr->tctx->thread_type != ThreadType::Fiber) { + CHECK_EQ(thr, cur_thread()); + Processor *proc = thr->proc(); + ProcUnwire(proc, thr); + ProcDestroy(proc); + DTLS_Destroy(); + cur_thread_finalize(); + } } } // namespace __tsan @@ -912,7 +914,7 @@ static void thread_finalize(void *v) { } return; } - DestroyThreadState(); + ThreadFinish(cur_thread()); } #endif @@ -2551,7 +2553,7 @@ TSAN_INTERCEPTOR(void *, __tls_get_addr, void *arg) { #if SANITIZER_NETBSD TSAN_INTERCEPTOR(void, _lwp_exit) { SCOPED_TSAN_INTERCEPTOR(_lwp_exit); - DestroyThreadState(); + ThreadFinish(cur_thread()); REAL(_lwp_exit)(); } #define TSAN_MAYBE_INTERCEPT__LWP_EXIT TSAN_INTERCEPT(_lwp_exit) @@ -2562,7 +2564,7 @@ TSAN_INTERCEPTOR(void, _lwp_exit) { #if SANITIZER_FREEBSD TSAN_INTERCEPTOR(void, thr_exit, tid_t *state) { SCOPED_TSAN_INTERCEPTOR(thr_exit, state); - DestroyThreadState(); + ThreadFinish(cur_thread()); REAL(thr_exit(state)); } #define TSAN_MAYBE_INTERCEPT_THR_EXIT TSAN_INTERCEPT(thr_exit) diff --git a/compiler-rt/lib/tsan/rtl/tsan_platform.h b/compiler-rt/lib/tsan/rtl/tsan_platform.h index 63eb14f..66c2ab3 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_platform.h +++ b/compiler-rt/lib/tsan/rtl/tsan_platform.h @@ -1020,7 +1020,7 @@ int call_pthread_cancel_with_cleanup(int(*fn)(void *c, void *m, void *abstime), void *c, void *m, void *abstime, void(*cleanup)(void *arg), void *arg); -void DestroyThreadState(); +void PlatformThreadFinished(ThreadState *thr); } // namespace __tsan diff --git a/compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp b/compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp index 33fa586..84f5619 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp @@ -460,7 +460,7 @@ void ReplaceSystemMalloc() { } #if !SANITIZER_GO #if SANITIZER_ANDROID // On Android, one thread can call intercepted functions after -// DestroyThreadState(), so add a fake thread state for "dead" threads. +// ThreadFinish(), so add a fake thread state for "dead" threads. static ThreadState *dead_thread_state = nullptr; ThreadState *cur_thread() { diff --git a/compiler-rt/lib/tsan/rtl/tsan_platform_mac.cpp b/compiler-rt/lib/tsan/rtl/tsan_platform_mac.cpp index fdda701..7d4bbe3 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_platform_mac.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_platform_mac.cpp @@ -226,7 +226,7 @@ static void my_pthread_introspection_hook(unsigned int event, pthread_t thread, if (thread == pthread_self()) { ThreadState *thr = cur_thread(); if (thr->tctx) { - DestroyThreadState(); + ThreadFinish(thr); } } } diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp index 98beb5a..107eabd 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp @@ -144,6 +144,7 @@ void ThreadContext::OnFinished() { thr->clock.ResetCached(&thr->proc()->clock_cache); #if !SANITIZER_GO thr->last_sleep_clock.ResetCached(&thr->proc()->clock_cache); + PlatformThreadFinished(thr); #endif thr->~ThreadState(); #if TSAN_COLLECT_STATS diff --git a/compiler-rt/test/tsan/fiber_cleanup.cpp b/compiler-rt/test/tsan/fiber_cleanup.cpp new file mode 100644 index 0000000..a9552d3 --- /dev/null +++ b/compiler-rt/test/tsan/fiber_cleanup.cpp @@ -0,0 +1,88 @@ +// RUN: %clang_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s +// REQUIRES: linux +#include "test.h" + +//#include +//#include +//#include + +#include +#include +#include + +//long count_memory_mappings() { +// pid_t my_pid = getpid(); +// std::ifstream proc_file{"/proc/" + std::to_string(my_pid) + "/maps"}; +// +// std::string line; +// long line_count{0}; +// while (std::getline(proc_file, line)) { +// line_count++; +// } +// +// return line_count; +//} + +long count_memory_mappings() { + pid_t my_pid = getpid(); + char proc_file_name[128]; + snprintf(proc_file_name, 128, "/proc/%ld/maps", my_pid); + + FILE *proc_file = fopen(proc_file_name, "r"); + long line_count = 0; + char c; + do { + c = fgetc(proc_file); + if (c == '\n') { + line_count++; + } + } while (c != EOF); + fclose(proc_file); + + return line_count; +} + +void fiber_iteration() { + void *orig_fiber = __tsan_get_current_fiber(); + void *fiber = __tsan_create_fiber(0); + + pthread_mutex_t mutex; + pthread_mutex_init(&mutex, NULL); + + // Running some code on the fiber that triggers handling of pending signals. + __tsan_switch_to_fiber(fiber, 0); + pthread_mutex_lock(&mutex); + pthread_mutex_unlock(&mutex); + __tsan_switch_to_fiber(orig_fiber, 0); + + // We expect the fiber to clean up all resources (here the sigcontext) when destroyed. + __tsan_destroy_fiber(fiber); +} + +// Magic-Number for some warmup iterations, +// as tsan maps some memory for the first runs. +const size_t num_warmup = 100; + +int main() { + for (size_t i = 0; i < num_warmup; i++) { + fiber_iteration(); + } + + long memory_mappings_before = count_memory_mappings(); + fiber_iteration(); + fiber_iteration(); + long memory_mappings_after = count_memory_mappings(); + + // Is there a better way to detect a resource leak in the + // ThreadState object? (i.e. a mmap not being freed) + if (memory_mappings_before == memory_mappings_after) { + fprintf(stderr, "PASS\n"); + } else { + fprintf(stderr, "FAILED\n"); + } + + return 0; +} + +// CHECK-NOT: WARNING: ThreadSanitizer: +// CHECK: PASS -- 2.7.4