Repository navigation
fix(ext/napi): don't invoke napi_wrap finalizer twice at teardown - #36556
Conversation
A `napi_wrap`/`napi_create_external`/`napi_add_finalizer` finalizer can be
triggered by two independent paths:
- The GC weak callback (`Reference::weak_callback`), which deregisters the
reference's entry from the shutdown finalizer list, then runs the finalizer.
- Env teardown (`run_napi_ref_finalizers` -> `RefTracker::take_pending`), which
drains the whole list and runs each finalizer.
The recent `NapiFinalizerId` rework made removal-by-id correct, fixing the
GC-first direction (shared `data` pointers no longer remove the wrong entry).
But the teardown-first direction is still broken: `take_pending` runs a
finalizer without resetting the still-live `Reference`, so a later GC fires
`weak_callback`, which unconditionally runs the finalizer a second time. In
`deno run` this is masked because the process exits right after teardown, but
the test runner keeps polling the event loop after draining finalizers (a
lingering threadsafe function that keeps creating wraps), exposing the double
invocation — a double-free for real addons.
Make the tracker the single source of truth for "run exactly once": whichever
of {GC, teardown} removes the entry first runs the finalizer, the other skips
it. `RefTracker::remove` / `remove_ref_finalizer` / `Reference::reset` now
report whether the entry was still pending, and `weak_callback` only runs the
finalizer when its `reset()` actually removed a live entry.
Adds a regression test (`tsfn_finalizer_test.js`) reproducing the reported
scenario: a threadsafe function churns wrapped objects while the GC runs, and
the native finalizer aborts on a duplicate invocation.
Fixes #36499
04e4051 to
1099ac5
Compare
Mark the new "was still pending" return values #[must_use] so future call sites have to decide explicitly, and make the two intentionally-discarding paths say so. On the test side, assert the threadsafe function actually dispatched (the repro could previously pass vacuously), drop the global AtomicPtr in favour of a Send wrapper, and document the tsfn thread-count handoff and what a regression looks like.
The worker-thread closure in test_tsfn_wrap_finalizer destructured
`SendTsfn(tsfn)` directly, so RFC 2229 disjoint closure capture took only
the inner non-`Send` `*mut napi_threadsafe_function__` field rather than the
`Send` wrapper. That failed to compile ("cannot be sent between threads
safely"), breaking the lint and every integration-test job.
Bind the whole `SendTsfn` inside the closure before projecting to its field
so the wrapper is captured.
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
The core fix looks right to me. Making the tracker the single arbiter is the correct model — the entry is the token, Vec::remove is the claim, and whichever path claims it runs the callback. That mirrors Node's Unlink-then-Finalize ordering in Reference::Finalize.
I checked the three let _ = discard sites and they're all justified: Reference::remove (userland napi_delete_reference must not run the finalizer, matching ~Reference) and Drop for Reference (nothing left to run). Behavior there is unchanged.
One design note that doesn't block this PR but is worth writing down somewhere: the tracker stores a copy of (cb, data, hint) rather than a back-pointer to the Reference, so teardown cannot clear the reference's own finalize_cb/finalize_data. A wrap finalized at teardown leaves a live Reference whose finalize_data is dangling — and unwrap() hands that pointer straight back to the caller through result. That UAF window is pre-existing and out of scope here, but the boolean handshake papers over it rather than closing it. A follow-up where pending entries hold *mut Reference and teardown calls a Reference::finalize() would collapse both problems into one invariant.
On the test: the repro is well constructed and the comments are unusually good about what a failure looks like. My main reservation is that it is a probabilistic negative — at ~2/3 pre-fix, CI will not reliably catch a regression. Worth pairing with a deterministic test of the invariant itself: an exported native function that registers a finalizer, triggers run_napi_ref_finalizers, then forces GC on the wrapped object and asserts a single invocation. That one would fail 100% of the time on a regression.
Style nit across both files: the #[must_use] message on Reference::reset is three lines and prescribes the remedy ("discard it explicitly with let _ ="); the other two are terser. Worth matching. Also note Env::remove_ref_finalizer is pub, so adding a return type + #[must_use] is a (minor) deno_napi API change.
| } | ||
| /// | ||
| /// Returns `true` if the finalizer was still registered on the shutdown | ||
| /// list. A `false` result means env teardown (`run_napi_ref_finalizers`) |
There was a problem hiding this comment.
reset() also returns false when there was never a finalizer at all (finalizer_id == None), not only when teardown claimed it. That's harmless — finalizer_id is Some iff finalize_cb is Some, since they're set together in Reference::new and cleared together here — but the doc asserts only the teardown meaning, and the safety of the other case depends on an invariant stated nowhere. Suggest a clause:
| /// list. A `false` result means env teardown (`run_napi_ref_finalizers`) | |
| /// list. A `false` result means either that env teardown | |
| /// (`run_napi_ref_finalizers`) already consumed and ran it, or that this | |
| /// reference never had a finalizer — `finalizer_id` is `Some` iff | |
| /// `finalize_cb` is. Either way, callers must not run it. |
| /// finalizer, and the other observes `false` and skips it. | ||
| #[must_use = "the return value decides whether the finalizer may still \ | ||
| be run"] | ||
| fn remove(&mut self, id: NapiFinalizerId) -> bool { |
There was a problem hiding this comment.
Two things here, neither blocking.
Perf: this is an O(n) scan plus an O(n) shift per removal. pending is append-only with monotonically increasing ids, so it is always sorted ascending — binary_search_by_key(&id.0, |f| f.id.0) would make the lookup O(log n). Addons holding thousands of live wraps (exactly the shape of the repro in this PR: 50 wraps × 150 ticks) pay quadratic cost on GC churn. Fine as a follow-up.
Invariant worth documenting: the whole fix rests on NapiFinalizerId never being reused. That holds only because run_napi_ref_finalizers drains via take_pending(), which preserves next_id. A future refactor to std::mem::take(&mut *tracker) would reset next_id to 0, let a post-teardown reference collide with a stale id held by a drained-but-still-live Reference, and silently reintroduce exactly this double-invocation — reset() would remove the wrong entry and return true. A one-line comment on take_pending ("must not reset next_id; see remove") would protect that.
| return; | ||
| } | ||
| let mut scope = ptr::null_mut(); | ||
| assert_napi_ok!(napi_open_handle_scope(env, &mut scope)); |
There was a problem hiding this comment.
call_js_cb runs during and after worker teardown, and assert_napi_ok! on this path panics across an extern "C" boundary (or aborts) if the env is shutting down and, say, napi_create_object returns non-ok. That failure would look almost exactly like the regression this test exists to detect — a dead napi test process — but for an unrelated reason.
Suggest bailing out instead of asserting for the calls inside this callback, so that abort() in finalize_cb remains the only way this test can kill the process:
if napi_open_handle_scope(env, &mut scope) != napi_ok {
return;
}Same for the napi_create_object/napi_wrap calls in the loop below. The asserts in test_tsfn_wrap_finalizer and get_tsfn_wrap_finalizer_stats are fine to keep — those run on a live env.
| // off to the worker thread, which releases it when it is done — so the tsfn | ||
| // is closed exactly once and never outlives the worker. | ||
| let tsfn = SendTsfn(tsfn); | ||
| thread::spawn(move || { |
There was a problem hiding this comment.
This is the one thing I'd want confirmed before landing. deno test . runs every napi test file in a single process, so this detached thread keeps calling napi_call_threadsafe_function for ~30ms after this file's worker is torn down, while later files are already starting.
The TsFn box itself stays valid — handing off the single thread count is the right call and is well explained above. But both TsFn::call and TsFn::release go through sender.spawn(..) on a V8CrossThreadTaskSpawner whose JsRuntime is gone. If that's inert, the cost is just a leaked TsFn (its queued drop never runs, so thread_finalize_cb never fires — None here, so harmless). If it isn't inert, it's a UAF that takes down all 145 napi tests and gets attributed to whichever file happened to be running at the time.
Worth a quick check, and a comment recording the answer either way.
| // the inner non-`Send` pointer. | ||
| let tsfn = tsfn; | ||
| let SendTsfn(tsfn) = tsfn; | ||
| for _ in 0..TICKS { |
There was a problem hiding this comment.
Nothing structurally guarantees that ticks remain when the test body returns, which is the whole point of the repro. 150 × 200µs ≈ 30ms of dispatch, versus a JS poll loop that typically exits after its first 10ms sleep — so ~120 ticks are left in practice, but only incidentally, and it depends on these two constants keeping their current ratio.
Looping until a wall-clock deadline (or until an AtomicBool set by a stop export) rather than a fixed tick count would make hitting the teardown window a property of the test rather than a coincidence of tuning.
| stats = lib.get_tsfn_wrap_finalizer_stats(); | ||
| } | ||
|
|
||
| assert( |
There was a problem hiding this comment.
finalized is exported from the native side, plumbed through get_tsfn_wrap_finalizer_stats, and then never asserted on. Either assert something about it — e.g. that finalizers actually ran by the time the loop exits, which strengthens the "the repro did not run" guard — or drop it from the stats object so the native side doesn't carry a counter nobody reads.
Addresses review feedback on #36556: - Add deterministic unit tests for the finalizer run-once contract. The threadsafe-function repro is a probabilistic negative (~2/3 pre-fix); these cover both race directions (teardown-first and GC-first) of the RefTracker arbiter and fail 100% of the time on a regression. - Match Reference::reset's #[must_use] message to the terser wording used on RefTracker::remove and NapiState::remove_ref_finalizer.
1 participant
Fixes #36499.
Problem
A NAPI wrap finalizer (registered via
napi_wrap,napi_create_external, ornapi_add_finalizer) can be invoked twice for the same wrapped value, which is a double-free for real addons. This reproduces underdeno test.A finalizer can be triggered by two independent paths:
Reference::weak_callback): deregisters this reference's entry from the shutdown finalizer list (reset()), then runs the finalizer.run_napi_ref_finalizers→RefTracker::take_pending): drains the whole pending list and runs each finalizer, matching Node'sRefTracker::FinalizeAllat env teardown.The recent
NapiFinalizerIdrework made removal-by-id correct and fixed the GC-first direction (finalizers sharing adatapointer no longer remove the wrong entry). But the teardown-first direction is still broken:take_pending()runs a finalizer without resetting the still-liveReference, so a later GC firesweak_callback, which unconditionally runs the finalizer a second time.In
deno runthis is masked because the process exits right after teardown. The test runner, however, keeps polling the event loop after draining finalizers (cli/tools/test/mod.rs), and a lingeringnapi_threadsafe_functionthat keeps creating wraps makes the GC fire in that window — exposing the double invocation.Fix
Make the tracker the single source of truth for the "run exactly once" contract: whichever of {GC, teardown} removes the entry first runs the finalizer, and the other skips it.
RefTracker::remove,NapiState::remove_ref_finalizer, andReference::resetnow report whether the entry was still pending.weak_callbackonly runs the finalizer when itsreset()actually removed a live entry. If teardown already drained the list (and ran it),reset()returnsfalseand the GC path does not run it again.Test
Adds
tests/napi/tsfn_finalizer_test.js+ nativetest_tsfn_wrap_finalizer, porting the reporter's reproduction: a threadsafe function repeatedly wraps freshly created objects and churns allocations so the GC runs while wraps are pending. The native finalizer aborts on a duplicate invocation.main): aborts (SIGABRT) intermittently but reliably (~2/3 runs).