fix(wall): don't remove the env cleanup hook from inside itself (#360)

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) <noreply@anthropic.com>

Author: 2 people

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);
+    }
   }
 }
 

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.

suppressions/lsan_suppr.txt

@@ -1 +1,2 @@
-leak:CRYPTO_zalloc
+leak:CRYPTO_zalloc
+leak:ossl_load_builtin_compressions