napi: keep async cleanup hook handles alive until the addon removes them

src/jsc/bindings/napi.h

@@ -30,8 +30,6 @@ extern "C" void napi_internal_threadsafe_function_env_teardown(void* tsfn);
 extern "C" void napi_internal_suppress_crash_on_abort_if_desired();
 extern "C" void Bun__crashHandler(const char* message, size_t message_len);
 
-static bool equal(napi_async_cleanup_hook_handle, napi_async_cleanup_hook_handle);
-
 namespace Napi {
 
 static constexpr int DEFAULT_NAPI_VERSION = 10;
@@ -80,15 +78,7 @@ struct AsyncCleanupHook : CleanupHook {
 
     bool operator==(const AsyncCleanupHook& other) const
     {
-        if (this == &other || (function == other.function && data == other.data)) {
-            if (handle && other.handle) {
-                return equal(handle, other.handle);
-            }
-
-            return !handle && !other.handle;
-        }
-
-        return false;
+        return this == &other || (function == other.function && data == other.data && handle == other.handle);
     }
 };
 
@@ -129,27 +119,18 @@ using HookSet = std::unordered_set<EitherCleanupHook, EitherCleanupHook::Hash>;
 napi_status defineProperty(napi_env env, JSC::JSObject* to, const napi_property_descriptor& property, JSC::ThrowScope& scope);
 }
 
+// Owned by the addon: allocated by napi_add_async_cleanup_hook and freed only
+// by napi_remove_async_cleanup_hook, which the addon may call after the hook
+// itself has already run (that call is how it signals completion).
 struct napi_async_cleanup_hook_handle__ {
     napi_env env;
-    Napi::HookSet::iterator iter;
 
-    napi_async_cleanup_hook_handle__(napi_env env, decltype(iter) iter)
+    explicit napi_async_cleanup_hook_handle__(napi_env env)
         : env(env)
-        , iter(iter)
     {
     }
-
-    bool operator==(const napi_async_cleanup_hook_handle__& other) const
-    {
-        return this == &other || (env == other.env && iter == other.iter);
-    }
 };
 
-static bool equal(napi_async_cleanup_hook_handle one, napi_async_cleanup_hook_handle two)
-{
-    return one == two || *one == *two;
-}
-
 #define NAPI_ABORT(message)                                    \
     do {                                                       \
         napi_internal_suppress_crash_on_abort_if_desired();    \
@@ -382,33 +363,28 @@ struct NapiEnv : public WTF::RefCounted<NapiEnv> {
             }
         }
 
-        auto handle = std::make_unique<napi_async_cleanup_hook_handle__>(this, m_cleanupHooks.end());
+        auto handle = std::make_unique<napi_async_cleanup_hook_handle__>(this);
 
-        auto [iter, inserted] = m_cleanupHooks.emplace(Napi::AsyncCleanupHook(function, handle.get(), data, ++m_cleanupHookCounter));
+        bool inserted = m_cleanupHooks.emplace(Napi::AsyncCleanupHook(function, handle.get(), data, ++m_cleanupHookCounter)).second;
         NAPI_RELEASE_ASSERT(inserted, "Attempted to add a duplicate async NAPI environment cleanup hook");
-        handle->iter = iter;
         return handle.release();
     }
 
-    bool removeAsyncCleanupHook(napi_async_cleanup_hook_handle handle)
+    // The caller has already rejected null handles (napi_invalid_arg).
+    void removeAsyncCleanupHook(napi_async_cleanup_hook_handle handle)
     {
-        if (handle == nullptr) {
-            return false; // Invalid handle
-        }
-
-        for (const auto& hook : m_cleanupHooks) {
-            if (auto* async = std::get_if<Napi::AsyncCleanupHook>(&hook)) {
+        for (auto iter = m_cleanupHooks.begin(), end = m_cleanupHooks.end(); iter != end; ++iter) {
+            if (auto* async = std::get_if<Napi::AsyncCleanupHook>(&*iter)) {
                 if (async->handle == handle) {
-                    m_cleanupHooks.erase(handle->iter);
-                    delete handle;
-                    return true;
+                    m_cleanupHooks.erase(iter);
+                    break;
                 }
             }
         }
 
-        // Node.js silently ignores removal of non-existent handles
-        // See: node/src/node_api.cc:849-855
-        return false;
+        // Freed unconditionally, matching Node: for an already-drained hook
+        // this call is the addon's completion signal.
+        delete handle;
     }
 
     bool inGC() const
@@ -656,8 +632,9 @@ struct NapiEnv : public WTF::RefCounted<NapiEnv> {
             } else {
                 auto& async = std::get<Napi::AsyncCleanupHook>(hook);
                 ASSERT(async.function != nullptr);
+                // The addon owns the handle and frees it via
+                // napi_remove_async_cleanup_hook, possibly after this returns (#37201).
                 async.function(async.handle, async.data);
-                delete async.handle;
             }
             // Same invariant as the finalizer loop in cleanup(): a hook
             // that leaked an exception must not poison the next hook.

test/napi/napi-app/binding.gyp

@@ -164,6 +164,17 @@
                 "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1",
             ],
         },
+        {
+            "target_name": "test_async_cleanup_hook_tsfn_release",
+            "sources": ["test_async_cleanup_hook_tsfn_release.c"],
+            "include_dirs": ["<!@(node -p \"require('node-addon-api').include\")"],
+            "libraries": [],
+            "dependencies": ["<!(node -p \"require('node-addon-api').gyp\")"],
+            "defines": [
+                "NAPI_DISABLE_CPP_EXCEPTIONS",
+                "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1",
+            ],
+        },
         {
             "target_name": "test_cleanup_hook_duplicates",
             "sources": ["test_cleanup_hook_duplicates.c"],

test/napi/napi-app/module.js

@@ -1259,6 +1259,11 @@ nativeTests.test_async_cleanup_hook_remove_nonexistent = () => {
   addon.test();
 };
 
+nativeTests.test_async_cleanup_hook_tsfn_release = () => {
+  const addon = require("./build/Debug/test_async_cleanup_hook_tsfn_release.node");
+  addon.start();
+};
+
 nativeTests.test_cleanup_hook_duplicates = () => {
   const addon = require("./build/Debug/test_cleanup_hook_duplicates.node");
   addon.test();

test/napi/napi-app/test_async_cleanup_hook_tsfn_release.c

@@ -0,0 +1,65 @@
+// An async cleanup hook releases a threadsafe function, and the threadsafe
+// function's finalizer (which runs later during env teardown) calls
+// napi_remove_async_cleanup_hook. The handle must stay valid until the addon
+// removes it; freeing it as soon as the hook returns is a use-after-free.
+// See https://github.com/oven-sh/bun/issues/37201
+#define NAPI_EXPERIMENTAL
+#include <node_api.h>
+#include <stdio.h>
+
+// Statuses are printed (and compared against Node's output by the test), so a
+// call that fails without crashing still fails the test.
+#define CHECK(expr)                                  \
+  do {                                               \
+    napi_status status_ = (expr);                    \
+    if (status_ != napi_ok) {                        \
+      printf(#expr " failed: status=%d\n", status_); \
+      fflush(stdout);                                \
+      return NULL;                                   \
+    }                                                \
+  } while (0)
+
+static napi_async_cleanup_hook_handle hook_handle;
+static napi_threadsafe_function tsfn;
+
+static void async_cleanup_hook(napi_async_cleanup_hook_handle handle, void* arg) {
+  printf("async cleanup hook fired\n");
+  fflush(stdout);
+  napi_status status = napi_release_threadsafe_function(tsfn, napi_tsfn_release);
+  printf("released tsfn: status=%d\n", status);
+  fflush(stdout);
+}
+
+static void tsfn_finalize(napi_env env, void* data, void* hint) {
+  printf("tsfn finalize: removing async cleanup hook\n");
+  fflush(stdout);
+  napi_status status = napi_remove_async_cleanup_hook(hook_handle);
+  printf("async cleanup hook removed: status=%d\n", status);
+  fflush(stdout);
+}
+
+static void call_js_cb(napi_env env, napi_value js_cb, void* ctx, void* data) {}
+
+static napi_value noop(napi_env env, napi_callback_info info) {
+  return NULL;
+}
+
+static napi_value start(napi_env env, napi_callback_info info) {
+  napi_value name;
+  CHECK(napi_create_string_utf8(env, "repro", NAPI_AUTO_LENGTH, &name));
+  napi_value js_cb;
+  CHECK(napi_create_function(env, "noop", NAPI_AUTO_LENGTH, noop, NULL, &js_cb));
+  CHECK(napi_create_threadsafe_function(env, js_cb, NULL, name, 0, 1, NULL, tsfn_finalize, NULL, call_js_cb, &tsfn));
+  CHECK(napi_unref_threadsafe_function(env, tsfn));
+  CHECK(napi_add_async_cleanup_hook(env, async_cleanup_hook, NULL, &hook_handle));
+  return NULL;
+}
+
+napi_value Init(napi_env env, napi_value exports) {
+  napi_value fn;
+  napi_create_function(env, "start", NAPI_AUTO_LENGTH, start, NULL, &fn);
+  napi_set_named_property(env, exports, "start", fn);
+  return exports;
+}
+
+NAPI_MODULE(NODE_GYP_MODULE_NAME, Init)

test/napi/napi.test.ts

@@ -1734,6 +1734,22 @@ describe.skipIf(!canBuildNodeAddons())("cleanup hooks", () => {
       // Test that removing non-existent async hooks doesn't crash
       await checkSameOutput("test_async_cleanup_hook_remove_nonexistent", []);
     });
+
+    it("hook handle stays valid until the addon removes it (#37201)", async () => {
+      // An async cleanup hook releases a threadsafe function whose finalizer
+      // then calls napi_remove_async_cleanup_hook; the handle must not be
+      // freed when the hook returns.
+      const output = await checkSameOutput("test_async_cleanup_hook_tsfn_release", []);
+      // Pin the successful lifecycle, so matching Node on a shared failure
+      // (non-zero status in both) cannot pass. printf() via the Windows CRT
+      // emits \r\n, so split on either ending.
+      expect(output.split(/\r?\n/).slice(-4)).toEqual([
+        "async cleanup hook fired",
+        "released tsfn: status=0",
+        "tsfn finalize: removing async cleanup hook",
+        "async cleanup hook removed: status=0",
+      ]);
+    });
   });
 
   describe("duplicate prevention", () => {