diff --git a/CLAUDE.md b/CLAUDE.md index bb7812ec54f..14b4e42d029 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 00000000000..7643dfbae36 --- /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 9616a4225f2..cb77254c3ee 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 00bcaa3797e..3c9bed3de60 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 dba208d8eec..d2734e0a275 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 34a57dfd7ea..0682ec1ec97 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 f1f477345a8..145fc99522a 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 39f93ba0d83..3e863e000db 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 35197f21d47..91ef851377d 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 021dee0d538..e40467ddfd1 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 1e773466c55..cda3970325d 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 bba2da3eb08..eaa13d8023a 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 ba74e47e67a..dbea3049056 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 62ebb339453..66cecfd5983 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 4c52e8c0ea9..55bc9fda33e 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 eac25168c5f..9cfc5097e91 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 fba4bcbd842..07d826382c2 100644 --- a/scripts/gc_root_dominance_allowlist.json +++ b/scripts/gc_root_dominance_allowlist.json @@ -26,32 +26,19 @@ "Do not hand-write it from reading the IR; copy it out of the checker's own", "output (`-v` prints one per violation). A hand-derived entry that guesses", "program order matches nothing, and an entry that matches nothing is a", - "build failure." + "build failure.", + "", + "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 6c1d61e85d7..5c75ba8cb5f 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,68 @@ 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 + # `--min-funcs` is the BREADTH floor, and it used to be evaluated below + # the mode branches -- so `--stale-registers` and `--unrooted-allocas` + # both returned before reaching it and ran to a verdict over a corpus + # too thin to have exercised anything. A documented liveness control + # disarmed by a mode flag, inside the script whose job is to catch + # exactly that. The planted fixture has 2 functions; `--min-funcs 50` + # must be an error in every mode and `--min-funcs 2` must not be one in + # any of them, so the guard can be neither skipped nor always-on. + for mode in (["--stale-registers"], ["--unrooted-allocas"], []): + label = mode[0] if mode else "(bind-anchored default)" + if _main_probe(mode + ["--min-funcs", "50", "--min-binds", "1", + planted]) != 2: + print(f"self-test FAIL: {label} over a 2-function corpus must " + "exit 2 at --min-funcs 50. A mode that returns above the " + "breadth floor silently disables it.", file=sys.stderr) + ok = False + for mode, want in ((["--stale-registers"], 0), + (["--unrooted-allocas"], 0), + ([], 1)): + label = mode[0] if mode else "(bind-anchored default)" + if _main_probe(mode + ["--min-funcs", "2", "--min-binds", "1", + planted]) != want: + print(f"self-test FAIL: {label} over a corpus that MEETS " + f"--min-funcs must exit {want}, not be rejected by the " + "breadth floor. A guard that rejects every corpus 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 +2051,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)") # Same rule: --stale-registers returns before the --unrooted-allocas block, # so passing both would run the stale scan and silently skip the alloca one # while the command line claims both. @@ -1996,9 +2108,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 @@ -2008,6 +2117,53 @@ def main(): if BIND_RE.search(ins.text) ) + # `--min-funcs` is a corpus-BREADTH floor that belongs to every mode, so it + # is computed HERE, above the mode branches, and not below them. + # + # It used to be computed below. `--stale-registers` and `--unrooted-allocas` + # both return before that point, so `--stale-registers --min-funcs 1200` ran + # to a verdict over a two-function corpus and exited 0 -- a documented + # liveness control silently disarmed by a mode flag, in the very script whose + # job is to catch gates that cannot fail. Reproduced on the self-test + # fixture (2 functions): stale mode and alloca mode both exited 0 at + # `--min-funcs 50` while the default bind-anchored mode correctly exited 2. + # Both modes consult it before returning now, and `self_test()` carries a + # failing and a passing arm for each so it cannot regress silently. + n_funcs = sum(len(fs) for _m, fs in parsed) + + def funcs_floor_violated(): + if n_funcs >= ns.min_funcs: + return False + print(f"error: checked {n_funcs} function(s), need at least " + f"{ns.min_funcs}. The corpus compiled but is too thin to have " + "exercised the lowerings this invariant runs through.", + file=sys.stderr) + return True + + 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 + # Breadth, same as the bind-anchored path applies below. A corpus can + # carry plenty of root stores and still be one module deep. + if funcs_floor_violated(): + 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 @@ -2051,10 +2207,10 @@ def main(): print(f"error: {n_allocas} gc-capable alloca(s) in the corpus, need " f"at least {ns.min_binds}. Nothing was checked.", file=sys.stderr) return 2 + if funcs_floor_violated(): + return 2 return 1 if total else 0 - n_funcs = sum(len(fs) for _m, fs in parsed) - found = [] for mod, fs in parsed: for f in fs: @@ -2106,11 +2262,7 @@ def render(v): "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 - if n_funcs < ns.min_funcs: - print(f"error: checked {n_funcs} function(s), need at least " - f"{ns.min_funcs}. The corpus compiled but is too thin to have " - "exercised the lowerings this invariant runs through.", - file=sys.stderr) + if funcs_floor_violated(): return 2 # --- allowlist hygiene -------------------------------------------------- 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 00000000000..0d6c9978ac6 --- /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 00000000000..75d3275f5d4 --- /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());