fix(ext/napi): run JS-calling finalizers safely, NULL string result, and Float16Array - #36572
Merged
Merged
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
… Float16Array Three addon-triggered process kills in ext/napi, each of which any npm native addon can hit. Fixes match Node behavior. 1. Node-API finalizer that calls into JS (#36568) napi finalizers are allowed to call back into JavaScript (e.g. napi_call_function). Deno ran them directly from V8's second-pass weak callback, which runs inside a DisallowJavascriptExecutionScope, so the first call into JS aborted the process ("Invoke in DisallowJavascriptExecutionScope"). Node never runs finalizers from the GC pass — it drains them from a SetImmediate. Reference::weak_callback now defers the finalizer via Env::defer_gc_finalizer: it pushes onto an isolate-local queue and requests a V8 interrupt, which drains the queue under a HandleScope at the next JS-safe point. Env teardown drains the same queue, so a finalizer still runs at exit if the loop ends first. Deferral was tried before (#33260) and reverted (#34023) because the cross-thread task spawner's mutex + tokio waker signalling from inside the second-pass callback corrupted state (#33924 / #34008). The interrupt request is isolate-local and avoids that hazard. 2. NULL result pointer segfault (#36569) napi_create_string_utf8/latin1/utf16 and the node_api_create_property_key_* variants wrote the created value through `result` without checking it, so a NULL out-pointer was a write to address zero. Add check_arg! so they return napi_invalid_arg like Node. 3. Float16Array panic (#36570) napi_get_typedarray_info on a Float16Array hit unreachable!(), and napi_create_typedarray rejected napi_float16_array. Add the float16 arm to both (element type 11, matching Node) and replace the unreachable!() with an error return so a future element type can't abort the process either. Adds regression tests to the Node-API test suite for all three.
…36568-36569-36570 # Conflicts: # ext/napi/js_native_api.rs # ext/napi/lib.rs # tests/napi/src/tsfn_finalizer.rs # tests/napi/tsfn_finalizer_test.js
The #36568 fix deferred JS-calling finalizers out of V8's second-pass weak callback (a DisallowJavascriptExecutionScope) using request_interrupt. But a V8 interrupt is serviced from the stack guard, which V8 also checks while unwinding a GC, so the drain still landed inside the disallowed scope: the 'napi finalizer can call into JS' test aborted 100% of the time locally with 'Invoke in DisallowJavascriptExecutionScope' (masked only by timing when a syscall was inserted). Drain from the same-thread V8TaskSpawner instead. Its tasks run from dispatch_task_spawner during an event-loop poll, with a real context scope and a microtask checkpoint -- a genuinely JS-safe point, matching Node's SetImmediate drain. This is not the cross-thread spawner reverted in #34023; that hazard was a double-run, now prevented independently by the reset()/was_pending run-once handshake (#36499). Full napi suite: 148 passed, 0 failed; deferred_finalizer 20/20 and the tsfn/finalizer/worker-termination risk set 15/15 green over repeated runs.
bartlomieju
changed the title
- Use `v8::Local::cast_unchecked` for the typed-array upcast instead of a hand-rolled `transmute_copy`. The `Local<T>: TryFrom<Local<Value>>` bound restricts the helper to types V8 can downcast back, where the upcast is infallible; `transmute_copy` accepted any `T` with no size check. - Don't swallow a finalizer's exception. The napi entry point the finalizer called (e.g. `napi_call_function`) records the throw in `env.last_exception` and returns `napi_pending_exception`, so it never reaches the drain's `TryCatch` — and leaving it set makes every later napi call on that env fail with `napi_pending_exception`. Clear it and log it, the way Node reports a finalizer's uncaught exception through its uncaught-exception policy and carries on. Adds a regression test. - Take `*mut Env` in `defer_gc_finalizer` instead of `&mut self`, so the pointer handed to the queued finalizer keeps the caller's provenance rather than being derived from a borrow that ends when the call returns. - `PendingNapiFinalizer::id` is now `Option<NapiFinalizerId>`; GC-ready entries are `None` instead of a `u64::MAX` sentinel. - Note in `weak_callback` why freeing a runtime-owned `Reference` before the deferred finalizer runs (Node frees after) is unobservable. - Fix `deno fmt` in deferred_finalizer_test.js and drop the pointless IIFE.
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
1 participant
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 three addon-triggered process kills in
ext/napi, each of which any npm native addon can trip. All three surfaced while running the Node-API conformance suite against Deno. Behavior now matches Node.Fixes #36568
Fixes #36569
Fixes #36570
1. Node-API finalizer that calls into JS (#36568)
napi finalizers are allowed to call back into JavaScript (e.g.
napi_call_function). Deno ran them directly from V8's second-pass weak callback, which runs inside aDisallowJavascriptExecutionScope, so the first call into JS aborted the process:Node never runs finalizers from the GC pass — it drains them from a
SetImmediate.Reference::weak_callbacknow defers the finalizer viaEnv::defer_gc_finalizer, which pushes onto an isolate-local queue and schedules a drain on the same-threadV8TaskSpawner. That task runs fromdispatch_task_spawnerduring an event-loop poll, with a real context scope and a microtask checkpoint — a genuinely JS-safe point (the equivalent of Node'sSetImmediatedrain). Env teardown drains the same queue, so a finalizer still runs at exit if the event loop ends first.Note on the mechanism: an earlier revision used V8's
RequestInterruptto reach a "safe point". That is not sufficient — the interrupt is serviced from the stack guard, which V8 also checks while unwinding a GC, so the drain can still land inside theDisallowJavascriptExecutionScope(it aborted ~100% of the time locally; only a stray syscall in the drain path perturbed timing enough to hide it). The same-thread task queue is the reliable fix.This is also not the cross-thread spawner that #33260 used and #34023 reverted: that hazard was a finalizer running twice (#33924 / #34008), which the
reset()/was_pendingrun-exactly-once handshake (#36499) now prevents independently. The task only pushes onto an isolate-local queue on the isolate thread.2. NULL
resultpointer segfault (#36569)napi_create_string_utf8/latin1/utf16(and thenode_api_create_property_key_*variants) wrote the created value throughresultwithout checking it — a NULL out-pointer was a write to address zero. Addedcheck_arg!(env, result)so they returnnapi_invalid_arglike Node.3.
Float16Arraypanic (#36570)napi_get_typedarray_infoon aFloat16Arrayhitunreachable!(), andnapi_create_typedarrayrejectednapi_float16_array. Added the float16 arm to both (element type 11, matching Node) and replaced theunreachable!()with an error return so a future element type can't abort the process either. (v8 150.4.0 ships theFloat16Arraydowncasts but not theLocal<Float16Array> -> Local<Value>upcast, so the typed-array constructor reinterprets the handle uniformly, as the generatedFromimpls do.)Testing
#36499run-at-most-once, double-delete, worker-termination finalizers, wrap-leak-at-shutdown).#36568"finalizer can call into JS" test passes 20/20, and the tsfn/finalizer/worker-termination risk set 15/15, over repeated local runs.deferred_finalizer_test.jsfor the now-deferred (previously synchronous) finalizer semantics.