fix(codegen): #5459 — store array head back to module-global GC root on push from a closure

[proggeramlug]

Fixes #5459.

Problem

arr.push(...) where arr is a module-level global accessed from inside a nested function (closure/IIFE) caused a use-after-free / SIGSEGV under allocation churn:

declare function gc(): void;
const strong: any[] = [];
(function setup() { for (let n = 0; n < 50; n++) strong.push({ id: n }); })();  // populate from a fn
for (let c = 0; c < 12; c++) { churn(80000); gc(); }
// strong.length reads garbage; strong[n] faults → SIGSEGV (deterministic)

Right after setup() the array is correct (length 50); the first gc() corrupts it. The same population done at module scope is fine — the only delta is the IIFE.

Root cause

In crates/perry-codegen/src/expr/array_push.rs, the boxed_vars write-back branch returned early after handling the captured-box / local-box cases:

if ctx.boxed_vars.contains(array_id) {
    if let Some(..) = ctx.closure_captures.get(array_id) { /* box_set */ }
    else if let Some(..) = ctx.locals.get(array_id) { /* box_set */ }
    return Ok(...);   // ❌ returns even when NEITHER arm matched
}

A module-level global that lands in boxed_vars (because a nested function references it) but has no box location in the callee context falls through both inner arms — and the unconditional return then skips the realloc write-back entirely. The load path read the global directly (load @perry_global_…), but the relocated head from js_array_push_f64 was never stored back. Confirmed in the emitted IR: the closure computes new_box and then drops it (no store, no js_write_barrier_root_nanbox), whereas the module-scope path emits the store-back.

So under GC evacuation the array head moves, the registered GC-root global slot keeps the stale (freed) pointer, and reads dangle.

Fix

Only return early when a box location was actually written; otherwise fall through to the existing module-global store-back (emit_root_nanbox_store_on_block, which stores to @global + emits the root write barrier). Applied to both ArrayPush and ArrayPushSpread.

Notes

• Generational-GC-independent — reproduces with PERRY_GEN_GC=0 (full mark-sweep) too; it's a codegen root-write bug, not a GC-policy bug.
• Surfaced while investigating WeakMap/WeakSet weakness (globals: make WeakRef and FinalizationRegistry actually weak #2656) but independent: plain WeakRef / FinalizationRegistry with live targets held in a module-global array hit the same UAF (FinReg lost 18/50 live registered targets / crashed; now 50/50).

Verification

• New regression test crates/perry/tests/issue_5459_module_global_array_push_in_closure.rs — direct push and spread push into a module-global from an IIFE survive 12× churn+gc (pre-fix: SIGSEGV / garbage length). Both green.
• 30 array/closure/GC test_gap_* files pass; existing issue_2656 weakref/finalization, issue_5139 arraylike-dispatch, and issue_5195 tests unaffected.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Fixed an issue where module-level arrays updated inside closures (including both push and spread-based push) could lose or corrupt elements after allocation churn and garbage collection. Arrays now reliably preserve length and contents after these operations.
• Tests
  • Added regression coverage for module-global array push and spread-push invoked from an IIFE, validating correct behavior across repeated allocation/gc() cycles.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 46f8cdab-abf6-446b-a554-a4d3c6e56172

📥 Commits

Reviewing files that changed from the base of the PR and between f56c44d and 8c61716.

📒 Files selected for processing (2)

• crates/perry-codegen/src/expr/array_push.rs
• crates/perry/tests/issue_5459_module_global_array_push_in_closure.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• crates/perry-codegen/src/expr/array_push.rs
• crates/perry/tests/issue_5459_module_global_array_push_in_closure.rs

---

📝 Walkthrough

Walkthrough

Fixes a GC use-after-free (issue #5459) in array_push.rs by restructuring the boxed-var control flow so that when a module-global array is pushed to from inside a nested function, the relocated array header is correctly written back to the GC-root slot. Two regression tests covering regular and spread-push are added.

Changes

GC Root Store-Back Fix for Module-Global Array Push

Layer / File(s)	Summary
ArrayPush / ArrayPushSpread boxed-var fallthrough fix crates/perry-codegen/src/expr/array_push.rs	In both Expr::ArrayPush and Expr::ArrayPushSpread, the boxed-var branch is restructured: the js_box_set path for a locally-boxed slot now returns immediately, while the case where array_id is in boxed_vars but has no local box location no longer returns early—allowing execution to fall through to the module-global store-back that updates the registered GC-root slot. Both cases are annotated with #5459 comments.
Regression tests for issue #5459 crates/perry/tests/issue_5459_module_global_array_push_in_closure.rs	Adds two integration tests: one for regular push and one for spread-push (push(...xs)) into a module-global array from an IIFE, each verifying the array survives repeated churn()+gc() rounds without crashing or losing elements. Shared utilities perry_bin() and compile_and_run() locate and drive the perry binary.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• #5202: Addresses SIGSEGV crashes in function declarations accessing module-level object arrays from closures; while both this PR and #5202 involve module-level state accessed from nested functions, they target distinct code paths and failure modes rather than the same implementation sites.

Poem

🐰 A box had a flaw, a gap in the root,
The GC swept by and ate all the loot!
Now fallthrough leads home, the store-back complete,
Arrays pushed in closures no longer delete.
Fifty survivors, all safe in their slot —
The rabbit fixed memory, crash: there is not! 🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main code change: fixing a bug where array push from a closure doesn't properly store the array head back to the module-global GC root, referencing the issue number #5459.
Description check	✅ Passed	The PR description comprehensively covers the problem, root cause, fix, and verification with concrete examples and references to the issue. All key aspects are well-documented following the repository's conventions.
Linked Issues check	✅ Passed	The changes directly address all coding requirements from issue #5459: the codegen fix in array_push.rs ensures module-global arrays are properly stored back when pushed from closures, and the regression test verifies both direct and spread push operations work correctly under GC churn.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing issue #5459: modifications to the array push codegen logic and addition of a targeted regression test. No unrelated refactoring or feature additions are present.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 fix/5459-module-global-array-push-uaf

---

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

[coderabbitai]

🧹 Nitpick comments (1)

crates/perry/tests/issue_5459_module_global_array_push_in_closure.rs (1)

24-45: 💤 Low value

Consider capturing stderr for better crash diagnostics.

When the compiled binary crashes (e.g., SIGSEGV), diagnostic information may appear in stderr rather than stdout. Returning stderr alongside stdout would improve debugging when assertions fail.

💡 Optional improvement

-fn compile_and_run(dir: &std::path::Path, entry: &std::path::Path) -> (bool, String) {
+fn compile_and_run(dir: &std::path::Path, entry: &std::path::Path) -> (bool, String, String) {
     let output = dir.join("main_bin");
     let compile = Command::new(perry_bin())
         .current_dir(dir)
         .arg("compile")
         .arg(entry)
         .arg("-o")
         .arg(&output)
         .output()
         .expect("run perry compile");
     assert!(
         compile.status.success(),
         "perry compile failed\nstdout:\n{}\nstderr:\n{}",
         String::from_utf8_lossy(&compile.stdout),
         String::from_utf8_lossy(&compile.stderr)
     );
     let run = Command::new(&output).output().expect("run compiled binary");
     (
         run.status.success(),
         String::from_utf8_lossy(&run.stdout).to_string(),
+        String::from_utf8_lossy(&run.stderr).to_string(),
     )
 }

Then update call sites to include stderr in assertion messages:

let (ok, stdout, stderr) = compile_and_run(dir.path(), &entry);
assert!(ok, "binary crashed\nstdout:\n{stdout}\nstderr:\n{stderr}");

🤖 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/tests/issue_5459_module_global_array_push_in_closure.rs` around
lines 24 - 45, The compile_and_run function currently only captures and returns
stdout from the executed binary, but stderr contains critical diagnostic
information when the binary crashes. Modify the return type of compile_and_run
to return a tuple of three elements (bool, String, String) instead of two, where
the third element is stderr. Update the return statement to capture stderr from
the run command output using run.stderr and convert it to a string, then include
it in the tuple alongside the existing stdout return value. Finally, update all
call sites of compile_and_run to unpack the three return values and include the
stderr output in assertion error messages for better crash diagnostics.

🤖 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/tests/issue_5459_module_global_array_push_in_closure.rs`:
- Around line 24-45: The compile_and_run function currently only captures and
returns stdout from the executed binary, but stderr contains critical diagnostic
information when the binary crashes. Modify the return type of compile_and_run
to return a tuple of three elements (bool, String, String) instead of two, where
the third element is stderr. Update the return statement to capture stderr from
the run command output using run.stderr and convert it to a string, then include
it in the tuple alongside the existing stdout return value. Finally, update all
call sites of compile_and_run to unpack the three return values and include the
stderr output in assertion error messages for better crash diagnostics.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a9222876-6818-4d98-981c-b2abed3fe724

📥 Commits

Reviewing files that changed from the base of the PR and between f30a2f5 and f56c44d.

📒 Files selected for processing (2)

• crates/perry-codegen/src/expr/array_push.rs
• crates/perry/tests/issue_5459_module_global_array_push_in_closure.rs