Repository navigation
Reject json() with the JSON.parse SyntaxError on the buffered path - #41387
Conversation
BunString__toJSON and EncodedSlice__toJSONObject called JSC::JSONParse, which returns an empty value on a syntax error, and then threw a generic "Failed to parse JSON". The streamed path of ReadableStream.prototype.json() calls JSONParseWithException, which throws the parser message. So the error of proc.stdout.json() depended on whether the pipe was at EOF when JS first read proc.stdout. Both helpers now call JSONParseWithException. They parse "" for an empty input, because JSONParseWithException does not throw for a null string. For the same text, the helpers now throw the SyntaxError that JSON.parse throws. The empty-body checks in Blob.rs run before the helpers, and they do not change.
|
Reproduction, on the released build (1.4.1):
With this branch, both paths reject with the |
|
Updated 1:29 PM PT - Sep 4th, 2026
✅ @robobun, your commit f681cf8b9d560fc1892fb10e9c4637d9f34bcf0c passed in 🧪 To try this PR locally: bunx bun-pr 41387That installs a local version of the PR into your bun-41387 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughSummaryJSON bindings now use ChangesJSON error consistency
Baseline static allowlist
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The update records additional LLInt decoder false positives in the x64 baseline allowlist without changing runtime behavior. No merge-readiness risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
…sitive The x64-musl static scan of this branch flags one RDPMC in llint_op_jmp_wide32. This is the documented LLInt data-in-.text false positive. Each opcode handler starts with an embedded opcode id, and the linear sweep desyncs on it. The .text layout changed, so the desync moved from llint_op_enter_wide32 and llint_op_wide16_wide16 to this sibling. The same scan lists both of those entries as stale, and the emulator phase of the same job passes.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Each helper now picks emptyString() for an empty input when it reads the input, so the later parse needs no special case. A one-line comment gives the reason: the conversion returns a null string for an empty input, and JSONParseWithException throws nothing for null.
|
Two commits since the last review:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ries The triage guide says to allowlist a confirmed data-in-.text misdecode as a blanket pass. The LLInt handlers are offlineasm output, so no compiler flag can add an instruction to them, and the feature that the scan reports is garbage that moves with link layout. This branch shows it: the same desync decoded as AVX in two handlers, and as RDPMC in a third after the layout changed. A ceiling on these entries only makes the next layout fail again.
|
The review found nothing blocking. I took its one optional note in |
There was a problem hiding this comment.
LGTM — the allowlist entries are now blanket passes per the data-in-.text rule, and the (3 symbols) count matches.
Checked the two C++ call sites: the empty-input guard precedes JSONParseWithException so a null WTF::String never reaches it, RETURN_IF_EXCEPTION follows the parse, and the !result && !scope.exception() fallback still covers the dead-string / failed-conversion case so zero_is_throw holds on every path. The isNull() branch in EncodedSlice__toJSONObject is now unreachable for len == 0, so the updated comment and the too-long check remain coherent. Tests exercise both the buffered and streamed paths against JSON.parse's own message, and the BOM case covers the empty-after-decode edge.
Extended reasoning...
Overview
This PR swaps JSC::JSONParse for JSC::JSONParseWithException in BunString__toJSON (src/jsc/bindings/BunString.cpp) and EncodedSlice__toJSONObject (src/jsc/bindings/bindings.cpp) so that buffered body.json() / ReadableStream.prototype.json() reject with the same SyntaxError message as JSON.parse and the streamed path, eliminating a timing-dependent flake in readablestream-helpers.test.ts. Both sites substitute emptyString() for the null WTF::String that empty input would otherwise produce. Tests in three existing files assert the exact JSON.parse message on both paths. Separately, scripts/verify-baseline-static/allowlist-x64.txt adds llint_op_jmp_wide32 and — after my earlier inline comment — converts all three LLInt embedded-opcode-ID entries to blanket passes with an updated group comment and count.
Security risks
None. This changes only which error message surfaces on a JSON parse failure; no new inputs are accepted, no bounds or allocation logic changes, and the zero_is_throw FFI contract is preserved on every path (fallback throw remains for a dead/null string). The allowlist change is a documented data-in-.text misdecode of LLInt opcode IDs, backed by the SDE emulation phase in the same job.
Level of scrutiny
Moderate. C++ JSC bindings with exception scopes warrant care, but the change is a two-line API swap plus a null-guard, matching the existing structure. The author reports running the affected tests under BUN_JSC_validateExceptionChecks=1 on the ASAN build. I verified the throw-scope discipline (RETURN_IF_EXCEPTION after the throwing call, no path returning encoded 0 without a pending exception) and that the empty-input branch cannot bypass the too-long check in EncodedSlice__toJSONObject.
Other factors
My previous review's only finding — that the LLInt allowlist entries needed to be blanket passes per scripts/verify-baseline-static/CLAUDE.md — was addressed in commit f681cf8: entries are now bare symbols, the comment explains "a ceiling has nothing to bound", and the (N symbols) count is correct. No outstanding third-party CHANGES_REQUESTED reviews. Test additions follow the conventions (existing files, test.each, single .toEqual for the compound assertion, no timing waits).
Problem
test/js/bun/spawn/readablestream-helpers.test.tsfails at random. On PR builds 110091 (debian 13 aarch64) and 110186 (windows 11 aarch64), a case gotFailed to parse JSONwhere it expectedJSON Parse error: Expected '}', or the reverse.json()on a stream that holds the whole body parses it inBunString__toJSON(src/jsc/bindings/BunString.cpp:522) orEncodedSlice__toJSONObject(src/jsc/bindings/bindings.cpp:2748). These callJSC::JSONParseand throw the generic message. A streamed body gets theJSON.parsemessage.proc.stdout, timing selects the path. ReadableStream .text(), .json(), .blob(), .bytes() #20694 pinned one message per case. Since ci: retry only the test files listed in test/flaky-tests.txt #41204, one failure is final.Fix
JSONParseWithException, and parse""for an empty input. For the same text, they throw the SyntaxError thatJSON.parsethrows. The removed "See Bun.serve crashes with seg fault when decoding certain binary data as JSON #5859" comment is stale (see Notes).Response.json(),Request.json(),Blob.json()andBun.file().json()change the same way. The fetch spec rethrows theJSON.parseexception. Node does too.scripts/verify-baseline-static/allowlist-x64.txtgetsllint_op_jmp_wide32, a documented LLInt decode false positive that moved with the code layout (see Notes).BUN_JSC_validateExceptionChecks=1:readablestream-helpers.test.ts(new cases),body.test.ts,serve.test.ts. Self-reviewed: 5 concerns raised, 4 addressed.Background
json()tries it first.JSC::JSONParsereturns an empty value on a syntax error and throws nothing.proc.stdoutwhen JS first reads it. If the pipe is at EOF then, it is a Blob stream.Bun.spawn()reads the pipe once before it returns, and on Windowsexitedcan resolve before EOF.Notes
new Blob([s]).stream().json()rejects withFailed to parse JSON. A stream with a JavaScript source rejects withJSON Parse error: Expected '}'.proc.stdout.json()with no await afterBun.spawn(): 11 of 50 runs took the buffered path when the parent and a fast child (/bin/echo) share one CPU. This is the debian case.await proc.exited, a grandchild that keeps the pipe open makes the streamed path run. This is the Windows case.JSONParseWithExceptionthrows nothing for a null string, and an empty input converts to one. A child that printed only a byte order mark gotJSON Parse error: Unexpected EOFon the streamed path andFailed to parse JSONon the buffered path. Both paths now give the first. A zero-byte stdout was already the same on both paths.BUN_JSC_validateExceptionChecks=1.:linux: x64-musl - verify-baselinestep flagged oneRDPMCinllint_op_jmp_wide32. Each LLInt opcode handler starts with an embedded 4-byte opcode id, and the linear-sweep decoder desyncs on it. The allowlist already covers two sibling handlers for this reason. The same scan lists those two entries as stale, so the desync moved to this handler. The emulator phase of the same job passes, and the step passed on main at the same base commit. The three LLInt entries are blanket passes, as the triage guide inscripts/verify-baseline-static/CLAUDE.mdsays for a data-in-.text misdecode.Failed to parse JSONfor a string that failed to convert (a dead string, or a failed allocation).Blob.rsrun before the helpers. They resolvenullor throwUnexpected end of JSON input. That is Response.json behavior is different between bun and node #24955, and this PR does not change them.Blob.rschange.Blob::to_json_with_bytesdecodes a body that starts with FF FE as UTF-16LE, and the streamed path decodes it as UTF-8. That difference is separate from this change.BUN_JSC_validateExceptionChecks=1.test/js/bun/spawn/readablestream-helpers.test.ts: 32 pass. The released build fails 3 (the two new cases and(after exited)).test/js/web/fetch/body.test.ts: 476 pass. The released build fails 34 (17 inputs, forRequestandResponse).test/js/bun/http/serve.test.ts: 295 pass, 2 fail. The 2 failures (root range port(#7187)and/bun:info to loopback clients) also fail on the released build in this environment, which runs as root.bun-file.test.ts,regression/issue/02367.test.ts,express.json.test.ts,spawn.ipc.test.ts,spawn.ipc.bun-node.test.ts, and the json cases offetch.test.ts. The one failure there,bad permissions throws, also fails as root on the released build.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/body.test.ts, test/js/bun/http/serve.test.ts