From 673751c0913d5abaa79850dff0457e95b890cad5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 2 Aug 2026 08:48:48 +0200 Subject: [PATCH] fix(gc): root the ten unrooted runtime-side caches, and the scanner that walked 1 of 3 sibling slots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #7231 enumeration, verified and closed. This is #7226's class -- a runtime table holding a GC pointer that is not a registered root -- which is strictly worse than #7154's stale-register class (it goes bad at collection #0 and stays bad, rather than needing a collection to land in a narrow window) and which no static instrument can find: `gc_root_dominance_check.py` reads emitted LLVM IR, and a runtime table is not in it. ★ `CACHED_ENV`, the `process.env` object, is the load-bearing one and it is a hard crash rather than a subtle wrong answer. `js_process_env_impl` builds it once with `js_object_alloc` -- the NURSERY -- and caches it in a thread-local `Cell` that is the ENTIRE reference graph: `process.env` is a `js_process_env()` call, not a field of the `process` object. So the first minor swept or evacuated it and every later `process.env.X = v` wrote through a dangling pointer. The sibling `PROCESS_FINALIZATION_OBJECT` uses the identical materialize-once idiom and was already rooted, which is what makes this an omission rather than a design. The observable is ENUMERATION -- `Object.keys(process.env)`, `for…in`, spread, which is how `@next/env` and `dotenv` consume it. A direct `process.env.KEY` read lowers to `js_getenv` and asks the OS, so a witness built on the read would be a gate that cannot fail. Also rooted: `CACHED_PERMISSION` and `CACHED_REPORT` (same shape; the `runtime_write_barrier_root_nanbox` beside the first is an incremental MARK barrier, not a root registration, and is now labelled as such); `ERROR_CONSTRUCTOR_PTR` (a raw duplicate of a `globalThis` closure, outside the object graph, stale after a move); `INPUT_HANDLER` (the inline `useInput` arrow, which nothing else refers to); `RESIZE_CALLBACK` (a native slot that bypasses the rooted EventEmitter listener array); `FRAME_CALLBACKS` (rooted only transiently during registration -- its `unsafe impl Send` SAFETY comment asserted the opposite and is corrected in place); `CURRENT_NEW_TARGET`; `ACCESSOR_RECEIVER_OVERRIDE`; and `PENDING_FETCH_SIGNAL`. Scanner gap, the shape #7230 found twice: `worker_threads.rs`'s `scan_parent_port_event_roots_mut` visited `MESSAGE_EVENT_CALLBACKS` and neither `MESSAGE_CALLBACK` nor `CLOSE_CALLBACK` -- three slots in the same `thread_local!` block holding the same raw `ClosureHeader*`. The box/visit/ unbox dance is factored into one helper, because three copies of it is how the fourth gets forgotten. Two further windows closed in `frame.rs` while rooting its queue. `js_frame_tick` drained into an unrooted local `Vec` and rooted each callback only as it invoked it, leaving #2..#N naked while #1 ran arbitrary user code (#7230's staging-buffer shape); one batch `RuntimeHandleScope` now covers the set. And `js_on_frame_callback` held the queue mutex across an allocating `capture_context()` -- harmless before, a self-deadlock once a scanner locks the same mutex. Verified against c9cd73ba5 with `test_gap_gc_process_env_cache_rooting.ts`, registered in `test-parity/gc_repsel_corpus.txt`. Compiled AND run under `PERRY_GC_MOVING_LOOP_POLLS=1` with the evacuating base: SIGBUS (exit 138) 10/10 at base with no output at all, `bad 0` 10/10 after, byte-exact vs node 26.5.1. Clean 5/5 on the shipped default both sides, so this is a `requires=move` witness rather than a pre-existing failure. Under `ZEAL=1 PROTECT_FROMSPACE=1 DEPTH=800` the base arm faults at the stale dereference (`obj_type=2 size=416`, RETIRED FROM-SPACE) and this build is clean, so the instrument is a detector here and not a noise generator. `scripts/gc_repsel_matrix.sh --arms loop_polls --filter test_gap_gc_`: PASS=21 UNVER=0 FAIL=0, copy-minor live 21/21. Refs #7231, #7226, #7230, #7210, #7154, #7196. --- .../7239-gc-unrooted-runtime-caches.md | 56 +++++++++++ crates/perry-runtime/src/frame.rs | 50 +++++++++- crates/perry-runtime/src/gc/mod.rs | 35 +++++++ .../src/object/class_registry.rs | 1 + .../src/object/class_registry/construct.rs | 26 +++++ .../perry-runtime/src/object/field_get_set.rs | 1 + .../src/object/field_get_set/accessors.rs | 25 +++++ .../perry-runtime/src/object/global_fetch.rs | 16 ++++ .../perry-runtime/src/object/global_this.rs | 3 +- .../src/object/global_this/populate.rs | 19 ++++ crates/perry-runtime/src/object/mod.rs | 7 +- crates/perry-runtime/src/process.rs | 6 +- crates/perry-runtime/src/process/env_misc.rs | 31 ++++++ .../perry-runtime/src/process/permission.rs | 28 +++++- crates/perry-runtime/src/process/report.rs | 25 ++++- crates/perry-runtime/src/tty.rs | 16 ++++ crates/perry-runtime/src/tui/input.rs | 20 ++++ crates/perry-stdlib/src/worker_threads.rs | 42 +++++++- .../test_gap_gc_process_env_cache_rooting.ts | 95 +++++++++++++++++++ test-parity/gc_repsel_corpus.txt | 22 +++++ 20 files changed, 503 insertions(+), 21 deletions(-) create mode 100644 changelog.d/7239-gc-unrooted-runtime-caches.md create mode 100644 test-files/test_gap_gc_process_env_cache_rooting.ts diff --git a/changelog.d/7239-gc-unrooted-runtime-caches.md b/changelog.d/7239-gc-unrooted-runtime-caches.md new file mode 100644 index 0000000000..7eee8b681b --- /dev/null +++ b/changelog.d/7239-gc-unrooted-runtime-caches.md @@ -0,0 +1,56 @@ +**GC: root the ten unrooted runtime-side caches, and close a scanner that walked one of three sibling slots (#7231).** + +The class #7226 established — a runtime table holding a GC pointer that is not a +registered root. It is strictly worse than the #7154 stale-register class (it goes +bad at collection #0 and stays bad, rather than needing a collection to land in a +narrow window) and it is **invisible to every static instrument**, because +`gc_root_dominance_check.py` reads emitted LLVM IR and a runtime table is not in it. + +★ **`process.env`** is the load-bearing one. `js_process_env_impl` builds it once +with `js_object_alloc` — the nursery — and caches it in a thread-local `Cell` +that is the *entire* reference graph: `process.env` is a getter CALL, not a field of +the `process` object. The first minor swept or evacuated it, and every later +`process.env.X = v` wrote through a dangling pointer. The observable is +ENUMERATION (`Object.keys`, `for…in`, spread — how `@next/env` and `dotenv` consume +it), because a direct read lowers to `js_getenv` and asks the OS. Measured at +`c9cd73ba5` under `PERRY_GC_MOVING_LOOP_POLLS=1` at compile and run: +**SIGBUS 10/10 before, `bad 0` 10/10 after**, byte-exact against node 26.5.1, clean +on the shipped default both sides. The sibling `PROCESS_FINALIZATION_OBJECT` uses the +same materialize-once idiom and was already rooted — this was an omission, not a +design. + +Also rooted: `CACHED_PERMISSION` and `CACHED_REPORT` (same shape; the +`runtime_write_barrier_root_nanbox` beside the first is an incremental *mark* +barrier, not a root registration); `ERROR_CONSTRUCTOR_PTR` (a raw duplicate of a +`globalThis` closure, outside the object graph, so stale after a move); +`tui/input.rs` `INPUT_HANDLER` (the inline `useInput` arrow, which nothing else +refers to); `tty.rs` `RESIZE_CALLBACK` (bypasses the rooted EventEmitter listener +array); `frame.rs` `FRAME_CALLBACKS` (rooted only transiently during registration — +its `unsafe impl Send` SAFETY comment asserted the opposite and is corrected); +`CURRENT_NEW_TARGET`; `ACCESSOR_RECEIVER_OVERRIDE`; and `PENDING_FETCH_SIGNAL`. + +**Scanner gap**, the shape #7230 found twice: `worker_threads.rs`'s +`scan_parent_port_event_roots_mut` visited `MESSAGE_EVENT_CALLBACKS` and neither +`MESSAGE_CALLBACK` nor `CLOSE_CALLBACK` — three slots in the same `thread_local!` +block holding the same raw `ClosureHeader*`. `parentPort.on('message')` / +`on('close')` handlers were reclaimed by the next collection inside a worker. + +Two further windows closed in `frame.rs` while rooting its queue: `js_frame_tick` +drained into an unrooted local `Vec` and rooted each callback only as it invoked it, +leaving the rest of the batch naked across arbitrary user code (#7230's +staging-buffer shape); and `js_on_frame_callback` held the queue mutex across an +allocating `capture_context()`, which becomes a self-deadlock once a scanner locks +the same mutex. + +**Refuted, and worth recording.** All 8 budgeted (FULL, STEP) scanner pairs were +diffed field-by-field: **no drift** — #7230's `IntervalTimer.args` fix reached both +twins. `buffer/header.rs`'s nine address registries are not root gaps +(`GC_TYPE_BUFFER`/`GC_TYPE_TYPED_ARRAY` are `movable: false`; they are identity sets, +not liveness references). `static_plugins.rs` is unreachable — `perry_register_static_plugin` +has no caller in the workspace. + +Not closed, and stated: `promise/rejection.rs`'s `internally_handled` needs a +**rekey** rather than a root; the save/restore idiom in `CURRENT_NEW_TARGET` and +`ACCESSOR_RECEIVER_OVERRIDE` still parks the displaced value in a bare Rust local; +and `MODULE_PATH_REGISTRY` is process-global while arenas are per-thread, so a naive +scanner would be unsound. diff --git a/crates/perry-runtime/src/frame.rs b/crates/perry-runtime/src/frame.rs index 605b0d89e5..5b61feeb50 100644 --- a/crates/perry-runtime/src/frame.rs +++ b/crates/perry-runtime/src/frame.rs @@ -24,9 +24,22 @@ struct FrameCallback { cleared: bool, } -// SAFETY: closure pointers point to global compiled code / GC-rooted data. +// SAFETY: `callback` is a heap `ClosureHeader*`, kept live and address-current +// by `scan_frame_callback_roots_mut` (registered in `gc_init`). It is NOT +// "global compiled code", which is what this comment used to claim — that was +// the premise under which the queue went unrooted for its whole existence +// (#7231). unsafe impl Send for FrameCallback {} +/// Pending `onFrame(cb)` registrations. +/// +/// **This is a GC root, and must stay one (#7231).** `js_on_frame_callback` +/// roots the closure with a `RuntimeHandleScope` only for the duration of the +/// registration call; once that scope drops, this queue is the sole reference +/// to it until the next `js_frame_tick`. Before +/// `scan_frame_callback_roots_mut` a collection between registration and the +/// next vsync reclaimed or relocated the closure and `js_closure_call2` then +/// called through a dangling `ClosureHeader*`. static FRAME_CALLBACKS: Mutex> = Mutex::new(Vec::new()); static NEXT_FRAME_ID: Mutex = Mutex::new(1); static LAST_FIRE_BY_CLOSURE: Mutex>> = Mutex::new(None); @@ -60,17 +73,34 @@ pub extern "C" fn js_on_frame_callback(callback: i64) -> i64 { let cb_handle = scope.root_raw_const_ptr(callback as *const ClosureHeader); let id = next_frame_id(); + // `capture_context` can allocate, so it runs BEFORE the queue lock is + // taken. Evaluating it inside the `push(...)` argument list held the + // mutex across an allocation, which — now that a root scanner locks the + // same mutex — would be a self-deadlock the moment that allocation + // triggered a collection. + let context = crate::async_context::capture_context(); FRAME_CALLBACKS.lock().unwrap().push(FrameCallback { id, + // Re-read below the allocation: `capture_context` may have moved it. callback: cb_handle.get_raw_const_ptr::() as i64, - context: crate::async_context::capture_context(), + context, cleared: false, }); id } +/// Root + rewrite every pending frame callback closure. +pub(crate) fn scan_frame_callback_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + let mut queue = FRAME_CALLBACKS + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + for cb in queue.iter_mut() { + visitor.visit_i64_slot(&mut cb.callback); + } +} + /// Cancel a previously-registered frame callback. No-op if `id` is unknown /// or already fired. #[no_mangle] @@ -109,10 +139,22 @@ pub extern "C" fn js_frame_tick(timestamp_ms: f64) -> i32 { queue.drain(..).filter(|t| !t.cleared).collect() }; + // The drain removes every callback from the (now scanned) queue, so for + // the length of this loop `pending` is the ONLY reference to all of them + // — and callback #1 runs arbitrary user code that allocates. Rooting each + // one individually inside the loop covers the callback being invoked and + // leaves #2..#N naked, which is the same shape as #7230's staging buffer. + // One batch scope, entered before the first call, covers the whole set. + let batch = crate::gc::RuntimeHandleScope::new(); + let batch_handles: Vec<_> = pending + .iter() + .map(|cb| batch.root_raw_const_ptr(cb.callback as *const ClosureHeader)) + .collect(); + let mut fired = 0; - for cb in pending { + for (cb, batch_handle) in pending.iter().zip(batch_handles.iter()) { let scope = crate::gc::RuntimeHandleScope::new(); - let cb_handle = scope.root_raw_const_ptr(cb.callback as *const ClosureHeader); + let cb_handle = scope.root_raw_const_ptr(batch_handle.get_raw_const_ptr::()); let delta_ms = { let mut slot = LAST_FIRE_BY_CLOSURE.lock().unwrap(); diff --git a/crates/perry-runtime/src/gc/mod.rs b/crates/perry-runtime/src/gc/mod.rs index dbea304905..c92f85918d 100644 --- a/crates/perry-runtime/src/gc/mod.rs +++ b/crates/perry-runtime/src/gc/mod.rs @@ -625,6 +625,41 @@ pub fn gc_init() { gc_register_mutable_root_scanner(crate::tls::scan_tls_roots_mut); gc_register_mutable_root_scanner(crate::process::scan_process_finalization_roots_mut); gc_register_mutable_root_scanner(crate::process::scan_process_module_loader_roots_mut); + // #7231: the materialize-once `process.*` caches. Each is a thread-local + // cell holding a NURSERY-allocated object that nothing else refers to — + // `process.env` / `.permission` / `.report` are getter CALLS, not fields + // of the `process` object, so the cache is the whole reference graph. + // `scan_process_finalization_roots_mut` above is the identical idiom and + // was already registered; these three were an omission, not a design. + // `CACHED_ENV` is the load-bearing one: `process.env` is touched by nearly + // every real Node program, and every `process.env.X = v` after the first + // collection wrote through a dangling pointer. + gc_register_mutable_root_scanner(crate::process::scan_process_env_cache_roots_mut); + gc_register_mutable_root_scanner(crate::process::scan_permission_cache_roots_mut); + gc_register_mutable_root_scanner(crate::process::scan_report_cache_roots_mut); + // #7231: the raw `Error` constructor address behind + // `Error.prepareStackTrace`. The closure is reachable through `globalThis` + // so it is not swept, but this duplicate lives outside the object graph + // and goes stale on a move. + gc_register_mutable_root_scanner(crate::object::scan_error_constructor_root_mut); + // #7231: native callback slots that bypass their rooted sibling + // structures. `RESIZE_CALLBACK` bypasses the EventEmitter listener array; + // `FRAME_CALLBACKS` is rooted only transiently by a `RuntimeHandleScope` + // during registration; `INPUT_HANDLER` holds the `useInput` arrow, which + // in idiomatic inline form has no other reference at all. + gc_register_mutable_root_scanner(crate::tty::scan_tty_resize_callback_root_mut); + gc_register_mutable_root_scanner(crate::frame::scan_frame_callback_roots_mut); + gc_register_mutable_root_scanner(crate::tui::input::scan_tui_input_handler_root_mut); + // #7231: three in-flight cells that hold a NaN-boxed heap value across a + // window in which user code can run. Each is a second copy of a value + // whose original is rooted elsewhere, or the only copy for the length of + // the window; both shapes are the #7226 `prev_this` family. Rooting the + // CELL is the half a scanner can close — the displaced value each + // save/restore idiom parks in a bare Rust local is noted at each + // declaration and needs `RuntimeHandleScope` plumbing, not a scanner. + gc_register_mutable_root_scanner(crate::object::scan_current_new_target_root_mut); + gc_register_mutable_root_scanner(crate::object::scan_accessor_receiver_override_root_mut); + gc_register_mutable_root_scanner(crate::object::scan_pending_fetch_signal_root_mut); gc_register_mutable_root_scanner(crate::os::scan_process_event_listener_roots_mut); // #6077: keep promises tracked for an unhandled rejection alive + address- // stable until reported, so the program-end report is not a stale/UAF read. diff --git a/crates/perry-runtime/src/object/class_registry.rs b/crates/perry-runtime/src/object/class_registry.rs index c2859ae85f..ea7c66b89c 100644 --- a/crates/perry-runtime/src/object/class_registry.rs +++ b/crates/perry-runtime/src/object/class_registry.rs @@ -39,6 +39,7 @@ use super::*; mod class_meta; mod construct; +pub(crate) use construct::scan_current_new_target_root_mut; mod dispatch; mod gc_roots; pub(crate) mod parent_static; diff --git a/crates/perry-runtime/src/object/class_registry/construct.rs b/crates/perry-runtime/src/object/class_registry/construct.rs index 69c85162a0..5714fa3645 100644 --- a/crates/perry-runtime/src/object/class_registry/construct.rs +++ b/crates/perry-runtime/src/object/class_registry/construct.rs @@ -2,10 +2,36 @@ use super::*; use crate::JSValue; thread_local! { + /// `new.target` for the construction currently on this thread's stack. + /// + /// **This is a GC root, and must stay one (#7231).** It holds a NaN-boxed + /// closure/class value for the whole constructor body, and a constructor + /// body runs arbitrary user code. `this_binding.rs`'s `NEW_TARGET` holds + /// the same value under `scan_implicit_this_roots_mut`; this is a second + /// copy on a different path, and a second copy of a root that is not + /// itself a root is exactly the shape #7226 found in `prev_this`. + /// + /// RESIDUAL, deliberately not closed here: the save/restore idiom parks + /// the DISPLACED value in a bare Rust local (`prev_current_new_target`) + /// across the construction and republishes it afterwards. Runtime frames + /// are not covered by the precise scan, so that local is #7226's + /// `prev_this` defect in Rust rather than in codegen. Closing it means + /// routing the three save sites through a `RuntimeHandleScope`, which + /// wants its own before/after rather than being appended here. static CURRENT_NEW_TARGET: std::cell::Cell = const { std::cell::Cell::new(crate::value::TAG_UNDEFINED) }; } +/// Root + rewrite the in-flight `new.target`. +pub(crate) fn scan_current_new_target_root_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + CURRENT_NEW_TARGET.with(|cell| { + let mut bits = cell.get(); + if visitor.visit_nanbox_u64_slot(&mut bits) { + cell.set(bits); + } + }); +} + #[no_mangle] pub extern "C" fn js_new_target_value() -> f64 { f64::from_bits(CURRENT_NEW_TARGET.with(|value| value.get())) diff --git a/crates/perry-runtime/src/object/field_get_set.rs b/crates/perry-runtime/src/object/field_get_set.rs index b8736f420f..06551e2ea9 100644 --- a/crates/perry-runtime/src/object/field_get_set.rs +++ b/crates/perry-runtime/src/object/field_get_set.rs @@ -117,6 +117,7 @@ pub(crate) fn is_fetch_subclass_body_method(name: &[u8]) -> bool { // ── Topical sub-modules (issue #1103: keep every file < 2000 lines) ── mod accessors; +pub(crate) use accessors::scan_accessor_receiver_override_root_mut; mod buffer_own_prop; mod class_object_props; mod crypto_key; diff --git a/crates/perry-runtime/src/object/field_get_set/accessors.rs b/crates/perry-runtime/src/object/field_get_set/accessors.rs index 3e78eabce5..2a9ae7ea91 100644 --- a/crates/perry-runtime/src/object/field_get_set/accessors.rs +++ b/crates/perry-runtime/src/object/field_get_set/accessors.rs @@ -206,10 +206,35 @@ thread_local! { /// `[[Get]](P, Receiver)`. (object-literal getters on a `Object.create` /// prototype — e.g. @hono/node-server's request prototype reading /// `this[incomingKey].method`.) + /// + /// **This is a GC root, and must stay one (#7231).** The stashed receiver + /// is a NaN-boxed heap value that stays armed for the whole prototype + /// walk, and a walk can reach a Proxy `get` trap — arbitrary user code + /// that allocates. Nothing else refers to it while it sits here, so + /// without `scan_accessor_receiver_override_root_mut` the getter is + /// invoked with a `this` naming from-space. + /// + /// RESIDUAL, same shape as `CURRENT_NEW_TARGET`: the displaced value that + /// `accessor_receiver_override_begin` returns rides a bare Rust local + /// through the walk and is republished by `_end`. Rooting the cell + /// protects the ARMED value, not the saved one. static ACCESSOR_RECEIVER_OVERRIDE: std::cell::Cell> = const { std::cell::Cell::new(None) }; } +/// Root + rewrite the in-flight inherited-accessor receiver. +pub(crate) fn scan_accessor_receiver_override_root_mut( + visitor: &mut crate::gc::RuntimeRootVisitor<'_>, +) { + ACCESSOR_RECEIVER_OVERRIDE.with(|cell| { + if let Some(mut value) = cell.get() { + if visitor.visit_nanbox_f64_slot(&mut value) { + cell.set(Some(value)); + } + } + }); +} + pub(crate) fn accessor_receiver_override_begin(receiver: f64) -> Option { ACCESSOR_RECEIVER_OVERRIDE.with(|c| { // Keep the OUTERMOST receiver across multi-hop prototype walks. diff --git a/crates/perry-runtime/src/object/global_fetch.rs b/crates/perry-runtime/src/object/global_fetch.rs index e835b06986..323872e551 100644 --- a/crates/perry-runtime/src/object/global_fetch.rs +++ b/crates/perry-runtime/src/object/global_fetch.rs @@ -12,10 +12,26 @@ thread_local! { /// The `signal` from the in-progress `fetch(url, { signal })` call, stashed /// so the stdlib `js_fetch_with_options` (whose 4-arg ABI predates /// AbortSignal support) can pick it up at entry without an ABI change. + /// + /// **This is a GC root, and must stay one (#7231).** The `AbortSignal` is + /// a NaN-boxed heap object, and between the stash and + /// `js_fetch_with_options`'s consume the argument lowering for the fetch + /// call itself still runs and allocates. The window is short, but the + /// cell is the only reference across it. static PENDING_FETCH_SIGNAL: Cell = const { Cell::new(f64::from_bits(crate::value::TAG_UNDEFINED)) }; } +/// Root + rewrite the stashed in-flight `fetch` `AbortSignal`. +pub(crate) fn scan_pending_fetch_signal_root_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + PENDING_FETCH_SIGNAL.with(|cell| { + let mut value = cell.get(); + if visitor.visit_nanbox_f64_slot(&mut value) { + cell.set(value); + } + }); +} + /// Stash the `signal` for the fetch call about to be dispatched. Set on the main /// thread immediately before the fetch call and consumed at the start of /// `js_fetch_with_options` — JS is single-threaded between those two points, so diff --git a/crates/perry-runtime/src/object/global_this.rs b/crates/perry-runtime/src/object/global_this.rs index c319350fce..8d59bf39f6 100644 --- a/crates/perry-runtime/src/object/global_this.rs +++ b/crates/perry-runtime/src/object/global_this.rs @@ -143,7 +143,8 @@ pub(crate) use math_temporal::install_temporal_namespace; pub(crate) use math_temporal::temporal_kind_prototype; pub(crate) use math_temporal::{install_math_namespace, temporal_ctor_kind}; pub(crate) use populate::{ - default_prepare_stack_trace_func_ptr, populate_global_this_builtins, ERROR_CONSTRUCTOR_PTR, + default_prepare_stack_trace_func_ptr, populate_global_this_builtins, + scan_error_constructor_root_mut, ERROR_CONSTRUCTOR_PTR, }; pub(crate) use proto_methods::{ install_error_prototype_data_properties, populate_builtin_prototype_methods, diff --git a/crates/perry-runtime/src/object/global_this/populate.rs b/crates/perry-runtime/src/object/global_this/populate.rs index b2a2e97621..f615cd9a19 100644 --- a/crates/perry-runtime/src/object/global_this/populate.rs +++ b/crates/perry-runtime/src/object/global_this/populate.rs @@ -819,10 +819,29 @@ thread_local! { /// `perry/thread` agent has its own arena + realm, and an `Error` /// constructor / `prepareStackTrace` from another thread's arena can be a /// foreign or freed pointer — the same reason `globalThis` is per-thread. + /// + /// **This is a GC root, and must stay one (#7231).** The address is a RAW + /// `*mut ClosureHeader` from `js_closure_alloc` — a nursery allocation. + /// The canonical `Error` closure is also a field of `globalThis`, so the + /// structural trace keeps it alive and rewrites THAT reference; this + /// duplicate lives outside the object graph, so an evacuating collection + /// leaves it naming from-space and `error_prepare_stack_trace_override` + /// then reads `prepareStackTrace` off an abandoned closure. Rooted by + /// `scan_error_constructor_root_mut`. pub(crate) static ERROR_CONSTRUCTOR_PTR: std::cell::Cell = const { std::cell::Cell::new(0) }; } +/// Root + rewrite the raw `Error` constructor address. +pub(crate) fn scan_error_constructor_root_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + ERROR_CONSTRUCTOR_PTR.with(|cell| { + let mut addr = cell.get(); + if addr != 0 && visitor.visit_usize_slot(&mut addr) { + cell.set(addr); + } + }); +} + /// The default `Error.prepareStackTrace` thunk's address — used to tell a /// user override apart from Perry's built-in default. pub(crate) fn default_prepare_stack_trace_func_ptr() -> usize { diff --git a/crates/perry-runtime/src/object/mod.rs b/crates/perry-runtime/src/object/mod.rs index ee060e360b..aed7c4038a 100644 --- a/crates/perry-runtime/src/object/mod.rs +++ b/crates/perry-runtime/src/object/mod.rs @@ -36,6 +36,7 @@ mod class_constructors; mod class_gc_roots; mod class_handles; mod class_registry; +pub(crate) use class_registry::scan_current_new_target_root_mut; mod collection_proto_thunks; mod data_view_registry; mod dataview_proto_thunks; @@ -45,12 +46,16 @@ mod descriptors; mod disposable_proto_thunks; pub(crate) mod exotic_expando; mod field_get_set; +pub(crate) use field_get_set::scan_accessor_receiver_override_root_mut; mod field_set_by_name; mod global_fetch; +pub(crate) use global_fetch::scan_pending_fetch_signal_root_mut; mod global_this; pub mod handle_expando; pub(crate) mod prop_plan; -pub(crate) use global_this::{default_prepare_stack_trace_func_ptr, ERROR_CONSTRUCTOR_PTR}; +pub(crate) use global_this::{ + default_prepare_stack_trace_func_ptr, scan_error_constructor_root_mut, ERROR_CONSTRUCTOR_PTR, +}; mod global_this_tables; mod groupby; pub(crate) mod has_own_helpers; diff --git a/crates/perry-runtime/src/process.rs b/crates/perry-runtime/src/process.rs index 480975c34f..a36a81a3dc 100644 --- a/crates/perry-runtime/src/process.rs +++ b/crates/perry-runtime/src/process.rs @@ -17,6 +17,7 @@ mod env_misc; pub(crate) use env_misc::{ exit_after_current_thread_collection_teardown, format_out_of_range_number, process_env_delete_field, process_env_get_field, process_env_has_field, process_env_set_field, + scan_process_env_cache_roots_mut, }; mod finalization; pub(crate) mod ipc; @@ -56,7 +57,10 @@ pub use finalization::{ }; // ── permission re-exports ─────────────────────────────────────────────────── -pub(crate) use permission::process_permission_enabled; +pub(crate) use permission::{process_permission_enabled, scan_permission_cache_roots_mut}; + +// ── report re-exports ─────────────────────────────────────────────────────── +pub(crate) use report::scan_report_cache_roots_mut; // ── node_module re-exports ────────────────────────────────────────────────── pub use node_module::{ diff --git a/crates/perry-runtime/src/process/env_misc.rs b/crates/perry-runtime/src/process/env_misc.rs index 643ce1ef12..7db15a7809 100644 --- a/crates/perry-runtime/src/process/env_misc.rs +++ b/crates/perry-runtime/src/process/env_misc.rs @@ -1073,12 +1073,43 @@ pub extern "C" fn js_process_env() -> f64 { } thread_local! { + /// The materialized `process.env` object. + /// + /// **This is a GC root, and must stay one (#7231).** The object is + /// `js_object_alloc`'d in the NURSERY by `js_process_env_impl`, and this + /// cell is the only reference to it: `process.env` is not a field on the + /// `process` object, it is a `js_process_env()` call that returns the + /// cache. Before `scan_process_env_cache_roots_mut` existed the first + /// minor collection swept or evacuated it, and every later + /// `process.env.X = v` wrote through a dangling pointer into abandoned + /// memory — so `Object.keys(process.env)` / spread / `for…in` after any + /// collection saw a stale key set, and under an evacuating minor the write + /// landed in whatever object was recycled into those bytes. + /// + /// Not a stale-register defect: the cache goes wrong at collection #0 and + /// stays wrong, which is why its reproducer is 10/10 rather than + /// intermittent (#7226). static CACHED_ENV: std::cell::Cell = const { std::cell::Cell::new(0.0) }; /// Prevent the cached object's internal mirror update from re-entering the /// `process.env` object hooks and calling `set_var` / `remove_var` again. static ENV_CACHE_MUTATION: std::cell::Cell = const { std::cell::Cell::new(false) }; } +/// Root + rewrite the cached `process.env` object. +/// +/// Mirrors `scan_process_finalization_roots_mut`, the sibling +/// materialize-once cache that was already registered — the omission this +/// closes is that the two idioms are identical and only one of them was a +/// root. +pub(crate) fn scan_process_env_cache_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + CACHED_ENV.with(|cell| { + let mut value = cell.get(); + if value != 0.0 && visitor.visit_nanbox_f64_slot(&mut value) { + cell.set(value); + } + }); +} + #[cfg(windows)] thread_local! { /// Folded Windows name -> first spelling exposed through Object.keys(). diff --git a/crates/perry-runtime/src/process/permission.rs b/crates/perry-runtime/src/process/permission.rs index 787b93ba6d..be0e5174e7 100644 --- a/crates/perry-runtime/src/process/permission.rs +++ b/crates/perry-runtime/src/process/permission.rs @@ -186,14 +186,34 @@ extern "C" fn process_permission_drop_thunk( undefined_value() } +thread_local! { + /// The materialized `process.permission` object. + /// + /// **This is a GC root, and must stay one (#7231).** Nursery-allocated by + /// `process_permission_value` and referenced by nothing else — `process` + /// has no `permission` field, the getter returns this cache. The write + /// barrier below is an incremental MARK barrier, not a root registration: + /// it keeps a value published during an in-progress mark from being + /// missed, and does nothing at all for the sweep or the evacuation + /// rewrite. Hoisted out of the function body so + /// `scan_process_lazy_singleton_roots_mut` can reach it. + static CACHED_PERMISSION: std::cell::Cell = const { std::cell::Cell::new(0.0) }; +} + +/// Root + rewrite the cached `process.permission` object. +pub(crate) fn scan_permission_cache_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + CACHED_PERMISSION.with(|cell| { + let mut value = cell.get(); + if value != 0.0 && visitor.visit_nanbox_f64_slot(&mut value) { + cell.set(value); + } + }); +} + pub(crate) fn process_permission_value() -> Option { if !process_permission_enabled() { return None; } - use std::cell::Cell; - thread_local! { - static CACHED_PERMISSION: Cell = const { Cell::new(0.0) }; - } let cached = CACHED_PERMISSION.with(|c| c.get()); if cached != 0.0 { diff --git a/crates/perry-runtime/src/process/report.rs b/crates/perry-runtime/src/process/report.rs index 0918879a59..a6f1290407 100644 --- a/crates/perry-runtime/src/process/report.rs +++ b/crates/perry-runtime/src/process/report.rs @@ -105,12 +105,27 @@ fn process_report_default_filename() -> String { format!("report.{}.json", std::process::id()) } -pub(crate) fn process_report_value() -> f64 { - use std::cell::Cell; - thread_local! { - static CACHED_REPORT: Cell = const { Cell::new(0.0) }; - } +thread_local! { + /// The materialized `process.report` controller object. + /// + /// **This is a GC root, and must stay one (#7231).** Same shape as + /// `CACHED_PERMISSION` and `CACHED_ENV`: nursery-allocated, cached + /// forever, referenced by nothing else. Hoisted out of the function body + /// so the scanner can reach it. + static CACHED_REPORT: std::cell::Cell = const { std::cell::Cell::new(0.0) }; +} + +/// Root + rewrite the cached `process.report` controller object. +pub(crate) fn scan_report_cache_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + CACHED_REPORT.with(|cell| { + let mut value = cell.get(); + if value != 0.0 && visitor.visit_nanbox_f64_slot(&mut value) { + cell.set(value); + } + }); +} +pub(crate) fn process_report_value() -> f64 { let cached = CACHED_REPORT.with(|c| c.get()); if cached != 0.0 { return cached; diff --git a/crates/perry-runtime/src/tty.rs b/crates/perry-runtime/src/tty.rs index a44e80a90a..be057a9968 100644 --- a/crates/perry-runtime/src/tty.rs +++ b/crates/perry-runtime/src/tty.rs @@ -55,9 +55,25 @@ static RAW_MODE_SAVED: Mutex> = Mutex::new(None); thread_local! { /// Callback for `process.stdout.on('resize', cb)`. Stored on main /// thread; only touched by the drain (which runs on main). + /// + /// **This is a GC root, and must stay one (#7231).** `register_resize_callback` + /// is a NATIVE slot that bypasses the (rooted) EventEmitter listener + /// array, so this is the only reference to the closure. Before + /// `scan_tty_resize_callback_root_mut`, a collection between registration + /// and the first SIGWINCH left `js_closure_call0` calling a reclaimed or + /// relocated `ClosureHeader*`. static RESIZE_CALLBACK: RefCell> = const { RefCell::new(None) }; } +/// Root + rewrite the `process.stdout.on('resize')` callback closure. +pub(crate) fn scan_tty_resize_callback_root_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + RESIZE_CALLBACK.with(|cell| { + if let Some(cb) = cell.borrow_mut().as_mut() { + visitor.visit_i64_slot(cb); + } + }); +} + // --------------------------------------------------------------------------- // Per-platform isatty + winsize // --------------------------------------------------------------------------- diff --git a/crates/perry-runtime/src/tui/input.rs b/crates/perry-runtime/src/tui/input.rs index 93c46424a3..45eb564127 100644 --- a/crates/perry-runtime/src/tui/input.rs +++ b/crates/perry-runtime/src/tui/input.rs @@ -35,6 +35,14 @@ static READER_STARTED: AtomicBool = AtomicBool::new(false); static READING: AtomicBool = AtomicBool::new(false); /// Registered useInput handler — at most one for v1. Multiple-handler /// dispatch lands in Phase 2.5. Stored as the raw closure pointer. +/// +/// **This is a GC root, and must stay one (#7231).** `useInput(cb)` is +/// written inline in idiomatic ink style, so the arrow closure is a +/// nursery allocation that NOTHING else refers to once +/// `js_perry_tui_use_input` returns. Before +/// `scan_tui_input_handler_root_mut` the next collection reclaimed it and +/// `drain_input` called through a dangling `ClosureHeader*` on the first +/// keystroke. static INPUT_HANDLER: AtomicI64 = AtomicI64::new(0); /// Set when the user calls exit() — render loop checks this each frame. pub static EXIT_FLAG: AtomicBool = AtomicBool::new(false); @@ -164,6 +172,18 @@ pub fn disable_raw_mode() { READING.store(false, Ordering::Release); } +/// Root + rewrite the registered `useInput` handler closure. +/// +/// Process-global rather than thread-local, matching the slot it scans. A +/// `perry/thread` agent registering its own handler would already be +/// clobbering the main thread's, which is a pre-existing single-handler +/// limitation and not something this scanner changes: it visits whatever +/// address is currently published, and a foreign-arena address is rejected by +/// the visitor's own heap-attribution check rather than rewritten. +pub(crate) fn scan_tui_input_handler_root_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + visitor.visit_atomic_i64_slot(&INPUT_HANDLER, Ordering::Acquire, Ordering::Release); +} + /// Register the user's `useInput` handler. Replaces any prior handler /// — v1 supports a single handler. #[no_mangle] diff --git a/crates/perry-stdlib/src/worker_threads.rs b/crates/perry-stdlib/src/worker_threads.rs index 8870f88aad..cf85d9cd33 100644 --- a/crates/perry-stdlib/src/worker_threads.rs +++ b/crates/perry-stdlib/src/worker_threads.rs @@ -1870,14 +1870,46 @@ fn ensure_parent_port_event_gc_scanner() { }); } +/// Visit one raw closure pointer stored as `i64`. +/// +/// Factored out because this scanner covers three sibling slots and three +/// copies of the box/visit/unbox dance is how the fourth one gets forgotten — +/// which is precisely what happened to `MESSAGE_CALLBACK` and `CLOSE_CALLBACK` +/// (#7231). +fn visit_raw_closure_i64(visitor: &mut perry_runtime::gc::RuntimeRootVisitor<'_>, cb: &mut i64) { + // Box it into a NaN-boxed pointer slot so the GC can visit + relocate it, + // then unbox. + let mut boxed = perry_runtime::value::js_nanbox_pointer(*cb).to_bits(); + visitor.visit_nanbox_u64_slot(&mut boxed); + *cb = perry_runtime::value::js_nanbox_get_pointer(f64::from_bits(boxed)); +} + +/// Root + rewrite every `parentPort` listener closure. +/// +/// **#7231: this scanner used to walk one of three sibling slots.** +/// `MESSAGE_EVENT_CALLBACKS`, `MESSAGE_CALLBACK` and `CLOSE_CALLBACK` are +/// declared in the same `thread_local!` block and all hold the same thing — a +/// raw `ClosureHeader*` as `i64`, the only reference to a closure the user +/// passed to `parentPort.on(...)` / `.addEventListener(...)`. Only the first +/// was visited, so the Node-style `on('message')` and `on('close')` handlers +/// were reclaimed or relocated by the next collection inside a worker. +/// +/// A partially-correct scanner is worse than an absent one: it reads as +/// covered. fn scan_parent_port_event_roots_mut(visitor: &mut perry_runtime::gc::RuntimeRootVisitor<'_>) { MESSAGE_EVENT_CALLBACKS.with(|cbs| { for cb in cbs.borrow_mut().iter_mut() { - // Stored as a raw closure pointer (i64). Box it into a NaN-boxed - // pointer slot so the GC can visit + relocate it, then unbox. - let mut boxed = perry_runtime::value::js_nanbox_pointer(*cb).to_bits(); - visitor.visit_nanbox_u64_slot(&mut boxed); - *cb = perry_runtime::value::js_nanbox_get_pointer(f64::from_bits(boxed)); + visit_raw_closure_i64(visitor, cb); + } + }); + MESSAGE_CALLBACK.with(|cb| { + if let Some(ptr) = cb.borrow_mut().as_mut() { + visit_raw_closure_i64(visitor, ptr); + } + }); + CLOSE_CALLBACK.with(|cb| { + if let Some(ptr) = cb.borrow_mut().as_mut() { + visit_raw_closure_i64(visitor, ptr); } }); } diff --git a/test-files/test_gap_gc_process_env_cache_rooting.ts b/test-files/test_gap_gc_process_env_cache_rooting.ts new file mode 100644 index 0000000000..ab08bc18db --- /dev/null +++ b/test-files/test_gap_gc_process_env_cache_rooting.ts @@ -0,0 +1,95 @@ +// #7231: the materialized `process.env` object is a GC root and must be +// registered as one. +// +// `js_process_env_impl` (process/env_misc.rs) builds `process.env` once with +// `crate::object::js_object_alloc` — the NURSERY — and stores it in a +// thread-local `Cell`. That cell is the whole reference graph: +// `process.env` is not a field of the `process` object, it is a +// `js_process_env()` CALL that returns the cache. So before this fix the first +// minor collection swept or evacuated the object and every later +// `process.env.X = v` wrote through a dangling pointer into abandoned memory. +// +// The sibling `PROCESS_FINALIZATION_OBJECT` (process.rs) uses the *same* +// materialize-once-cache idiom and IS rooted +// (`scan_process_finalization_roots_mut`), which is what makes this an +// omission rather than a design. +// +// WHY ENUMERATION IS THE OBSERVABLE. A direct `process.env.KEY` READ lowers to +// `js_getenv`, which asks the OS and is therefore correct whatever state the +// cached object is in. What walks the cached object is ENUMERATION — +// `Object.keys(process.env)`, `for…in`, spread — which is exactly how +// `@next/env` and `dotenv` consume it. Testing the read would be a gate that +// cannot fail. +// +// NOT A STALE REGISTER, which is the diagnostic signature worth internalising +// (#7226): an unrooted register goes bad only if a collection lands in a narrow +// window, so it reproduces intermittently. An unregistered CACHE goes bad at +// collection #0 and stays bad, so it reproduces every time — and no static +// checker can see it, because `scripts/gc_root_dominance_check.py` reads +// emitted LLVM IR and a runtime-side table is not in it. +// +// LIVE BY CONSTRUCTION. The keys are written across the churn loop rather than +// all up front, so the object is mutated on both sides of a collection: the +// early keys test that a swept/moved object still enumerates, and the late ones +// test that the write itself landed in the live object rather than in whatever +// was recycled into its bytes. Keys are namespaced and the output is filtered +// to them, so the expectation does not depend on the machine's environment. + +const PREFIX = "PERRY_T7231_"; + +function churn(x: number): number { + const bits: any[] = []; + for (let i = 0; i < 400; i++) { + bits.push({ i: i, s: "env" }); + } + return x + bits.length - 400; +} + +function run(): number { + let bad = 0; + + // Prime the cache before any collection, then keep writing through it. + process.env[PREFIX + "0"] = "v0"; + + for (let r = 0; r < 400; r++) { + churn(r); + if (r % 100 === 0) { + process.env[PREFIX + String(r / 100 + 1)] = "v" + String(r / 100 + 1); + } + // Enumeration walks the CACHED object. A collection that reclaimed or + // relocated it without a root makes this read abandoned memory. + const seen: string[] = []; + for (const k of Object.keys(process.env)) { + if (k.indexOf(PREFIX) === 0) { + seen.push(k); + } + } + // key "0" is written before the loop and one more at every r % 100 === 0, + // and the write for this iteration has already happened above. + const want = 2 + ((r / 100) | 0); + if (seen.length !== want) { + bad++; + } + } + + // Spread is the other consumer (`{ ...process.env }`), and it walks the same + // object through a different path. + const copy: any = { ...process.env }; + for (let n = 0; n < 4; n++) { + if (copy[PREFIX + String(n)] !== "v" + String(n)) { + bad++; + } + } + + return bad; +} + +console.log("bad", run()); +const finalKeys: string[] = []; +for (const k of Object.keys(process.env)) { + if (k.indexOf(PREFIX) === 0) { + finalKeys.push(k); + } +} +finalKeys.sort(); +console.log("keys", finalKeys.join(",")); diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index 497bbb9f15..5c469558bd 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -368,3 +368,25 @@ test_gap_gc_staging_args_rooting # only the step scanner. A partially-correct scanner is worse than an absent one # — it reads as covered. Measured at base: `BAD interval.a` from tick 2 onward. test_gap_gc_interval_args_rooting + +# --- #7231: unrooted runtime-side caches (nothing static can find these) ---- +# `js_process_env_impl` builds `process.env` once with `js_object_alloc` (the +# NURSERY) and stores it in a thread-local `Cell` that no scanner visited. +# That cell is the whole reference graph — `process.env` is a getter CALL, not +# a field of `process` — so the first minor swept or evacuated the object and +# every later `process.env.X = v` wrote through a dangling pointer. +# +# The observable is ENUMERATION (`Object.keys(process.env)`, spread, `for…in`), +# because a direct `process.env.KEY` read lowers to `js_getenv` and asks the OS. +# +# Measured at base (`c9cd73ba5`), compiled and run under +# `PERRY_GC_MOVING_LOOP_POLLS=1` with `PERRY_GC_INCREMENTAL=0 +# PERRY_CONSERVATIVE_STACK_SCAN=off PERRY_GC_HEAP_LIMIT=8`: **SIGBUS 10/10**, +# `[gc-fromspace-protect] obj_type=2 size=416 … RETIRED FROM-SPACE`. Clean 5/5 +# on the shipped default both before and after, so this is a `requires=move` +# witness. 10/10 green after, byte-exact vs node 26.5.1. +# +# The determinism is the diagnostic signature (#7226): an unrooted CACHE goes +# bad at collection #0 and stays bad, where an unrooted REGISTER needs a +# collection to land in a narrow window. +test_gap_gc_process_env_cache_rooting