From 4e99c1bad79f542965f31b080cc62d77c203b4b0 Mon Sep 17 00:00:00 2001 From: John-David Dalton Date: Sat, 1 Aug 2026 21:01:36 -0400 Subject: [PATCH 1/3] fix(gc): root the interned typeof strings, the rawJSON key, and every saved implicit `this` Three unrooted-value bugs behind #7154's registry crash, found by pointing #7196's from-space reporter at `sfw-registry --help` rather than by grinding the static checker's tail. 1. RUNTIME CACHES (the one that mattered). `js_value_typeof` interned its eight result strings in thread-local `Cell<*mut StringHeader>`s that nothing registered as GC roots, so the FIRST minor collection swept or evacuated them and every later `typeof x === "..."` compared against from-space. `json/raw_json.rs`'s cached `"rawJSON"` key had the identical defect. Both now go through `gc_register_mutable_root_scanner`. This is why the registry failed 10/10 rather than intermittently: an unrooted register goes bad only when a collection lands in its window, an unrooted cache goes bad at collection #0 and stays bad. It is also structurally invisible to `scripts/gc_root_dominance_check.py`, which reads emitted LLVM IR and cannot see a runtime table. 2. CODEGEN, implicit `this`. `js_implicit_this_set` returns the value it displaced from the `IMPLICIT_THIS` cell -- a scanned MUTABLE root -- and that value was then held in a bare SSA register across the whole call the bind exists to scope. The restore published a pre-move address back INTO a root. #7214 found this in `js_closure_callN` and left it; it was in fact seven lowerings with seven copies of the same three lines, now one shared `temp_root::implicit_this_save` / `implicit_this_restore` pair. 3. CODEGEN, ClassExprFresh (#7211). Its `protect_handle` predicate asked only whether the AUTHOR's static initializers could collect, never whether the `js_object_set_field_by_name` the lowering itself emits per static could -- and that allocates. The four allowlist entries it covered are deleted, which is the ratchet working: the fix made them match nothing. Checker: `js_implicit_this_set` is now a root READ (being non-collecting is what makes a call one), which takes its sink from 214 hits to 0 and keeps the class gated. `--stale-registers` now honours `--min-binds`, and `--any-def` with it is a usage error -- both the "a silently ignored knob is a disarmed knob" rule the mode already applied to `--max-stale` and `--fatal-sinks`. Refs #7154, #7211, #7213, #7196, #7206, #7214, #7161 --- CLAUDE.md | 3 +- .../7219-registry-gc-unrooted-caches.md | 180 ++++++++++++++++++ .../src/expr/static_field_meta.rs | 35 +++- crates/perry-codegen/src/expr/temp_root.rs | 56 ++++++ .../src/lower_call/console_promise.rs | 72 ++++++- .../src/lower_call/early_branches.rs | 41 ++-- .../perry-codegen/src/lower_call/func_ref.rs | 13 +- .../src/lower_call/method_override.rs | 9 +- .../property_get/dynamic_dispatch.rs | 22 +-- .../property_get/static_dispatch.rs | 9 +- .../perry-runtime/src/builtins/arithmetic.rs | 143 +++++++++++--- crates/perry-runtime/src/builtins/mod.rs | 5 +- crates/perry-runtime/src/gc/mod.rs | 12 ++ crates/perry-runtime/src/json/mod.rs | 4 +- crates/perry-runtime/src/json/raw_json.rs | 33 +++- crates/perry-runtime/src/string/alloc.rs | 31 +++ scripts/gc_root_dominance_allowlist.json | 41 ++-- scripts/gc_root_dominance_check.py | 108 ++++++++++- ...t_gap_gc_closure_call_prev_this_rooting.ts | 66 +++++++ ...test_gap_gc_typeof_string_cache_rooting.ts | 69 +++++++ 20 files changed, 814 insertions(+), 138 deletions(-) create mode 100644 changelog.d/7219-registry-gc-unrooted-caches.md create mode 100644 test-files/test_gap_gc_closure_call_prev_this_rooting.ts create mode 100644 test-files/test_gap_gc_typeof_string_cache_rooting.ts diff --git a/CLAUDE.md b/CLAUDE.md index 65580f82a7..aa5ffdbf9d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -250,4 +250,5 @@ Corollary: a *new* gate has never been green, so promoting it to required immedi - **Async-to-generator transform, body locals.** It boxes every body local into a shared mutable cell typed `Any`. Two consequences seen in the wild: per-iteration `let`/`const` bindings collapse for closures created in a loop, and computed numeric-key calls (`arr[i](x)`) lose their type proof and silently resolve by *method name*, evaporating the call. - **Native base-class subclassing.** A native base's surface is installed at `super()` time and its parent edge lives in the class registry; keying any of that on a literal `extends` name loses it for fieldless classes, indirect subclasses, and class expressions. - **Two prototype-resolution paths.** `CLASS_PROTOTYPE_OBJECTS` (synthetic: `Object.create`, plain-function ctors) vs `CLASS_DECL_PROTOTYPE_OBJECTS` (declared classes). `in`/`for…in` and `getPrototypeOf` have disagreed about the same chain. -- **Root-store dominance in codegen.** *A GC-managed value's root store must **dominate** every subsequent site that can collect.* Three ways it has broken, all shipped: the store's slot index fell outside the pushed shadow frame so `js_shadow_slot_bind` bounds-checked it into a silent no-op (#7184); the store was emitted in-frame but **after** a call that allocates (#7192); and the value lives in a plain `alloca_entry` that is neither a shadow slot nor a temp root, so the collector never rewrites it (`lower_call/new.rs`'s inline-ctor `this_slot`, closed by #7207; `--unrooted-allocas` is the detector for that shape, and its remaining hits are #7210's). All three present identically — a *rooted* slot holding a dangling pointer, surfacing cycles later as `TypeError: value is not a function` — and **none is visible to any runtime GC probe**, because at the moment of the collection there is nothing for the collector to find. That is why #7154's from-space scan only ever saw offenders whose targets had already died. The instrument is static: `scripts/gc_root_dominance_check.py` over `--trace llvm` output (`--self-test` proves it can still fail). Only bites under `PERRY_GC_MOVING_LOOP_POLLS=1`, off by default since #7161 — so a green default run says nothing about this class. **Full writeup, every known shape and how to check your work: `docs/src/internals/gc-rooting-invariant.md`.** The CI gate is `gc-root-dominance.yml` over `scripts/gc_root_dominance_corpus.sh`; known-remaining hits are named one-per-entry in `scripts/gc_root_dominance_allowlist.json` (an entry that matches nothing FAILS, so a fix must delete its entry). A fifth shape is open as #7211: `ClassExprFresh` roots only when it thinks the static *initializers* collect, and never asks whether its own emitted `js_object_set_field_by_name` does — the sophisticated version of the mistake, where the author wrote a rooting predicate and it asked the wrong question. +- **Root-store dominance in codegen.** *A GC-managed value's root store must **dominate** every subsequent site that can collect.* Three ways it has broken, all shipped: the store's slot index fell outside the pushed shadow frame so `js_shadow_slot_bind` bounds-checked it into a silent no-op (#7184); the store was emitted in-frame but **after** a call that allocates (#7192); and the value lives in a plain `alloca_entry` that is neither a shadow slot nor a temp root, so the collector never rewrites it (`lower_call/new.rs`'s inline-ctor `this_slot`, closed by #7207; `--unrooted-allocas` is the detector for that shape, and its remaining hits are #7210's). All three present identically — a *rooted* slot holding a dangling pointer, surfacing cycles later as `TypeError: value is not a function` — and **none is visible to any runtime GC probe**, because at the moment of the collection there is nothing for the collector to find. That is why #7154's from-space scan only ever saw offenders whose targets had already died. The instrument is static: `scripts/gc_root_dominance_check.py` over `--trace llvm` output (`--self-test` proves it can still fail). Only bites under `PERRY_GC_MOVING_LOOP_POLLS=1`, off by default since #7161 — so a green default run says nothing about this class. **Full writeup, every known shape and how to check your work: `docs/src/internals/gc-rooting-invariant.md`.** The CI gate is `gc-root-dominance.yml` over `scripts/gc_root_dominance_corpus.sh`; known-remaining hits are named one-per-entry in `scripts/gc_root_dominance_allowlist.json` (an entry that matches nothing FAILS, so a fix must delete its entry) — **that list is now empty**, which #7211 emptied by fixing the fifth shape: `ClassExprFresh` rooted only when it thought the static *initializers* could collect and never asked whether its own emitted `js_object_set_field_by_name` could. The sophisticated version of the mistake — the author wrote a rooting predicate and it asked the wrong question. +- **A runtime-side cache of a raw heap pointer is a GC root, and the static checker cannot see it.** This is the sibling class, and it is the one that actually kept `sfw-registry --help` red after every codegen register in #7192/#7206/#7214 was closed. `js_value_typeof` interned its eight result strings in thread-local `Cell<*mut StringHeader>`s that nothing registered, so the FIRST minor collection invalidated them and every later `typeof x === "…"` compared against from-space (#7211). Two things to carry forward. **The failure signature is different**: an unrooted register goes bad only when a collection lands in its window, so it is intermittent; an unrooted cache goes bad at collection #0 and stays bad, so it fails 10/10 — if a GC bug is perfectly reproducible, suspect a table, not a register. And **`scripts/gc_root_dominance_check.py` reads emitted LLVM IR, so it is structurally blind to this**; the detector is `PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800` on a real workload, whose reporter names the address, `obj_type`, size and retiring cycle. Point the runtime instruments at the workload *before* grinding the static checker's tail. The root registry is `gc_register_mutable_root_scanner` in `gc/mod.rs` (~55 entries); when you add a cache of a heap pointer, add it there in the same commit. diff --git a/changelog.d/7219-registry-gc-unrooted-caches.md b/changelog.d/7219-registry-gc-unrooted-caches.md new file mode 100644 index 0000000000..7643dfbae3 --- /dev/null +++ b/changelog.d/7219-registry-gc-unrooted-caches.md @@ -0,0 +1,180 @@ +### Fixed + +- **`gc`: the interned `typeof` strings and `JSON.rawJSON`'s interned key are GC + roots, and are now registered as such** (#7211). `js_value_typeof` caches its + eight possible results in thread-local `Cell<*mut StringHeader>`s so each is + built once. Those cells held a **raw pointer into the nursery** with nothing + referencing the string, so the first minor collection swept or evacuated it + and the cache named abandoned memory for the rest of the process — every + later `typeof x === "string"` handed `js_string_equals` a from-space address. + `json/raw_json.rs`'s cached `"rawJSON"` key had the identical defect and is + fixed with it. Both now go through `gc_register_mutable_root_scanner`, so they + are marked (never swept) and rewritten (never stale after a copying minor). + + **This is what kept `sfw-registry --help` red** after #7192, #7206 and #7214 + had closed every stale *register* they could find, and the difference in + failure signature is the lesson worth keeping: + + | | unrooted register (#7154 class) | unrooted cache (this) | + |---|---|---| + | goes bad | only if a collection lands in a few-instruction window | at collection #0, permanently | + | reproduces | intermittently; needed a zod workload and ten rounds | **10/10** | + | found by | `scripts/gc_root_dominance_check.py` over emitted IR | nothing static — the tool cannot see a runtime table | + + A perfectly reproducible GC bug is evidence *against* a stale register. The + detector here was `PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 + PERRY_GC_PROTECT_FROMSPACE_DEPTH=800`, whose reporter named it outright: + + ``` + [gc-fromspace-protect] FAULT: signal 10 at 0x…558a4 + last-known object: user_ptr=0x…558a0 obj_type=3 size=40 + retired_by_minor=#0 + … perry_closure_node_modules_zod_src_v4_core_schemas_ts__222 + 100 + ``` + + `obj_type` 3 is a string and 40 bytes is a 32-byte header plus `"string"`; + `retired_by_minor=#0` is the tell for a table rather than a register. + +- **`codegen`: the saved implicit `this` is rooted across every dispatch that + binds one** (#7211). `js_implicit_this_set` swaps the `IMPLICIT_THIS` cell and + returns what was there. That cell is a registered *mutable* root + (`scan_implicit_this_roots_mut`, `object/this_binding.rs:176`) and the swap has + already overwritten it, so the returned value is held only in an SSA register + — across the whole call the bind exists to scope. A minor inside that call + moves the object and rewrites every root naming it, leaving the register on + from-space; the restore then writes that pre-move address **back into a root + the collector scans**, so the corruption outlives the call that caused it. + + #7214 identified this in `js_closure_callN` and left it measured but unfixed. + It was in fact **seven** lowerings with seven copies of the same three lines: + `js_closure_callN` (`lower_call/console_promise.rs`), the + `js_native_call_value` override arms in `lower_call/method_override.rs` and + both `lower_call/property_get` dispatchers, the static-dispatch arm, the + #3576 receiverless reset in `lower_call/func_ref.rs`, and both closure-call + arms in `lower_call/early_branches.rs`. They are now one shared pair, + `temp_root::implicit_this_save` / `implicit_this_restore`, so the eighth + lowering that needs it gets the root for free rather than the bug. + +- **`codegen`: `Expr::ClassExprFresh` roots the fresh class object across the + `js_object_set_field_by_name` calls it emits itself** (#7211). The old + `protect_handle` predicate had four disjuncts and every one asked whether + something the *author* supplied could collect — a captured argument, a symbol + static, a `static { … }` body, an initializer expression. None asked whether + the lowering's own emitted field-store could, and it can: that helper performs + the keys-array transition and allocates. So `class C { static tag = tag }`, + one inert `LocalGet`, got no root at all. + + `js_object_mark_class` does not rescue it, and that is the reason this + survived review once already: it files the pointer in `CLASS_OBJECT_VALUES`, + which is a registered root and *is* forwarded — which keeps the OBJECT alive + and the side table's copy correct, and does nothing for the register. + Reachability is not the invariant; the invariant is that the register you are + still going to use was rewritten. + +### Changed + +- **`scripts/gc_root_dominance_check.py`: `js_implicit_this_set` is a root + READ.** Being non-collecting is exactly what makes a call a root read — the + same rule `js_closure_get_capture_bits` is listed under — and this one was + `NONCOLLECTING` without being a source, so the checker saw a safe call, never + classified the result as a heap value, and reported nothing at either end. + That blind spot is why `prev_this` survived #7206 and #7214. It reports the + class now: **214 hits on the parent, 0 after**, the largest single named sink + in the corpus. + +- **`scripts/gc_root_dominance_allowlist.json` is empty.** The four #7211 + entries were deleted because the fix made every one of them match nothing — + rule 1 working as designed: the gate goes red on a stale entry, and that red + is the instruction to delete it. An empty list is not a disarmed gate; rule 3 + still fails any violation, the `--min-files`/`--min-binds`/`--min-funcs` + floors still refuse a corpus that did not exercise the subject, and + `--self-test` still proves the checker can report a planted violation. + +- **`--stale-registers` now honours `--min-binds`**, and `--any-def` with + `--stale-registers` is a usage error. Both are the "a knob that is silently + ignored is a disarmed knob" rule the mode already applied to `--max-stale` + and `--fatal-sinks`, applied in the other direction. `--min-binds` is not a + bind-anchored-check detail: `run_stale` classifies a shadow-slot load as a + heap-value source by looking it up in the same `BIND_RE`-derived map, so a + corpus compiled with `PERRY_INLINE_SHADOW_SLOT=1` loses those sources + entirely, reports `total 0`, exits 0, and is indistinguishable from a clean + corpus. That is hazard 4 — the gate runs but its subject never did. Both + directions are asserted in `--self-test`, over-budget and under. + +## Verification + +Everything below is measured against the parent commit (`7d1dc9ca2`), built +from its own worktree rather than borrowed from another one. + +`sfw-registry --help`, 141 modules, compiled **and** run with +`PERRY_GC_MOVING_LOOP_POLLS=1` (it is compile-time since #7161 *and* +runtime-armed at `gc/policy.rs:1759`; arming only one is the false green that +cost #7214 a round): + +| | parent | this PR | +|---|---|---| +| `POLLS=1` | **0/10** — SIGSEGV every run | **10/10 clean** | +| default (no polls) | 10/10 clean | **10/10 clean** | + +Gap tests: + +| | parent | this PR | +|---|---|---| +| `test_gap_gc_typeof_string_cache_rooting.ts`, `POLLS=1` | `bad 444` **10/10** | `bad 0` **10/10** | +| same, `POLLS=1` + `PERRY_GEN_GC=0` | `bad 0` | `bad 0` | +| `test_gap_gc_closure_call_prev_this_rooting.ts`, `POLLS=1` + `PERRY_GC_ZEAL=1` | `bad 400` **5/5** | `bad 0` **10/10** | +| same, `PERRY_GEN_GC=0` + zeal | `bad 0` | `bad 0` | + +The `prev_this` test needs zeal and the reason is worth recording: the window +is a user call, so the collection that exploits it has to be a *moving* one, +and the only moving collections are the loop back-edge poll and the +microtask-pump safepoint. Allocation-triggered collections take +`ManualGcScanGuard::force_full_scan`, which makes the copying minor ineligible. +Zeal is the sanctioned instrument for exactly that, and the `PERRY_GEN_GC=0` +arm proves the test tracks collector mode rather than merely being flaky. + +`ClassExprFresh` has **no runtime gap test, deliberately**, and the same +argument is why: its window contains only `js_object_set_field_by_name` — no +user code and no loop, therefore no moving collection can land in it today. The +subject is the invariant, so the gate is the gap test, and it moves: + +| checker, `--moving-only`, 130 modules / 2170 functions | parent | this PR | +|---|---|---| +| bind-anchored violations | 5 (all 4 allowlist entries used) | **0**, allowlist empty | +| `--stale-registers`, total | 2912 | **2817** | +| `--stale-registers`, `sink=js_implicit_this_set` | 214 | **0** | +| `--stale-registers --fatal-sinks` | 279 | 279 (untouched by this change) | + +`cargo test -p perry-codegen`: **6 failures on the parent, the identical 6 +here** — all `loop_safepoint_purity`, which is #7161's default flip, measured on +the parent rather than assumed. + +Cost, over the 141-module `sfw-registry` corpus. The implicit-`this` root is on +the dynamic-dispatch path, so it was measured rather than argued: + +| | parent | this PR | delta | +|---|---|---|---| +| linked binary | 26,848,048 B | 26,881,088 B | **+0.12 %** | + +## Not fixed here, and why + +`js_get_string_pointer_unified` hands generated code a **raw** `*StringHeader`, +and its SSO branch allocates. Every `unbox_str_handle` site in +`expr/compare.rs`, `lower_string_method.rs` and `lower_array_method.rs` unboxes +its operands back-to-back into bare registers before one consuming call, so the +first handle is live across the second unbox with no root describing it. It is +**not exploitable today** — that allocation reaches the alloc-point arm of +`gc_check_trigger`, which forces a conservative stack scan, which makes the +copying minor ineligible, so the collection it can cause never moves anything +and the same scan keeps the handle alive. Both halves are closed by accident, +and by accident is the point: it rests on the alloc-point arm staying +non-moving, which is the property the moving-GC work keeps eroding. + +Filed as **#7213** rather than fixed here. A `GcSuppressScope` around that +allocation makes the window sound and costs nothing measurable, and it was +written and then reverted: shipping a GC-trigger change with no test that can +fail without it is exactly what CLAUDE.md's knob-kill policy exists to stop. +`js_get_string_pointer_unified` is likewise left out of the checker's +`ROOT_READ_CALLS` — classifying it would report the whole family, roughly forty +sites, and that population deserves its own measured count rather than being +folded in here. diff --git a/crates/perry-codegen/src/expr/static_field_meta.rs b/crates/perry-codegen/src/expr/static_field_meta.rs index 9616a4225f..cb77254c3e 100644 --- a/crates/perry-codegen/src/expr/static_field_meta.rs +++ b/crates/perry-codegen/src/expr/static_field_meta.rs @@ -471,10 +471,39 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // and the block's body could then relocate the object out from under // the register the final `nanbox_pointer_inline` reads. let block_fns = static_block_fns(ctx, template); - let protect_handle = !captured_args.is_empty() + // #7211: `!named_statics.is_empty()` is the disjunct the original + // predicate was missing, and its absence is the interesting part. + // + // Every other clause here asks the same question — "can something + // the AUTHOR wrote collect?" — about a captured argument, a symbol + // static, a `static { … }` body, or an initializer expression. + // None of them asks whether the lowering's OWN emitted calls can, + // and the loop directly below unconditionally emits one + // `js_object_set_field_by_name` per named static. That helper + // performs the keys-array transition and allocates. So + // `class C { static tag = tag }` — a single inert `LocalGet` + // initializer — took `protect_handle == false`, kept the fresh + // object in a bare SSA register across a collection point, and + // then bound a shadow slot to the pre-move address. + // + // `js_object_mark_class` does NOT cover this, and it is the + // natural reason to wave it off: it files the pointer in + // `CLASS_OBJECT_VALUES`, which is a registered root and IS + // forwarded (`class_registry/gc_roots.rs:138`). That keeps the + // OBJECT alive and the side table's copy correct — and does + // nothing for `%obj`, a separate copy the collector cannot see. + // Reachability is not the invariant; the invariant is that the + // register you are still going to use was rewritten. + // The old `any_may_trigger_gc(named_statics)` disjunct is gone + // rather than kept alongside: it is now strictly subsumed — it can + // only be true when `named_statics` is non-empty, which is the new + // clause. Leaving it would read as a second, narrower opinion + // about the same operand and invite someone to "restore" the + // narrow one. + let protect_handle = !named_statics.is_empty() + || !captured_args.is_empty() || !symbol_statics.is_empty() - || !block_fns.is_empty() - || super::temp_root::any_may_trigger_gc(ctx, named_statics.iter().map(|(_, v)| v)); + || !block_fns.is_empty(); let rooted = super::temp_root::rooted_handle_begin(ctx, &obj, protect_handle); for (name, init) in named_statics { let key_idx = ctx.strings.intern(name); diff --git a/crates/perry-codegen/src/expr/temp_root.rs b/crates/perry-codegen/src/expr/temp_root.rs index 00bcaa3797..3c9bed3de6 100644 --- a/crates/perry-codegen/src/expr/temp_root.rs +++ b/crates/perry-codegen/src/expr/temp_root.rs @@ -102,6 +102,62 @@ pub(crate) fn temp_root_truncate(ctx: &mut FnCtx<'_>, idx: &str) { .call_void("js_gc_temp_root_truncate", &[(I32, idx)]); } +/// A saved implicit `this`, held in a temp-root slot for the duration of a +/// dispatch (#7211). +/// +/// `js_implicit_this_set` swaps the `IMPLICIT_THIS` cell and returns what was +/// there. That cell is a registered MUTABLE root — `scan_implicit_this_roots_mut` +/// (`object/this_binding.rs:176`) marks it and rewrites it on an evacuating +/// cycle — and the swap has already overwritten it, so the returned value is +/// now held ONLY in an SSA register, across the whole call the bind exists to +/// scope. A minor inside that call moves the object and rewrites every root +/// that names it, leaving this register on from-space; the restore then writes +/// that pre-move address BACK INTO the cell, so the corruption outlives the +/// call and lands on whatever reads `this` next. +/// +/// Seven lowerings emit this save/restore pair — `js_closure_callN`, the +/// `js_native_call_value` override arms in `method_override.rs` and both +/// `property_get` dispatchers, the static-dispatch arm, the direct-call +/// `#3576` reset in `func_ref.rs` and the two closure-call arms in +/// `early_branches.rs`. They had seven copies of the same three lines and +/// therefore seven copies of the same bug, which is why this is a helper +/// rather than seven edits: the next lowering that needs the pair gets the +/// root for free. +/// +/// Unconditional, unlike [`RootedOperands`]: the window is a user or native +/// call, so [`operand_protection`]'s "can this window collect?" test has +/// exactly one answer and there is nothing to gate on. +pub(crate) struct ImplicitThisSave { + slot: String, +} + +/// Bind `new_this` as the implicit `this` and root the value it displaced. +pub(crate) fn implicit_this_save(ctx: &mut FnCtx<'_>, new_this: &str) -> ImplicitThisSave { + let prev = ctx + .block() + .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, new_this)]); + let slot = temp_root_push_double(ctx, &prev); + ImplicitThisSave { slot } +} + +/// Restore the saved implicit `this`, re-read from its root. +/// +/// Reading the slot rather than the register is the fix, not a precaution: the +/// slot is a mutable root, so an evacuating cycle inside the dispatch rewrote +/// it and the register pushed beforehand names from-space. +/// +/// The truncate is emitted BEFORE the restore call so that nested saves — an +/// override arm inside an outer bind — release inner to outer. +/// `js_gc_temp_root_truncate` drops everything at or above its argument, so a +/// caller holding a LOWER group (`RootedOperands`) may release it afterwards +/// and drop this slot again harmlessly. +pub(crate) fn implicit_this_restore(ctx: &mut FnCtx<'_>, save: ImplicitThisSave) { + let prev = temp_root_get_double(ctx, &save.slot); + temp_root_truncate(ctx, &save.slot); + ctx.block() + .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &prev)]); +} + /// Push `value` onto the array held in temp-root slot `idx`, writing the /// possibly-reallocated array pointer back into the slot. pub(crate) fn temp_rooted_array_push(ctx: &mut FnCtx<'_>, idx: &str, value: &str) { diff --git a/crates/perry-codegen/src/lower_call/console_promise.rs b/crates/perry-codegen/src/lower_call/console_promise.rs index dba208d8ee..d2734e0a27 100644 --- a/crates/perry-codegen/src/lower_call/console_promise.rs +++ b/crates/perry-codegen/src/lower_call/console_promise.rs @@ -1413,18 +1413,57 @@ pub fn try_lower_closure_call_fallthrough( }; let recv_box = roots.reread_one(ctx, &operand_exprs, callee_slot)?; - let prev_this: Option = if let Some(ref this_val) = method_recv { - let blk = ctx.block(); - Some(blk.call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, this_val)])) + // #7211: the value `js_implicit_this_set` hands back is the PREVIOUS + // implicit `this`, read straight out of the `IMPLICIT_THIS` cell — which + // `object/this_binding.rs:176` registers as a scanned MUTABLE root the + // collector rewrites in place (`scan_implicit_this_roots_mut`). The swap + // has already overwritten the cell by the time we hold it, so this + // register is now the only copy this frame has, and it stays live across + // the allocating rebind unbox below AND the entire user-code dispatch. + // + // Two ways that hurts, and the second is the one that makes this worse + // than an ordinary stale read: + // + // * the enclosing frame still roots the same object (its own operand + // group, one temp-root frame down), so an evacuating minor inside the + // callee MOVES it and rewrites that root — leaving this register + // naming from-space. The restore then publishes a pre-move address + // back INTO a root the collector scans, so the corruption outlives the + // call that caused it and surfaces in whatever reads `this` next. + // * where no other root holds it, the object is simply collected. + // + // It WAS invisible to `scripts/gc_root_dominance_check.py` at both ends, + // which is how it survived #7206 and #7214: `js_implicit_this_set` was + // NONCOLLECTING but not in `ROOT_READ_CALLS`, so the register had no + // recognised heap-value source, and the restore is not a `RECEIVER_SINKS` + // fatal sink, so it would not have ranked even if it had. Being + // non-collecting is precisely what makes a call a root READ — the same + // rule `js_closure_get_capture_bits` is listed under — and it is now + // classified that way, so the checker reports any lowering that + // reintroduces this. + // + // Unconditional, unlike the operand groups above: the window is the user + // call itself, so `operand_protection`'s "can this window collect?" test + // has exactly one answer here and there is nothing to gate on. + // + // Six sibling lowerings emit the same pair; `implicit_this_save` / + // `implicit_this_restore` is the shared form, so a seventh cannot + // reintroduce this by copy-paste. + let prev_this_root = if let Some(ref this_val) = method_recv { + Some(crate::expr::temp_root::implicit_this_save(ctx, this_val)) } else if !matches!(callee, Expr::PropertyGet { .. }) { // Receiverless closure-value call (`fn()`, IIFE, `curry(1)(2)`): // OrdinaryCallBindThis binds `this` to undefined — without the // reset the enclosing method dispatch's IMPLICIT_THIS leaks into // the callee (#3576). Member-shaped callees keep their existing // receiver/skip behavior above. + // + // The value pushed here is the ENCLOSING method's receiver, not + // `undefined`: this arm is the one that runs for `helper()` called + // from inside `o.m()`, and dropping that object is #3576's leak with + // the sign flipped. let undef = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); - let blk = ctx.block(); - Some(blk.call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &undef)])) + Some(crate::expr::temp_root::implicit_this_save(ctx, &undef)) } else { None }; @@ -1518,13 +1557,26 @@ pub fn try_lower_closure_call_fallthrough( ) }; + // #7211: re-read the saved implicit `this` from its slot. Mandatory, not + // defensive, and for the same reason the operand re-reads above are: the + // temp-root slot is a MUTABLE root, so an evacuating cycle anywhere inside + // the dispatch rewrote the slot and left the register that was pushed + // naming from-space. Restoring the register instead of the slot is the + // whole bug. + // + // Ordered inner-to-outer: this slot was pushed above `roots`' first slot, + // so it is dropped first and `roots.release` then drops the group below + // it. `js_gc_temp_root_truncate` drops everything at or above its + // argument, so `roots.release` alone would in fact take this slot with it + // — but only when `roots` actually pushed one, and a receiverless call on + // inert arguments pushes nothing at all. Releasing this one explicitly is + // what makes the order correct in both shapes rather than in the common + // one. + if let Some(prev) = prev_this_root { + crate::expr::temp_root::implicit_this_restore(ctx, prev); + } // Released AFTER the dispatch, not before: the dispatcher allocates while // it reads these values. roots.release(ctx); - - if let Some(prev) = prev_this { - ctx.block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &prev)]); - } Ok(Some(result)) } diff --git a/crates/perry-codegen/src/lower_call/early_branches.rs b/crates/perry-codegen/src/lower_call/early_branches.rs index 34a57dfd7e..0682ec1ec9 100644 --- a/crates/perry-codegen/src/lower_call/early_branches.rs +++ b/crates/perry-codegen/src/lower_call/early_branches.rs @@ -401,12 +401,13 @@ pub fn try_lower_closure_typed_local_call( &format!("closure:{}", func_id), TypedFeedbackContract::closure_direct_call(), ); + // #7211: rooted save/restore. The displaced value is the + // enclosing method's receiver and it is live across the + // callee body; the restore below sits in the merge block, + // so the slot index crosses the diamond exactly as the + // bare register used to. let prev_this = if callee_reads_this { - Some(ctx.block().call( - DOUBLE, - "js_implicit_this_set", - &[(DOUBLE, &undef_this)], - )) + Some(crate::expr::temp_root::implicit_this_save(ctx, &undef_this)) } else { None }; @@ -928,11 +929,7 @@ pub fn try_lower_closure_typed_local_call( // body codegen never saw — reset `this` here (and only // here) when the static gating skipped the outer reset. let fallback_prev_this = if prev_this.is_none() { - Some(ctx.block().call( - DOUBLE, - "js_implicit_this_set", - &[(DOUBLE, &undef_this)], - )) + Some(crate::expr::temp_root::implicit_this_save(ctx, &undef_this)) } else { None }; @@ -943,10 +940,11 @@ pub fn try_lower_closure_typed_local_call( fallback_args.push((DOUBLE, v.as_str())); } let fallback_value = ctx.block().call(DOUBLE, &runtime_fn, &fallback_args); - if let Some(prev) = &fallback_prev_this { - let _ = ctx - .block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, prev)]); + // Inner save, released inside its own arm — so the outer + // slot (restored in the merge block) is still live and the + // temp-root depth matches on both paths into the merge. + if let Some(prev) = fallback_prev_this { + crate::expr::temp_root::implicit_this_restore(ctx, prev); } let after_fallback = ctx.block().label.clone(); if !ctx.block().is_terminated() { @@ -961,10 +959,8 @@ pub fn try_lower_closure_typed_local_call( (fallback_value.as_str(), after_fallback.as_str()), ], ); - if let Some(prev) = &prev_this { - let _ = ctx - .block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, prev)]); + if let Some(prev) = prev_this { + crate::expr::temp_root::implicit_this_restore(ctx, prev); } return Ok(Some(merged)); } @@ -972,18 +968,15 @@ pub fn try_lower_closure_typed_local_call( // Generic js_closure_callN dispatch (unknown func id, rest // params, or arity mismatch): the runtime-resolved callee may // read `this`, so the reset is unconditional here. - let prev_this = - ctx.block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &undef_this)]); + // #7211: rooted save/restore across the runtime-resolved callee. + let prev_this = crate::expr::temp_root::implicit_this_save(ctx, &undef_this); let runtime_fn = format!("js_closure_call{}", lowered_args.len()); let mut call_args: Vec<(crate::types::LlvmType, &str)> = vec![(I64, &closure_handle)]; for v in &lowered_args { call_args.push((DOUBLE, v.as_str())); } let result = ctx.block().call(DOUBLE, &runtime_fn, &call_args); - let _ = ctx - .block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &prev_this)]); + crate::expr::temp_root::implicit_this_restore(ctx, prev_this); return Ok(Some(result)); } } diff --git a/crates/perry-codegen/src/lower_call/func_ref.rs b/crates/perry-codegen/src/lower_call/func_ref.rs index f1f477345a..145fc99522 100644 --- a/crates/perry-codegen/src/lower_call/func_ref.rs +++ b/crates/perry-codegen/src/lower_call/func_ref.rs @@ -488,12 +488,11 @@ pub fn try_lower_func_ref_call( // are lowered BEFORE the reset: `this` inside an argument expression // still sees the enclosing binding. let resets_this = ctx.funcs_reading_dynamic_this.contains(fid); + // #7211: rooted save/restore. The value displaced here is the ENCLOSING + // method's receiver, held across the callee body — arbitrary user code. let prev_this = if resets_this { let undef = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); - Some( - ctx.block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &undef)]), - ) + Some(crate::expr::temp_root::implicit_this_save(ctx, &undef)) } else { None }; @@ -905,10 +904,8 @@ pub fn try_lower_func_ref_call( } else { ctx.block().call(DOUBLE, &fname, &arg_slices) }; - if let Some(prev) = &prev_this { - let _ = ctx - .block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, prev)]); + if let Some(prev) = prev_this { + crate::expr::temp_root::implicit_this_restore(ctx, prev); } if ctx.local_generator_funcs.contains(fid) { let wrap_ptr = format!("@__perry_wrap_{}", fname); diff --git a/crates/perry-codegen/src/lower_call/method_override.rs b/crates/perry-codegen/src/lower_call/method_override.rs index 39f93ba0d8..3e863e000d 100644 --- a/crates/perry-codegen/src/lower_call/method_override.rs +++ b/crates/perry-codegen/src/lower_call/method_override.rs @@ -119,9 +119,9 @@ pub(super) fn emit_own_method_override_check( } else { this_box.to_string() }; - let prev_this = ctx - .block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &recv_for_this)]); + // #7211: rooted save/restore — the displaced implicit `this` is live + // across `js_native_call_value`, which runs arbitrary user code. + let prev_this = crate::expr::temp_root::implicit_this_save(ctx, &recv_for_this); let v_override = ctx.block().call( DOUBLE, "js_native_call_value", @@ -131,8 +131,7 @@ pub(super) fn emit_own_method_override_check( (I64, &args_len), ], ); - ctx.block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &prev_this)]); + crate::expr::temp_root::implicit_this_restore(ctx, prev_this); let after_override = ctx.block().label.clone(); if !ctx.block().is_terminated() { ctx.block().br(&merge_label); diff --git a/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs b/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs index 35197f21d4..91ef851377 100644 --- a/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs +++ b/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs @@ -392,11 +392,9 @@ pub(crate) fn try_lower_instance_method_call( // garbage. Mirrors `lower_call.rs:2607` for the closure- // call fallthrough pattern (#519). let recv_for_this_probe = recv_box.clone(); - let prev_this_probe = ctx.block().call( - DOUBLE, - "js_implicit_this_set", - &[(DOUBLE, &recv_for_this_probe)], - ); + // #7211: rooted save/restore across the user-code dispatch. + let prev_this_probe = + crate::expr::temp_root::implicit_this_save(ctx, &recv_for_this_probe); let v_override_probe = ctx.block().call( DOUBLE, "js_native_call_value", @@ -406,11 +404,7 @@ pub(crate) fn try_lower_instance_method_call( (I64, &probe_args_len_str), ], ); - ctx.block().call( - DOUBLE, - "js_implicit_this_set", - &[(DOUBLE, &prev_this_probe)], - ); + crate::expr::temp_root::implicit_this_restore(ctx, prev_this_probe); let after_override_probe = ctx.block().label.clone(); if !ctx.block().is_terminated() { ctx.block().br(&probe_outer_merge_label); @@ -1372,9 +1366,8 @@ fn emit_collapsed_instance_dispatch( // Override arm: bind IMPLICIT_THIS to the receiver and call the stored // function value (#632 — a class-field non-arrow function reads `this`). ctx.current_block = override_idx; - let prev_this = ctx - .block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, recv_box)]); + // #7211: rooted save/restore across the user-code dispatch. + let prev_this = crate::expr::temp_root::implicit_this_save(ctx, recv_box); let v_override = ctx.block().call( DOUBLE, "js_native_call_value", @@ -1384,8 +1377,7 @@ fn emit_collapsed_instance_dispatch( (I64, &args_len), ], ); - ctx.block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &prev_this)]); + crate::expr::temp_root::implicit_this_restore(ctx, prev_this); let after_override = ctx.block().label.clone(); if !ctx.block().is_terminated() { ctx.block().br(&merge_label); diff --git a/crates/perry-codegen/src/lower_call/property_get/static_dispatch.rs b/crates/perry-codegen/src/lower_call/property_get/static_dispatch.rs index 021dee0d53..e40467ddfd 100644 --- a/crates/perry-codegen/src/lower_call/property_get/static_dispatch.rs +++ b/crates/perry-codegen/src/lower_call/property_get/static_dispatch.rs @@ -213,9 +213,9 @@ pub(crate) fn try_lower_static_dispatch( lowered.push(undef.clone()); } } - let prev_this = - ctx.block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &recv_box)]); + // #7211: rooted save/restore — the displaced implicit `this` is + // live across the static method body below, which is user code. + let prev_this = crate::expr::temp_root::implicit_this_save(ctx, &recv_box); // Receiver-sensitive static `this`: arm the one-shot override with // the ACTUAL receiver box so the callee prologue's // `js_static_this_resolve` binds `this` to it (spec @@ -240,8 +240,7 @@ pub(crate) fn try_lower_static_dispatch( let arg_slices: Vec<(crate::types::LlvmType, &str)> = lowered.iter().map(|s| (DOUBLE, s.as_str())).collect(); let result = ctx.block().call(DOUBLE, &fn_name, &arg_slices); - ctx.block() - .call(DOUBLE, "js_implicit_this_set", &[(DOUBLE, &prev_this)]); + crate::expr::temp_root::implicit_this_restore(ctx, prev_this); return Ok(Some(result)); } // #1787 / #321: the call target is a static FIELD holding a callable, diff --git a/crates/perry-runtime/src/builtins/arithmetic.rs b/crates/perry-runtime/src/builtins/arithmetic.rs index 1e773466c5..cda3970325 100644 --- a/crates/perry-runtime/src/builtins/arithmetic.rs +++ b/crates/perry-runtime/src/builtins/arithmetic.rs @@ -495,43 +495,126 @@ pub extern "C" fn js_ge(a: JSValue, b: JSValue) -> JSValue { JSValue::from_bits(js_rel_ge(f64::from_bits(a.bits()), f64::from_bits(b.bits())).to_bits()) } +// `typeof` returns one of eight strings, so each is allocated once and cached +// rather than rebuilt per call. +// +// #7211: these cells are GC ROOTS and are registered as such +// (`scan_typeof_string_roots_mut`, wired in `gc/mod.rs`). They are at module +// scope rather than inside `js_value_typeof` for exactly that reason — a +// scanner has to be able to reach them. +// +// Before that registration this cache was a deterministic use-after-free, and +// it is the bug that killed `sfw-registry --help` 10/10 under a +// `PERRY_GC_MOVING_LOOP_POLLS=1` build. `js_string_from_bytes` allocates in the +// NURSERY; nothing else references the result; so the first minor either swept +// it or evacuated it, and this raw pointer named the abandoned bytes from then +// on. Every later `typeof x === "string"` handed `js_string_equals` a from-space +// address. `PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800` +// reported it precisely: `obj_type=3 size=40 retired_by_minor=#0` — a string, +// 32-byte header plus `"string"`, retired by the very first collection. +// +// `retired_by_minor=#0` is the tell for this whole shape, and it is worth +// recognising: an ordinary #7154-class stale register goes bad at whichever +// collection lands inside a few-instruction window, so it is timing-dependent. +// A cache that is never rooted goes bad at the FIRST collection and stays bad, +// which is why this reproduced 10/10 while the register bugs needed a zod +// workload and ten rounds. +// +// It is invisible to `scripts/gc_root_dominance_check.py` by construction: that +// tool reads emitted LLVM IR, and this is a runtime-side table. The static +// checker could never have found it, which is why the runtime instruments had +// to be pointed at the registry first. +thread_local! { + static TYPEOF_UNDEFINED: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; + static TYPEOF_OBJECT: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; + static TYPEOF_BOOLEAN: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; + static TYPEOF_NUMBER: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; + static TYPEOF_STRING: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; + static TYPEOF_FUNCTION: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; + static TYPEOF_BIGINT: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; + static TYPEOF_SYMBOL: std::cell::Cell<*mut StringHeader> = const { std::cell::Cell::new(std::ptr::null_mut()) }; +} + +/// Get or initialize a cached `typeof` string. +fn get_cached( + cache: &'static std::thread::LocalKey>, + s: &str, +) -> *mut StringHeader { + cache.with(|cell| { + let ptr = cell.get(); + if !ptr.is_null() { + return ptr; + } + let new_ptr = crate::string::js_string_from_bytes(s.as_ptr(), s.len() as u32); + cell.set(new_ptr); + new_ptr + }) +} + +/// GC mutable-root scanner for the eight cached `typeof` strings (#7211). +/// +/// Marks them, so an unreferenced cache entry is never swept, AND rewrites +/// them, so an evacuating minor that relocates one leaves the cell naming the +/// new address instead of from-space. Both halves matter: marking alone would +/// still hand out a pre-move pointer after a copying minor, which is the +/// distinction `gc-rooting-invariant.md` keeps having to make. +/// +/// `STRING_TAG` rather than the default `POINTER_TAG` because these are +/// `StringHeader`s, matching `json::scan_parse_roots_mut`'s interned-key +/// treatment. +pub fn scan_typeof_string_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + fn visit( + cache: &'static std::thread::LocalKey>, + visitor: &mut crate::gc::RuntimeRootVisitor<'_>, + ) { + cache.with(|cell| { + let mut ptr = cell.get() as *const StringHeader; + if ptr.is_null() { + return; + } + if visitor.visit_tagged_raw_const_ptr_slot(&mut ptr, crate::value::STRING_TAG) { + cell.set(ptr as *mut StringHeader); + } + }); + } + visit(&TYPEOF_UNDEFINED, visitor); + visit(&TYPEOF_OBJECT, visitor); + visit(&TYPEOF_BOOLEAN, visitor); + visit(&TYPEOF_NUMBER, visitor); + visit(&TYPEOF_STRING, visitor); + visit(&TYPEOF_FUNCTION, visitor); + visit(&TYPEOF_BIGINT, visitor); + visit(&TYPEOF_SYMBOL, visitor); +} + +/// Drop every cached `typeof` string. Test-only: the unit-test harness resets +/// arenas between tests while thread-locals persist, so a cache entry from a +/// previous test names memory the new arena does not own. +#[cfg(test)] +pub(crate) fn reset_typeof_string_cache_for_test() { + for cache in [ + &TYPEOF_UNDEFINED, + &TYPEOF_OBJECT, + &TYPEOF_BOOLEAN, + &TYPEOF_NUMBER, + &TYPEOF_STRING, + &TYPEOF_FUNCTION, + &TYPEOF_BIGINT, + &TYPEOF_SYMBOL, + ] { + cache.with(|cell| cell.set(std::ptr::null_mut())); + } +} + /// Return the typeof a value as a string /// Takes an f64 that uses NaN-boxing to distinguish types. /// Returns a pointer to a string: "undefined", "boolean", "number", "string", "object", "function" /// -/// Optimization: typeof only returns 7 possible strings, so we cache them as +/// Optimization: typeof only returns 8 possible strings, so we cache them as /// pre-allocated StringHeader pointers to avoid heap allocation on every call. +/// The cache is a registered GC root — see the `thread_local!` above. #[no_mangle] pub extern "C" fn js_value_typeof(value: f64) -> *mut StringHeader { - use std::cell::Cell; - - thread_local! { - static TYPEOF_UNDEFINED: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - static TYPEOF_OBJECT: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - static TYPEOF_BOOLEAN: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - static TYPEOF_NUMBER: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - static TYPEOF_STRING: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - static TYPEOF_FUNCTION: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - static TYPEOF_BIGINT: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - static TYPEOF_SYMBOL: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - } - - /// Get or initialize a cached typeof string. - fn get_cached( - cache: &'static std::thread::LocalKey>, - s: &str, - ) -> *mut StringHeader { - cache.with(|cell| { - let ptr = cell.get(); - if !ptr.is_null() { - return ptr; - } - let new_ptr = crate::string::js_string_from_bytes(s.as_ptr(), s.len() as u32); - cell.set(new_ptr); - new_ptr - }) - } - let jsval = JSValue::from_bits(value.to_bits()); if jsval.is_undefined() { diff --git a/crates/perry-runtime/src/builtins/mod.rs b/crates/perry-runtime/src/builtins/mod.rs index bba2da3eb0..eaa13d8023 100644 --- a/crates/perry-runtime/src/builtins/mod.rs +++ b/crates/perry-runtime/src/builtins/mod.rs @@ -38,7 +38,10 @@ pub(crate) use println; pub(crate) use crate::string::{js_string_from_bytes, js_string_from_wtf8_bytes, StringHeader}; pub(crate) use crate::JSValue; -mod arithmetic; +// `pub(crate)` so `gc::mod` can register `scan_typeof_string_roots_mut` +// (#7211): the interned `typeof` strings are GC roots and the scanner has to +// be nameable from the registration list. +pub(crate) mod arithmetic; mod console; mod formatting; mod globals; diff --git a/crates/perry-runtime/src/gc/mod.rs b/crates/perry-runtime/src/gc/mod.rs index ba74e47e67..dbea304905 100644 --- a/crates/perry-runtime/src/gc/mod.rs +++ b/crates/perry-runtime/src/gc/mod.rs @@ -485,6 +485,18 @@ pub fn gc_init() { gc_register_mutable_root_scanner(async_hooks_mutable_root_scanner); gc_register_mutable_root_scanner(shape_cache_mutable_root_scanner); gc_register_mutable_root_scanner(crate::regex::scan_last_exec_groups_root_mut); + // #7211: the eight interned `typeof` result strings, and JSON.rawJSON's + // interned `"rawJSON"` key. Both are thread-local caches of a RAW + // `StringHeader*` allocated in the nursery and referenced by nothing else, + // so before this registration the FIRST minor collection sweeps or + // evacuates them and the cached pointer names abandoned memory forever + // after. Not a timing-dependent stale register: a permanently wrong cache, + // which is why `sfw-registry --help` under a + // `PERRY_GC_MOVING_LOOP_POLLS=1` build failed 10/10 rather than + // intermittently, and why the from-space reporter blamed + // `retired_by_minor=#0`. + gc_register_mutable_root_scanner(crate::builtins::arithmetic::scan_typeof_string_roots_mut); + gc_register_mutable_root_scanner(crate::json::raw_json::scan_raw_json_key_root_mut); gc_register_mutable_root_scanner(crate::object::scan_exotic_expando_roots_mut); gc_register_mutable_root_scanner(crate::array::scan_template_raw_roots_mut); // #6981: the memoized `Array.prototype` / `Object.prototype` addresses in diff --git a/crates/perry-runtime/src/json/mod.rs b/crates/perry-runtime/src/json/mod.rs index 62ebb33945..66cecfd598 100644 --- a/crates/perry-runtime/src/json/mod.rs +++ b/crates/perry-runtime/src/json/mod.rs @@ -23,7 +23,9 @@ use std::cell::RefCell; mod parse_api; mod parser; -mod raw_json; +// `pub(crate)` so `gc::mod` can register `scan_raw_json_key_root_mut` (#7211): +// the interned `"rawJSON"` key is a GC root. +pub(crate) mod raw_json; mod replacer; mod reviver; mod simd; diff --git a/crates/perry-runtime/src/json/raw_json.rs b/crates/perry-runtime/src/json/raw_json.rs index 4c52e8c0ea..55bc9fda33 100644 --- a/crates/perry-runtime/src/json/raw_json.rs +++ b/crates/perry-runtime/src/json/raw_json.rs @@ -78,13 +78,36 @@ pub(crate) unsafe fn ptr_is_raw_json_wrapper(ptr: *const u8) -> bool { && (*obj).class_id == RAW_JSON_CLASS_ID } +// #7211: same defect as the `typeof` string cache, found by grepping for the +// shape once that one was diagnosed — a thread-local holding a raw nursery +// `StringHeader*` with nothing registering it as a root. Allocated once, +// referenced by nothing else, so the first minor sweeps or evacuates it and +// every later `JSON.rawJSON(...)` writes its own property under a from-space +// key. Hoisted out of the function body so `scan_raw_json_key_root_mut` can +// reach it. +thread_local! { + static RAW_JSON_KEY: std::cell::Cell<*mut StringHeader> = + const { std::cell::Cell::new(std::ptr::null_mut()) }; +} + +/// GC mutable-root scanner for the cached `"rawJSON"` key (#7211). Marks it so +/// it is not swept and rewrites it so an evacuating minor cannot leave the cell +/// naming from-space. +pub fn scan_raw_json_key_root_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + RAW_JSON_KEY.with(|cell| { + let mut ptr = cell.get() as *const StringHeader; + if ptr.is_null() { + return; + } + if visitor.visit_tagged_raw_const_ptr_slot(&mut ptr, crate::value::STRING_TAG) { + cell.set(ptr as *mut StringHeader); + } + }); +} + /// Cached `"rawJSON"` key string used for the wrapper's own property. fn raw_json_key() -> *const StringHeader { - use std::cell::Cell; - thread_local! { - static KEY: Cell<*mut StringHeader> = const { Cell::new(std::ptr::null_mut()) }; - } - KEY.with(|c| { + RAW_JSON_KEY.with(|c| { let p = c.get(); if !p.is_null() { return p as *const StringHeader; diff --git a/crates/perry-runtime/src/string/alloc.rs b/crates/perry-runtime/src/string/alloc.rs index eac25168c5..9cfc5097e9 100644 --- a/crates/perry-runtime/src/string/alloc.rs +++ b/crates/perry-runtime/src/string/alloc.rs @@ -18,6 +18,37 @@ pub extern "C" fn js_string_from_bytes(data: *const u8, len: u32) -> *mut String /// materialized value, so use sparingly — only as a last-resort /// compatibility shim on paths that truly need the heap /// representation. +/// +/// # The SSO branch allocates, and its callers hold raw pointers (#7213) +/// +/// This branch is a collection point, and `js_get_string_pointer_unified` — +/// its main caller — hands generated code a **raw** `*mut StringHeader` that +/// no root describes. Every `unbox_str_handle` site in +/// `perry-codegen/src/expr/compare.rs`, `lower_string_method.rs` and +/// `lower_array_method.rs` lowers its operands first and then unboxes them +/// back-to-back before one consuming call: +/// +/// ```text +/// %l = call i64 @js_get_string_pointer_unified(double %lbox) +/// %r = call i64 @js_get_string_pointer_unified(double %rbox) +/// call i32 @js_string_equals(i64 %l, i64 %r) +/// ``` +/// +/// `%l` is live across the second unbox. **It is not exploitable today**, and +/// the reason is worth writing down rather than rediscovering: an allocation +/// here reaches the alloc-point arm of `gc_check_trigger`, which takes +/// `ManualGcScanGuard::force_full_scan`, and a forced conservative stack scan +/// makes the copying minor ineligible (`CopiedMinorFallbackReason:: +/// ConservativeStack`). So the collection this allocation can cause never +/// MOVES anything, and the same conservative scan finds `%l` on the stack and +/// keeps it alive. Both halves of the hazard are closed by accident. +/// +/// By accident is the operative phrase — it rests on the alloc-point arm +/// staying non-moving, which is exactly the property the moving-GC work keeps +/// eroding. Tracked as #7213 rather than pre-emptively fixed here: a +/// `GcSuppressScope` around this allocation makes the window sound and costs +/// nothing measurable, but shipping a GC-trigger change with no test that can +/// fail without it is the thing CLAUDE.md's knob-kill policy exists to stop. #[no_mangle] pub extern "C" fn js_string_materialize_to_heap(value: f64) -> *mut StringHeader { let bits = value.to_bits(); diff --git a/scripts/gc_root_dominance_allowlist.json b/scripts/gc_root_dominance_allowlist.json index 936b642598..e14e9b36f7 100644 --- a/scripts/gc_root_dominance_allowlist.json +++ b/scripts/gc_root_dominance_allowlist.json @@ -20,32 +20,19 @@ "new violation was introduced.", "", "Fingerprint format: .ll::::->", - "Get it from the checker's own output (`-v` prints one per violation)." + "Get it from the checker's own output (`-v` prints one per violation).", + "", + "EMPTY IS THE GOAL AND IT IS CURRENTLY MET. The four #7211 entries that", + "lived here -- ClassExprFresh holding its fresh class object across the", + "js_object_set_field_by_name calls that install its own statics -- were", + "deleted when that predicate was fixed. That is rule 1 working as designed:", + "the fix made every one of them match nothing, the gate went red, and the", + "red is what says 'now delete the entries'.", + "", + "An empty list is not a disarmed gate. Rule 3 still fails any violation,", + "the --min-files / --min-binds / --min-funcs floors still refuse a corpus", + "that did not actually exercise the subject, and --self-test still proves", + "the checker reports a planted violation and clears the matching control." ], - "entries": [ - { - "fingerprint": "test_gap_class_expr_identity__test_gap_class_expr_identity_ts.ll::main::js_object_alloc->js_object_set_field_by_name", - "issue": "#7211", - "count": 2, - "justification": "Expr::ClassExprFresh (expr/static_field_meta.rs:432) holds the fresh class object in an SSA register across the js_object_set_field_by_name calls that install its named statics. Its protect_handle predicate only asks whether the AUTHOR'S initializer expressions can collect, never whether the lowering's own emitted field-store can -- so a class expression with inert statics gets no temp root. Two evaluations in this module, hence count 2. Pre-existing on main and reported red by this same gate since #7198; not fixed here because crates/perry-codegen/src/expr/ is being actively edited under #7206." - }, - { - "fingerprint": "test_gap_class_expr_instance_fields__test_gap_class_expr_instance_fields_ts.ll::main::js_object_alloc->js_object_set_field_by_name", - "issue": "#7211", - "count": 1, - "justification": "Same ClassExprFresh predicate gap as the entry above, reached through a class expression carrying instance fields. Tracked as one fix in #7211; listed separately because the fingerprint is per-module and a per-module entry is what keeps a NEW violation in this module from being absorbed." - }, - { - "fingerprint": "test_gap_class_expr_new_instanceof__test_gap_class_expr_new_instanceof_ts.ll::main::js_object_alloc->js_object_set_field_by_name", - "issue": "#7211", - "count": 1, - "justification": "Same ClassExprFresh predicate gap as the entries above, reached via `new`/`instanceof` on a class-expression value. Note that js_object_mark_class does put this object in CLASS_OBJECT_VALUES, which is scanned and forwarded -- that keeps the OBJECT alive but does not rewrite the register, so the violation is real rather than a false positive." - }, - { - "fingerprint": "test_gap_class_expr_static_this__test_gap_class_expr_static_this_ts.ll::main::js_object_alloc->js_object_set_field_by_name", - "issue": "#7211", - "count": 1, - "justification": "Same ClassExprFresh predicate gap as the entries above, reached through a static method that reads `this`. Same fix in #7211 retires all four entries at once; the checker will then fail on the stale entries, which is the intended prompt to delete them." - } - ] + "entries": [] } diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index a6214460d4..be88869222 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -272,6 +272,10 @@ def build_cfg(f): # object/this_binding.rs:160 -- a thread-local cell swap "js_implicit_this_set", "js_implicit_this_get", "js_gc_note_slot_layout", "js_string_addref_if_heap_string", + # `js_get_string_pointer_unified` is deliberately NOT here. Its SSO branch + # calls `js_string_materialize_to_heap`, which allocates (value/nanbox.rs:268), + # so it is a collection point by this file's one-sided rule even though the + # collection it can cause is currently always non-moving. See #7213. } # The single site where an evacuating (moving) minor runs. @@ -1166,6 +1170,22 @@ def seeded_violation_test(paths, moving_only, anchor, want_sites, verbose=False) "js_box_get_bits", # box.rs mutable-capture cell read "js_implicit_this_get", # object/this_binding.rs:160 thread-local "js_new_target_get", + # object/this_binding.rs:159 -- `js_implicit_this_SET` is a swap, so its + # RETURN value is a read of the same scanned mutable cell + # (`scan_implicit_this_roots_mut`, this_binding.rs:176) and the swap has + # already overwritten the only other copy. Listing the setter as a reader + # looks odd, which is exactly why #7214 left `prev_this` unrooted for a + # whole PR: the checker saw a call it knew could not collect, never + # classified the result as a heap value, and reported nothing at either + # end. Being non-collecting is what makes a call a root READ. + "js_implicit_this_set", + # `js_get_string_pointer_unified` is a candidate and is deliberately left + # out for now: its result IS a raw heap address in a bare register, but it + # is not in NONCOLLECTING (its SSO branch allocates), so classifying it as + # a source would report the whole `unbox_str_handle` family in one go -- + # roughly forty sites across lower_string_method.rs alone. That is a real + # population and it needs its own measured count and its own triage rather + # than being folded into this change. #7213. } # Argument positions that make a stale pointer FATAL rather than merely wrong: @@ -1646,6 +1666,27 @@ def _stale_probe(path, max_stale): return (int(m.group(1)) if m else -1), rc +def _main_probe(argv): + """Exit status of a full `main()` run over `argv` (no `sys.exit`). + + The guards these arms cover live in `main()`'s argument handling rather + than in a scannable function, and argparse reports a usage error by + raising `SystemExit(2)`. Driving `main()` is the only way to assert them; + calling the inner helpers would skip exactly the code under test. + """ + saved = sys.argv + buf = io.StringIO() + try: + sys.argv = ["gc_root_dominance_check.py"] + list(argv) + with contextlib.redirect_stdout(buf), contextlib.redirect_stderr(buf): + try: + return main() + except SystemExit as exc: + return exc.code if isinstance(exc.code, int) else 1 + finally: + sys.argv = saved + + def self_test(): """Assert the checker reports the planted violation and clears the control. @@ -1716,6 +1757,41 @@ def self_test(): "report 0 uses and exit 0", file=sys.stderr) ok = False + # --- the stale mode's own subject-liveness assertion ---------------- + # + # `--stale-registers` classifies a shadow-slot load as a heap-value + # source by looking the alloca up in a map built from + # `js_shadow_slot_bind`. A corpus with no binds therefore has almost no + # sources, reports `total 0`, and is indistinguishable from a corpus + # with no stale registers. `--min-binds` is what makes that case an + # error, and before #7211 the stale path returned above the guard. + # Both directions, so the guard cannot be "always 2". + if _main_probe(["--stale-registers", "--min-binds", "50", planted]) != 2: + print("self-test FAIL: --stale-registers over a corpus with fewer " + "than --min-binds root stores must exit 2. The scan's " + "shadow-slot sources come from those stores, so a clean " + "verdict over zero of them proves nothing.", file=sys.stderr) + ok = False + if _main_probe(["--stale-registers", "--min-binds", "2", planted]) != 0: + print("self-test FAIL: --stale-registers over a corpus that MEETS " + "--min-binds must still exit 0 (diagnostic). The guard " + "rejects every corpus, which is a gate that always fails.", + file=sys.stderr) + ok = False + # A knob that is silently ignored is a disarmed knob -- the same rule + # `--max-stale` and `--fatal-sinks` already carry, read the other way. + if _main_probe(["--stale-registers", "--any-def", planted]) != 2: + print("self-test FAIL: --any-def with --stale-registers must be a " + "usage error; the stale scan never consults the anchor, so " + "accepting it promises a widening that never happens", + file=sys.stderr) + ok = False + if _main_probe(["--any-def", planted]) != 1: + print("self-test FAIL: --any-def WITHOUT --stale-registers must " + "still run the bind-anchored check and report the planted " + "violations", file=sys.stderr) + ok = False + try: _scan([broken], False, "alloc") except MalformedIR: @@ -1948,6 +2024,15 @@ def main(): ap.error("--max-stale requires --stale-registers") if ns.fatal_sinks and not ns.stale_registers: ap.error("--fatal-sinks requires --stale-registers") + # Same rule read the other way. `--any-def` only selects the bind-anchored + # check's ANCHOR (`anchor = "any" if ns.any_def else "alloc"`, below), and + # the stale-register path never consults it -- it anchors on every + # heap-value source by construction, so there is no narrower or wider + # setting for it to pick. Passing both reads like "widen the stale scan" + # and does nothing at all. + if ns.any_def and ns.stale_registers: + ap.error("--any-def has no effect with --stale-registers " + "(the stale scan already anchors on every heap-value source)") if ns.self_test: return self_test() @@ -1990,9 +2075,6 @@ def main(): parsed.append((os.path.basename(p), parse_file(p))) poll_reaching, _known = compute_poll_reaching( [f for _m, fs in parsed for f in fs]) - if ns.stale_registers: - return run_stale(parsed, poll_reaching, verbose, moving_only, - ns.fatal_sinks, ns.max_stale) n_binds = sum( 1 for _m, fs in parsed @@ -2002,6 +2084,26 @@ def main(): if BIND_RE.search(ins.text) ) + if ns.stale_registers: + # `--min-binds` is a CORPUS-sanity assertion, not a bind-anchored-check + # detail, so it has to be honoured here too. `heap_source_kind` decides + # a `slotload` is a heap-value source by looking the pointer up in + # `slot_of_alloca`, and that map is built from the same `BIND_RE` + # (`stale_uses_in_function`). Compile the corpus with + # PERRY_INLINE_SHADOW_SLOT=1 (or with a broken `--trace llvm`) and + # every shadow-slot source vanishes: `run_stale` reports `total 0`, + # exits 0, and looks exactly like a corpus with no stale registers in + # it. That is hazard 4 -- the gate runs but its subject never did. + if n_binds < ns.min_binds: + print(f"error: {n_binds} root store(s) in the corpus, need at " + f"least {ns.min_binds}. The stale-register scan derives its " + "shadow-slot sources from those stores, so a clean verdict " + "here means the IR was not the IR you think it is " + "(compile with PERRY_INLINE_SHADOW_SLOT=0).", file=sys.stderr) + return 2 + return run_stale(parsed, poll_reaching, verbose, moving_only, + ns.fatal_sinks, ns.max_stale) + if ns.unrooted_allocas: total = 0 moving_total = 0 diff --git a/test-files/test_gap_gc_closure_call_prev_this_rooting.ts b/test-files/test_gap_gc_closure_call_prev_this_rooting.ts new file mode 100644 index 0000000000..0d6c9978ac --- /dev/null +++ b/test-files/test_gap_gc_closure_call_prev_this_rooting.ts @@ -0,0 +1,66 @@ +// #7211: the PREVIOUS implicit `this` that `js_closure_callN` saves across a +// dynamic call must be rooted, because the restore publishes it back into a +// root the collector scans. +// +// `js_implicit_this_set(v)` swaps the `IMPLICIT_THIS` cell and returns what was +// there. That cell is a registered MUTABLE root — `scan_implicit_this_roots_mut` +// (object/this_binding.rs:176) marks it and rewrites it on an evacuating cycle. +// The swap has already overwritten it, so the returned value is now held only +// in a bare SSA register, and it stays there across the allocating rebind unbox +// AND the entire user call. A minor inside the callee moves the object, rewrites +// the enclosing frame's copy, and leaves this register naming from-space. The +// restore then writes that pre-move address BACK INTO the cell — so the damage +// outlives the call and lands on whatever reads `this` next. +// +// #7214 fixed the callee, the receiver and every argument of this same lowering +// and deliberately left this one, measured but unfixed. It was invisible to +// scripts/gc_root_dominance_check.py at both ends: `js_implicit_this_set` was +// NONCOLLECTING but not a root READ, so the register had no recognised +// heap-value source, and the restore is not a fatal sink. Both ends are now +// classified, so this shape stays gated as well as fixed. +// +// LIVE BY CONSTRUCTION. `outer` is a non-arrow function assigned onto an object +// literal, so `obj.outer(1)` takes the generic `js_closure_callN` fallthrough +// and binds `obj` as the implicit `this`. `h(v)` inside it is a RECEIVERLESS +// dynamic call, which per #3576 must reset `this` to undefined and restore it — +// that restore is the subject. `this.tag` is then read AFTER the restore, so a +// corrupted cell is observable rather than merely present. + +function churn(x: number): number { + const bits: any[] = []; + for (let i = 0; i < 600; i++) { + bits.push({ i: i, s: "x" }); + } + return x + bits.length - 600; +} + +function helper(v: number): number { + return churn(v); +} + +function outer(this: any, v: number): number { + const before = this.tag; + // Receiverless dynamic call: saves the current implicit `this` (= this + // object), binds undefined, and restores the saved value afterwards. + const h: any = helper; + const got = h(v); + // Read `this` again BELOW the restore. If the restore republished a + // pre-move address, this reads out of abandoned memory — a wrong number if + // the bytes were recycled, a SIGSEGV if the page was retired. + const after = this.tag; + return before === after ? after + got - v : -1; +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 400; r++) { + const obj: any = { tag: r, outer: outer }; + const got = obj.outer(1); + if (got !== r) { + bad++; + } + } + return bad; +} + +console.log("bad", run()); diff --git a/test-files/test_gap_gc_typeof_string_cache_rooting.ts b/test-files/test_gap_gc_typeof_string_cache_rooting.ts new file mode 100644 index 0000000000..75d3275f5d --- /dev/null +++ b/test-files/test_gap_gc_typeof_string_cache_rooting.ts @@ -0,0 +1,69 @@ +// #7211: the interned `typeof` result strings are GC roots and must be +// registered as such. +// +// `js_value_typeof` (builtins/arithmetic.rs) returns one of eight strings and +// caches each in a thread-local `Cell<*mut StringHeader>` so it is built once +// rather than per call. Those cells held a RAW pointer into the NURSERY and +// nothing else referenced the string, so the first minor collection either +// swept it or evacuated it — and the cache kept naming the abandoned bytes for +// the rest of the process. Every later `typeof x === "…"` then handed +// `js_string_equals` a from-space address. +// +// This is the bug that kept `sfw-registry --help` red under a genuine +// `PERRY_GC_MOVING_LOOP_POLLS=1` build after #7206 and #7214 had closed every +// stale register they could find. It is worth naming the difference, because it +// is why the previous rounds could not find it: +// +// * a #7154-class stale REGISTER goes bad only if a collection happens to +// land inside a few-instruction window, so it is timing-dependent and needs +// a workload plus repetition to surface; +// * an unregistered CACHE goes bad at the first collection and stays bad, so +// it fails 10/10 — and it is invisible to +// `scripts/gc_root_dominance_check.py`, which reads emitted LLVM IR and +// cannot see a runtime-side table at all. +// +// The from-space reporter named it exactly: `obj_type=3 size=40 +// retired_by_minor=#0` — a string, a 32-byte header plus `"string"`, retired by +// the very first collection. +// +// LIVE BY CONSTRUCTION. Every `typeof` here is applied to a value read out of an +// array at a runtime index, so none of it folds at compile time and each call +// really does reach `js_value_typeof` and really does read the cache. The churn +// forces minor collections between the first population of the cache and the +// later reads, which is the whole subject: the FIRST iteration primes the cache +// and the ones after it are the test. + +function churn(x: number): number { + const bits: any[] = []; + for (let i = 0; i < 400; i++) { + bits.push({ i: i, s: "x" }); + } + return x + bits.length - 400; +} + +function run(): number { + let bad = 0; + const vals: any[] = ["a", 1, true, {}, undefined, run]; + const want: string[] = [ + "string", + "number", + "boolean", + "object", + "undefined", + "function", + ]; + for (let r = 0; r < 500; r++) { + churn(r); + for (let k = 0; k < 6; k++) { + // Reads the cached string and compares it against a literal. A cache + // entry the collector moved but never rewrote makes this compare read + // from-space. + if (typeof vals[k] !== want[k]) { + bad++; + } + } + } + return bad; +} + +console.log("bad", run()); From 28dc70d27b00e43aed35241daadc38783fe0943b Mon Sep 17 00:00:00 2001 From: jdalton Date: Sat, 1 Aug 2026 23:39:10 -0400 Subject: [PATCH 2/3] fix(gc): root the regexp receiver across ToString, and audit ALLOC_RE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Expr::RegExpTest` / `Expr::RegExpExec` unboxed the receiver to a raw `RegExpHeader*` before emitting `js_jsvalue_to_string_coerce`, which allocates and dispatches a user `toString`. Under a moving minor the register named from-space by the time `js_regexp_test` dereferenced it — #7154's residual, at `defineApiCall + 404` in the registry. Both take the established `guard_store_operand_across` / `reread_store_operand` pair and the unbox moves below the coerce. The checker missed it because `ALLOC_RE` spelled the allocator `regexp_alloc\w*` and the call is `js_regexp_new`. Reconciling every alternative against the runtime's real symbol table found four that match nothing at all (`regexp_alloc`, `promise_alloc`, `bigint_alloc`, `typed_array_alloc`): the runtime materializes fresh GC objects under three naming conventions (`_alloc*`, `_new*`, `_create*`) and the pattern modelled one. All three are now matched as conventions, with the non-conforming constructors enumerated explicitly. Second blind spot, and the one that matters for CI: the ToPrimitive family was not `POLL_CAPABLE_RUNTIME`, so even with `ALLOC_RE` widened the site was invisible to `--moving-only`, which is the mode the gate runs. Refs #7154, #7226, #7211, #7161 --- ...exp-receiver-rooting-and-alloc-re-audit.md | 201 ++++++++++++++++++ .../perry-codegen/src/expr/instance_misc1.rs | 46 +++- scripts/gc_root_dominance_check.py | 115 +++++++++- .../test_gap_gc_regexp_receiver_rooting.ts | 80 +++++++ 4 files changed, 429 insertions(+), 13 deletions(-) create mode 100644 changelog.d/7227-regexp-receiver-rooting-and-alloc-re-audit.md create mode 100644 test-files/test_gap_gc_regexp_receiver_rooting.ts diff --git a/changelog.d/7227-regexp-receiver-rooting-and-alloc-re-audit.md b/changelog.d/7227-regexp-receiver-rooting-and-alloc-re-audit.md new file mode 100644 index 0000000000..2a66dd37ce --- /dev/null +++ b/changelog.d/7227-regexp-receiver-rooting-and-alloc-re-audit.md @@ -0,0 +1,201 @@ +### Fixed + +- **`codegen`: the receiver of `re.test(s)` / `re.exec(s)` is rooted across the + `ToString(s)` coercion the lowering emits below it** (#7154). Both + `Expr::RegExpTest` and `Expr::RegExpExec` lowered the receiver first, unboxed + it to a raw `RegExpHeader*` in a bare SSA register, and only then emitted + `js_jsvalue_to_string_coerce`. That coerce is not a bystander: it allocates, + and on an object argument it dispatches a user `[Symbol.toPrimitive]` / + `toString` / `valueOf`, which is arbitrary JS with its own loop back-edge + polls. Under `PERRY_GC_MOVING_LOOP_POLLS=1` one of those polls runs an + evacuating minor while the regexp is live only in that register. + + This is the residual #7226 measured and named rather than fixed. In the + `sfw-registry` reproducer it is `src/lib/api/shared.ts:67`, + `/\[[a-zA-Z]+\]/.test(url)`, faulting at + `perry_fn_src_lib_api_shared_ts__defineApiCall + 404`: + + ```asm + bl js_regexp_new ; ALLOCATES + and x20, x0, #0xffffffffffff ; raw regexp pointer -> bare register + ldr d0, [sp, #0x28] + bl js_jsvalue_to_string_coerce ; ALLOCATES, runs user toString + mov x0, x20 ; STALE + bl js_regexp_test ; faults here + ``` + + The receiver now takes the established `guard_store_operand_across` / + `reread_store_operand` pair, and the unbox moves BELOW the coerce — unboxing + above it is what parked the pre-move address in a register in the first + place. `RegExpExec` had the identical defect and is fixed with it. + +### Changed + +- **`scripts/gc_root_dominance_check.py`: `ALLOC_RE` audited against the + runtime's real symbol table instead of an assumed naming convention.** This + is the more valuable half of the change, because the miss above was the + *second* allocator to escape this pattern (`js_implicit_this_set` was the + first, #7226) and each one has cost a full investigation round. + + The old pattern carried an alternative spelled `regexp_alloc\w*`. **No such + symbol has ever existed.** It was not a typo — it was an extrapolation: + whoever wrote it knew a RegExp allocates and inferred the `_alloc` suffix + from its neighbours. Reconciling every alternative against + `extern "C" fn js_\w+` over perry-runtime + perry-stdlib, intersected with + the names perry-codegen actually declares, found **four alternatives matching + nothing at all** — `regexp_alloc\w*`, `promise_alloc\w*`, `bigint_alloc\w*` + and `typed_array_alloc\w*`. A quarter of the pattern was decorative. + + The root cause is that the runtime materializes fresh GC objects under + **three** naming conventions and the pattern modelled one: + + | convention | examples | matched before | + |---|---|---| + | `*_alloc*` | `js_object_alloc`, `js_array_alloc`, `js_closure_alloc`, `js_uint8array_alloc`, `js_inline_arena_slow_alloc` | yes | + | `*_new*` | `js_regexp_new`, `js_promise_new`, `js_symbol_new`, `js_date_new`, `js_error_new`, `js_typed_array_new`, `js_weakmap_new`, `js_url_new`, `js_boxed_string_new`, ~140 more | **no** | + | `*_create*` | `js_object_create`, `js_array_create`, `js_vm_create_context`, `js_crypto_create_hash`, ~40 more | only `object_create*` | + + All three are now matched as conventions, and the constructors that use none + of them are enumerated explicitly: the `_construct*` ctor forms, the fresh + string producers (`string_coerce`, `jsvalue_to_string*`, `string_slice`, + `string_to_*_case`, `string_pad_*`, `string_trim*`, …), the copy-on-read and + ES2023 change-by-copy array family (`array_to_sorted*`, `array_to_spliced`, + `array_with`, `array_flat*`, `array_like_to_array`, `iterator_to_array`, …), + the whole-object producers (`object_keys*`, `object_entries*`, + `object_from_entries`, `object_get_own_property_descriptor*`, + `structured_clone*`, the Set-methods family), the namespace/class-shape + helpers, and BigInt's `bigint_from*` (which has neither `_alloc` nor `_new`). + + Widening is safe in the checker's one-sided direction: a name that turns out + not to allocate costs a false positive to triage, while a missing one costs a + shipped use-after-free plus the round it takes to find by hand. The file now + says so, so the next person extends it rather than guessing. + +- **`scripts/gc_root_dominance_check.py`: the ToPrimitive / ToString / ToNumber + coercion family is `POLL_CAPABLE_RUNTIME`.** This is the second half of the + same blind spot and it is the half that matters for CI, because + `--moving-only` is the mode `gc-root-dominance.yml` gates on. With `ALLOC_RE` + widened but the coercions unmodelled, the `/re/.test(s)` site was reported by + the raw `--stale-registers` count and **still invisible to `--moving-only`**: + nothing in its window was classified as reaching a moving minor. A coercion + does not look like a call into user code, but ToPrimitive is exactly that — + and `js_string_coerce`'s own doc comment already said so ("a `POINTER_TAG` + object routes through `js_jsvalue_to_string`, which can invoke a user + `toString` / `valueOf`"). The checker just never read it. + +- **`scripts/gc_root_dominance_check.py`: `js_regexp_test` / `js_regexp_exec` + are fatal sinks.** A stale `RegExpHeader*` is dereferenced immediately by + both, and this one faulted rather than merely answering wrong, so it belongs + in the `--fatal-sinks` ranking and not only in the raw count. + +## Verification + +Measured against the parent (`4e99c1bad`, #7226's head), built from this +worktree rather than borrowed from another one. + +The checker change is what makes the codegen change checkable, so it is +reported first. Over the gap-test IR for the new reproducer: + +| `--stale-registers` over `test_gap_gc_regexp_receiver_rooting.ts` | parent | this PR | +|---|---|---| +| base checker (`regexp_alloc\w*`) | **0 reported** | — | +| widened `ALLOC_RE`, raw count | 3 | **0** | +| widened + `--moving-only` (the gate's mode) | **3**, `MOVING: YES via js_jsvalue_to_string_coerce` | **0** | + +All three are named exactly: `source (alloc): call i64 @js_regexp_new`, +`stale use: call i32 @js_regexp_test` / `@js_regexp_exec`, with +`js_jsvalue_to_string_coerce` in the window. + +Over the 130-module / 2170-function gap corpus, both checker widenings +together: + +| `--moving-only` | parent | this PR | +|---|---|---| +| bind-anchored violations (**the gate**) | 0 | **0**, allowlist still empty | +| `--stale-registers`, total | 2730 | 2738 | +| `--stale-registers --moving-only` | 2 | **62** | +| `--stale-registers --fatal-sinks` | 279 | 282 | +| `--stale-registers --moving-only --fatal-sinks` | 0 | **0** | +| `--unrooted-allocas`, moving-reachable | 57 | 85 | + +The gate does not move. The 60 newly-*moving* stale-register leads were all +already in the 2730-entry diagnostic list; modelling the coercions is what +reclassified their windows. Triaged mechanically by the shape of the stale +use: + +| stale use | count | verdict | +|---|---|---| +| `lshr … , 48` | 37 | NaN-box **tag** read. Relocation rewrites the low 48 bits; the tag is unchanged. Not a bug. | +| `fadd double` | 3 | float arithmetic on a value the mode could not prove non-pointer. Not a bug. | +| `getelementptr i8, …, 32` | 15 | direct field access in the `*__pshape` pointer-shape specializations. A real dereference shape and a real population — **left for its own PR**, since it is the `PERRY_PTR_SHAPE_LOCALS` family and wants its own measured count. | +| call argument | 7 | typed-feedback array receivers held across `js_number_coerce`. Same call: real shape, own population, own PR. | + +Nothing in the newly-visible set is a fatal sink, which is why the fatal count +moves only by the three regexp entries this PR then fixes. + +Gap test — `test_gap_gc_regexp_receiver_rooting.ts`, compiled **and** run with +`PERRY_GC_MOVING_LOOP_POLLS=1`: + +| | parent | this PR | +|---|---|---| +| `POLLS=1` + `PERRY_GC_ZEAL=1` | **0/10 — SIGSEGV/SIGBUS every run** | `bad 0` **10/10** | +| `POLLS=1` + zeal + `PERRY_GEN_GC=0` | `bad 0` | `bad 0` | + +The parent arm is a hard fault rather than a nonzero `bad`, and that is the +honest signature: with a regex **literal** receiver — the registry's shape — +`js_regexp_new`'s result is held only in the register, so the evacuating minor +retires the block under it and the deref lands in from-space. The +`PERRY_GEN_GC=0` arm proves the test tracks collector mode rather than being +flaky. Zeal is required for the same structural reason #7226 recorded for +`prev_this`: the window is a user call, so only a *moving* collection exploits +it, and allocation-triggered collections take +`ManualGcScanGuard::force_full_scan`, which makes the copying minor ineligible. + +## `sfw-registry` moves, and does NOT reach 30/30 + +`sfw-registry --help`, 141 modules, `PERRY_FORCE_WELL_KNOWN=iovalkey`, compiled +**and** run with `PERRY_GC_MOVING_LOOP_POLLS=1`: + +| | parent (`4e99c1bad`) | this PR | +|---|---|---| +| `POLLS=1`, **30 runs** | **0/30** | **28/30** | + +Both arms use the same runtime archives (the codegen fix is the only +difference) and the same firewall tree, so the comparison is like-for-like. +The parent's 30 failures are **not** crashes — every one is a deterministic +`TypeError: Cannot convert undefined or null to object`, which is what a stale +regexp receiver produces here: `defineApiCall` computes +`urlRequiresInterpolation = /\[[a-zA-Z]+\]/.test(url)` at definition time, the +stale read returns the wrong boolean, and the wrong branch hands `undefined` +to a downstream `Object` operation. The fix removes that failure mode +completely. + +Note that #7226 reported 26/30 for this same parent commit. That measurement +was taken against a different firewall checkout; on the tree measured here the +parent is 0/30. The delta this PR is responsible for is the one measured above, +on one tree, with one runtime. + +**This does not close #7154 and #7161's stopgap stays.** Two runs in thirty +still SIGSEGV, and the residual is a *different* object from the one this PR +fixes. Under `PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_PROTECT_FROMSPACE=1 +PERRY_GC_PROTECT_FROMSPACE_DEPTH=800` it is deterministic — **40/40** — and the +reporter names it: + +``` +[gc-fromspace-protect] FAULT: signal 10 at 0x…5d5c + block=0x…20000 +220508 retired_bytes=253416 retired_by_minor=#155 + last-known object: user_ptr=0x…5d58 obj_type=3 size=80 +``` + +`obj_type=3` is a **string**, not a `RegExpHeader`, so it is not the receiver +this PR rooted. Disassembling the faulting frame confirms it: the return +address is `defineApiCall + 428`, and `+424` is `bl js_regexp_test` — the same +call site, twenty bytes further along, which is exactly the size of the +`js_gc_temp_root_push` / `js_gc_temp_root_get` pair this PR inserts. The +receiver operand is now correct; the surviving stale value is a string reaching +the same call. + +That the protected arm is 40/40 while the unprotected arm is 2/30 is the useful +part: the next round has a deterministic reproducer instead of a 7 % one. It is +**not** fixed speculatively here — no edit ships without a test that can fail +without it. diff --git a/crates/perry-codegen/src/expr/instance_misc1.rs b/crates/perry-codegen/src/expr/instance_misc1.rs index 0a206a77ab..c68ca6b52d 100644 --- a/crates/perry-codegen/src/expr/instance_misc1.rs +++ b/crates/perry-codegen/src/expr/instance_misc1.rs @@ -1064,22 +1064,44 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // a NaN-tagged string. Both must be unboxed before the call. Expr::RegExpTest { regex, string } => { let regex_box = lower_expr(ctx, regex)?; + // #7154: the receiver is live across BOTH the string operand's own + // lowering and the `js_jsvalue_to_string_coerce` below it, and the + // coerce is unconditional — it allocates, and on an object argument + // it runs a user `toString`, which is arbitrary JS with its own + // back-edge polls. So the window always exists and `collects` is + // `true` rather than a `expr_may_trigger_gc(string)` test. + // + // This is the site the registry reproducer faults at + // (`defineApiCall + 404`, `obj_type=3 size=80`): `js_regexp_new`'s + // raw result went into a bare `x20`, the coerce drove an evacuating + // minor that moved it, and `js_regexp_test` dereferenced from-space. + // The static checker could not see it because `ALLOC_RE` spelled the + // allocator `regexp_alloc\w*` and the call is `js_regexp_new`. + let guard = super::temp_root::guard_store_operand_across(ctx, regex, ®ex_box, true); let str_box = lower_expr(ctx, string)?; - let blk = ctx.block(); - let regex_handle = unbox_to_i64(blk, ®ex_box); // Per spec `RegExp.prototype.test` does `ToString(argument)`, so a // String wrapper (`re.test(new String("x"))`), a number // (`re.test(123)`), or an object with a custom `toString` must be // coerced — and a throwing `toString`/`valueOf` must propagate. // `js_get_string_pointer_unified` only unwraps real strings, so use // the coercing ToString that dispatches `toString` on objects. - let str_handle = blk.call(I64, "js_jsvalue_to_string_coerce", &[(DOUBLE, &str_box)]); + let str_handle = + ctx.block() + .call(I64, "js_jsvalue_to_string_coerce", &[(DOUBLE, &str_box)]); + // Re-read BELOW the coerce, then unbox. Unboxing above it is what + // parked the pre-move address in a register in the first place. + let regex_box = super::temp_root::reread_store_operand(ctx, &guard, regex, ®ex_box)?; + let blk = ctx.block(); + let regex_handle = unbox_to_i64(blk, ®ex_box); let i32_v = blk.call( I32, "js_regexp_test", &[(I64, ®ex_handle), (I64, &str_handle)], ); - Ok(i32_bool_to_nanbox(blk, &i32_v)) + let out = i32_bool_to_nanbox(ctx.block(), &i32_v); + // After the call: `js_regexp_test` allocates while reading these. + super::temp_root::release_store_operand(ctx, guard); + Ok(out) } Expr::RegExpExec { regex, string } => { // Returns ArrayHeader* or null. For a null (0) result we must @@ -1088,18 +1110,28 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // non-null pointer value that compares unequal to null, causing // infinite loops + segfaults when callers IndexGet on the result. let regex_box = lower_expr(ctx, regex)?; + // #7154, identical shape to `RegExpTest` above and found with it: + // the receiver is live across the string operand's lowering and + // across the unconditional coerce, which allocates and can run a + // user `toString`. + let guard = super::temp_root::guard_store_operand_across(ctx, regex, ®ex_box, true); let str_box = lower_expr(ctx, string)?; - let blk = ctx.block(); - let regex_handle = unbox_to_i64(blk, ®ex_box); // `RegExp.prototype.exec` does `ToString(argument)` — coerce String // wrappers / numbers / objects (and propagate a throwing toString) // rather than only unwrapping real strings (see RegExpTest above). - let str_handle = blk.call(I64, "js_jsvalue_to_string_coerce", &[(DOUBLE, &str_box)]); + let str_handle = + ctx.block() + .call(I64, "js_jsvalue_to_string_coerce", &[(DOUBLE, &str_box)]); + let regex_box = super::temp_root::reread_store_operand(ctx, &guard, regex, ®ex_box)?; + let blk = ctx.block(); + let regex_handle = unbox_to_i64(blk, ®ex_box); let result = blk.call( I64, "js_regexp_exec", &[(I64, ®ex_handle), (I64, &str_handle)], ); + super::temp_root::release_store_operand(ctx, guard); + let blk = ctx.block(); // Branch on result == 0 → TAG_NULL; else NaN-box as pointer. let is_null = blk.icmp_eq(I64, &result, "0"); let ptr_boxed = nanbox_pointer_inline(ctx.block(), &result); diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index be88869222..7b37f2fb9a 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -282,13 +282,90 @@ def build_cfg(f): MOVING_POLL = "js_gc_loop_safepoint" # Result-producing calls that materialize a fresh GC object. +# +# ------------------------------------------------------------------ THE AUDIT +# This list is enumerated from the runtime's actual exported entry points, NOT +# guessed from a naming convention, because guessing a convention is precisely +# how it has failed twice: +# +# * `js_implicit_this_set` (#7226) -- a root READ that was not modelled, so +# `prev_this` survived two PRs that were looking straight at it; +# * `js_regexp_new` (#7154, this change) -- the regex carried an alternative +# spelled `regexp_alloc\w*`, and **no such symbol has ever existed**. The +# `/re/.test(s)` lowering holds `js_regexp_new`'s raw result in a register +# across `js_jsvalue_to_string_coerce`, and the checker reported nothing +# because the register had no recognised heap-value source. +# +# The second one is the instructive one. `regexp_alloc\w*` was not a typo, it +# was an ASSUMED convention: whoever wrote it knew a RegExp allocates and +# extrapolated the `_alloc` suffix from its neighbours. Reconciling every +# alternative below against the real symbol table (`grep -rhoE 'extern "C" fn +# js_\w+'` over perry-runtime + perry-stdlib, intersected with the names +# perry-codegen actually declares) found FOUR alternatives in the same state -- +# `regexp_alloc\w*`, `promise_alloc\w*`, `bigint_alloc\w*` and +# `typed_array_alloc\w*` matched nothing whatsoever. A quarter of the pattern +# was decorative. +# +# The root cause is that the runtime materializes fresh GC objects under THREE +# naming conventions and the old pattern modelled one: +# +# `_alloc*` js_object_alloc, js_array_alloc, js_closure_alloc, js_box_alloc, +# js_map_alloc, js_set_alloc, js_buffer_alloc, js_uint8array_alloc, +# js_arguments_object_alloc, js_inline_arena_slow_alloc, ... +# `_new*` js_regexp_new, js_promise_new, js_symbol_new, js_date_new, +# js_error_new, js_typed_array_new, js_weakmap_new, js_weakset_new, +# js_url_new, js_boxed_string_new, js_array_buffer_new, ~140 more +# `_create*` js_object_create, js_array_create, js_vm_create_context, +# js_crypto_create_hash, js_readline_create_interface, ~40 more +# +# So the three conventions are matched as conventions, and the constructors +# that use none of them are enumerated explicitly below. Widening is SAFE in +# the checker's one-sided direction: a name that is in fact not an allocation +# costs a false positive to triage, while a missing one costs a shipped +# use-after-free plus the investigation round it takes to find it by hand. +# When in doubt, add it. ALLOC_RE = re.compile( r"^js_(" - r"object_alloc\w*|array_alloc\w*|closure_alloc\w*|box_alloc\w*|" - r"string_alloc\w*|string_concat\w*|string_coerce|string_from\w*|" - r"map_alloc\w*|set_alloc\w*|promise_alloc\w*|bigint_alloc\w*|" - r"typed_array_alloc\w*|buffer_alloc\w*|regexp_alloc\w*|" - r"object_create\w*|array_from\w*|build_class_keys_array" + # -- convention 1: `*_alloc*`, including the arena's own slow path. + r"\w*_alloc\w*|" + # -- convention 2: `*_new*`. `js_regexp_new` is the #7154 residual. + r"\w*_new\w*|" + # -- convention 3: `*_create*`. + r"\w*_create\w*|create_\w+|" + # -- constructors using none of the three conventions ------------------- + # `new X(...)` / `X(...)` forms folded at HIR into a direct ctor call. + r"\w*_construct|\w*_construct_call|\w*_construct_apply|" + r"reflect_construct|new_function_construct\w*|super_construct_apply|" + # fresh strings. Every one of these returns a *mut StringHeader the + # caller holds raw; `string_coerce` and `jsvalue_to_string_coerce` are + # the two the `/re/` lowerings feed. + r"string_concat\w*|string_coerce|string_from\w*|string_append|" + r"jsvalue_to_string\w*|value_to_string\w*|number_to_string\w*|" + r"string_repeat|string_slice|string_substring|string_substr|" + r"string_pad_\w+|string_trim\w*|string_to_\w+_case|string_replace\w*|" + r"string_split|string_normalize|string_at|string_char_at|" + r"string_index_get_boxed|boxed_string\w*|" + # fresh arrays: the copy-on-read Array.prototype methods, the ES2023 + # change-by-copy family, and the iterable/array-like converters. + r"array_from\w*|array_clone\w*|array_concat\w*|array_slice|array_splice|" + r"array_to_spliced|array_to_sorted\w*|array_to_reversed|array_with|" + r"array_flat\w*|array_map|array_filter|array_like_to_array|" + r"iterator_to_array|array_of|array_group_by|" + # fresh objects/collections handed back as a whole + r"object_keys\w*|object_values\w*|object_entries\w*|object_from_entries|" + r"object_assign\w*|object_group_by|object_coerce|" + r"object_get_own_property_descriptor\w*|object_get_own_property_names|" + r"object_get_own_property_symbols|" + r"map_from_iterable|set_from_iterable|map_group_by|" + r"set_union|set_intersection|set_difference|set_symmetric_difference|" + r"structured_clone\w*|" + # module namespace objects, class-shape side tables, generator plumbing + r"build_class_keys_array|create_namespace|create_native_module_namespace|" + r"generator_attach_prototype|proxy_revocable|" + # BigInt: no `_alloc` and no `_new`; the constructors are `_from_*`. + r"bigint_from\w*|bigint_\w+_op|" + # Buffers / typed arrays that spell it neither way. + r"buffer_from\w*|typed_array_from\w*|array_buffer_slice" r")$" ) @@ -362,6 +439,21 @@ def is_collecting(callee): # ------------------------------------------------- interprocedural poll reach # Runtime helpers that re-enter compiled JS (and therefore its back-edge polls). +# +# The COERCION family is the half of this set that is easy to leave out, and +# leaving it out is what kept `--moving-only` blind to #7154's `/re/.test(s)` +# residual even once `ALLOC_RE` had been widened to recognise `js_regexp_new`. +# A coercion does not *look* like a call into user code, but ToPrimitive is +# exactly that: `js_jsvalue_to_string_coerce` runs `to_string_method_impl(…, +# skip_to_primitive = false)`, which consults `[Symbol.toPrimitive]`, then +# `toString`, then `valueOf` — arbitrary JS, with its own loop back-edge polls. +# `js_string_coerce`'s own doc comment already said so ("a `POINTER_TAG` object +# routes through `js_jsvalue_to_string`, which can invoke a user `toString` / +# `valueOf`"); the checker just never read it. +# +# This matters more than the raw-count modes suggest, because `--moving-only` +# is the mode the `gc-root-dominance.yml` gate runs. A source the gate cannot +# classify as reaching a moving minor is a source the gate cannot fail on. POLL_CAPABLE_RUNTIME = { "js_call_function", "js_call_closure", "js_invoke_closure", "js_call_value", "js_apply_function", "js_function_call", @@ -370,6 +462,13 @@ def is_collecting(callee): "js_array_sort", "js_array_map", "js_array_filter", "js_array_for_each", "js_array_reduce", "js_json_stringify", "js_string_replace", "js_promise_run_microtasks", "js_gc_loop_safepoint", + # ToPrimitive / ToString / ToNumber: every one of these dispatches a user + # `[Symbol.toPrimitive]` / `toString` / `valueOf` on an object operand. + "js_to_primitive", + "js_jsvalue_to_string", "js_jsvalue_to_string_coerce", + "js_jsvalue_to_string_method", "js_jsvalue_to_string_radix", + "js_string_coerce", "js_string_coerce_method_this", + "js_number_coerce", "js_object_coerce", } @@ -1198,7 +1297,11 @@ def seeded_violation_test(paths, moving_only, anchor, want_sites, verbose=False) r"object_get_property|object_set_property|put_value_set_dyn_ic|" r"get_value_dyn_ic|closure_call\w*|call_closure|call_function|" r"call_value|invoke_closure|apply_function|" - r"array_\w+|map_\w+|set_\w+|typed_feedback_\w*call\w*" + r"array_\w+|map_\w+|set_\w+|typed_feedback_\w*call\w*|" + # A stale RegExpHeader* is dereferenced immediately by both of these — + # this is #7154's residual, and it faulted rather than merely answering + # wrong, so it belongs in the fatal ranking and not just the raw count. + r"regexp_test|regexp_exec|regexp_match\w*|regexp_replace\w*" r")$" ) diff --git a/test-files/test_gap_gc_regexp_receiver_rooting.ts b/test-files/test_gap_gc_regexp_receiver_rooting.ts new file mode 100644 index 0000000000..c30e12b737 --- /dev/null +++ b/test-files/test_gap_gc_regexp_receiver_rooting.ts @@ -0,0 +1,80 @@ +// #7154: the RECEIVER of `re.test(s)` / `re.exec(s)` must be rooted across the +// `ToString(s)` coercion the lowering emits below it. +// +// `Expr::RegExpTest` / `Expr::RegExpExec` lowered the receiver first, unboxed it +// to a raw `RegExpHeader*` in a bare SSA register, and only THEN emitted +// `js_jsvalue_to_string_coerce`. That coerce is not a bystander: it allocates, +// and on an object argument it dispatches a user `toString`, which is arbitrary +// JS with its own loop back-edge polls. Under `PERRY_GC_MOVING_LOOP_POLLS=1` one +// of those polls runs an evacuating minor while the regexp is live only in that +// register, and `js_regexp_test` then dereferences abandoned from-space memory. +// +// This is the residual #7226 measured and named rather than fixed. In the +// `sfw-registry` reproducer it is `src/lib/api/shared.ts:67`, +// `/\[[a-zA-Z]+\]/.test(url)`, faulting at +// `perry_fn_src_lib_api_shared_ts__defineApiCall + 404`: +// +// bl js_regexp_new ; ALLOCATES +// and x20, x0, #0xffffffffffff ; raw regexp pointer -> bare register +// bl js_jsvalue_to_string_coerce ; ALLOCATES, runs user toString +// mov x0, x20 ; STALE +// bl js_regexp_test ; faults here +// +// The static checker could not see it, and that is the other half of the bug: +// `ALLOC_RE` carried an alternative spelled `regexp_alloc\w*` while the call is +// named `js_regexp_new`, so the register had no recognised heap-value source. +// No such symbol as `js_regexp_alloc` has ever existed. +// +// LIVE BY CONSTRUCTION. Both arms use a regex LITERAL receiver, which is the +// registry's shape and the strongest one: `js_regexp_new`'s result is held +// ONLY in the register, so nothing else keeps it alive across the coerce. The +// coercion allocates long enough that the minor runs early inside it and the +// abandoned bytes are then reused by the rest of the coercion's own work. The +// answers are checked against known-correct booleans and match text, so a stale +// read is observable rather than latent. Clean under `PERRY_GEN_GC=0`, so the +// evacuating arms are the ones that bite. + +// The loop is what matters, not its trip count: under `PERRY_GC_ZEAL=1` the +// FIRST back-edge poll inside it already runs an evacuating minor, which is +// the collection the receiver has to survive. The count is kept modest on +// purpose — zeal collects at every safepoint, so a 4000-trip churn (what the +// sibling #7154 tests use, where the collection has to arrive on its own +// budget) turns this file into a multi-hour run for no extra coverage. +function churn(tag: string): string { + const bits: any[] = []; + for (let i = 0; i < 120; i++) { + bits.push({ i: i, s: "x", pad: [i, i + 1, i + 2] }); + } + return bits.length === 120 ? tag : "unreachable"; +} + +class Coercer { + tag: string; + constructor(tag: string) { + this.tag = tag; + } + toString(): string { + return churn(this.tag); + } +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 40; r++) { + // `.test` — the exact registry site. + if (!/^tag-[0-9]+$/.test(new Coercer("tag-" + r) as any)) { + bad++; + } + if (/^tag-[0-9]+$/.test(new Coercer("nope-" + r) as any)) { + bad++; + } + // `.exec` — the same lowering with the same defect, fixed with it. + const m = /^tag-([0-9]+)$/.exec(new Coercer("tag-" + r) as any); + if (m === null || m[1] !== String(r)) { + bad++; + } + } + return bad; +} + +console.log("bad", run()); From 6aeef5baf64e4fb03b04a37f89397d1b29b3efc8 Mon Sep 17 00:00:00 2001 From: jdalton Date: Sun, 2 Aug 2026 02:50:34 -0400 Subject: [PATCH 3/3] fix(gc): root every argument of a cross-module direct call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An argument list is evaluated left to right and each finished value sits in a bare SSA register while the later ones are lowered. `lower_call/extern_func.rs`'s generic `perry_fn___` path lowered the whole list in a plain `for a in args` loop with no protection at all, so `f(A, B, {…}, Schema.array(), body => …)` leaves A and B naming pre-collection addresses the moment an evacuating minor lands in argument 3, 4 or 5 — and it does: argument 3 allocates an object, argument 4 runs user code with its own back-edge polls, argument 5 allocates a closure. This is #7227's residual. In the registry it is `src/lib/api/alerts.ts`'s module init calling `defineApiCall(url, method, …)` across the module boundary; the fault surfaces one frame down inside `js_regexp_test`, because the stale `url` is what `/\[[a-zA-Z]+\]/.test(url)` hands it. Measured at the fault rather than read off the disassembly: the string literal's `__perry_init_strings_*` handle global held the post-move address evacuation wrote back, while the register held the retired from-space one. A string-literal argument therefore takes `OperandProtection::Reload` — its handle global is a registered root, so the string is never swept, and re-emitting the load below the collection point costs no runtime call. Other arguments take a real temp root. Both come from `lower_exprs_rooted`, which already does this for the `new C(…)` list, and each argument is gated on `any_later_ref_may_trigger_gc` so lists with nothing allocating after them emit the IR they emitted before. The static checker missed it because it classifies a heap-value source as an `ALLOC_RE` call or a shadow-slot load; a load of a string-literal handle global is neither. Over the new gap test's IR it reports 24 `--moving-only` stale uses at the call and names none of them `%r9`/`%r10`, the two literals that actually faulted. Refs #7154, #7226, #7227, #7161 --- ...7240-cross-module-call-argument-rooting.md | 63 +++++++++++ .../src/lower_call/extern_func.rs | 13 ++- crates/perry-codegen/src/lower_call/mod.rs | 59 ++++++++++ .../gc_call_arg_rooting_pkg/callee.ts | 20 ++++ .../test_gap_gc_call_argument_rooting.ts | 106 ++++++++++++++++++ 5 files changed, 255 insertions(+), 6 deletions(-) create mode 100644 changelog.d/7240-cross-module-call-argument-rooting.md create mode 100644 test-files/fixtures/gc_call_arg_rooting_pkg/callee.ts create mode 100644 test-files/test_gap_gc_call_argument_rooting.ts diff --git a/changelog.d/7240-cross-module-call-argument-rooting.md b/changelog.d/7240-cross-module-call-argument-rooting.md new file mode 100644 index 0000000000..ee106c37ff --- /dev/null +++ b/changelog.d/7240-cross-module-call-argument-rooting.md @@ -0,0 +1,63 @@ +### Fixed + +- **`codegen`: every argument of a cross-module direct call is now protected + across the lowering of the arguments that follow it** (#7154). An argument + list is evaluated left to right and each finished value sits in a bare SSA + register while the later ones are lowered. `lower_call/extern_func.rs`'s + generic `perry_fn___` path lowered the whole list in a plain + `for a in args` loop with no protection at all, so + `f(A, B, {…}, Schema.array(), body => …)` leaves `A` and `B` naming + pre-collection addresses the moment an evacuating minor lands in argument 3, + 4 or 5 — and it does: argument 3 allocates an object, argument 4 runs user + code with its own loop back-edge polls, argument 5 allocates a closure. + + This is the residual #7227 measured and named rather than fixed. In the + `sfw-registry` reproducer it is `src/lib/api/alerts.ts`'s module init calling + `defineApiCall(url, method, {…}, SocketAlert.array(), body => …)` across the + module boundary. The fault surfaces one frame down, inside `js_regexp_test` + called from `defineApiCall + 428`, because the stale `url` argument is what + `/\[[a-zA-Z]+\]/.test(url)` hands to it: + + ```asm + ldp d9, d10, [x24, #0x18] ; url + method, loaded from their + ; __perry_init_strings_* handle globals + bl js_object_alloc_class_inline_keys ; argument 3 -- ALLOCATES + bl perry_fn_…zod… ; argument 4 -- USER CODE + bl js_closure_alloc_singleton ; argument 5 -- ALLOCATES + fmov d0, d9 ; STALE + fmov d1, d10 ; STALE + bl perry_fn_src_lib_api_shared_ts__defineApiCall + ``` + + The diagnosis is a measurement rather than a reading of the disassembly. At + the fault the `__perry_init_strings_*` handle global held `0x…76561xxx` — the + post-move address evacuation wrote back — while `defineApiCall`'s shadow slot + (and the register it was stored from) held `0x…74eb5d58`, inside the + quarantined from-space block the reporter named. Root rewritten, register not. + + A string-literal argument therefore takes `OperandProtection::Reload`: its + handle global is a registered root, so the string is never *swept*, and the + fix is to emit the load again below the collection point — no runtime call at + all. Non-literal arguments take a real temp root, as + `temp_root::lower_exprs_rooted` already does for the `new C(…)` argument list + (#6969). Each argument is gated on `any_later_ref_may_trigger_gc`, so an + argument list with nothing allocating after it emits exactly the IR it did + before. + + **Why `scripts/gc_root_dominance_check.py` reports nothing here**, which is + the part worth carrying forward: the checker classifies a heap-value SOURCE + as an `ALLOC_RE` call or a shadow-slot load. A load of a string-literal + handle global is neither, so the register it defines is never tracked as a + heap value and no stale use can be attributed to it. That is a third shape of + the same blind spot `js_implicit_this_set` (#7226) and `js_regexp_new` (#7227) + each cost a round for — and unlike those two it is not fixed by adding a name + to a pattern, because the source is a `load`, not a `call`. + +### Added + +- `test-files/test_gap_gc_call_argument_rooting.ts` (+ its cross-module fixture + `test-files/fixtures/gc_call_arg_rooting_pkg/callee.ts`). It has to be two + files: the defect is in the cross-module lowering, and a same-file callee + compiles through `func_ref.rs` instead. Both protections are exercised — one + call passes two string literals (`Reload`), the other passes a local holding + a freshly-allocated string plus a literal (`Root` + `Reload`). diff --git a/crates/perry-codegen/src/lower_call/extern_func.rs b/crates/perry-codegen/src/lower_call/extern_func.rs index f38173d875..93034f81ed 100644 --- a/crates/perry-codegen/src/lower_call/extern_func.rs +++ b/crates/perry-codegen/src/lower_call/extern_func.rs @@ -1952,6 +1952,7 @@ pub fn try_lower_extern_func_call( ctx.pending_declares .push((fname.clone(), DOUBLE, param_types)); let mut lowered: Vec = Vec::with_capacity(target_arity); + let mut arg_guard: Option = None; if has_rest { // Fixed (non-rest) params: pass through. let fixed_count = declared_count.saturating_sub(1); @@ -1984,16 +1985,16 @@ pub fn try_lower_extern_func_call( let rest_box = nanbox_pointer_inline(ctx.block(), ¤t); lowered.push(rest_box); } else { - for a in args { - lowered.push(lower_expr(ctx, a)?); - } + // #7154: the registry's residual. See `super::lower_call_args_rooted`. + let (values, guard) = super::lower_call_args_rooted(ctx, args)?; + arg_guard = guard; + lowered.extend(values); // Pad with TAG_UNDEFINED for the missing trailing args. let undefined_lit = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); while lowered.len() < target_arity { lowered.push(undefined_lit.clone()); } } - let arg_slices: Vec<(crate::types::LlvmType, &str)> = - lowered.iter().map(|s| (DOUBLE, s.as_str())).collect(); - Ok(Some(ctx.block().call(DOUBLE, &fname, &arg_slices))) + let call = super::emit_rooted_call(ctx, &fname, &lowered, arg_guard); + Ok(Some(call)) } diff --git a/crates/perry-codegen/src/lower_call/mod.rs b/crates/perry-codegen/src/lower_call/mod.rs index f542a13e6b..e9b0cb1826 100644 --- a/crates/perry-codegen/src/lower_call/mod.rs +++ b/crates/perry-codegen/src/lower_call/mod.rs @@ -135,6 +135,65 @@ pub(crate) use options::extract_options_fields; // API as `lower_call::iter_native_module_table` — keep that path stable. pub(crate) use native_table::iter_native_module_table; +/// #7154: lower a direct call's argument list with each already-evaluated +/// argument protected across the evaluation of the ones that follow it. +/// +/// An argument list is evaluated left to right, and every value produced so +/// far lives in a bare SSA register while the later ones are lowered. The +/// cross-module `perry_fn___` path lowered the whole list in a +/// plain `for a in args` loop with no protection at all, so +/// `f(URL, "GET", {…}, Schema.array(), body => …)` — the `sfw-registry` +/// reproducer's `defineApiCall(…)` shape — leaves arguments 1 and 2 naming +/// pre-collection addresses the moment an evacuating minor lands in argument +/// 3, 4 or 5. It does: argument 3 is an object literal (allocates), argument 4 +/// runs user code with its own loop back-edge polls, argument 5 allocates a +/// closure. +/// +/// The two string literals are the case that faulted, and they are the *cheap* +/// case to fix. A literal lowers to a load of a `__perry_init_strings_*` handle +/// global, which IS a registered root — so the string is never swept — but an +/// evacuating cycle REWRITES that global while the register keeps the pre-move +/// address. [`OperandProtection::Reload`] re-emits the load below the collection +/// point and costs no runtime call at all. Measured at the fault: the handle +/// global held the post-move address, the register held the retired from-space +/// one. +/// +/// [`temp_root::lower_exprs_rooted`] gates each argument on +/// `any_later_ref_may_trigger_gc`, so an argument list nothing allocating +/// follows emits exactly the IR it emitted before. +/// +/// Returns the values to pass and the guard for [`emit_rooted_call`]. +/// +/// [`OperandProtection::Reload`]: crate::expr::temp_root +/// [`temp_root::lower_exprs_rooted`]: crate::expr::temp_root::lower_exprs_rooted +pub(crate) fn lower_call_args_rooted( + ctx: &mut FnCtx<'_>, + args: &[Expr], +) -> Result<(Vec, Option)> { + let refs: Vec<&Expr> = args.iter().collect(); + crate::expr::temp_root::lower_exprs_rooted(ctx, &refs) +} + +/// Emit a direct call over an already-lowered argument list, then release the +/// [`lower_call_args_rooted`] guard. +/// +/// The release has to sit BELOW the call, not above it: the callee allocates +/// while reading these arguments, so the slots have to outlive the call itself. +pub(crate) fn emit_rooted_call( + ctx: &mut FnCtx<'_>, + fname: &str, + lowered: &[String], + guard: Option, +) -> String { + let arg_slices: Vec<(crate::types::LlvmType, &str)> = lowered + .iter() + .map(|s| (crate::types::DOUBLE, s.as_str())) + .collect(); + let result = ctx.block().call(crate::types::DOUBLE, fname, &arg_slices); + crate::expr::temp_root::temp_root_release(ctx, guard); + result +} + /// Lower a `Call` expression. Two shapes are supported: /// 1. `FuncRef(id)(args...)` — direct call to a user function by HIR id. /// 2. `console.log(expr)` where `expr` lowers to a double — emits a diff --git a/test-files/fixtures/gc_call_arg_rooting_pkg/callee.ts b/test-files/fixtures/gc_call_arg_rooting_pkg/callee.ts new file mode 100644 index 0000000000..b87657a052 --- /dev/null +++ b/test-files/fixtures/gc_call_arg_rooting_pkg/callee.ts @@ -0,0 +1,20 @@ +// #7154 fixture: the CROSS-MODULE callee for +// `test-files/test_gap_gc_call_argument_rooting.ts`. +// +// It has to live in its own module because the defect under test is in the +// cross-module direct-call lowering (`lower_call/extern_func.rs`'s +// `perry_fn___` path), which is a different code path from the +// same-module one. A same-file callee would compile through `func_ref.rs` and +// never touch the arm this test pins. +// +// The body reads every string argument, so a caller that handed over a +// pre-collection address produces wrong text rather than a latent bad pointer. +export function joinArgs( + url: string, + method: string, + opts: { n: number }, + schemaTag: number, + parseTag: number, +): string { + return url + " " + method + " " + opts.n + " " + schemaTag + " " + parseTag; +} diff --git a/test-files/test_gap_gc_call_argument_rooting.ts b/test-files/test_gap_gc_call_argument_rooting.ts new file mode 100644 index 0000000000..d37e6ad10c --- /dev/null +++ b/test-files/test_gap_gc_call_argument_rooting.ts @@ -0,0 +1,106 @@ +// #7154: every argument of a cross-module direct call must survive the +// lowering of the arguments that follow it. +// +// An argument list is evaluated left to right and each finished value sits in +// a bare SSA register while the later ones are lowered. `lower_call/ +// extern_func.rs`'s `perry_fn___` path lowered the whole list in a +// plain loop with no protection at all, so `f(A, B, alloc(), userCode(), …)` +// leaves A and B naming pre-collection addresses the moment an evacuating +// minor lands in argument 3, 4 or 5. +// +// This is the residual #7227 measured and named. In the `sfw-registry` +// reproducer it is `src/lib/api/alerts.ts`'s module init calling +// `defineApiCall(url, method, {…}, Schema.array(), body => JSON.parse(body))` +// across the module boundary, faulting one frame down at +// `perry_fn_src_lib_api_shared_ts__defineApiCall + 428` inside `js_regexp_test` +// with `obj_type=3` (a string). The compiled shape: +// +// ldp d9, d10, [x24, #0x18] ; url + method, loaded from their +// ; `__perry_init_strings_*` handle globals +// bl js_object_alloc_class_inline_keys ; argument 3 -- ALLOCATES +// bl perry_fn_…__SocketAlert / zod ; argument 4 -- USER CODE +// bl js_closure_alloc_singleton ; argument 5 -- ALLOCATES +// fmov d0, d9 ; STALE +// fmov d1, d10 ; STALE +// bl perry_fn_src_lib_api_shared_ts__defineApiCall +// +// Measured at the fault, which is what makes the diagnosis a fact rather than +// a reading of the disassembly: the handle global held `0x…76561xxx` — the +// post-move address evacuation wrote back — while the register (and therefore +// the callee's shadow slot) held `0x…74eb5d58`, inside the quarantined +// from-space block the reporter named. +// +// Two protections, both exercised below: +// +// * a STRING LITERAL argument is `OperandProtection::Reload`. Its handle +// global is a registered root, so the string is never swept — but an +// evacuating cycle REWRITES that global, so the fix is to emit the load +// again below the collection point. No runtime call at all. +// * a LOCAL argument is `OperandProtection::Root`. Re-deriving it would +// observe an assignment made after the call-time value was taken, so it +// takes a real temp-root slot instead. +// +// Why the static checker reports nothing here: `gc_root_dominance_check.py` +// classifies a heap-value SOURCE as an `ALLOC_RE` call or a shadow-slot load. +// A load of a string-literal handle global is neither, so the register it +// defines is never tracked as a heap value and no stale use is attributed to +// it. That is the same shape of blind spot `js_implicit_this_set` (#7226) and +// `js_regexp_new` (#7227) each cost a round for. +// +// LIVE BY CONSTRUCTION. `churn` keeps allocating AFTER the back-edge poll that +// collects, so the abandoned from-space bytes are recycled before the callee +// reads them — a stale read returns wrong text instead of the right answer out +// of memory nobody has reused yet. Both arms compare against strings built +// from values re-read after the call, so a stale argument is observable. +// +// The literal arm needs the collection EARLY: a string literal is allocated by +// `__perry_init_strings_*` at startup, so it is young for the first couple of +// minors and tenured after that, and only a young object is evacuated. Under +// `PERRY_GC_ZEAL=1` the first back-edge poll inside `churn` already runs an +// evacuating minor, so iteration 0 is where the literal arm bites. The loop is +// short on purpose — zeal collects at every safepoint. + +import { joinArgs } from "./fixtures/gc_call_arg_rooting_pkg/callee.ts"; + +// Allocates hard, and keeps allocating after the poll that collects, so the +// retired bytes are reused rather than left intact. +function churn(n: number): number { + const bits: any[] = []; + for (let i = 0; i < 200; i++) { + bits.push({ i: i, s: "y" + i, pad: [i, i + 1, i + 2] }); + } + return bits.length === 200 ? n : -1; +} + +function freshUrl(i: number): string { + return "/v0/orgs/" + i + "/full-scans/[full_scan_id]"; +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 8; r++) { + // Reload arm: BOTH string operands are literals, so both are loads of a + // `__perry_init_strings_*` handle global — the registry's exact shape. + const litOut = joinArgs( + "/v0/orgs/[org_slug]/full-scans", + "GET", + { n: churn(r) }, + churn(r), + churn(r), + ); + if (litOut !== "/v0/orgs/[org_slug]/full-scans GET " + r + " " + r + " " + r) { + bad++; + } + // Root arm: argument 1 is a local holding a freshly-allocated string + // (always young, so it moves on every evacuating minor), argument 2 is a + // literal. One call, both protections. + const url = freshUrl(r); + const freshOut = joinArgs(url, "POST", { n: churn(r) }, churn(r), churn(r)); + if (freshOut !== url + " POST " + r + " " + r + " " + r) { + bad++; + } + } + return bad; +} + +console.log("bad", run());