fix(gc): resolve the memoized Array.prototype address across relocation (#6981) - #7071
Conversation
…on (#6981) `array::indexing` memoizes `Array.prototype`'s (and `Object.prototype`'s) heap address in a process-global `AtomicUsize`. That is a raw pointer to a MOVABLE object, and nothing maintained it. Two independent relocations leave the cache naming a `GC_FLAG_FORWARDED` stub: 1. `js_array_grow` — an indexed write past the dense capacity (`Array.prototype[300] = v`) reallocates the backing store and forwards the old head. No GC is involved at all. 2. the copying young-gen minor — it evacuates the prototype and forwards. Every reader of an array pointer resolves it through `clean_arr_ptr`, which follows forwarding chains. The cache did not, and the hole/OOB read fallback's self-recursion guard is the object-identity test `proto != receiver`. After a move those are two different addresses for the SAME object, the guard stops firing, and js_array_get_f64 ⇄ array_oob_prototype_get recurse without bound until the thread's stack guard page: `EXC_BAD_ACCESS` / `KERN_PROTECTION_FAILURE`, "Thread stack size exceeded due to excessive recursion", exit 139. It is a control-flow defect, not memory corruption — every dereference on the way down is to valid, mapped memory, which is why a 64 MB stack does not help and the evacuation verifier never fires. Three defences, matching the two ways the address goes stale: * `array_prototype_addr` / `object_prototype_addr` resolve the forwarding chain and heal the cache in place. This is what covers `js_array_grow`, which the collector never sees. The not-forwarded fast path stays call-free (`try_read_gc_header` is `#[inline(always)]`: two range compares plus one load of a permanently-hot `gc_flags` byte). * `scan_prototype_addr_cache_roots_mut` is registered in `gc_init`, so a relocating cycle REWRITES the slot like every other address-holding side table. Healing alone is not sufficient: once the from-space stub is swept and its block recycled the forwarded bit is gone. * `array_oob_prototype_get` resolves the receiver before the identity compare, so the guard is exact by construction rather than contingent on cache freshness. Regression coverage, sabotage-verified in both directions: * `crates/perry/tests/gc_array_prototype_hole_read_6981.rs` — two programs (grow-forwarding with no GC at all, and relocation under a heap budget) x 5 collector arms, byte-exact vs node 26.5.0. Against the unfixed runtime the grow program dies at the very first arm, with the collector switched off. * `gc::tests::runtime_roots::prototype_addr_cache` — 5 unit tests, one per defence plus registration and the unset sentinel. Removing the heal reddens 2; removing the registration/scanner body reddens 2. * `test-files/test_gap_array_proto_grow_hole_read.ts` — parity file. `scripts/gc_repsel_matrix.sh --arms all --pressure 8` over 440 cells goes from `PASS=325 UNVER=100 XFAIL=1 FAIL=14` to `PASS=339 UNVER=100 XFAIL=1 FAIL=0`; byte-exact vs node 26.5.0 425/440 -> 439/440. All 14 FAILs were `test_gap_repsel_p4a3_numarray_barriers`, in exactly the 14 arms that run a copying minor. `evac_minor` and `force_verify` go back into `PR_ARMS`, as that script's own comment instructed once #6981 closed.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughArray and object prototype address caches now heal through forwarding chains and are rewritten by relocating GC. The recursion guard resolves receivers before comparison, with runtime-root and end-to-end tests covering growth, relocation, forwarding chains, and GC configurations. ChangesPrototype Cache Relocation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Closes #6981.
The recursion frame, symbolised
PERRY_DEBUG_SYMBOLS=1+ the OS crash report +atosgives the frame the issue was missing:EXC_BAD_ACCESS/KERN_PROTECTION_FAILURE, "Thread stack size exceeded due to excessive recursion". So the earlier retraction was right: this is not memory corruption. Every dereference on the way down is to valid, mapped memory. It is a two-function cycle that never terminates.Mechanism
array::indexingmemoizesArray.prototype's heap address in a process-globalAtomicUsize(ARRAY_PROTO_ADDR;Object.prototypehas the twin). That is a raw pointer to a movable object, and nothing maintained it.Every reader of an array pointer resolves it through
clean_arr_ptr, which followsGC_FLAG_FORWARDEDchains. The cache did not. And the hole/OOB read fallback's self-recursion guard is an object-identity test:Once
Array.prototypemoves,protoandreceiverare two different addresses for the same object. The guard stops firing, and:That is the unbounded recursion. It also explains the three necessary conditions exactly: the index write is what sets
ARRAY_PROTO_HAS_INDEXand arms the fallback (naming the prototype does not); the hole read is what enters the fallback; relocation is what splits the identity.It was never only a GC bug
Array.prototyperelocates two ways, and only one involves the collector:js_array_grow— indexed write past the dense capacity reallocates and forwards the old headArray.prototype[300] = 555PERRY_GEN_GC=0PERRY_GC_HEAP_LIMIT=8The second is what #6981 was measuring. The first is a GC-free reproducer of the same defect on
main, found while reducing this one, and it is the stronger regression test because no collector arm is required to trip it.Minimal reproducers
GC-free (exit 139 on
main, no env at all):Relocation (exit 139 on
mainunder the pressure knob; correct without it):Both are in the acceptance suite verbatim; both are byte-exact against the pinned Node 26.5.0 oracle.
Fix
Three defences, one per way the address can go stale:
array_prototype_addr/object_prototype_addrheal the cache through the forwarding chain and store the healed address back. This is what coversjs_array_grow, which the collector never sees. The not-forwarded path stays call-free:try_read_gc_headeris#[inline(always)]and reduces to two range compares plus one load of a permanently-hotgc_flagsbyte, sonote_array_index_write's per-write probe keeps its shape.scan_prototype_addr_cache_roots_mutis registered ingc_init, so a relocating cycle rewrites the slot like every other address-holding side table (CLASS_PROTOTYPE_OBJECTS,TYPED_ARRAY_VIEW_META, …). Healing alone is not sound here: once the from-space stub is swept and its block recycled, the forwarded bit is gone and the cache would name an unrelated live object.array_oob_prototype_getresolves the receiver before the identity compare, so the guard is exact by construction instead of contingent on cache freshness.resolve_forwardingalready existed invalue/equality.rs— its own doc says it exists "so callers can compare resolved addresses for object identity", which is precisely the invariant this site was violating. It is promoted topub(crate)and reused rather than duplicated.Teeth — sabotage-verified in both directions
crates/perry/tests/gc_array_prototype_hole_read_6981.rs— both programs above x 5 collector arms (default,heap_limit_8,precise_roots,force_evacuate,gen_gc_off), each byte-exact vs Node 26.5.0. The suite clears the wholePERRY_GC_*family before each arm so an exported kill switch cannot turn it into a control.gc::tests::runtime_roots::prototype_addr_cache— 5 unit tests,cargo test --lib -p perry-runtimevisible (the per-PR gate), one per defence plus scanner registration and the unset sentinel.return cached)prototype_addr_reads_through_a_forwarding_stub+..._multi_hop_...FAIL, other 3 passgc_initregistration + empty the scanner body..._is_rewritten_by_the_collector+..._scanner_is_registeredFAIL, other 3 passperryand the-staticwrappers[grow/default]and[relocate/heap_limit_8]die by signalThe last row is the one that matters and it caught me first time round:
cargo test -p perrydoes not rebuildlibperry_runtime.a(the-staticwrappers do), so the initial sabotage run linked the fixed archive and passed vacuously. Re-run aftercargo build --release -p perry -p perry-runtime -p perry-stdlib -p perry-runtime-static -p perry-stdlib-static, it goes red. Recorded here because the trap is easy to repeat.Also added
test-files/test_gap_array_proto_grow_hole_read.ts(parity file, byte-exact with and without the pressure knob).Matrix delta
scripts/gc_repsel_matrix.sh --arms all --pressure 8, release, macOS arm64, pinned Node 26.5.0, isolated worktree + isolatedCARGO_TARGET_DIR, full-p perry -p perry-runtime -p perry-stdlib -p perry-runtime-static -p perry-stdlib-staticbuild set. Denominator unchanged: 22 rows x 20 arms = 440 cells.+14 PASS / -14 FAIL, nothing regressed.
UNVERis bit-for-bit unchanged at 100 (19 =p4a3_numarray_growth/ #7016, 81 = the 9 loop-free rows that went inert per #7059) and the singleXFAILis still the pre-existingrepsel_ptr_shape_locals x rep_ptr_shape_off(#6976).All 14 FAILs were
test_gap_repsel_p4a3_numarray_barriers, in exactly the 14 arms with a non-zerocopy-minorcount — verified per the #7040 distinction ([gc-copy-minor] ran copied_objects=), never the summedmoved. That row is now PASS in all 20 arms, and the arm-liveness table is unchanged:evac_minor/force_evac/force_verify/loop_polls/ the sixrep_*_offarms atcopy-minor 21/22,default/verify_evac/cons_scan_off/cons_scan_off_forceat 12/22, the fivecollectarms at 0/22 by construction.evac_minorandforce_verifyare put back intoPR_ARMS, which is what that script's own comment instructed once #6981 closed. Per-PR now carries the relocating-minor arms.Full
RUST_TEST_THREADS=1 cargo test --lib -p perry-runtime: 1531 passed, 0 failed.What I could not verify
array_has_own_index(stub, index)to answer true once, and I did not instrument which bytes make it do so (dead payload vs a recycled block). It does not change the fix — the fix removes the address split that lets the cycle exist at all — but the entry condition is inferred, not measured.Object.prototype's half is fixed symmetrically but has no observed reproducer.OBJECT_PROTO_ADDRhas the identical shape and the identical consumers (note_object_prototype_index_write,object_prototype_addr_matches), so it is fixed alongside; the unit tests cover it, no end-to-end program does.note_array_index_writecompares the live receiver against the cache, so before this change a firstArray.prototypeindex write occurring after a relocation would not setARRAY_PROTO_HAS_INDEXat all — holes would readundefinedinstead of the inherited value. That is a wrong answer rather than a crash; I did not build a reproducer for it.Summary by CodeRabbit
Bug Fixes
Array.prototyperelocation.Tests