From 9bb05058de1834ab91a005df7f72067a3d215b1c Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Fri, 26 Jun 2026 15:05:25 +0200 Subject: [PATCH 1/2] fix(wall): don't remove the env cleanup hook from inside itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WallProfiler registers an environment cleanup hook (CleanupHook) via node::AddEnvironmentCleanupHook so it can stop profiling when a worker isolate is terminated without a beforeExit notification. On termination, CleanupHook -> Cleanup -> Dispose called node::RemoveEnvironmentCleanupHook for that same hook. Node's cleanup-hook trampoline (CleanupHookThunkRun) invokes the hook and then dereferences its internal hook record again to remove the hook itself. Removing the hook from within the hook frees that record early, so when our callback returns Node reads freed memory — a use-after-free that segfaults on worker termination (observed reliably on Node 26, exercised by the "should not crash when worker is terminated" test). Add a removeCleanupHook flag to Dispose (default true). Cleanup, which only runs from inside CleanupHook, passes false and lets Node remove the hook itself. The still-running call sites (StopCore, StartImpl failure) keep removing it, since there the hook is live and must not fire later against a destroyed profiler. Reproduced and verified fixed in an Alpine/musl Node 26 container under gdb: crashed within 1-2 runs before, 30/30 clean after. Co-Authored-By: Claude Opus 4.8 (1M context) --- bindings/profilers/wall.cc | 13 +++++++++---- bindings/profilers/wall.hh | 9 ++++++++- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/bindings/profilers/wall.cc b/bindings/profilers/wall.cc index 0fb382f0..95600d3b 100644 --- a/bindings/profilers/wall.cc +++ b/bindings/profilers/wall.cc @@ -467,7 +467,10 @@ void WallProfiler::Cleanup(Isolate* isolate) { if (interceptSignal()) { SignalHandler::DecreaseUseCount(); } - Dispose(isolate); + // We're running inside the environment cleanup hook itself; Node removes + // the hook (and frees its internal record) after we return, so we must not + // remove it here — doing so would be a use-after-free. See Dispose(). + Dispose(isolate, /*removeCleanupHook=*/false); } } @@ -689,7 +692,7 @@ WallProfiler::~WallProfiler() { liveContextPtrHead_ = nullptr; } -void WallProfiler::Dispose(Isolate* isolate) { +void WallProfiler::Dispose(Isolate* isolate, bool removeCleanupHook) { if (cpuProfiler_ != nullptr) { cpuProfiler_->Dispose(); cpuProfiler_ = nullptr; @@ -704,8 +707,10 @@ void WallProfiler::Dispose(Isolate* isolate) { isolate->RemoveGCEpilogueCallback(&GCEpilogueCallback, this); } - node::RemoveEnvironmentCleanupHook( - isolate, &WallProfiler::CleanupHook, isolate); + if (removeCleanupHook) { + node::RemoveEnvironmentCleanupHook( + isolate, &WallProfiler::CleanupHook, isolate); + } } } diff --git a/bindings/profilers/wall.hh b/bindings/profilers/wall.hh index 74a03c2b..5fdf0d22 100644 --- a/bindings/profilers/wall.hh +++ b/bindings/profilers/wall.hh @@ -113,7 +113,14 @@ class WallProfiler : public Nan::ObjectWrap { ContextBuffer contexts_; ~WallProfiler(); - void Dispose(v8::Isolate* isolate); + // removeCleanupHook must be false when Dispose is called from within the + // environment cleanup hook (CleanupHook). Node removes the hook itself after + // invoking it and dereferences its internal hook record again afterwards, so + // calling node::RemoveEnvironmentCleanupHook from inside the hook frees that + // record early and leaves Node reading freed memory (use-after-free crash on + // worker termination). In all other (still-running) call sites the hook is + // live and must be removed so it doesn't fire later against a dead profiler. + void Dispose(v8::Isolate* isolate, bool removeCleanupHook = true); // A new CPU profiler object will be created each time profiling is started // to work around https://bugs.chromium.org/p/v8/issues/detail?id=11051. From c0d6c644fd30341a43f134cc572b3244dd7d1d1e Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Fri, 26 Jun 2026 14:35:42 +0200 Subject: [PATCH 2/2] fix(test): suppress OpenSSL builtin-compressions LSan leak on Node 26 Node 26's OpenSSL populates a global builtin-compressions list during OPENSSL_init_crypto (ossl_load_builtin_compressions) that is never freed at process exit, causing the ASAN job to fail with a 24-byte leak report. This is the same class of benign one-time process-init leak we already suppress via CRYPTO_zalloc, but this path allocates via CRYPTO_malloc and so slips through. Suppress the specific ossl_load_builtin_compressions frame rather than broadening to CRYPTO_malloc, which is OpenSSL's general allocator and could mask genuine leaks in crypto code paths. Co-Authored-By: Claude Opus 4.8 (1M context) --- suppressions/lsan_suppr.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/suppressions/lsan_suppr.txt b/suppressions/lsan_suppr.txt index 2f08beb7..083464a2 100644 --- a/suppressions/lsan_suppr.txt +++ b/suppressions/lsan_suppr.txt @@ -1 +1,2 @@ -leak:CRYPTO_zalloc \ No newline at end of file +leak:CRYPTO_zalloc +leak:ossl_load_builtin_compressions \ No newline at end of file