Skip to content

fix(runtime): root repeat's receiver across the count coercion - #8443

Merged
proggeramlug merged 2 commits into
PerryTS:mainfrom
proggeramlug:fix/8427-string-repeat-gc
Aug 20, 2026
Merged

fix(runtime): root repeat's receiver across the count coercion#8443
proggeramlug merged 2 commits into
PerryTS:mainfrom
proggeramlug:fix/8427-string-repeat-gc

Conversation

@proggeramlug

Copy link
Copy Markdown
Contributor

Closes #8427.

Problem

js_string_repeat borrowed the receiver's inline WTF-8 payload and then
coerced count:

let str_data = string_as_str(s);                                   // borrow of s's payload
let count_number = crate::builtins::js_number_coerce(count_value);  // runs user valueOf
...
let result = str_data.repeat(count);                                // reads the borrow

ToNumber on an object count runs user JS (valueOf / Symbol.toPrimitive).
A loop in that callback hits back-edge safepoint polls (default-on, #7721)
where the copying minor is eligible; when it evacuates a young receiver,
str_data names retired from-space and repeat copies garbage.

Fix

Coerce first, borrow second — but that alone is not sufficient. s is a raw
pointer in a native Rust frame, which the collector does not scan by
default, so the later string_as_str(s) would still deref a stale address.
It is parked in a RuntimeHandleScope and the post-collection address is
taken back out via across_const, the sanctioned combinator for this shape
(no new bare raw-handle reads — scripts/raw_handle_debt.py stays at its
978 baseline).

The count == 0 early return moves above the borrow, so the payload is read
only on the path that actually needs it. Observable ordering per ECMA-262
§22.1.3.17 is unchanged: ToIntegerOrInfinity still runs for an empty
receiver, and a negative count still throws before the empty-string return —
both are asserted in the fixture and byte-compared against Node 26.5.1.

padStart / padEnd: audited, unaffected

The issue asked to check them. They are not affected, and the PR carries
evidence rather than a reading: codegen emits both of their coercions —
js_number_coerce for maxLength and js_string_pad_fill (ToString) for
the fill — before reread_recv produces the receiver handle
(lower_string_method.rs:855-870). No user code runs inside
js_string_pad_start / _pad_end after string_as_str; everything past it
(to_length, decode_wtf8_units, build_pad_chunk, finish_pad_result) is
pure Rust, and finish_pad_result copies the payload into an owned Vec
before the destination allocation. The fixture exercises both with reentrant
allocating maxLength and fill callbacks anyway, so a future move of
either coercion into the helper lands on a test already watching.

Validation

test-files/test_gap_gc_string_repeat_reentrant_count.ts — a gap fixture, so
it runs in the per-PR tier, not only the nightly sweep. Built with
cargo build --profile perry-dev -p perry -p perry-runtime-static -p perry-stdlib-static (.a mtimes verified to move after each edit), run
under the instruments in its parity-env line.

The gate was proved able to go red. Same fixture, same instruments, the
only difference being the runtime .a:

repeat (valueOf) repeat (Symbol.toPrimitive) padStart padEnd vs Node
pre-fix runtime false false true true 2 lines differ
post-fix runtime true true true true byte-identical

Both arms report copying_minors=530 moved_objects=16930 — the subject
collector ran, so the green verdict is not vacuous. The pad rows staying
green in both arms is the empirical half of the audit above (that code is
byte-identical across the two builds).

Without the instruments the fixture passes on both runtimes, which is the
point of the parity-env line: this class is invisible to an unpaced run.

Also green: cargo fmt --all -- --check, scripts/check_file_size.sh,
scripts/addr_class_inventory.py (the two handle-floor notes it prints are
pre-existing on main), scripts/raw_handle_debt.py (978, at baseline),
scripts/gc_runtime_root_holders.py, scripts/check_gc_env_knobs.py,
scripts/check_node_version_consistency.py.

No version bump (maintainer bumps at merge).

Siblings in the same class, filed separately: #8426 (js_string_normalize),
#8428 (js_regexp_exec lastIndex).

Ralph Küpper added 2 commits August 20, 2026 06:54
`js_string_repeat` borrowed the receiver's inline WTF-8 payload and only
then coerced `count`. ToNumber on an object count runs user JS
(`valueOf` / `Symbol.toPrimitive`), whose loop back-edge polls are
moving-GC safepoints, so an evacuating minor inside the callback left the
borrow naming retired from-space and the result was copied out of
garbage.

Coerce first, then borrow. Deferring the borrow alone is not enough: `s`
is a raw pointer in a native Rust frame, which the collector does not
scan by default, so it is parked in a `RuntimeHandleScope` and the
post-collection address is taken back out via `across_const`.

padStart/padEnd were audited for the same shape and are unaffected —
codegen emits both of their coercions (`js_number_coerce` for maxLength,
`js_string_pad_fill` for the fill) before the receiver handle is re-read,
so no user code runs inside those helpers while the payload is borrowed.
The new fixture covers them too, so a future move of either coercion into
the helper lands on a test that is already watching.
@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@proggeramlug, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 34 minutes

Limit details: You’ve used all 8 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b05aab08-168b-4e8b-9a0f-156aa6c21147

📥 Commits

Reviewing files that changed from the base of the PR and between 526e0b5 and fad400a.

📒 Files selected for processing (3)
  • changelog.d/8443-string-repeat-reentrant-count.md
  • crates/perry-runtime/src/string/pad.rs
  • test-files/test_gap_gc_string_repeat_reentrant_count.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

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

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Validated as part of an 11-PR batch (#8439, #8440, #8441, #8442, #8443, #8444, #8446,
#8448, #8450, #8453, #8454) stacked on main and built once, then merged individually.

  • 19/19 sweep corpus byte-exact against the Node oracle
  • perry-runtime --lib 2601 · perry --bin perry 1007 · perry-codegen --lib 1113
  • scripts/run_lint_gates.sh — all 50 gates
  • A re-entrancy probe (user JS re-entering via toString/valueOf/replacer/comparator
    during normalize, repeat, regex lastIndex coercion, replace, JSON.stringify,
    sort, and punycode host conversion) matches Node exactly, including under
    PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1.

One thing stated plainly: these are hardening, not demonstrated repairs

I could not make the underlying bugs reproduce. My probe passes on unmodified main, and so
do all four of the fixtures this series ships
(test_issue_8426_normalize_reentrant, test_issue_8428_exec_lastindex_reentrant,
test_gap_gc_string_copy_source_rooting, test_gap_gc_string_repeat_reentrant_count) —
including under PERRY_GC_FORCE_EVACUATE=1, PERRY_GC_VERIFY_EVACUATION=1, and
PERRY_GC_PROTECT_FROMSPACE=1 at depth 800.

That is consistent with the string audit having found these windows by reading rather than by
reproducing, and with this bug class being invisible at collection time. The changes are
still worth landing — an unrooted borrow across user JS is a real latent hazard. But the
merge rests on "correct by construction and regression-free", not on "fixes an observed
failure", and the fixtures should be understood as no-regression guards rather than
reproducers.

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.

runtime: js_string_repeat holds a payload borrow across user valueOf — moving GC can relocate the subject mid-call

1 participant