Skip to content

v0.0.133 — Iterative array builders no longer leak closure return-value root (closes #570)#574

Merged
aallan merged 3 commits into
mainfrom
claude/fix-570-array-map-shadow-stack
May 6, 2026
Merged

v0.0.133 — Iterative array builders no longer leak closure return-value root (closes #570)#574
aallan merged 3 commits into
mainfrom
claude/fix-570-array-map-shadow-stack

Conversation

@aallan

@aallan aallan commented May 5, 2026

Copy link
Copy Markdown
Owner

Summary

  • Iterative array builders (array_map, array_mapi, array_fold, array_sort_by) no longer leak the closure return-value root onto the GC shadow stack. Pre-fix: a 5 000-element array_map<_, ADT> trapped at iteration ~4 000.
  • Fix is per-callsite in vera/wasm/calls_arrays.py: each builder emits a 4-byte $gc_sp decrement immediately after consuming the closure's heap-pointer return. array_fold additionally moves the unwind ahead of its gc_sp - 8 overwrite math (the math was assuming zero post-call slots; with the leak it drifted onto leaked slots from prior iterations — second symptom of the same bug class, incidentally closed).
  • Bool-returning predicates (filter / find / any / all) and callback-less builders (flatten / reverse / range) are unaffected — Bool is excluded from the closure's ret_is_pointer flag in vera/codegen/closures.py, so no post-restore root push happens.

Closes

Closes #570.

Test plan

  • pytest tests/ -v — 3,706 passed, 14 skipped
  • mypy vera/ — clean
  • python scripts/check_conformance.py — 82 / 82
  • python scripts/check_examples.py — 33 / 33
  • pytest tests/test_browser.py — 99 / 99 (Python ↔ JS runtime parity)
  • New TestIterativeBuilderShadowStack (4 tests in tests/test_codegen_closures.py) covers each fixed builder at the size that previously overflowed:
    • array_map over 5 000 MkBox returns (sum = 12 497 500)
    • array_mapi over 5 000 with (elem + idx) Box (sum = 24 995 000)
    • array_fold with 5 000-element Box accumulator (sum = 12 497 500)
    • array_sort_by over 200 reverse-sorted ints with Ordering comparator (length = 200; ~19 900 comparisons)
  • Pre-commit hooks passed (mypy, pytest, conformance, examples, doc counts, limitations sync, site assets, license, allowlists)

Why this is a clean fix

The root cause is a real soundness contract in the lifted-closure epilogue: when the return is a heap pointer, the closure pushes it onto the shadow stack as a fresh root after restoring its entry sp, so a generic stash-then-GC caller stays sound. Iterative array builders are special — they consume the return synchronously into an already-rooted destination (dst[idx] or in-place accumulator slot) — so the per-call root is pure overhead. Each builder unwinds its own callback's post-push; no change to the closure epilogue, no change to other call sites.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Fixed a memory leak in iterative array operations when closures return heap values (release v0.0.133).
  • Tests

    • Added 4 regression tests covering array_map, array_mapi, array_fold and array_sort_by with closure results.
  • Documentation

    • Updated release notes, history, roadmap and known-issues entries to document the fix and project status for v0.0.133.

…ue root (closes #570)

The lifted-closure epilogue in vera/codegen/closures.py restores its
entry $gc_sp and then, when the return type is a heap pointer (Vera
ADT, String, Array<T>), pushes the return as a fresh root so generic
stash-then-GC callers stay sound. The iterative array builders in
vera/wasm/calls_arrays.py consume the return synchronously (store
into a rooted dst[idx], or in-place overwrite a rooted accumulator
slot), so the per-call root is redundant. Accumulating one leaked
slot per iteration overflowed the 16 KiB / 4 096-entry shadow stack;
a 5 000-element array_map<_, ADT> trapped at iteration ~4 000.

Per-callsite unwind:

* array_map / array_mapi: emit a 4-byte $gc_sp decrement after
  storing the return into dst[idx].
* array_fold: emit the same unwind BEFORE the gc_sp - 8 overwrite
  math. Without it the second iteration overwrite addressed the
  previous-call leaked slot instead of the accumulator pre-call
  slot — a second symptom of the same bug class that this fix
  incidentally closes.
* array_sort_by: same 4-byte unwind after the comparator Ordering
  tag is read via i32.load offset=0. Insertion sort issues up to
  n*(n-1)/2 comparisons; the leak surfaces at ~200 reverse-sorted
  elements.

Builders that take a Bool-returning predicate (array_filter,
array_find, array_any, array_all) and builders without a callback
(array_flatten, array_reverse, array_range) are unaffected — Bool is
excluded from the closure ret_is_pointer flag, so no post-restore
root push happens.

New TestIterativeBuilderShadowStack (4 tests) in
tests/test_codegen_closures.py exercises each fixed builder at the
size that previously overflowed.

Co-Authored-By: Claude <noreply@anthropic.invalid>
@coderabbitai

coderabbitai Bot commented May 5, 2026

Copy link
Copy Markdown

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock, !uv.lock

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0c88b07b-96a0-48b7-a165-6105fd363d94

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR fixes a GC shadow‑stack leak in iterative array builders by emitting per‑callsite gc_sp unwinds after closure calls that return heap pointers; changes affect array_map, array_mapi, array_fold, and array_sort_by. Four regression tests were added and package/version metadata bumped to 0.0.133.

Changes

Iterative Array Builder Shadow‑Stack Fix

Layer / File(s) Summary
Data / Metadata
pyproject.toml, vera/__init__.py, CHANGELOG.md, HISTORY.md
Version bumped to 0.0.133; changelog and history entries added describing per‑callsite gc_sp unwind fixes for iterative builders.
Behavioral Spec / Docs
README.md, ROADMAP.md, TESTING.md, KNOWN_ISSUES.md
Documentation, roadmap, testing and known‑issues entries updated: test counts and release counts adjusted; ROADMAP prioritisation and bug-survey narrative updated.
Core Implementation
vera/wasm/calls_arrays.py
Added per‑iteration classification flags (b_is_adt, b_needs_unwind) and emitted per‑callsite GC shadow‑stack unwind sequences across _translate_array_map, _translate_array_mapi, _translate_array_fold, and _translate_array_sort_by. Unwinds are placed relative to stores/accumulator writes to avoid leaking closure return roots.
Tests
tests/test_codegen_closures.py
Added TestIterativeBuilderShadowStack with four regression tests covering large inputs for array_map, array_mapi, array_fold, and array_sort_by to assert no shadow‑stack overflow / root leakage.
sequenceDiagram
    autonumber
    actor Compiler
    participant WasmGen as WasmCodegen
    participant GC as ShadowStack
    participant Runtime as WasmRuntime

    Compiler->>WasmGen: translate array_* with closure call
    WasmGen->>Runtime: emit call_indirect (closure)
    WasmGen->>GC: push root (if closure returns heap ptr)
    Runtime-->>WasmGen: closure result (ADT / primitive)
    WasmGen->>WasmGen: store result into dst/accumulator
    WasmGen->>GC: emit per‑iteration unwind (pop root) when needed
    note over WasmGen,GC: ensures no lingering roots per iteration
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

  • aallan/vera#572: Related changes to GC‑rooting/unwind behavior in vera/wasm/calls_arrays.py.
  • aallan/vera#489: Prior work on iterative array_fold and in‑loop rooting adjustments in the same module.
  • aallan/vera#485: Earlier modifications to array_map codegen/shadow‑stack handling that this PR extends.

Suggested labels

compiler, tests, docs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and specifically describes the main fix: resolving a GC shadow-stack leak in iterative array builders, and references the closed issue (#570), which aligns with the substantial code changes in vera/wasm/calls_arrays.py and the test additions in test_codegen_closures.py.
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.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/fix-570-array-map-shadow-stack

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

@codecov

codecov Bot commented May 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.97%. Comparing base (a04e39e) to head (bef1b8b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #574      +/-   ##
==========================================
+ Coverage   90.93%   90.97%   +0.03%     
==========================================
  Files          59       59              
  Lines       22596    22620      +24     
  Branches      259      259              
==========================================
+ Hits        20548    20578      +30     
+ Misses       2041     2035       -6     
  Partials        7        7              
Flag Coverage Δ
javascript 56.83% <ø> (ø)
python 94.80% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@HISTORY.md`:
- Line 270: Update the tagged-release counts to reflect the new v0.0.133
release: change the "132 tagged releases" text in the Stage 11 row(s) of
HISTORY.md to "133 tagged releases", update the roll-up footer total in
HISTORY.md to 133, and also bump the tagged-release total in ROADMAP.md to 133
so all three places (Stage 11 row(s), HISTORY.md footer, and ROADMAP.md) remain
consistent with the new release.
🪄 Autofix (Beta)

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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8bd61d25-441c-4daf-88fa-f539e990f46d

📥 Commits

Reviewing files that changed from the base of the PR and between a04e39e and 55c6b50.

⛔ Files ignored due to path filters (4)
  • docs/index.html is excluded by !docs/**
  • docs/index.md is excluded by !docs/**
  • docs/llms-full.txt is excluded by !docs/**
  • docs/llms.txt is excluded by !docs/**
📒 Files selected for processing (10)
  • CHANGELOG.md
  • HISTORY.md
  • KNOWN_ISSUES.md
  • README.md
  • ROADMAP.md
  • TESTING.md
  • pyproject.toml
  • tests/test_codegen_closures.py
  • vera/__init__.py
  • vera/wasm/calls_arrays.py
💤 Files with no reviewable changes (1)
  • KNOWN_ISSUES.md

Comment thread HISTORY.md
aallan and others added 2 commits May 5, 2026 23:24
…rom forward-looking files

ROADMAP had grown a 12-version retrospective paragraph in the
bug-killing-campaign intro plus historical residue scattered
through priority entries, the VeraBench section, Phase 3a/3b/4c
items, the CI table, and a duplicate Completed-Phases table that
restated HISTORY.md verbatim. None of this was forward-looking; it
was "what shipped two PRs ago" stuff that belongs in HISTORY (per-
release table) or CHANGELOG (long lists per version).

Changes:

* ROADMAP intro: drop the "Vera v0.0.132 delivers..." version peg
  (will go stale every release; not load-bearing).
* ROADMAP campaign paragraph: collapse 12 sentences of release
  retrospective to 2 — count of remaining issues + pointer to
  HISTORY/CHANGELOG.
* ROADMAP priority table: drop "splits out from #346 closed as
  superseded after v0.0.132 shipped..." and "phase 1 ... shipped
  in v0.0.117" framing; keep only what an agent picking up the
  issue needs to know.
* ROADMAP VeraBench section: replace per-rerun delta narrative
  with a high-variance disclaimer.
* ROADMAP Phase 3a/3b/4c entries: drop "during the v0.0.121 review
  cycle", "the v0.0.119 homepage redesign moved the score from
  3+2 to 2+1", "shipped in v0.0.117/v0.0.118" — keep current state
  only. CI table loses "the tag/release gap that hit v0.0.113".
* ROADMAP Completed Phases: replace 11-row table (which duplicated
  HISTORY.md Stages 1–9) with a one-liner pointing to HISTORY.

* KNOWN_ISSUES Bugs table: tightened all three entries. The #573
  entry was a 6-clause sentence with parenthetical-of-paren-
  thetical recapping the v0.0.132 host_gc_sweep design discussion;
  trimmed to symptom + recommended fix. #549 and #568 trimmed
  similarly.

* HISTORY by-the-numbers row: bump v0.0.132 column to v0.0.133
  (3,706 tests, 133 releases).

No content removed — anything trimmed from these three files is
either redundant with HISTORY/CHANGELOG or now lives in the
underlying GitHub issue body.

Co-Authored-By: Claude <noreply@anthropic.invalid>
CI 'uv lock --check' failed on PR #574 because the version bump
0.0.132 -> 0.0.133 wasn't reflected in uv.lock.  Single-line bump
of the vera package metadata; no dependency resolution change.

Co-Authored-By: Claude <noreply@anthropic.invalid>
@aallan

aallan commented May 6, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 6, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@aallan aallan merged commit bde6611 into main May 6, 2026
20 checks passed
@aallan aallan deleted the claude/fix-570-array-map-shadow-stack branch May 6, 2026 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GC shadow-stack overflow in array_map (and sibling iterative builders) at ~4000 heap-allocating elements

1 participant