diff --git a/src/jsc/bindings/napi.h b/src/jsc/bindings/napi.h index dd96d7688e51..05b95066da16 100644 --- a/src/jsc/bindings/napi.h +++ b/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; 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 { } } - auto handle = std::make_unique(this, m_cleanupHooks.end()); + auto handle = std::make_unique(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(&hook)) { + for (auto iter = m_cleanupHooks.begin(), end = m_cleanupHooks.end(); iter != end; ++iter) { + if (auto* async = std::get_if(&*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 { } else { auto& async = std::get(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. diff --git a/test/napi/napi-app/binding.gyp b/test/napi/napi-app/binding.gyp index 94f07ed09603..d0daf9c422a9 100644 --- a/test/napi/napi-app/binding.gyp +++ b/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": [" { 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(); diff --git a/test/napi/napi-app/test_async_cleanup_hook_tsfn_release.c b/test/napi/napi-app/test_async_cleanup_hook_tsfn_release.c new file mode 100644 index 000000000000..b2b76b8b4118 --- /dev/null +++ b/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 +#include + +// 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) diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index eed5a385af46..4ac4b1d4e702 100644 --- a/test/napi/napi.test.ts +++ b/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", () => {