fix(runtime): string scanners must not read past exact-sized slice allocations (#6085) by proggeramlug · Pull Request #6286 · PerryTS/perry

proggeramlug · 2026-07-11T10:12:00Z

Root cause

Perry heap-string payloads are exact-sized: string_storage_alloc reserves size_of::<StringHeader>() + byte_len bytes — no NUL terminator, no tail padding — and the bytes are not guaranteed to be valid UTF-8. Buffer.toString(), FFI blobs, and WTF-8 lone-surrogate strings all reach js_string_from_bytes with raw bytes, and string_as_str hands those bytes to str::from_utf8_unchecked.

Several scanners then decoded that view with std's UTF-8-validity-assuming iterators (str::chars(), str::encode_utf16()). core::str::validations::next_code_point reads the continuation bytes of a multi-byte lead with unwrap_unchecked — no bounds check. So a payload whose last sequence is a truncated multi-byte lead reads 1–3 bytes past the end of its own allocation.

That is exactly the reported shape: reads just past the end of a heap page, layout-sensitive (a relink hides or resurfaces it), fatal only when an allocation happens to land flush against an unmapped page (0x...FFF8-style faulting addresses).

How the repro reaches it. A native FFI / Buffer blob is not required to be well-formed UTF-8. Splitting such a blob on an ASCII delimiter (\n, |) can cut inside a multi-byte sequence, and each split() part is a fresh exact-sized allocation ending in a dangling lead byte. Any subsequent scan of that part over-reads. This matches the reporter's "6/6 crashes within 7–29 seconds" on a per-frame split() + parse hot path, where the churn of fresh exact-sized allocations eventually places one against a guard page.

OOB sites fixed (each proven to fault, see below)

Site	Read class
`string/mod.rs` `utf16_offset_to_byte_offset` / `byte_offset_to_utf16_index` — reached by `slice` / `substring` / `substr` / `startsWith(pos)` / `endsWith(pos)`	`chars()` / `encode_utf16()` continuation-byte read past `byte_len`
`string/char_ops.rs` `js_string_char_code_at`, `js_string_code_point_at`	`chars()` walk past `byte_len`
`string/char_ops.rs` `js_string_at`	`encode_utf16().collect()` over the whole payload
`string/char_ops.rs` `js_string_to_char_array` (spread `[...s]`)	`chars()` walk past `byte_len`
`string/slice_ops.rs` `trim` / `trimStart` / `trimEnd`	`str::trim_matches` → `chars()`
`string/slice_ops.rs` `toLowerCase` / `toUpperCase`	`str::to_lowercase/to_uppercase` → `chars()`
`string/split.rs` empty-delimiter `split("")`	`chars()` walk past `byte_len`

Explicitly NOT a fix I am claiming

parseFloat is already bounds-safe. parse_float_bytes / float_prefix_end / trim_leading_js_whitespace guard every index with i < n and operate on the exact byte slice. I audited it and changed nothing there. The fault is on the scan side, not the parse side.
split()'s non-empty-delimiter path is already bounds-safe (byte search + exact-length copies). Only its split("") character walk over-read.
js_string_char_at and js_string_index_of were already bounds-driven; they get guard-page tests as regression cover, not fixes.
fix(runtime): bound WTF-8 decode against truncated trailing lead bytes (#6085) #6255 previously bounded decode_wtf8_units (padStart/padEnd). This PR closes the remaining sites in the same class.

Convention enforced: bounds-driven scanning

Perry strings carry an explicit byte_len and no terminator, and the payload is not guaranteed well-formed. Adding a NUL terminator would mean touching every allocation site, growing every string by a byte, and would still not make chars() safe (it stops at a lead byte, not a NUL). So the invariant enforced is: never read past byte_len; never assume UTF-8 validity.

All the walks above now go through one shared helper, string::wtf8_step, which classifies a sequence from its lead byte exactly as compute_utf16_len_wtf8 already does (so cursor walks agree with the utf16_len in the header) and reads continuation bytes only via bounds-checked get(), substituting 0 for bytes that do not exist. Well-formed input decodes byte-for-byte identically.

Case conversion additionally now round-trips lone surrogates and truncated sequences verbatim instead of depending on from_utf8_unchecked.

Verification

1. Guard-page tests (deterministic, red → green)

New crates/perry-runtime/src/string/tests_guard_page.rs (#[cfg(all(test, unix))]): mmap two pages, mprotect(PROT_NONE) the second, and place the StringHeader so the last payload byte is the last byte of the first page. Any read past byte_len hits the guard page. Payload is C3 A9 41 C3 ("é", "A", dangling 2-byte lead) with utf16_len (3) != byte_len (4) so the non-ASCII decode path is actually taken.

Against the pre-fix scanners, release build:

char_code_at_does_not_read_past_payload              (signal: 10, SIGBUS)
code_point_at_does_not_read_past_payload             (signal: 10, SIGBUS)
string_at_does_not_read_past_payload                 (signal: 10, SIGBUS)
to_char_array_does_not_read_past_payload             (signal: 10, SIGBUS)
trim_and_case_conversion_do_not_read_past_payload    (signal: 10, SIGBUS)
split_by_empty_delimiter_does_not_read_past_payload  (signal: 10, SIGBUS)
slice_and_substring_do_not_read_past_payload         (signal: 10, SIGBUS)

(macOS maps the guard-page read to SIGBUS; it is the same access violation Windows reports as c0000005. In a debug build the same reads instead panic inside core/src/str/validations.rs:48 — literally next_code_point's unwrap_unchecked — which pinpoints the over-read.)

After the fix, all 12 pass:

test result: ok. 12 passed; 0 failed

2. Behavior parity

cargo test -p perry-runtime -- --test-threads=1: 1244 passed, 0 failed. (The suite has pre-existing order-dependent flakes when run in parallel — a different unrelated test fails on each parallel run, on main as well as here.)

TS repro compiled with this build — per-frame blob.split('\n') → line.split('|') → parseFloat, 500 frames, plus edge cases ("", "1.23e5", " 1.5", "abc", "Infinity", "-Infinity", "-0" (incl. 1/x sign check), "3.", ".5", "1e", "+2.5", "0x10", "5px", empty parts, trailing newline) and every scanner this PR touches over multi-byte text (split, spread, charCodeAt, codePointAt, at, charAt, slice, substring, indexOf, trim*, case conversion, .length):

vs a stock main build: output byte-identical → this fix changes no observable behavior.
vs node --experimental-strip-types: identical, except one line — "😀x".at(0) === "\ud83d" — which is the pre-existing, documented lone-surrogate/WTF-8 categorical gap (at() substitutes U+FFFD). Verified it prints the same wrong value on unmodified main, so it is untouched by this PR and out of scope.

cargo fmt --all -- --check clean.

Fixes #6085

Summary by CodeRabbit

Bug Fixes
- Improved JavaScript string character/index/code point access for non-ASCII, truncated multi-byte, and lone-surrogate data.
- Updated splitting, trimming, and case conversion to avoid out-of-bounds reads on malformed or truncated UTF-8-like payloads.
- Preserved correct UTF-16/astral and surrogate-pair behavior for valid Unicode inputs.
Tests
- Added guarded-memory regression tests to verify safe decoding and correct results for both valid and truncated sequences.

…locations (#6085) Perry heap-string payloads are EXACT-SIZED: string_storage_alloc reserves size_of::<StringHeader>() + byte_len bytes — no NUL terminator, no tail padding — and the bytes are NOT guaranteed to be valid UTF-8 (Buffer toString, FFI blobs and WTF-8 lone-surrogate strings all reach js_string_from_bytes with raw bytes). Several scanners decoded such a payload through std's UTF-8-validity-assuming iterators (str::chars() / str::encode_utf16() over a from_utf8_unchecked view). core::str::validations::next_code_point reads the continuation bytes of a multi-byte lead with unwrap_unchecked, so a payload whose last sequence is a truncated multi-byte lead reads 1-3 bytes PAST the end of its own allocation. When that allocation sits flush against an unmapped page the read faults — the layout-sensitive access violations reported in #6085. Convention enforced: BOUNDS-DRIVEN scanning. Perry strings carry an explicit byte_len and no terminator, so every scanner now decodes via the shared wtf8_step helper, which reads continuation bytes only through bounds-checked get(). Well-formed input decodes byte-for-byte identically. Sites fixed (each proven to fault by a guard-page test before the fix): - string/mod.rs utf16_offset_to_byte_offset / byte_offset_to_utf16_index (reached by slice / substring / substr / startsWith(pos) / endsWith(pos)) - string/char_ops.rs js_string_char_code_at, js_string_code_point_at, js_string_at, js_string_to_char_array - string/slice_ops.rs trim / trimStart / trimEnd, toLowerCase / toUpperCase - string/split.rs the empty-delimiter split("") character walk Case conversion now also round-trips lone surrogates and truncated sequences verbatim instead of relying on from_utf8_unchecked. Adds string/tests_guard_page.rs: mmaps two pages, mprotect(PROT_NONE) on the second, and places the string so its last payload byte is the last byte of the first page — any read past byte_len faults. All 12 tests SIGBUS/SIGSEGV on the pre-fix scanners and pass after.

coderabbitai · 2026-07-11T10:12:19Z

📝 Walkthrough

Walkthrough

The runtime replaces UTF-8-dependent string scans with bounded WTF-8 traversal, updates character, splitting, trimming, and case-conversion operations to preserve truncated or lone-surrogate payloads, and adds Unix guard-page regression tests for exact-sized strings.

Changes

String safety and decoding

Layer / File(s)	Summary
Bounded WTF-8 decoder `crates/perry-runtime/src/string/mod.rs`	Adds `wtf8_step` and rewrites UTF-16/byte offset conversion to use bounds-checked payload traversal.
Character and index operations `crates/perry-runtime/src/string/char_ops.rs`	Updates character access, character arrays, indexing, and code-point lookup to use bounded UTF-16 unit decoding.
String transformations and splitting `crates/perry-runtime/src/string/slice_ops.rs`, `crates/perry-runtime/src/string/split.rs`	Reworks trimming, case conversion, and delimiter splitting to scan raw WTF-8 bytes, preserve invalid or surrogate data, and copy source ranges safely after allocation.
Guard-page regression coverage `crates/perry-runtime/src/string/mod.rs`, `crates/perry-runtime/src/string/tests_guard_page.rs`	Adds Unix-only guarded allocations and tests for truncated, valid multibyte, and astral payloads across string operations.

Estimated code review effort: 4 (Complex) | ~60 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately summarizes the runtime string-scan out-of-bounds fix.
Description check	✅ Passed	The description covers the root cause, fix, linked issue, and verification, though it doesn't use the template headings verbatim.
Linked Issues check	✅ Passed	The changes fix the reported scanner overreads with bounded WTF-8 walks and regression tests for `#6085`.
Out of Scope Changes check	✅ Passed	All changes are string-scanner fixes, rooting adjustments, and guard-page tests directly tied to `#6085`.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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/6085-string-scanner-oob

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

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (3)

crates/perry-runtime/src/string/tests_guard_page.rs (2)
94-251: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Test coverage is thorough; consider parseFloat if applicable.

The test suite covers all operations listed in the PR summary (char_code_at, code_point_at, char_at, string_at, to_char_array, slice/substring, index_of, split(""), trim, case conversion, well-formed multibyte, astral). Surrogate and code-point assertions are numerically correct.

Issue #6085 also mentions parseFloat() as a hot-path operation that triggered access violations. If parseFloat was determined to be unaffected by the over-read problem, that's fine — but if it shares the same scanner pattern, a guard-page test for it would close the loop.
🤖 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/string/tests_guard_page.rs` around lines 94 - 251,
Assess whether parseFloat uses the same unbounded string-scanning path affected
by the truncated UTF-8 payload. If it does, add a guard-page regression test
near the existing TRUNCATED_TAIL tests that invokes parseFloat with the guarded
string and verifies it does not read past the payload; otherwise leave the
current coverage unchanged.
39-79: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Guard-page setup and Drop are correct.

The GuardedString::new implementation is sound: two-page mmap, second page PROT_NONE, payload placed flush against the guard, alignment verified, sanity-checked end position. The Drop impl correctly calls munmap. All test payloads are length 4, satisfying the len ≡ 4 mod 8 alignment constraint for the 20-byte header.

One minor note: if an assertion between mmap (line 43) and the GuardedString return (line 77) panics, the mapped region leaks since Drop isn't wired yet. This is acceptable for test code (process aborts, OS reclaims), but a small cleanup guard or deferred munmap would make it robust against --no-run panics.
🤖 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/string/tests_guard_page.rs` around lines 39 - 79, No
code change is required; the review confirms GuardedString::new and its Drop
cleanup are correct. If hardening is desired, add deferred cleanup for the mmap
allocation so assertion panics during setup unmap the region, while preserving
the existing successful-construction and Drop behavior.
crates/perry-runtime/src/string/char_ops.rs (1)
250-251: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use size_of::<ArrayHeader>() here
crates/perry-runtime/src/string/char_ops.rs:251 should mirror the other array helpers and derive the element offset from ArrayHeader instead of hardcoding 8.
♻️ Proposed change
-    let elements = unsafe { (arr as *mut u8).add(8) as *mut f64 };
+    let elements =
+        unsafe { (arr as *mut u8).add(std::mem::size_of::<ArrayHeader>()) as *mut f64 };
🤖 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/string/char_ops.rs` around lines 250 - 251, Update
the element pointer offset in the span array construction near
js_array_alloc_with_length to use size_of::<ArrayHeader>() instead of the
hardcoded 8-byte offset, matching the established array helper layout.

🤖 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/string/slice_ops.rs`:
- Around line 215-256: Update js_whitespace_trim_range so a code point is
considered whitespace only when wtf8_step reports a complete sequence fully
contained in bytes. Treat truncated or incomplete tails as non-whitespace,
preserving them during trimEnd and trim while leaving valid whitespace trimming
unchanged.

In `@crates/perry-runtime/src/string/split.rs`:
- Around line 143-151: Update the split logic around the parts accumulation loop
to root each newly created string before subsequent js_string_from_bytes or
js_array_alloc allocations can trigger GC. Use the existing RuntimeHandleScope
mechanism for the accumulated strings, or allocate the result array first and
write each string directly into its slots, ensuring all earlier split parts
remain valid until the result is returned.

---

Nitpick comments:
In `@crates/perry-runtime/src/string/char_ops.rs`:
- Around line 250-251: Update the element pointer offset in the span array
construction near js_array_alloc_with_length to use size_of::<ArrayHeader>()
instead of the hardcoded 8-byte offset, matching the established array helper
layout.

In `@crates/perry-runtime/src/string/tests_guard_page.rs`:
- Around line 94-251: Assess whether parseFloat uses the same unbounded
string-scanning path affected by the truncated UTF-8 payload. If it does, add a
guard-page regression test near the existing TRUNCATED_TAIL tests that invokes
parseFloat with the guarded string and verifies it does not read past the
payload; otherwise leave the current coverage unchanged.
- Around line 39-79: No code change is required; the review confirms
GuardedString::new and its Drop cleanup are correct. If hardening is desired,
add deferred cleanup for the mmap allocation so assertion panics during setup
unmap the region, while preserving the existing successful-construction and Drop
behavior.

🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 082cdb7e-6380-4fe3-a704-cc0b2465c911

📥 Commits

Reviewing files that changed from the base of the PR and between 9229a14 and 82ed27e.

📒 Files selected for processing (5)

crates/perry-runtime/src/string/char_ops.rs
crates/perry-runtime/src/string/mod.rs
crates/perry-runtime/src/string/slice_ops.rs
crates/perry-runtime/src/string/split.rs
crates/perry-runtime/src/string/tests_guard_page.rs

…ed tails (#6085 review) Two review findings on the #6085 branch. 1. GC safety in js_string_split_n (pre-existing on main, not introduced here: main also parked raw *mut StringHeader in a plain Vec and only wrote them into the array afterwards). A Vec of raw pointers is neither a GC root nor rewritten by C4b evacuation, so a collection triggered by a later js_string_from_bytes / js_array_alloc could reclaim or move parts before they were published. The non-empty-delimiter path had the same shape plus the #5062 dangling-source class: it copied from &str slices borrowed from the source payload AFTER string_storage_alloc had already run. Both paths now root the source string and the result array in a RuntimeHandleScope, re-read both after every allocation, and publish each part into the rooted array as it is built. Part positions are carried as byte OFFSETS (not borrowed slices) so they stay valid across a move, and each sequence is staged through a stack buffer before allocating. The array length is bumped incrementally: js_array_alloc does not zero its storage, so publishing the full length up front would let a mid-loop GC scan uninitialized slots as JSValues. 2. Trim must not eat a truncated multi-byte tail. wtf8_step zero-fills missing continuation bytes, so a dangling E2 80 decoded as U+2000 (EN QUAD) — real JS whitespace — and trim/trimEnd silently deleted those bytes. Whitespace is now recognized only for a COMPLETE sequence fully contained in the payload, matching case_convert, which already preserved truncated tails verbatim. Adds trim_preserves_truncated_multibyte_tail (a truncated E2 80 survives trim byte-for-byte; a complete U+2000 is still trimmed) and asserts the part count on the empty-delimiter guard-page test.

coderabbitai

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/string/split.rs`:
- Around line 227-247: In the split-part construction loop, stop using the
aggregate src_is_ascii shortcut and derive each part’s UTF-16 length from its
own bounded bytes via compute_utf16_len. Also replace the hardcoded flags
argument in init_string_header with the same flag derivation used by
js_string_from_bytes, preserving lone-surrogate metadata for malformed parts.

🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 396eba08-4b2c-42a7-b2d3-7d09d537e254

📥 Commits

Reviewing files that changed from the base of the PR and between 82ed27e and 3525a31.

📒 Files selected for processing (3)

crates/perry-runtime/src/string/slice_ops.rs
crates/perry-runtime/src/string/split.rs
crates/perry-runtime/src/string/tests_guard_page.rs

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

crates/perry-runtime/src/string/tests_guard_page.rs
crates/perry-runtime/src/string/slice_ops.rs

proggeramlug · 2026-07-11T15:06:06Z

Heads up: this merged at 3525a31dd, which is one commit before the fix for the third review finding (split parts inheriting wrong utf16_len/flags from the aggregate is_ascii_string check). That fix is therefore not on main — it is now up on its own as #6295.

coderabbitai Bot reviewed Jul 11, 2026

View reviewed changes

Comment thread crates/perry-runtime/src/string/slice_ops.rs

Comment thread crates/perry-runtime/src/string/split.rs Outdated

coderabbitai Bot reviewed Jul 11, 2026

View reviewed changes

Comment thread crates/perry-runtime/src/string/split.rs

proggeramlug merged commit 80ff6fb into main Jul 11, 2026
25 checks passed

proggeramlug deleted the fix/6085-string-scanner-oob branch July 11, 2026 14:41

proggeramlug mentioned this pull request Jul 11, 2026

fix(runtime): derive each split part's metadata from its own bytes (#6085 follow-up) #6295

Merged

Navigation Menu

Uh oh!

Uh oh!

fix(runtime): string scanners must not read past exact-sized slice allocations (#6085) - #6286

fix(runtime): string scanners must not read past exact-sized slice allocations (#6085)#6286
proggeramlug merged 2 commits into
mainfrom
fix/6085-string-scanner-oob

proggeramlug commented Jul 11, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

proggeramlug commented Jul 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

proggeramlug commented Jul 11, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Root cause

OOB sites fixed (each proven to fault, see below)

Explicitly NOT a fix I am claiming

Convention enforced: bounds-driven scanning

Verification

1. Guard-page tests (deterministic, red → green)

2. Behavior parity

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

proggeramlug commented Jul 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

proggeramlug commented Jul 11, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading