[PATCH] D76073: [compiler-rt][tsan] Fix: Leak of ThreadSignalContext memory mapping when destroying fibers.

Dmitry Vyukov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 25 04:17:57 PDT 2020


dvyukov added inline comments.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:895
+
+  if (thr->tctx->thread_type != ThreadType::Fiber) {
+    CHECK_EQ(thr, cur_thread());
----------------
Florian wrote:
> dvyukov wrote:
> > I wonder if it's OK to use tctx here. We already called ctx->thread_registry->FinishThread(thr->tid) and tctx is owner by thread_registry. So by now it probably can be already reused for another thread.
> I looked into it and tctx can indeed be re-used by another thread at this point leading to a race.
> 
> The thread context is an essential part of an alive thread state, therefore I moved the call to the PlatformThreadFinish clean up call into the OnFinished callback of the thread state. In here, the thread context is still valid. My only concern is that we then perform a rather 'heavy' clean up work while holding the mutex of the ThreadRegistry (if threads are created/destroyed at a moderate pace this should be fine).
We could avoid holding the lock around the cleanup by passing ThreadType to the callback. That's the only bit it needs from tctx.
Currently we know this bit, so we don't access tctx at all. With the new code forget this bit when call ThreadFinish and then we try to restore the knowledge of what we are destroying by looking at tctx.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76073/new/

https://reviews.llvm.org/D76073





More information about the llvm-commits mailing list