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)

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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
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 <fstream>
+//#include <sstream>
+//#include <string>
+
+#include <pthread.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+//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