Fix sampling profiler race when registered thread exits

lib/VM/Profiler/SamplingProfilerPosix.cpp

@@ -9,6 +9,7 @@
 
 #if defined(HERMESVM_SAMPLING_PROFILER_POSIX)
 
+#include "hermes/Support/ErrorHandling.h"
 #include "hermes/Support/Semaphore.h"
 #include "hermes/VM/Callable.h"
 #include "hermes/VM/HostModel.h"
@@ -28,8 +29,11 @@
 #include <chrono>
 #include <cmath>
 #include <csignal>
+#include <memory>
+#include <mutex>
 #include <random>
 #include <thread>
+#include <vector>
 
 namespace hermes {
 namespace vm {
@@ -39,14 +43,32 @@ namespace {
 /// Name of the semaphore.
 constexpr char kSamplingDoneSemaphoreName[] = "/samplingDoneSem";
 
+struct SamplingProfilerPosix;
+
+/// Shared state between a SamplingProfilerPosix and any per-thread guards
+/// that reference it. Outlives whichever of the two is destroyed first, so
+/// thread-death handlers can safely observe "profiler is gone" without a
+/// use-after-free.
+struct ProfilerHandle {
+  std::mutex mu;
+  /// Back-pointer to the owning profiler. Cleared by the profiler's
+  /// destructor, at which point thread-death handlers skip this entry.
+  SamplingProfilerPosix *profiler{nullptr};
+};
+
 struct SamplingProfilerPosix : SamplingProfiler {
   SamplingProfilerPosix(Runtime &rt);
   ~SamplingProfilerPosix() override;
 
-  /// Thread that this profiler instance represents. This can be updated as the
-  /// runtime is invoked on different threads. Must only be accessed while
-  /// holding the runtimeDataLock_.
+  /// Thread that this profiler instance represents. A value of 0 means the
+  /// registered thread has exited or no thread is currently registered.
+  /// Must only be accessed while holding the runtimeDataLock_.
   pthread_t currentThread_;
+
+  /// Shared state with per-thread death guards referencing this profiler.
+  /// Set once at construction, never reassigned. The underlying object
+  /// outlives this profiler if any thread guard still references it.
+  std::shared_ptr<ProfilerHandle> handle_;
 };
 
 struct SamplerPosix : Sampler {
@@ -78,10 +100,69 @@ struct SamplerPosix : Sampler {
   /// Signal handler to walk the stack frames.
   static void profilingSignalHandler(int signo);
 };
+
+/// Per-thread set of profiler handles registered to this thread. On thread
+/// exit, destroyThreadDeathGuard iterates the handles and invalidates each
+/// profiler's registered thread identity (if it still matches) before the
+/// pthread_t becomes invalid and bionic's pthread_kill would abort.
+struct ThreadDeathGuard {
+  std::vector<std::shared_ptr<ProfilerHandle>> handles;
+};
+
+/// Pthread key holding the thread-local ThreadDeathGuard. Initialized
+/// lazily via std::call_once; never deleted (process-lifetime, matching
+/// the SamplerPosix singleton).
+pthread_key_t g_threadDeathGuardKey;
+std::once_flag g_threadDeathGuardKeyInit;
+
+void destroyThreadDeathGuard(void *ptr) {
+  std::unique_ptr<ThreadDeathGuard> guard{static_cast<ThreadDeathGuard *>(ptr)};
+  // For each handle, take handle->mu first (serializes with profiler
+  // destruction), then invoke the Sampler-internal hook which takes
+  // runtimeDataLock_. Lock order handle->mu -> runtimeDataLock_ does not
+  // overlap with the sampler path (profilerLock_ -> runtimeDataLock_), so
+  // there is no deadlock risk.
+  for (auto &h : guard->handles) {
+    std::lock_guard<std::mutex> lock(h->mu);
+    if (h->profiler) {
+      Sampler::onRegisteredThreadExit(h->profiler);
+    }
+  }
+}
+
+void ensureThreadDeathGuardKey() {
+  std::call_once(g_threadDeathGuardKeyInit, [] {
+    int ret =
+        pthread_key_create(&g_threadDeathGuardKey, destroyThreadDeathGuard);
+    if (ret != 0) {
+      hermes_fatal("pthread_key_create failed for sampling profiler");
+    }
+  });
+}
+
+/// Attach \p handle to the calling thread's death guard. Must be called on
+/// the thread that will host the profiler.
+void attachHandleToCurrentThread(std::shared_ptr<ProfilerHandle> handle) {
+  ensureThreadDeathGuardKey();
+  auto *guard = static_cast<ThreadDeathGuard *>(
+      pthread_getspecific(g_threadDeathGuardKey));
+  if (!guard) {
+    guard = new ThreadDeathGuard();
+    pthread_setspecific(g_threadDeathGuardKey, guard);
+  }
+  guard->handles.push_back(std::move(handle));
+}
 } // namespace
 
 SamplingProfilerPosix::SamplingProfilerPosix(Runtime &rt)
-    : SamplingProfiler(rt), currentThread_{pthread_self()} {
+    : SamplingProfiler(rt),
+      currentThread_{pthread_self()},
+      handle_{std::make_shared<ProfilerHandle>()} {
+  // Set the back-pointer before publishing to any other thread. No lock
+  // needed: this profiler is not reachable from any other thread yet.
+  handle_->profiler = this;
+  attachHandleToCurrentThread(handle_);
+
   // Note that we cannot register this in the base class constructor, because
   // all fields must be initialized before we register with the profiling
   // thread.
@@ -92,6 +173,12 @@ SamplingProfilerPosix::~SamplingProfilerPosix() {
   // TODO(T125910634): re-introduce the requirement for destroying the sampling
   // profiler on the same thread in which it was created.
   Sampler::get()->unregisterRuntime(this);
+  // After unregisterRuntime returns, profilerLock_ has been taken and
+  // released, which guarantees no sampler iteration is in-flight for this
+  // profiler. Clear the back-pointer so any thread-death handler still
+  // holding a shared_ptr to handle_ observes profiler==nullptr and skips.
+  std::lock_guard<std::mutex> lock(handle_->mu);
+  handle_->profiler = nullptr;
 }
 
 std::atomic<SamplerPosix *> SamplerPosix::instance_{nullptr};
@@ -213,14 +300,32 @@ void Sampler::platformPostSampleStack(SamplingProfiler *localProfiler) {}
 bool Sampler::platformSuspendVMAndWalkStack(SamplingProfiler *profiler) {
   auto *self = static_cast<SamplerPosix *>(this);
   auto *posixProfiler = static_cast<SamplingProfilerPosix *>(profiler);
+
+  // If the registered thread has exited, skip this sample. The caller
+  // holds runtimeDataLock_, which serializes with the thread-death handler
+  // (see Sampler::onRegisteredThreadExit), so currentThread_ will not be
+  // concurrently invalidated between this check and the pthread_kill
+  // below. This prevents bionic's pthread_kill from aborting on a recycled
+  // pthread_t after a registered JS thread has exited.
+  if (posixProfiler->currentThread_ == 0) {
+    return false;
+  }
+
   // Guarantee that the runtime thread will not proceed until it has
   // acquired the updates to domains_.
   self->profilerForSig_.store(profiler, std::memory_order_release);
 
   // Signal target runtime thread to sample stack. The runtimeDataLock is
   // held by the caller, ensuring the runtime won't start to be used on
   // another thread before sampling begins.
-  pthread_kill(posixProfiler->currentThread_, SIGPROF);
+  int result = pthread_kill(posixProfiler->currentThread_, SIGPROF);
+  if (result != 0) {
+    // On non-Android POSIX, pthread_kill may return ESRCH if the target
+    // terminated in the narrow window where its pthread_t is still in the
+    // thread list but the thread has exited.
+    self->profilerForSig_.store(nullptr, std::memory_order_release);
+    return false;
+  }
 
   // Threading: samplingDoneSem_ will synchronise this thread with the
   // signal handler, so that we only have one active signal at a time.
@@ -250,11 +355,33 @@ bool SamplingProfiler::belongsToCurrentThread() {
 
 void SamplingProfiler::setRuntimeThread() {
   auto profiler = static_cast<sampling_profiler::SamplingProfilerPosix *>(this);
-  std::lock_guard<std::mutex> lock(profiler->runtimeDataLock_);
-  profiler->currentThread_ = pthread_self();
-  threadID_ = oscompat::global_thread_id();
-  threadNames_[threadID_] = oscompat::thread_name();
+  {
+    std::lock_guard<std::mutex> lock(profiler->runtimeDataLock_);
+    profiler->currentThread_ = pthread_self();
+    threadID_ = oscompat::global_thread_id();
+    threadNames_[threadID_] = oscompat::thread_name();
+  }
+  // Register with the new thread's death guard. If an older thread's guard
+  // still references handle_, it will find currentThread_ no longer matches
+  // itself and skip on exit.
+  sampling_profiler::attachHandleToCurrentThread(profiler->handle_);
+}
+
+namespace sampling_profiler {
+void Sampler::onRegisteredThreadExit(SamplingProfiler *profiler) {
+  auto *posix = static_cast<SamplingProfilerPosix *>(profiler);
+  std::lock_guard<std::mutex> lock(posix->runtimeDataLock_);
+  // Only invalidate if currentThread_ still points at the caller. This
+  // correctly handles the setRuntimeThread() case where the profiler has
+  // moved to a different thread before the original thread exits. The
+  // explicit zero check avoids invoking pthread_equal with an invalid
+  // thread ID, which POSIX leaves implementation-defined.
+  if (posix->currentThread_ != 0 &&
+      pthread_equal(posix->currentThread_, pthread_self())) {
+    posix->currentThread_ = 0;
+  }
 }
+} // namespace sampling_profiler
 
 } // namespace vm
 } // namespace hermes

lib/VM/Profiler/SamplingProfilerSampler.h

@@ -116,6 +116,20 @@ struct Sampler {
   /// \return the singleton profiler instance.
   static Sampler *get();
 
+  /// Called by the platform's thread-death infrastructure (e.g. a pthread
+  /// TLS destructor on POSIX) when a thread that registered \p profiler is
+  /// exiting. If \p profiler is still registered to the calling thread,
+  /// invalidates the registered thread so subsequent sample attempts skip
+  /// \p profiler instead of signalling a dead thread. No-op on platforms
+  /// where no thread-death synchronization is required (e.g. Windows).
+  ///
+  /// Caller contract: must be invoked on the dying thread itself. The
+  /// implementation uses pthread_self() (or the platform equivalent) to
+  /// identify which registration to invalidate, so calling from any other
+  /// thread would either be a no-op (wrong identity) or unsafe (if the
+  /// dying thread is no longer in the thread list).
+  static void onRegisteredThreadExit(SamplingProfiler *profiler);
+
  protected:
   Sampler();
 

lib/VM/Profiler/SamplingProfilerWindows.cpp

@@ -137,6 +137,14 @@ void SamplingProfiler::setRuntimeThread() {
   threadNames_[threadID_] = oscompat::thread_name();
 }
 
+namespace sampling_profiler {
+void Sampler::onRegisteredThreadExit(SamplingProfiler *) {
+  // Not applicable on Windows: the sampling path uses HANDLE +
+  // SuspendThread / ResumeThread rather than pthread_kill, and does not
+  // abort on a stale HANDLE.
+}
+} // namespace sampling_profiler
+
 } // namespace vm
 } // namespace hermes
 #endif // !defined(HERMESVM_SAMPLING_PROFILER_WINDOWS)

unittests/VMRuntime/SamplingProfilerTest.cpp

@@ -14,6 +14,9 @@
 
 #include <gtest/gtest.h>
 
+#include <chrono>
+#include <thread>
+
 namespace {
 using namespace hermes::vm;
 
@@ -86,6 +89,41 @@ TEST(SamplingProfilerTest, RegisterIdenticalThread) {
   EXPECT_TRUE(rt->samplingProfiler->belongsToCurrentThread());
 }
 
+// Regression test for https://github.com/facebook/hermes/issues/1853 and
+// https://github.com/getsentry/sentry-react-native/issues/5441: if a thread
+// that registered a profiler exits while the profiler instance lives on, the
+// sampler must not call pthread_kill on the dead thread. On Android bionic
+// this would abort the process; on other POSIX platforms it would be an
+// ESRCH. With the thread-death guard, the sampler skips the profiler.
+TEST(SamplingProfilerTest, SamplingAfterRegisteredThreadExitDoesNotCrash) {
+  auto rt = makeRuntime(withSamplingProfilerEnabled);
+
+  std::thread worker([&]() {
+    rt->samplingProfiler->setRuntimeThread();
+    EXPECT_TRUE(rt->samplingProfiler->belongsToCurrentThread());
+  });
+  // std::thread::join() waits for the OS thread to fully terminate. Per
+  // POSIX, pthread_join does not return until after the thread's C++
+  // thread_local destructors and all pthread_key_create destructors have
+  // run. That means our ThreadDeathGuard destructor -- and therefore the
+  // call to Sampler::onRegisteredThreadExit that invalidates the
+  // registered thread -- has completed by the time join() returns. Do not
+  // relax this ordering assumption when editing the test.
+  worker.join();
+
+  // After the registered thread has exited, the profiler must no longer
+  // report ownership by any live thread.
+  EXPECT_FALSE(rt->samplingProfiler->belongsToCurrentThread());
+
+  SamplingProfiler::enable();
+  std::this_thread::sleep_for(std::chrono::milliseconds(50));
+  SamplingProfiler::disable();
+
+  // Re-register on the main thread so subsequent use works normally.
+  rt->samplingProfiler->setRuntimeThread();
+  EXPECT_TRUE(rt->samplingProfiler->belongsToCurrentThread());
+}
+
 } // namespace
 
 #endif // HERMESVM_SAMPLING_PROFILER_AVAILABLE