fix(gc): initialize keys_array before clone_with_extra's collecting allocation (#7683)

[proggeramlug]

Fixed

• js_object_clone_with_extra published a half-built object whose keys_array slot held recycled heap bytes (gc: perry-runtime lib tests SIGSEGV in typed_feedback::object_shape via js_object_clone_with_extra (1 in ~102 full-suite runs, no test reports FAILED) #7683). Both branches initialise object_type, class_id, parent_class_id, field_count and meta immediately after allocation, then set keys_array only at the end via set_object_keys_array. Between those two points sits crate::array::js_array_alloc.

  That call allocates, so it can collect — and the collector reads exactly this slot as a child edge (object::gc_keys_array_slot, enumerated by gc_child_slots). A collection landing in that window scans a pointer the mutator never wrote.

  The bytes are not zero. arena_alloc_gc_old's fast path deliberately reuses a swept, non-zeroed hole (GC: old-gen fragmentation — scattered survivors pin 105 MB of blocks for a ~1 MB live set #7437: "reuse a swept same-size hole … otherwise a block with any live object never yields its dead bytes back"), so the slot holds real leftover heap content from whatever last occupied it. Whether that content happens to look like a plausible-but-unmapped address depends on allocation history — which is the shape of the ~1-in-102 typed_feedback::object_shape SIGSEGV reported in gc: perry-runtime lib tests SIGSEGV in typed_feedback::object_shape via js_object_clone_with_extra (1 in ~102 full-suite runs, no test reports FAILED) #7683.

  Every sibling allocator in object/alloc.rs already nulls the slot at this point. This function was the one that did not.

  On the test, and why it checks source rather than behaviour. The runtime version was written first: force a collection into the window (force_next_general_arena_alloc_slow + GC_OLD_RECLAIM_PENDING, the levers GC: the two lazy intrinsic-tower builders need #7217's no-move window, and need a gate that can fail first #7251 established), then assert the published clone's keys_array is sane. It passed with the fix deleted. Two independent reasons: by the time the function returns, set_object_keys_array has written the slot correctly, so nothing observable survives the window; and a fresh arena block is zeroed, so even inside the window the garbage reads as null unless the allocation lands in a recycled old-space hole with the right history.

  Reproducing it in-suite therefore needs a specific swept-hole layout and a collection landing in a few-instruction window. That is exactly why the fix is a by-construction initialisation rather than a guard, and why the guard asserts the invariant where it is decidable — the source, in the style of scripts/gc_pin_sites.py's custody check for GC_FLAG_PINNED. Removing either initialisation fails the test.

  One note for anyone writing a similar source-level check: the first version matched the phrase arena_alloc_gc inside its own explanatory comment, registering a third allocation site and failing against correct code. It now strips comments before scanning. A source check that reads its own documentation as code is worse than no check.

Summary by CodeRabbit

• Bug Fixes

  • Improved object cloning reliability by preventing stale memory references from being observed during garbage collection.
  • Reduced the risk of crashes or corrupted data when cloning objects that include additional properties.

• Tests

  • Added regression coverage to verify safe initialization across relevant cloning and allocation scenarios.

• Documentation

  • Added release notes describing the cloning fix and its validation.

• Chores

  • Updated the application version to 0.5.1423.

[coderabbitai]

📝 Walkthrough

Walkthrough

The clone allocation paths now initialize keys_array before allocations that may trigger garbage collection. A source-level regression test checks this ordering across branches. Project version and changelog entries were updated.

Changes

Clone keys array GC safety

Layer / File(s)	Summary
Initialize clone keys before allocation crates/perry-runtime/src/object/alloc.rs	Both js_object_clone_with_extra clone paths clear keys_array before allocating the replacement keys array.
Validate and document initialization ordering crates/perry-runtime/src/gc/tests/clone_keys_array_init.rs, crates/perry-runtime/src/gc/tests/mod.rs, changelog.d/7726-clone-keys-array-init.md, Cargo.toml, CLAUDE.md	A source-level regression test checks initialization before each relevant allocation. The test module is registered, and version and changelog entries are updated.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• PerryTS/perry#7391: Addresses GC rooting and initialization during allocation-related runtime object construction.
• PerryTS/perry#7495: Addresses GC safety by initializing references before allocation-triggered collection in another runtime path.

Suggested labels: bug

Suggested reviewers: thehypnoo, andrewtdiz

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description explains the fix in detail but omits the required Summary, Changes, Related issue, Test plan, and Checklist sections.	Use the repository template and add the missing sections, including explicit test results, related issue status, and checklist confirmations.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes initializing keys_array before the allocation that can trigger garbage collection.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch gc/7683-clone-keys-array-init

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[proggeramlug]

Merging as v0.5.1423

js_object_clone_with_extra initialises object_type, class_id, parent_class_id, field_count and meta right after allocation, then sets keys_array only at the end via set_object_keys_array. Between those two points sits crate::array::js_array_alloc — which allocates, so it can collect, and the collector reads exactly that slot as a child edge (object::gc_keys_array_slot).

The bytes are not zero: arena_alloc_gc_old's fast path deliberately reuses a swept, non-zeroed hole (#7437), so the slot holds real leftover heap content. Whether it looks like a plausible-but-unmapped address depends on allocation history — the shape of the ~1-in-102 typed_feedback::object_shape SIGSEGV in #7683. Every sibling allocator in the file already nulls it here; this function was the one that didn't.

My first test was vacuous, and I'm recording that rather than quietly replacing it

I wrote the runtime version first: force a collection into the window (force_next_general_arena_alloc_slow + GC_OLD_RECLAIM_PENDING, #7251's levers), then assert the published clone's keys_array is sane. It passed with the fix deleted. Two independent reasons — by the time the function returns set_object_keys_array has written the slot correctly, so nothing observable survives the window; and a fresh arena block is zeroed, so even inside the window the garbage reads as null unless the allocation lands in a recycled old-space hole with the right history.

Reproducing it in-suite therefore needs a specific swept-hole layout and a collection landing in a few-instruction window. That is exactly why the fix is a by-construction initialisation rather than a guard, and why the guard asserts the invariant where it is decidable — the source, in the style of scripts/gc_pin_sites.py's GC_FLAG_PINNED custody check. Removing either initialisation fails it:

#7683: an allocation site in js_object_clone_with_extra has no
`keys_array = ptr::null_mut()` before its `js_array_alloc`.
test result: FAILED

And one more self-inflicted lesson worth leaving in the file: the first version of that source check matched the phrase arena_alloc_gc inside its own explanatory comment, registering a third allocation site and failing against correct code. It strips comments before scanning now. A source check that reads its own documentation as code is worse than no check.

Credit

The root cause came from an agent's static analysis after a 150-run soak found nothing — and it was right to call that soak inconclusive rather than clean: at a 1-in-102 rate, seeing zero crashes in 150 runs has a ~23% probability by luck, so N≈300–450 would be needed to conclude anything. Fixing the mechanism it found beats soaking harder.

Gates 21/21 (now including perry-runtime --lib and perry-codegen --lib, which my gate previously did not run).

[coderabbitai]

🧹 Nitpick comments (1)

crates/perry-runtime/src/object/alloc.rs (1)

757-766: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicate GC_STORE_AUDIT annotation line.

Line 765 repeats the same "GC_STORE_AUDIT(INIT): freshly allocated clone starts with no keys-array edge" statement that lines 757-758 already give in full. Line 807 repeats lines 799-800 the same way. Remove the short duplicate line in each branch and keep only the full explanation.

Also applies to: 799-808

🤖 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/object/alloc.rs` around lines 757 - 766, Remove the
duplicate short GC_STORE_AUDIT annotation immediately before
(*new_ptr).keys_array = ptr::null_mut() in both affected allocator branches,
including the branch around the corresponding second initialization. Preserve
the full explanatory comment above each assignment and leave the initialization
logic unchanged.

🤖 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.

Nitpick comments:
In `@crates/perry-runtime/src/object/alloc.rs`:
- Around line 757-766: Remove the duplicate short GC_STORE_AUDIT annotation
immediately before (*new_ptr).keys_array = ptr::null_mut() in both affected
allocator branches, including the branch around the corresponding second
initialization. Preserve the full explanatory comment above each assignment and
leave the initialization logic unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b4e0ef1c-f0fd-495f-ad2f-2d21d9d8eb53

📥 Commits

Reviewing files that changed from the base of the PR and between 73d1e1d and 7c65e77.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (6)

• CLAUDE.md
• Cargo.toml
• changelog.d/7726-clone-keys-array-init.md
• crates/perry-runtime/src/gc/tests/clone_keys_array_init.rs
• crates/perry-runtime/src/gc/tests/mod.rs
• crates/perry-runtime/src/object/alloc.rs