fix(ext/napi): don't free threadsafe function on abort while refs remain#36032
Merged
Conversation
napi_release_threadsafe_function with napi_tsfn_abort freed the TsFn regardless of the remaining thread_count. When more than one thread still held the function, that left the other threads with a dangling pointer; a later release from one of them could deref the freed box and spawn a second drop of the same allocation, causing a use-after-free and a double free. On the double free the assert in TsFn::drop trips: self.is_closed.compare_exchange(false, true, ...).is_ok() (node_api.rs:947), which is the Windows panic reported in #35990. Abort now only marks the function as closing (rejecting pending and future calls and waking blocked callers); the box is freed exactly once, when the last thread releases it and the thread count reaches zero. This matches Node.js, where abort stops further calls but destruction still waits for every acquired thread to release. Adds a regression test that aborts while a second reference is still outstanding and asserts the finalizer runs exactly once.
bartlomieju
added a commit
that referenced
this pull request
Jul 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the Windows panic reported in #35990:
napi_release_threadsafe_functionwithnapi_tsfn_abortfreed theTsFnregardless of the remaining
thread_count. When more than one thread still heldthe function, aborting spawned the drop immediately, freeing the box out from
under the other reference holders. Those threads were left with a dangling
pointer, and a later
releasefrom one of them could deref the freed box andspawn a second drop of the same allocation, producing a use-after-free and a
double free. On the second drop the
is_closedcompare-exchange inTsFn::dropfails and the assertion panics.
Abort now only marks the function as closing (rejecting pending and future calls
and waking any callers blocked on a full queue). The box is freed exactly once,
when the last thread releases it and the thread count reaches zero. This matches
Node.js semantics, where abort stops further calls but destruction still waits
for every acquired thread to release. The non-abort path is unchanged: it
already only freed on the final release.
The reproduction is inherently timing and allocator dependent (the second drop
only trips the assert once the freed memory is observed as reusable), which is
why it surfaced on Windows under a real addon rather than deterministically. The
added regression test creates a tsfn with
initial_thread_count = 2, abortsfrom one thread while a second reference is still outstanding, and asserts the
finalizer runs exactly once.