napi: keep async cleanup hook handles alive until the addon removes them - #37204
Conversation
NapiEnv::drain() deleted an async cleanup hook's handle as soon as the hook returned, but the addon owns that handle: Node.js frees it only in napi_remove_async_cleanup_hook, which the addon may call after the hook has run (e.g. from a threadsafe function finalizer that env teardown invokes later). Removing a handle whose hook had already run then read freed memory: segfault at address 0x88 in release builds, heap-use-after-free under ASAN. drain() no longer frees the handle; napi_remove_async_cleanup_hook now erases the hook from the set if still registered and frees the handle unconditionally, matching node/src/node_api.cc. The handle no longer stores a HookSet iterator (which could also dangle across rehashes), so handle identity is plain pointer equality. Fixes #37201 Co-authored-by: Marcel Laverdet <laverdet@users.noreply.github.com>
WalkthroughChangesThe N-API async cleanup-hook handle lifecycle now supports removal during thread-safe-function finalization. A native regression addon and test harness reproduce and validate this lifecycle. Async cleanup-hook lifecycle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/napi/napi-app/test_async_cleanup_hook_tsfn_release.c`:
- Around line 13-24: Validate and surface the return status of every N-API
operation in start(), async_cleanup_hook(), and tsfn_finalize(), including TSFN
creation, unref, hook registration, release, and hook removal. Stop setup or
cleanup on failure and make the fixture fail rather than continuing. Only print
“async cleanup hook removed” after napi_remove_async_cleanup_hook() succeeds,
ensuring checkSameOutput() cannot pass when the finalizer path was not exercised
successfully.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb77f9a5-49c8-4ba9-826a-16391653f3cc
📒 Files selected for processing (5)
src/jsc/bindings/napi.htest/napi/napi-app/binding.gyptest/napi/napi-app/module.jstest/napi/napi-app/test_async_cleanup_hook_tsfn_release.ctest/napi/napi.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/napi/napi-app/test_async_cleanup_hook_tsfn_release.c`:
- Around line 49-54: Update start() to route failures from
napi_unref_threadsafe_function() and napi_add_async_cleanup_hook() through a
cleanup path that releases tsfn, rather than returning via CHECK. Track whether
async_cleanup_hook registration succeeded, and have tsfn_finalize remove the
hook only when that registration flag is true, avoiding use of an invalid hook
handle.
- Around line 22-23: Prevent the exported start() flow from overwriting
hook_handle and tsfn when invoked more than once: either reject subsequent
start() calls before registration, or move both handles into per-registration
state passed to the cleanup hook. Preserve the existing first-registration
behavior and cleanup lifecycle.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f7f3d547-e997-48df-8d56-e252ccd9f45c
📒 Files selected for processing (1)
test/napi/napi-app/test_async_cleanup_hook_tsfn_release.c
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The two nits from my earlier pass (void return on removeAsyncCleanupHook, pinning the lifecycle output in the test) are addressed in 5365c7a — thanks. One new inline finding on the Windows CRLF assertion.
Beyond that, I checked whether dropping the delete async.handle in drain() leaks when napi_add_async_cleanup_hook is called with remove_handle == NULL — it doesn't, since the handle is still delivered as the hook callback's first argument and the addon removes it from there (matching Node).
Extended reasoning...
5365c7a addressed both prior review comments: removeAsyncCleanupHook is now void with the dead null guard removed, and the test now asserts toEndWith([...].join("\n")) on the four lifecycle lines. The remaining inline finding (Windows CRLF in the toEndWith suffix) is a deterministic Windows CI failure introduced by that same commit; the fix is the file's own .split(/\r?\n/) convention. I also verified that the ownership transfer to the addon does not introduce a leak on the remove_handle == NULL path, since drain() passes async.handle as the hook's first argument and the addon is required to call napi_remove_async_cleanup_hook on it — same as Node.
There was a problem hiding this comment.
LGTM — all three prior review findings (void return type, pinned test output, Windows CRLF split) were addressed in 5365c7a and c3a0369.
What was reviewed:
- Handle ownership transfer:
drain()no longer frees the handle,removeAsyncCleanupHookfrees unconditionally — matches Node's contract where the addon owns the handle andnapi_remove_async_cleanup_hookis the completion signal. - Removed the stored
HookSet::iterator(would dangle across rehash) in favor of pointer-equality search;operator==simplified accordingly. - Checked the
handle_out == NULLpath innapi_add_async_cleanup_hook— the hook callback still receives the handle as its first argument, so the addon can (and per N-API must) remove it. - Test pins the exact 4-line lifecycle with
status=0and splits on/\\r?\\n/for Windows.
Extended reasoning...
Overview
Fixes a heap-use-after-free in napi_remove_async_cleanup_hook (#37201). The change is confined to src/jsc/bindings/napi.h (~30 net lines): NapiEnv::drain() stops deleting the async cleanup hook handle after the hook body returns, and removeAsyncCleanupHook now erases-if-present then unconditionally deletes the handle. The napi_async_cleanup_hook_handle__ struct drops its stored HookSet::iterator (which could dangle across an unordered_set rehash) and AsyncCleanupHook::operator== becomes plain pointer equality. A new C fixture and checkSameOutput test with pinned lifecycle lines cover the regression.
Security risks
None. This is a lifetime/ownership fix in the NAPI compat layer; no user input parsing, auth, or crypto is touched. The change reduces a UAF surface rather than adding one.
Level of scrutiny
Medium-high — native C++ memory management in a Node-API compatibility path. I traced the sole caller at napi.cpp:3285-3301: it null-checks handle before dereferencing handle->env and calls removeAsyncCleanupHook unconditionally, so the new void signature and dropped null guard are correct. The ownership model now matches Node's documented contract (the hook callback receives the handle and must call napi_remove_async_cleanup_hook to signal completion, which is where it's freed). I also checked the napi_add_async_cleanup_hook path where handle_out is NULL — the handle is still passed to the hook callback as its first argument, so the addon can remove it; a handle only leaks if the addon violates the N-API contract, same as Node.
Other factors
This PR went through three review iterations. My prior findings (degenerate bool return, unpinned test output, Windows \r\n in toEndWith) were all addressed and the threads resolved. CodeRabbit's findings were either addressed or withdrawn. The bug-hunting system found nothing on this final revision. The test follows the file's existing conventions (checkSameOutput + .split(/\r?\n/)), asserts status=0 on both the release and remove calls, and was verified to fail under USE_SYSTEM_BUN=1 per the PR description.
|
Updated 10:07 AM PT - Aug 8th, 2026
❌ @robobun, your commit 0090ca1 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37204That installs a local version of the PR into your bun-37204 --bun |
There was a problem hiding this comment.
LGTM — all prior review feedback has been addressed (void return, pinned lifecycle output, Windows CRLF split), and the only change since my last pass is a CI retrigger.
What was reviewed:
- Handle ownership now matches Node's contract:
drain()stops freeing the handle,removeAsyncCleanupHookfrees unconditionally; checked remove-before-drain, remove-from-inside-hook, and remove-after-drain paths for double-free/UAF — none dereference the handle after delete. - Dropping the stored
HookSet::iteratorremoves a rehash-dangling hazard;operator==now compares handle pointers by value only, so thedrain()snapshot's find() on an already-removed hook is safe. - Test pins the exact four-line lifecycle including
status=0, splits on\\r?\\nfor Windows, andcheckSameOutputasserts exit 0 / empty stderr on both runtimes.
Extended reasoning...
Overview
This PR fixes a use-after-free (#37201) in napi_remove_async_cleanup_hook: NapiEnv::drain() was freeing the async cleanup hook handle as soon as the hook returned, but per Node's contract the addon owns the handle and frees it via napi_remove_async_cleanup_hook — which it may call after the hook has run (that call is the completion signal). The fix moves the delete handle from drain() into removeAsyncCleanupHook (unconditional, matching node/src/node_api.cc), and drops the stored HookSet::iterator from the handle struct in favor of plain pointer identity. Files touched: src/jsc/bindings/napi.h (~40 lines), plus a new C fixture, binding.gyp/module.js entries, and a test in napi.test.ts.
Security risks
None. This is a lifetime fix in the N-API compat layer; no user input parsing, no auth/crypto, no new external surface.
Level of scrutiny
Moderate-to-high — this is C++ memory-lifetime code in the most-blocked category per REVIEW.md. I traced the three call orderings (remove before drain, remove from inside the hook, remove after drain via a later finalizer) and confirmed each frees the handle exactly once with no dereference-after-free. The removed HookSet::iterator field was itself a latent hazard (unordered_set iterators invalidate on rehash), so the simplification is a net safety improvement. The one intentional leak — an addon that never calls napi_remove_async_cleanup_hook — matches Node's behavior and the documented contract.
Other factors
I reviewed this PR twice previously; both rounds of feedback (dead bool return + null guard, pinning expected lifecycle output, Windows CRLF handling) were applied in 5365c7a and c3a0369, and CodeRabbit's threads were resolved with reasoning I agree with. The only commit since my last review (0090ca1) is a CI retrigger with no code change. The regression test compares against Node via checkSameOutput and additionally asserts the exact four-line lifecycle with status=0, so a shared-failure mode cannot pass silently.
|
CI status: the only red is |
…hem (oven-sh#37204) Fixes oven-sh#37201 ### Repro The issue's addon creates a threadsafe function, registers an async cleanup hook, releases the tsfn from that hook, and calls `napi_remove_async_cleanup_hook` from the tsfn finalizer (which env teardown runs later): ``` $ bun repro.js main done [addon] async cleanup hook fired [addon] tsfn finalize: removing async cleanup hook panic(main thread): Segmentation fault at address 0x88 ``` Under a debug (ASAN) build this is a deterministic heap-use-after-free: `napi_remove_async_cleanup_hook` reads `handle->env` from a handle that `NapiEnv::drain()` freed. Node runs the same addon cleanly. ### Cause `NapiEnv::drain()` deleted an async cleanup hook's handle as soon as the hook body returned. But the addon owns that handle: in Node the handle is freed only by `napi_remove_async_cleanup_hook` (node/src/node_api.cc deletes it unconditionally), and the addon may legitimately call that after the hook has run, since that call is how it signals the async cleanup is complete. Here the call comes from a threadsafe function finalizer that `abortThreadSafeFunctions()` invokes after the cleanup hooks have drained. ### Fix - `drain()` no longer frees the handle after invoking the hook. - `napi_remove_async_cleanup_hook` (via `NapiEnv::removeAsyncCleanupHook`) erases the hook from the set if it is still registered, then frees the handle unconditionally, matching Node. - The handle no longer stores a `HookSet` iterator (it could dangle across rehashes); handle identity is plain pointer equality. Related: oven-sh#34136 addresses the separate semantic of waiting for an async cleanup hook that completes from a background thread. It does not cover this case: the removal here is queued behind env teardown itself, so handle lifetime has to be decoupled from the hook's return regardless. ### Verification New fixture `test/napi/napi-app/test_async_cleanup_hook_tsfn_release.c` (adopted from the issue) compared against Node via `checkSameOutput`: - `USE_SYSTEM_BUN=1 bun test test/napi/napi.test.ts -t 'hook handle stays valid'`: fails (segfault at address 0x88) - `bun bd test test/napi/napi.test.ts -t 'hook handle stays valid'`: passes - `bun bd test test/napi/napi.test.ts -t cleanup`: 27 pass - The issue's standalone repro prints all three addon lines and exits 0 under the debug ASAN build, matching Node. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: Marcel Laverdet <laverdet@users.noreply.github.com>
2 participants
Fixes #37201
Repro
The issue's addon creates a threadsafe function, registers an async cleanup hook, releases the tsfn from that hook, and calls
napi_remove_async_cleanup_hookfrom the tsfn finalizer (which env teardown runs later):Under a debug (ASAN) build this is a deterministic heap-use-after-free:
napi_remove_async_cleanup_hookreadshandle->envfrom a handle thatNapiEnv::drain()freed. Node runs the same addon cleanly.Cause
NapiEnv::drain()deleted an async cleanup hook's handle as soon as the hook body returned. But the addon owns that handle: in Node the handle is freed only bynapi_remove_async_cleanup_hook(node/src/node_api.cc deletes it unconditionally), and the addon may legitimately call that after the hook has run, since that call is how it signals the async cleanup is complete. Here the call comes from a threadsafe function finalizer thatabortThreadSafeFunctions()invokes after the cleanup hooks have drained.Fix
drain()no longer frees the handle after invoking the hook.napi_remove_async_cleanup_hook(viaNapiEnv::removeAsyncCleanupHook) erases the hook from the set if it is still registered, then frees the handle unconditionally, matching Node.HookSetiterator (it could dangle across rehashes); handle identity is plain pointer equality.Related: #34136 addresses the separate semantic of waiting for an async cleanup hook that completes from a background thread. It does not cover this case: the removal here is queued behind env teardown itself, so handle lifetime has to be decoupled from the hook's return regardless.
Verification
New fixture
test/napi/napi-app/test_async_cleanup_hook_tsfn_release.c(adopted from the issue) compared against Node viacheckSameOutput:USE_SYSTEM_BUN=1 bun test test/napi/napi.test.ts -t 'hook handle stays valid': fails (segfault at address 0x88)bun bd test test/napi/napi.test.ts -t 'hook handle stays valid': passesbun bd test test/napi/napi.test.ts -t cleanup: 27 passno test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/napi.test.ts