fix(json): reject raw unpaired surrogates in string lexing instead of aborting - #4056
fix(json): reject raw unpaired surrogates in string lexing instead of aborting#4056bobzhang wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes two JSON lexer/parser correctness/robustness issues: (1) preventing a process abort when parsing strings containing lone surrogates combined with escapes, and (2) preserving IEEE-754 negative zero for the literal -0 so it matches other negative-zero spellings.
Changes:
- Update
lex_string_slowto slice pending string runs usingStringView::view(...)(bounds-check-only) to avoid aborting on surrogate slice boundaries. - Update the safe-integer fast path in
lex_number_endto apply the sign after converting toDouble, preserving-0.0for-0. - Add deterministic regression tests for both issues.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| json/lex_string.mbt | Avoids abort on lone-surrogate boundaries in the slow string lexing path by using view(...) slicing. |
| json/lex_string_test.mbt | Adds regression coverage for lone surrogate + escape combinations to ensure parsing succeeds (no abort). |
| json/lex_number.mbt | Preserves the sign bit for -0 by applying negation after Int64 -> Double conversion in the safe-integer fast path. |
| json/lex_number_test.mbt | Adds regression coverage asserting sign-bit preservation across multiple negative-zero spellings and underflow-to-zero cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Coverage Report for CI Build 6071Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.007%) to 90.672%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
Independent review by OpenAI Codex CLI (codex-cli 0.147.0, read-only session; posted on its behalf): No blocking findings.
Verdict: LGTM |
6af3721 to
5cf2ae6
Compare
5cf2ae6 to
ca31837
Compare
…e policy The parser now keeps strings Unicode well-formed (#4056): unpaired surrogates — raw or as \uXXXX escapes — are rejected with a ParseError instead of being passed through. Update the adversarial suite to match: - the roundtrip and fully-escaped generators produce only Unicode scalar values (astral pairs still included), and the AdvString shrinker drops whole characters so candidates stay well-formed; - new property: a lone surrogate injected at any position of a hostile string — raw via stringify or spelled as a \uXXXX escape, with escapes/astral pairs/control characters nearby — is always rejected cleanly (parse raises, valid is false, never an abort); - the deterministic surrogate pins now assert rejection for raw, escaped, reversed-pair, and mixed raw/escaped-half spellings, while well-formed pairs (raw or split across two escapes) still parse. Note: "zero literals preserve the sign of zero" requires the parse(-0) fix from #4061 (based on main) and fails until that lands in this branch's history; all other tests are green on wasm-gc, js, and native. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… aborting Fixes #4049. parse aborted (panic, not ParseError) on strings mixing a raw lone trailing surrogate with any escape sequence, e.g. the 5-code-unit text " U+DC00 \n ": lex_string_slow's flush sliced with the checked ctx.input[start:end], which aborts when the code unit at a slice boundary is a trailing surrogate. Meanwhile the escape-free fast path silently *accepted* raw lone surrogates, producing ill-formed strings. Per the design rule that MoonBit Strings stay Unicode well-formed (unsafe_to_char is indeed unsafe), well-formed input cannot contain a raw lone surrogate in the first place — but unsafe code can manufacture such a String, and robustness demands a clean error over a process abort or an ill-formed result. The string lexer now rejects raw unpaired surrogates with the documented ParseError (InvalidChar) on both the fast path and the slow path, and flush slices with the bounds-check-only view(start_offset~, end_offset~) so no abort path remains. Escaped surrogate sequences (\uXXXX) are out of scope here; unpaired escape handling is tracked in #4062 and fixed separately. Deterministic regression tests in lex_string_test.mbt cover escape-free and escape-adjacent raw lone surrogates (the former abort), the exact ParseError shape, and still-accepted well-formed pairs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ca31837 to
41233d9
Compare
…e policy The parser now keeps strings Unicode well-formed (#4056): unpaired surrogates — raw or as \uXXXX escapes — are rejected with a ParseError instead of being passed through. Update the adversarial suite to match: - the roundtrip and fully-escaped generators produce only Unicode scalar values (astral pairs still included), and the AdvString shrinker drops whole characters so candidates stay well-formed; - new property: a lone surrogate injected at any position of a hostile string — raw via stringify or spelled as a \uXXXX escape, with escapes/astral pairs/control characters nearby — is always rejected cleanly (parse raises, valid is false, never an abort); - the deterministic surrogate pins now assert rejection for raw, escaped, reversed-pair, and mixed raw/escaped-half spellings, while well-formed pairs (raw or split across two escapes) still parse. Note: "zero literals preserve the sign of zero" requires the parse(-0) fix from #4061 (based on main) and fails until that lands in this branch's history; all other tests are green on wasm-gc, js, and native. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes #4049
The bug
@json.parsecrashed the process (aguard!abort, not a catchableParseError) on strings mixing a raw lone trailing surrogate with any escape sequence — minimal repro: the 5-code-unit text"U+DC00\n". Root cause:lex_string_slow'sflushsliced the pending run with the checkedctx.input[start:end], which aborts when a slice boundary lands on a trailing surrogate. Meanwhile the escape-free fast path silently accepted raw lone surrogates, producing ill-formed strings.The fix: reject raw unpaired surrogates
Per the project's design direction — "we should maintain String unicode safe, so
unsafe_to_charis indeed unsafe" — this is robustness/defense-in-depth: under the well-formedness invariant such input should not exist (a well-formedStringcannot contain a raw lone surrogate), but unsafe code can manufacture one, and a clean, documented error beats a process abort or an ill-formed parse result. The string lexer now rejects raw unpaired surrogates withParseError(InvalidChar, sensible position) on both the fast path (previous silent acceptance removed) and the slow path, andflushslices with the bounds-check-onlyview(start_offset~, end_offset~), so no abort path remains.Well-formed pairs — raw astral characters, including next to escapes — parse exactly as before.
Scope
Raw (unescaped) surrogates only. Unpaired
\uXXXXescape sequences (which currently manufacture ill-formed strings from valid ASCII input) are tracked in #4062 and fixed in a separate PR; rejection there aligns with serde_json (Python and JS accept lone surrogates, Go substitutes U+FFFD).Tests
Deterministic regressions in
json/lex_string_test.mbt: escape-free raw lone surrogates (leading, trailing, reversed pair), raw lone surrogates next to escapes (the former abort), the exactParseErrorshape, and still-accepted well-formed pairs with a stringify/parse roundtrip.Verified:
moon checkclean; json suite green on wasm-gc, js, native; no.mbtichanges. The adversarial QuickCheck suite that found the bug is #4045.🤖 Generated with Claude Code