test(gc): make the POINTER_FREE misdeclaration hazard detectable (#7635) - #7643
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds copying-GC regression tests for deferred JSON record layout finalization. It verifies pointer-free and pointer-bearing layouts, child-slot enumeration, relocation, and sabotage detection. It also documents verification limits and updates the workspace version. ChangesDeferred JSON GC layout validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
#7635 sabotaged `layout_finish_deferred_boxed_object(ptr, saw_pointer)` to `(ptr, false)` -- every JSON-parsed record claiming POINTER_FREE while holding heap strings -- and got byte-identical correct output under PERRY_GC_ZEAL + PERRY_GC_PROTECT_FROMSPACE and under PERRY_GC_FORCE_EVACUATE, with copying minors and retired quarantine sets observed live. Those knobs do not discriminate this hazard. Four tests that do, sabotage-verified in both directions: the child-slot enumerator (deterministic, no GC timing), relocation across a copying minor, the finalize's two exact outcomes, and the same invariant driven through the real js_json_parse entry point so the json/parser.rs call site is covered rather than only the helper. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
#7635's audit forced every JSON-parsed record to POINTER_FREE while it held heap strings and got byte-identical correct output under zeal + from-space protect and under force-evacuate. The instruments were not at fault: js_json_parse is LAZY for 1 KB-16 MB top-level arrays (json_tape), so parse_object runs when an element is first READ, and the probe read its records only after the last collection. A traced-object audit on the sabotaged build saw zero POINTER_FREE objects with pointer-bearing payload words -- nothing was stranded because nothing was there. Under PERRY_JSON_TAPE=0 the same sabotage SIGSEGVs, PERRY_GC_FROMSPACE_SCAN reports dangling=8000 owners=4000 (exactly 4000 records x 2 fields, vs 0 clean), and PERRY_GC_PROTECT_FROMSPACE names the faulting address. With the records merely touched before the churn, the default lazy path reads back 7,872 of 8,000 values wrong. Written onto the constant so the next PR in this family does not cite "clean under zeal + protect" as evidence without first showing the subject existed during a collection, and prefers the layout-independent PERRY_GC_FROMSPACE_SCAN. PERRY_GC_VERIFY_EVACUATION is genuinely blind here -- it walks the same enumeration the rewrite pass walks. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/gc/tests/copying/deferred_finalize_7635.rs`:
- Around line 114-132: Update materialise_record to root obj immediately after
js_object_alloc, keep that temporary root active while fresh_string allocates
each child, and reload the record’s rewritten address from the root before every
store_object_field_slot_layout_deferred call. Clear the temporary root after
layout_finish_deferred_boxed_object completes, preserving the existing
finalization behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: edd549ce-6606-4f6c-854b-2dca142b9b46
📒 Files selected for processing (5)
changelog.d/7633-json-materialiser-layout-deferred.mdchangelog.d/7643-pointer-free-misdeclaration-detectable.mdcrates/perry-runtime/src/gc/layout.rscrates/perry-runtime/src/gc/tests/copying.rscrates/perry-runtime/src/gc/tests/copying/deferred_finalize_7635.rs
| unsafe fn materialise_record(honest_finalize: bool) -> *mut ObjectHeader { | ||
| let obj = crate::object::js_object_alloc(0, FIELD_VALUES.len() as u32); | ||
| assert_eq!( | ||
| layout_state_of(obj as usize), | ||
| GC_LAYOUT_POINTER_FREE, | ||
| "test premise: a fresh record is born POINTER_FREE, which is why the \ | ||
| finalize is the only thing that can move it off that state" | ||
| ); | ||
| let mut saw_pointer = false; | ||
| for (index, bytes) in FIELD_VALUES.iter().enumerate() { | ||
| let child = fresh_string(bytes); | ||
| saw_pointer |= | ||
| crate::object::store_object_field_slot_layout_deferred(obj, index, string_bits(child)); | ||
| } | ||
| assert!( | ||
| saw_pointer, | ||
| "test premise: storing heap strings must be reported as pointer-bearing" | ||
| ); | ||
| layout_finish_deferred_boxed_object(obj as usize, saw_pointer && honest_finalize); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Root obj before allocating field strings.
Line 115 creates obj, but line 124 can allocate and collect before the next store. obj exists only as a raw Rust pointer during that collection point. A copying collection can reclaim or move the record, and the later store can dereference a stale pointer.
Root the record immediately after allocation. Reload its rewritten address after each allocation before storing the next field. Clear the temporary root after finalization.
As per coding guidelines, “GC-managed values must remain correctly rooted across collection points.” Based on learnings, raw Rust pointer locals are neither GC roots nor reliable pins.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/gc/tests/copying/deferred_finalize_7635.rs` around
lines 114 - 132, Update materialise_record to root obj immediately after
js_object_alloc, keep that temporary root active while fresh_string allocates
each child, and reload the record’s rewritten address from the root before every
store_object_field_slot_layout_deferred call. Clear the temporary root after
layout_finish_deferred_boxed_object completes, preserving the existing
finalization behavior.
Sources: Coding guidelines, Learnings
0423a6b to
f5875c5
Compare
Audit — merged as v0.5.1365I reproduced the decisive claim independently before reading the tests, and it corrects my finding, not the instruments'. Reproduced, sabotaging
Sabotage-verified in both directions, independently of your run:
What makes these tests worth having: they are workload-free, so no lazy path, GC-that-didn't-run, or conservative-scan residue can defeat them; and both positive tests assert Gates: I enumerated the Two things I'm keeping from this beyond the tests. The Your suggested follow-up — |
1 participant
Closes #7635.
The question
#7635 sabotaged
layout_finish_deferred_boxed_object(ptr, saw_pointer)to(ptr, false)— every JSON-parsed record claimingPOINTER_FREEwhile holdingheap strings, the stranded-live-child hazard the layout state exists to prevent
— and got byte-identical correct output under
PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1and underPERRY_GC_FORCE_EVACUATE=1, withcopying minors and retired quarantine sets observed live. Is the hazard
detectable at all?
Answer: yes, and the instruments were never at fault
js_json_parseroutes a top-level array of 1 KB–16 MB through the lazy tape(
json_tape, default since #179).parse_object— the function carrying thesabotage — therefore does not run at
JSON.parsetime; it runs when an elementis first read. The probe read its records only after the churn, so every
misdeclared record was materialised after the last collection.
Confirmed rather than inferred: a temporary audit in
heap_payload_slot_selectionon the sabotaged build counted zero objects inPOINTER_FREEstate with pointer-bearing payload words ever handed to thecollector, across the whole run. Nothing was stranded because nothing was there.
This is CLAUDE.md's hazard 4 — the gate ran but its subject never did — applied
to a probe rather than a job.
Re-run so the misdeclared records actually live across a collection (sabotaged
vs clean runtime,
perry-dev, macOS arm64,PERRY_NO_AUTO_OPTIMIZE=1):dangling=0PERRY_JSON_TAPE=0, read after churndangling=8000 owners=4000on the first scanned cycle (vsdangling=0clean);PERRY_GC_PROTECT_FROMSPACE=1printsFAULT: signal 10 at 0x…dangling=8000 owners=4000is exactly 4,000 records × 2 pointer fields.One instrument is structurally blind and it is worth naming:
PERRY_GC_VERIFY_EVACUATIONwalks the same enumeration the rewrite pass walks,i.e. it asks this very layout state which slots exist.
PERRY_GC_FROMSPACE_SCAN=1is the layout-independent one — a whole-payloadword scan consulting no root enumeration and no layout state — and it is the
knob to reach for on this hazard class.
What ships
crates/perry-runtime/src/gc/tests/copying/deferred_finalize_7635.rs— fourtests that need no workload at all, so they cannot be defeated by a lazy path, a
GC that did not happen to run, or conservative-scan residue:
finalize_settles_pointer_free_or_unknown_and_nothing_else— the finalize'stwo exact outcomes (no pointer ⟹ keep the birth state; any pointer ⟹
GC_LAYOUT_UNKNOWN, no mask, no stranded descriptor).a_materialised_record_keeps_its_children_traced_and_rewritten_7635— thematerialiser's construction loop byte for byte; the collector must enumerate
every field, and a copying minor must relocate both children and rewrite both
slots. Gated on
copied_objects >= 3so a cycle that moved nothing cannotpass.
json_parse_record_keeps_its_string_values_traced_and_rewritten_7635— thesame invariant through the real
js_json_parseentry point, so thefinalize call site in
json/parser.rsis covered and not merely the helper.gc: forcing POINTER_FREE on a pointer-bearing object strands nothing — our zeal/protect instruments do not discriminate the layout-state hazard #7635's sabotage was applied at that call site and tests 1–2 stay green
through it.
a_misdeclared_pointer_free_record_strands_its_child— the sabotage arm,made permanent: the identical construction with
saw_pointerforced tofalse, asserting the collector enumerates ZERO slots and that correcting thestate alone makes the same record fully enumerable. Asserted on the
enumerator, so it leaves no stale pointer behind.
Sabotage-verified in both directions, locally:
json/parser.rs→(js_obj as usize, false)(gc: forcing POINTER_FREE on a pointer-bearing object strands nothing — our zeal/protect instruments do not discriminate the layout-state hazard #7635's exact mutation): test 3fails at the state assertion, and with that assertion neutered it fails again
at the enumerator assertion.
layout_finish_deferred_boxed_objectitself made a no-op: all four fail.The doc comment on
GC_LAYOUT_POINTER_FREEnow records what can and cannotverify a claim about this state, so the next PR in this family does not cite
"clean under zeal + from-space protect" as evidence without first showing the
subject existed during a collection.
No GC behaviour changes; no new knobs; no codegen changes.
Validation
cargo test -p perry-runtime --lib --no-fail-fast(perry-dev)cargo check --all-targetscargo fmt --all -- --checklintscript set from.github/workflows/test.yml:workspace_architecture(+self-test),check_file_size.sh,binding_pins,gc_store_site_inventory(+self-test),addr_class_inventory(+self-test),class_id_collisions,raw_handle_debt(+self-test),gap_snapshotself-test,
parity_known_failures(+self-test, --audit),gc_gate_wiring_check(+self-test),gc_matrix_liveness_check(+self-test,--check-registry),
check_test_registration(+self-test) — all exit 0.Root-dominance is not re-run: nothing under
perry-codegenchanged.Follow-up worth considering (not in this PR)
The end-to-end shape is now a known-good detector:
PERRY_JSON_TAPE=0+PERRY_GC_FROMSPACE_SCAN=1over a parse-then-churn workload, assertingdangling=0and that a copying minor actually ran. That would make a realgate for the whole layout-state family. Per CLAUDE.md a new gate must be run
once and then promoted to required, which is a separate change.
Summary by CodeRabbit
Bug Fixes
Documentation
Chores