tryStreamingParse: input cut inside a \u escape or an exponent needs more data - #638
tryStreamingParse: input cut inside a \u escape or an exponent needs more data#638robobun wants to merge 1 commit into
Conversation
…more data tryStreamingParse reports NeedMoreData when a value fails to parse and the lexer stopped at the end of the input. Two lexer paths stop earlier when the input ends inside a token, so a chunk that ends there was an error: - lexStringSlow returned at the 'u' of a \uXXXX escape with fewer than four characters after it. It now stops at the first character that is not a hex digit, which is the end of the input when the escape was cut short. - parseJSONDouble accepts "1" from "1e-" and leaves "e-" behind, which lexes as an identifier. lexNumber now reports an exponent without digits through lexNumberError, which stops at the missing digit. A top-level "1e-" no longer produces the value 1 before the error. Malformed escapes and exponents whose bad character is in the input are still errors.
|
Status Reproduced on Bun 1.4.2 (
Verified on the preview build of this PR (
Before the preview existed, I ran the same checks with the changed |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it edits LiteralParser::lexNumber / lexStringSlow — a hot path shared by JSON.parse — and both changed paths fall under CODEOWNERS (@ WebKit/jsc-reviewers), a human look is still worthwhile.
What was reviewed:
\uslow path: verified thedo/whilestarts on'u', is bounded bym_end, and leavesm_ptr == m_endexactly when the truncated hex digits run out — non-hex before end still yields a hard error.- Exponent check: confirmed the peek matches
lexNumberError's own[eE][+-]?walk, so the new call always hits the existing "Exponent symbols…"TokErrorand cannot reachASSERT_NOT_REACHED();1e5e7still returnsTokNumber(1e5)because a digit follows the seconde. - No previously-valid input regresses: in strict/sloppy JSON a number token can never be legitimately followed by
e/E, so only error text changes for non-streaming callers. - New stress test throws on failure, does not print, and is placed in
JSTests/stressperJSTests/README.md.
Extended reasoning...
Overview
The PR adjusts two TokError return sites in Source/JavaScriptCore/runtime/LiteralParser.cpp so that m_ptr lands at m_end when a \uXXXX escape or a numeric exponent is merely truncated by end-of-input, letting tryStreamingParse (line 2081, m_lexer.isAtEnd()) classify the failure as NeedMoreData instead of Error. Both edits are wrapped in #if USE(BUN_JSC_ADDITIONS). A new JSTests/stress/streaming-json-parse-cut-input.js slices valid JSONL lines at every offset and asserts needMoreData, checks top-level 1e-/1E+ prefixes yield no premature value, and asserts genuinely malformed escapes/exponents still report error.
Security risks
None identified. The \u loop is bounded by (m_end - m_ptr) < 5 on entry and re-checks m_ptr < m_end each iteration, so it cannot read past the buffer. The exponent peek dereferences *m_ptr and *exponentDigits only after < m_end guards. No new allocation, no user-controlled indexing, no change to how parsed values are constructed. The only non-streaming observable effect is a different (pre-existing) error message string for inputs like JSON.parse("[1e-]").
Level of scrutiny
Medium-high. lexNumber sits on the JSON.parse hot path for every non-int32 number, and LiteralParser is instantiated for both 8-bit and 16-bit character types across strict/sloppy/JSONP modes. I traced that the int32 fast path (line 1164) already excludes a trailing e/E, so the new check is only reached after parseJSONDouble; and that lexNumberError re-derives the same [eE][+-]? position, guaranteeing it returns at the exponent-digit check rather than falling through to ASSERT_NOT_REACHED(). Because JSON grammar never allows an identifier immediately after a number, no previously-accepted input is now rejected — only error classification/message changes.
Other factors
Both changed paths are listed in .github/CODEOWNERS under @ WebKit/jsc-reviewers, which per the approval guidelines means a human owner should sign off. The regression test complies with JSTests/README.md conventions (throws on failure, no logging, lives in JSTests/stress, small iteration count). One minor imprecision I noted but did not flag as a bug: pathological input like 1e5e- at end-of-chunk now reports NeedMoreData even though no suffix can make it valid; the error simply surfaces one chunk later, which is acceptable streaming behavior. Given the CODEOWNERS coverage and the shared-with-JSON.parse surface, deferring to a human reviewer is the right call.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughThe streaming JSON parser now handles truncated Unicode escapes and incomplete exponent forms as incomplete input. A stress test validates truncation behavior, complete parsing, malformed input errors, and multibyte input handling. ChangesStreaming JSON parser
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The change is ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Preview Builds
|
1 participant
Problem
Bun.JSONL.parseChunkreturnsSyntaxError: Failed to parse JSONLfor a chunk that ends inside a\uXXXXescape or after thee-/e+of a number. Every other cut position reports an incomplete value, so NDJSON with\uescapes (Pythonjson.dumps) fails at random read boundaries.tryStreamingParse(LiteralParser.cpp:2060) reportsNeedMoreDataonly when the lexer stopped at the end of the input.lexStringSlowreturns at theuwhen fewer than 5 characters remain.parseJSONDoubleaccepts1from1e-, and the rest lexes as an identifier.1e-also produces the value1.Fix
lexStringSlowadvances over the hex digits that are present, then returns the error. It stops at the end of the input for a cut escape, and at the bad character otherwise.lexNumbersends an exponent that has no digits tolexNumberError, which stops at the missing digit.USE(BUN_JSC_ADDITIONS). Malformed input is still an error. TheJSON.parsemessage for[1e-]changes fromExpected ']'to thelexNumberErrormessage.JSTests/stress/streaming-json-parse-cut-input.jsfails on the pinnedjscand passes on the previewjsc. Bun tests: Bun.JSONL: a chunk cut inside a \u escape, an exponent or the BOM is incomplete, not an error bun#42488.Background
tryStreamingParseparses one JSON value per line and returnsComplete,NeedMoreDataorError. ForNeedMoreData,Bun.JSONL.parseChunkreturnserror: nulland the caller appends the next chunk.m_ptr == m_endis the only sign that the input ran out.parseJSONDoubleis fast_float injsonformat. That format includesfixed, so it ignores an exponent without digits.Notes
Reproduction (Bun 1.4.2
744846f844, canary, andmainat the pinned WebKitcf1b36ec87):It prints four failing cuts, all inside the
\u00e9. String input andUint8Arrayinput behave the same, and so do the 8-bit and the 16-bit lexer.Why only these two. I went through every place where the lexer returns
TokErroror gives up on a token. All the others already stop at the end of the input when the input ends inside the token: an unterminated string, a\as the last character,-or1.with nothing after it,tru(lexed as an identifier to the end). A cut after1ewas reported as incomplete only by accident: theelexes as an identifier that reaches the end of the input.lexNumberErrorcannot reach itsASSERT_NOT_REACHED()through the new call.lexNumbercalls it only after it checked that no digit follows[eE][+-]?, which is the conditionlexNumberErrorreturns on. For1e5e7, fast_float consumes1e5and a digit follows the seconde, so the number token is returned as before and the parse fails on the identifiere7.Behaviour that changes outside of
tryStreamingParse. Only the text of theJSON.parseerror for a number with an exponent that has no digits:JSON.parse("1e-")andJSON.parse("2e-+10")saidUnable to parse JSON string,JSON.parse("[1e-]")saidExpected ']'. They now sayExponent symbols should be followed by an optional '+' or '-' and then by at least one number, the messagelexNumberErrorhas had since before the fast_float change made it unreachable. No valid input has aneright after a number in strict or sloppy mode, so nothing that parsed before fails now.LayoutTests/js/dom/JSON-parse-expected.txtexpects the old text for2e-+10and2e+-10, which is why the change is behindUSE(BUN_JSC_ADDITIONS).On the preview build of this PR (
autobuild-preview-pr-638-2a5a22bc): its debug+ASANjscpassesstreaming-json-parse-cut-input.js, the pinnedjscexits 3 at{"name":"caf\u. BunmainwithWEBKIT_VERSIONset to the preview (oven-sh/bun#42488): the 7 new tests intest/js/bun/jsonl/jsonl-parse.test.tspass and the rest of the file gives the same result as before (274 of 276 pass, the two 4 GB tests time out under debug+ASAN).How I tested before the preview build existed. The fork's CI does not run JSTests. I compiled the changed
LiteralParser.cppwith the flags from the pinned debug+ASAN build'scompile_commands.jsonand linked the object ahead of the pinnedlibJavaScriptCore.a(every definition in it is a template instantiation, so the first one wins), once into thejscshell and once into a debug build of Bun. The baseline is the same link without the object.streaming-json-parse-cut-input.js{"name":"caf\uJSTests/stress/*json*teststoLocaleStringtest/js/bun/jsonl/jsonl-parse.test.ts(269 tests)Differential run. A seeded generator made 6000 lines of nested arrays, objects, strings with
\uescapes and numbers with exponents, 8-bit and 16-bit. Half of them were cut at every position (149725 chunks), the other half were mutated (a character inserted, removed or replaced, once or twice) and cut at a random position (2881 chunks). Bothjscshells parsed every chunk withstreamingJSONParse.errorfor 34501 chunks. With the change, none. Every cut of a line that is an array, an object or a string saysneedMoreDatawith no value, and the whole line sayscompletewith one value.errortoneedMoreData, and every one of them ends in\uplus 0 to 3 hex digits or in a digit pluse,e-ore+. No other result differs.Not changed here. A top-level number that ends exactly at the end of a chunk (
12then3\n) is still returned as a complete value, because nothing tellstryStreamingParsethat more input can follow. That is a question about the API and not about the lexer.