From 8f9920dd53dea26c489ab9083bcf7c9801cfd0e2 Mon Sep 17 00:00:00 2001 From: jdalton Date: Sat, 1 Aug 2026 01:55:11 -0400 Subject: [PATCH] fix(gc): disable evacuating minor by default pending #7154 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The evacuating (moving-loop) minor GC that #7019 flipped default-on has a use-after-free: a young closure referenced from a dynamically-added object field (field[1], holders built in proxy::create_or_update_receiver_property from zod's $constructor) is reclaimed while still live, so the field dangles and a later call dies with `TypeError: value is not a function`. A live-trace evacuation probe confirmed the edge IS enumerated on the evac path but classify() correctly rejects the target because it already points at reclaimed/garbage NurseryEden memory (dangling nanboxed pointer) — the closure died in an earlier cycle. This reproduces in the DEFAULT config (no env), so the shipped binary can corrupt the heap. Flip both the runtime gate (gc_moving_loop_polls_enabled) and the coherent codegen gate (moving_safepoint_polls_enabled) to DEFAULT OFF, restoring the previously-correct non-moving minor. The moving-loop path is unchanged and still reachable behind an explicit PERRY_GC_MOVING_LOOP_POLLS=1 opt-in (compile + run). Stopgap only; root cause tracked in #7154. Verification: - default `./bin-registry --help` (no env): clean 10/10 (was crashing) - compiled+run with PERRY_GC_MOVING_LOOP_POLLS=1: still crashes (path retained) - cargo test -p perry-runtime: added test_moving_loop_minor_off_by_default_7154; only pre-existing order-dependent / toolchain failures remain (confirmed identical on the clean base). --- ...154-gc-disable-evacuating-minor-default.md | 10 ++++ crates/perry-codegen/src/stmt/loops.rs | 15 +++--- crates/perry-runtime/src/gc/policy.rs | 46 +++++++++++++------ crates/perry-runtime/src/gc/tests/triggers.rs | 23 ++++++++++ 4 files changed, 74 insertions(+), 20 deletions(-) create mode 100644 changelog.d/7154-gc-disable-evacuating-minor-default.md diff --git a/changelog.d/7154-gc-disable-evacuating-minor-default.md b/changelog.d/7154-gc-disable-evacuating-minor-default.md new file mode 100644 index 0000000000..ddf9a0f3e8 --- /dev/null +++ b/changelog.d/7154-gc-disable-evacuating-minor-default.md @@ -0,0 +1,10 @@ +Disabled the evacuating (moving-loop) minor GC by default, restoring the +non-moving minor as the default collector. The evacuating minor #7019 made +default-on has a use-after-free (#7154): a young closure referenced from a +dynamically-added object field is reclaimed while still live, so a later call +dies with `TypeError: value is not a function`. This reproduced in the default +configuration, so the shipped binary could corrupt the heap. The moving-loop +path is unchanged and still available behind an explicit +`PERRY_GC_MOVING_LOOP_POLLS=1` opt-in (compile and run). This is a stopgap until +the root cause tracked in #7154 is fixed; it trades #7019's minor-GC +RSS/throughput win for correctness. diff --git a/crates/perry-codegen/src/stmt/loops.rs b/crates/perry-codegen/src/stmt/loops.rs index 646918be01..5477656bcb 100644 --- a/crates/perry-codegen/src/stmt/loops.rs +++ b/crates/perry-codegen/src/stmt/loops.rs @@ -5227,15 +5227,16 @@ fn lower_for_after_init_with_i32_bound( fn moving_safepoint_polls_enabled() -> bool { use std::sync::OnceLock; static CACHED: OnceLock = OnceLock::new(); - // DEFAULT ON (moving-nursery flip): emit the back-edge poll, but ONLY for - // allocating loops (see the `loop_may_allocate` gate in - // `emit_gc_loop_safepoint`) so numeric/vectorizable loops stay call-free. - // Kill switch: PERRY_GC_MOVING_LOOP_POLLS=0/off/false. Must match the runtime - // `gc_moving_loop_polls_enabled` (same env) so deferrals always have a drain. + // DEFAULT OFF (stopgap for #7154): the runtime moving-loop minor this poll + // drives has a use-after-free that corrupts the heap even in the default + // config, so the default reverts to the non-moving minor and the poll is + // emitted only under an explicit PERRY_GC_MOVING_LOOP_POLLS=1/on/true opt-in. + // Must match the runtime `gc_moving_loop_polls_enabled` (same env) so a + // deferred collection always has a drain and vice versa. *CACHED.get_or_init(|| { - !matches!( + matches!( std::env::var("PERRY_GC_MOVING_LOOP_POLLS").as_deref(), - Ok("0") | Ok("off") | Ok("false") + Ok("1") | Ok("on") | Ok("true") ) }) } diff --git a/crates/perry-runtime/src/gc/policy.rs b/crates/perry-runtime/src/gc/policy.rs index 9656a817ee..675a193ed9 100644 --- a/crates/perry-runtime/src/gc/policy.rs +++ b/crates/perry-runtime/src/gc/policy.rs @@ -359,18 +359,29 @@ pub(crate) fn gc_incremental_enabled() -> bool { /// Make the moving minor PRIMARY inside loops: defer the alloc-point nursery /// collection to a codegen loop back-edge poll (`js_gc_loop_safepoint`) instead /// of collecting non-moving mid-expression, so reallocation-heavy loops evacuate -/// (bounded RSS) instead of leaking. **DEFAULT ON** as of the moving-nursery flip -/// — the poll is now emitted only for ALLOCATING loop bodies (`body_may_allocate` -/// in codegen), so numeric/vectorizable loops stay call-free. Kill switch is an -/// explicit `PERRY_GC_MOVING_LOOP_POLLS=0`/`off`/`false` (bisection / max-throughput -/// batch). MUST match codegen `moving_safepoint_polls_enabled` (same env) so the -/// deferral and the polls that drain it stay coherent — a runtime default-on with -/// a codegen default-off (or vice versa) would defer collections that never drain. +/// (bounded RSS) instead of leaking. +/// +/// **DEFAULT OFF (stopgap for #7154).** This was flipped default-ON in #7019, but +/// the evacuating minor it makes primary has a use-after-free: a young closure +/// referenced from a dynamically-added object field (`field[1]`, holders built in +/// `proxy::create_or_update_receiver_property`) is reclaimed while still live, so +/// the field dangles and a later call dies with `TypeError: value is not a +/// function`. This reproduces in the DEFAULT config (no env) — the shipped binary +/// corrupts the heap. `PERRY_GC_MOVING_LOOP_POLLS=0` is confirmed to eliminate it, +/// so until #7154 is root-caused we default OFF (restoring the previously-correct +/// non-moving minor) and keep the moving-loop path behind an explicit +/// `PERRY_GC_MOVING_LOOP_POLLS=1`/`on`/`true` opt-in. Reverting the default costs +/// #7019's minor-GC RSS/throughput win but not correctness. +/// +/// MUST match codegen `moving_safepoint_polls_enabled` (same env) so the deferral +/// and the polls that drain it stay coherent — a runtime default that disagrees +/// with the codegen default would defer collections that never drain (or drain +/// collections that were never deferred). pub(crate) fn gc_moving_loop_polls_enabled() -> bool { // Test-only mode override (see `force_legacy_gc_pacing`). Consulted BEFORE - // the process-wide OnceLock so a single test can pin legacy (non-moving, - // budgeted/direct, 128 MiB-ceiling) pacing for its duration even though the - // process default is moving-on. Compiled out entirely in release builds. + // the process-wide OnceLock so a single test can pin a specific pacing mode + // for its duration even though the process default is off. Compiled out + // entirely in release builds. #[cfg(test)] if let Some(forced) = GC_MOVING_LOOP_POLLS_TEST_OVERRIDE.with(Cell::get) { return forced; @@ -378,13 +389,22 @@ pub(crate) fn gc_moving_loop_polls_enabled() -> bool { static CACHED: OnceLock = OnceLock::new(); *CACHED.get_or_init(|| { - !matches!( - std::env::var("PERRY_GC_MOVING_LOOP_POLLS").as_deref(), - Ok("0") | Ok("off") | Ok("false") + moving_loop_polls_enabled_from_env( + std::env::var("PERRY_GC_MOVING_LOOP_POLLS").ok().as_deref(), ) }) } +/// Pure env→enable decision for the moving-loop minor, factored out so the +/// default is unit-testable without touching process env / the cached `OnceLock`. +/// **Default OFF (#7154 stopgap):** an unset var (or any value other than an +/// explicit opt-in) selects the non-evacuating minor; only `1`/`on`/`true` +/// enables the moving-loop path. Codegen's `moving_safepoint_polls_enabled` +/// mirrors this exactly (same env, same predicate). +pub(crate) fn moving_loop_polls_enabled_from_env(value: Option<&str>) -> bool { + matches!(value, Some("1") | Some("on") | Some("true")) +} + #[cfg(test)] thread_local! { /// Test-only override for [`gc_moving_loop_polls_enabled`]. When `Some(v)`, diff --git a/crates/perry-runtime/src/gc/tests/triggers.rs b/crates/perry-runtime/src/gc/tests/triggers.rs index 62c13b4be8..2040bfb58c 100644 --- a/crates/perry-runtime/src/gc/tests/triggers.rs +++ b/crates/perry-runtime/src/gc/tests/triggers.rs @@ -429,6 +429,29 @@ fn test_effective_arena_trigger_respects_armed_values() { GC_TRIGGER_ARMED.with(|c| c.set(prev_armed)); } +// #7154 stopgap: the moving-loop (evacuating) minor must be OFF by default. +// #7019 flipped it default-on, but the evacuating minor has a use-after-free +// that corrupts the heap in the default config, so the default must select the +// non-evacuating minor; the moving path stays reachable only via an explicit +// PERRY_GC_MOVING_LOOP_POLLS=1/on/true opt-in. +#[test] +fn test_moving_loop_minor_off_by_default_7154() { + use super::super::policy::moving_loop_polls_enabled_from_env as enabled; + // Default (unset) is non-evacuating. + assert!(!enabled(None), "moving-loop minor must be OFF by default (#7154)"); + // Kill-switch values remain off. + assert!(!enabled(Some("0"))); + assert!(!enabled(Some("off"))); + assert!(!enabled(Some("false"))); + // Unknown / garbage values fall back to the safe default (off). + assert!(!enabled(Some(""))); + assert!(!enabled(Some("2"))); + // Explicit opt-in enables the moving path. + assert!(enabled(Some("1"))); + assert!(enabled(Some("on"))); + assert!(enabled(Some("true"))); +} + // #6184: the OS memory-pressure entry must run a real collection when the // thread is at a safe point, and must lower+arm the arena trigger. #[test]