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,11 +363,10 @@ 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();
     }
 
@@ -396,19 +376,21 @@ struct NapiEnv : public WTF::RefCounted<NapiEnv> {
             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;
+        // The handle is freed here unconditionally, matching Node.js
+        // (node/src/node_api.cc napi_remove_async_cleanup_hook): when drain()
+        // has already run the hook (and erased it from the set), the addon
+        // still owns the handle and this call is its completion signal.
+        delete handle;
+        return true;
     }
 
     bool inGC() const
@@ -656,8 +638,12 @@ struct NapiEnv : public WTF::RefCounted<NapiEnv> {
             } else {
                 auto& async = std::get<Napi::AsyncCleanupHook>(hook);
                 ASSERT(async.function != nullptr);
+                // The addon keeps ownership of the handle: it frees it via
+                // napi_remove_async_cleanup_hook, possibly after this hook
+                // returns (e.g. from a threadsafe function finalizer that runs
+                // later during cleanup()). Deleting it here is a use-after-free
+                // in that case (#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,51 @@
+// 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>
+
+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_release_threadsafe_function(tsfn, napi_tsfn_release);
+}
+
+static void tsfn_finalize(napi_env env, void* data, void* hint) {
+  printf("tsfn finalize: removing async cleanup hook\n");
+  fflush(stdout);
+  napi_remove_async_cleanup_hook(hook_handle);
+  printf("async cleanup hook removed\n");
+  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;
+  napi_create_string_utf8(env, "repro", NAPI_AUTO_LENGTH, &name);
+  napi_value js_cb;
+  napi_create_function(env, "noop", NAPI_AUTO_LENGTH, noop, NULL, &js_cb);
+  napi_create_threadsafe_function(env, js_cb, NULL, name, 0, 1, NULL, tsfn_finalize, NULL, call_js_cb, &tsfn);
+  napi_unref_threadsafe_function(env, tsfn);
+  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,13 @@ 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.
+      await checkSameOutput("test_async_cleanup_hook_tsfn_release", []);
+    });
   });
 
   describe("duplicate prevention", () => {