fix(runtime): derive each split part's metadata from its own bytes (#6085 follow-up) - #6295
Merged
Merged
Conversation
…6085 review) is_ascii_string(s) only compares byte_len == utf16_len over the WHOLE source. Malformed bytes can satisfy that aggregate while the individual parts cannot, so the ASCII fast path stamped wrong metadata onto every part. Verified on the branch with [0x80, '|', 0xF0]: SOURCE byte_len=3 utf16_len=3 is_ascii_string=true <- the aggregate lies part[0] bytes=[80] utf16_len RECORDED=1 CORRECT=0 part[1] bytes=[F0] utf16_len RECORDED=1 CORRECT=2 A stray continuation byte is 0 UTF-16 units and a 4-byte lead is 2, but both parts were recorded as 1. That wrong .length then propagates into every downstream index/length operation. Hardcoding flags = 0 also dropped lone-surrogate metadata. Verified with "\uD800|B": the part holding ED A0 80 came back with flags=0, so isWellFormed() on it wrongly returned true. (js_string_from_bytes hardcodes flags = 0 as well, so the flag has to be derived from the part's own bytes rather than mirrored from it.) Fix: - Replace the aggregate is_ascii_string() gate with an actual scan of the source payload. A genuinely all-ASCII source has all-ASCII parts, so that shortcut IS sound per-part; it is the only shortcut kept. - Otherwise derive utf16_len per part with the bounded compute_utf16_len over that part's own bytes. - Derive STRING_FLAG_HAS_LONE_SURROGATES per part via a new bounds-driven bytes_have_lone_surrogate() helper, for both the non-empty and the empty-delimiter paths. A part only carries the flag if the surrogate actually landed in that part. Adds split_parts_get_metadata_from_their_own_bytes and split_parts_preserve_lone_surrogate_flag.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesString split metadata
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #6286 (#6085). That PR was merged at
3525a31dd, one commit before this fix landed on the branch, so this review finding is not onmain. It is a real correctness bug and is filed here on its own.Root cause
js_string_split_n's ASCII fast path gated onis_ascii_string(s), which only comparesbyte_len == utf16_lenover the whole source. Malformed bytes can satisfy that aggregate while the individual parts cannot — so the fast path stamped wrongutf16_len(and hardcodedflags = 0) onto every part.This lands squarely in #6085's own threat model: a payload that is not well-formed UTF-8 is the entire reason that issue exists (FFI /
Bufferblobs split on an ASCII delimiter).Verified, not inferred
Instrumented
js_string_split_nwith[0x80, '|', 0xF0]:A stray continuation byte is 0 UTF-16 units; a 4-byte lead is 2. Both were recorded as 1. That wrong
.lengthpropagates into every downstream index/length operation.Lone-surrogate metadata was dropped too. With
"\uD800|B"(ED A0 80 | B):Note
js_string_from_bytesalso hardcodesflags = 0(it only derivesutf16_len), so the flag has to be derived from each part's own bytes, not mirrored from that constructor.Fix
is_ascii_string(s)gate with an actual byte scan of the source payload. A genuinely all-ASCII source provably has all-ASCII parts, so that shortcut is sound per-part — it is kept (common case, hot path), but now gated on something true.utf16_lenper part with the boundedcompute_utf16_lenover that part's own bytes.STRING_FLAG_HAS_LONE_SURROGATESper part via a new bounds-drivenbytes_have_lone_surrogate()helper, on both the non-empty and empty-delimiter paths. A part carries the flag only if the surrogate actually landed in it.Tests
split_parts_get_metadata_from_their_own_bytes— assertsutf16_len0 and 2 for the two parts, and asserts the precondition thatis_ascii_stringreally does misfire, so a regression back to the aggregate check fails the test.split_parts_preserve_lone_surrogate_flag— flagged part →isWellFormed() == false; the clean"B"part stays unflagged; covers thesplit("")path too.cargo test -p perry-runtime -- --test-threads=1: 1257 passed, 0 failed.cargo fmt --all -- --checkclean. Node parity repro (per-framesplit('\n')→split('|')→parseFloat, plus every scanner touched): byte-identical to a stockmainbuild and identical tonode --experimental-strip-types.Summary by CodeRabbit
Bug Fixes
Tests