Skip to content

fix(runtime): own punycode string bytes before decoding - #8440

Merged
proggeramlug merged 2 commits into
PerryTS:mainfrom
proggeramlug:fix/8424-punycode-owned-bytes
Aug 20, 2026
Merged

fix(runtime): own punycode string bytes before decoding#8440
proggeramlug merged 2 commits into
PerryTS:mainfrom
proggeramlug:fix/8424-punycode-owned-bytes

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Remove the unbounded borrow of movable GC string payloads from punycode.ucs2.decode. The decoder now copies heap bytes into owned storage before any later runtime allocation can relocate the source string.

Changes

  • Replace the &'static [u8] heap-payload helper with an owned Vec<u8> copy.
  • Decode UTF-16 units exclusively from the owned copy.
  • Add a regression test proving the copied bytes remain independent of the heap payload.
  • Add a changelog fragment; no version metadata was changed.

Related issue

Fixes #8424

Test plan

  • RUST_TEST_THREADS=1 cargo test --release -p perry-runtime punycode (5 passed)
  • ./scripts/pre-tag-check.sh --quick
  • Added a #[test] in perry-runtime
  • Full workspace build/test suite (left to CI)

Checklist

  • I have NOT bumped the workspace version or edited CLAUDE.md / CHANGELOG.md
  • My commit follows the repository commit-prefix convention
  • I have read CONTRIBUTING.md and agree to the Code of Conduct

Summary by CodeRabbit

  • Bug Fixes
    • Improved Unicode decoding reliability by preventing string data from being affected by later runtime changes.
    • Fixed a potential memory-safety issue when decoding malformed or unusually sized string payloads.
  • Tests
    • Added regression coverage to verify decoded data remains stable after the source string is modified.
  • Documentation
    • Documented the Unicode decoding fix in the changelog.

@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown

Review Change Stack

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: 313b894a-d5eb-446d-ac5f-5723c4d80807

📥 Commits

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

📒 Files selected for processing (2)
  • changelog.d/8440-punycode-owned-string-bytes.md
  • crates/perry-runtime/src/punycode.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.


📝 Walkthrough

Walkthrough

The runtime now copies punycode string payload bytes before UCS-2 decoding. A regression test verifies that copied bytes remain unchanged after source memory modification. A changelog entry documents the moving-GC safety fix.

Changes

Punycode string safety

Layer / File(s) Summary
Owned byte copy and decoding
crates/perry-runtime/src/punycode.rs, changelog.d/8440-punycode-owned-string-bytes.md
copy_string_bytes validates the string pointer and copies its payload into owned storage. js_punycode_ucs2_decode decodes from the copy. A regression test validates payload preservation, and the changelog records the fix.

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

Merge Risk: ⚪ Minimal · up to 644eb

This localized runtime change copies punycode string bytes before decoding and includes a passing regression test; no actionable merge-blocking risk remains beyond normal checks and review.

Possibly related issues

Possibly related PRs

  • PerryTS/perry#8439 — Both PRs modify runtime string-copy or decoding behavior to avoid unsafe borrowed payloads in different code paths.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main runtime fix: owning punycode string bytes before decoding.
Description check ✅ Passed The description includes the required summary, changes, issue, test plan, checklist, and relevant validation details.
Linked Issues check ✅ Passed The changes satisfy issue #8424 by removing the unbounded borrow, copying bytes before decoding, and adding regression coverage.
Out of Scope Changes check ✅ Passed The code, regression test, and changelog fragment are directly related to the linked issue and stated pull request objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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: punycode string_bytes manufactures &'static [u8] over a movable GC payload

1 participant