Bun.{JSON5,JSONC,TOML,YAML}.parse: reject inputs of 2^31 bytes or more instead of panicking - #32764
Conversation
…e instead of panicking
Every parser reached from `with_text_format_source` records source positions
as an `i32` (`ast::Loc` via `usize2loc` for JSONC and TOML, JSON5's token
locs, YAML's `Pos`), so once an offset passes `i32::MAX` the lexer's
`i32::try_from(..).expect("int cast")` aborts the whole process with
`panic: int cast: TryFromIntError(PosOverflow)`. A Buffer of 2^32 bytes (or
a latin1 string whose UTF-8 form exceeds 2^31 bytes) is enough to get there.
All four host fns share `with_text_format_source` as their only entry point,
so the byte length is checked there, before `Source::init_path_string`. Too
long throws `RangeError [ERR_OUT_OF_RANGE]: The value of "input.byteLength"
is out of range. It must be <= 2147483647. Received <n>`.
The check runs before any of the input is read, so the 2 GiB `Uint8Array`
the tests pass stays as untouched virtual pages.
|
Warning Review limit reached
More reviews will be available in 3 minutes and 20 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 8:57 AM PT - Jun 26th, 2026
❌ @robobun, your commit 93889b2 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32764That installs a local version of the PR into your bun-32764 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward defensive length guard following the established throw_range_error pattern; the inline nit about guarding the 2 GiB test allocation is optional.
Extended reasoning...
Overview
This PR adds a single if bytes.len() > i32::MAX as usize guard to with_text_format_source in src/runtime/api.rs — the shared entry point for Bun.{JSON5,JSONC,TOML,YAML}.parse. All four parsers store source positions as i32, so inputs ≥ 2^31 bytes previously aborted the process with panic: int cast: TryFromIntError(PosOverflow). The guard now throws a Node-style RangeError [ERR_OUT_OF_RANGE] before any parsing begins. Two near-identical regression tests are appended to the JSON5 and YAML test files.
Security risks
None introduced. This is strictly a hardening change: it converts an unrecoverable process abort (DoS-class) into a catchable JS exception. No new code paths accept input, no parsing of untrusted data is added, and the guard is a pure length comparison with no allocation or side effects.
Level of scrutiny
Low. The Rust change is ~10 lines and mirrors the exact throw_range_error / RangeErrorOptions { field_name, max, ..Default::default() } pattern already used in src/runtime/webcore/s3/credentials_jsc.rs, Blob.rs, and elsewhere. The boundary (> i32::MAX) is correct for the documented i32 offset constraint, the check sits after both the Blob/Buffer and string→UTF-8 resolution paths so it covers all four formats, and bytes.len() as i64 cannot lose precision for any allocation that fits in addressable memory. None of the touched files match CODEOWNERS patterns.
Other factors
The PR description is thorough — repro, root cause, fix rationale, and verified test results (including a negative test against the system bun) are all provided. The one inline finding is a test-robustness nit (unguarded ~2 GiB Uint8Array allocation may fail on Windows/ASAN runners rather than skip); the author already acknowledged the virtual-page assumption in both the PR body and the test comment, and CI will surface it immediately if it actually trips. It does not affect the correctness of the production fix and is not a blocker.
There was a problem hiding this comment.
LGTM — thanks for adding the allocation guard in the tests.
Extended reasoning...
Overview
The PR adds a single bounds check in with_text_format_source (src/runtime/api.rs), the shared entry point for Bun.{JSON5,JSONC,TOML,YAML}.parse. Inputs whose UTF-8 byte length exceeds i32::MAX now throw a RangeError [ERR_OUT_OF_RANGE] instead of aborting the process via i32::try_from(...).expect("int cast") deep in the lexers. Two regression tests (json5, yaml) assert the exact error class/code/message. The production change is ~10 lines of logic plus a comment; the tests are ~28 lines each.
Security risks
None. This is a defensive input-size guard that strictly narrows behavior from process-abort to a catchable JS error. It cannot affect any input under 2 GiB, and there is no new parsing, no new allocation path, and no user-controlled data flowing into the error message beyond the byte count.
Level of scrutiny
Low. The change is mechanical (if bytes.len() > i32::MAX as usize { throw_range_error(...) }), uses the existing throw_range_error / RangeErrorOptions helper already used across the codebase, and mirrors the just-merged #32752 fix for Bun.markdown. The guard sits after argument-to-bytes resolution and before Source::init_path_string, so all four parsers are covered with no per-format duplication. No CODEOWNERS paths are touched.
Other factors
My only prior feedback (a nit about the unguarded 2 GiB Uint8Array allocation in the tests potentially failing on Windows/ASAN runners) was addressed in commit 93889b2 — both tests now wrap the allocation in try { ... } catch { return; }, matching the pattern used by the other ≥2 GiB-allocation tests in the suite. That thread is resolved. The bug-hunting system found no issues on the current revision. The robobun CI ❌ references the earlier commit (6ae69ad), before the test guard was added; the follow-up commit targets exactly that class of failure.
|
CI status note for reviewers: no build has ever failed on anything this diff touches. The red lanes in build 65006 are |
… usize2loc (#39095) ### Problem - A source file of 2 GiB or more aborts the process instead of producing an error when it reaches a parser through `bun build`, `bun run`, `import` or `Bun.Transpiler`: - JS/TS: `panic: int cast: TryFromIntError(PosOverflow)` / `Crashed while parsing big.js` (`Lexer::loc` -> `bun_ast::usize2loc`, src/ast/lib.rs) - TOML: `panic: source length is bounded by i32::MAX: TryFromIntError(PosOverflow)` (`loc_of`, src/parsers/toml.rs); nothing enforced that bound - YAML: `panic: int cast: TryFromIntError(PosOverflow)` (`Pos::loc`, src/parsers/yaml.rs) - XML: `panic: assertion failed: contents.len() <= i32::MAX as usize` in debug builds (src/parsers/xml_index.rs:40); release builds silently saturate positions - Cause: every parser records positions as an `i32` `Loc`, and #32764 only added the length check to the `Bun.*.parse` JS entry points (src/runtime/api.rs). The parsers themselves accept any length, and the file-based entry points (src/bundler/ParseTask.rs, src/bundler/transpiler.rs), `bunfig.toml` loading, pnpm migration and `Bun.Transpiler` hand them whatever they read. - Reproduces on 1.4.0 with a sparse file (`writeFileSync(f, "/*"); truncateSync(f, 2 ** 31); appendFileSync(f, "*/\nx")`, then `bun f.js` or `bun build f.js`), and with 2 GiB of newlines followed by `a = 1` / `a: 1` for TOML and YAML. JSON and JSONC are not affected: their structural indexer already reports `JSON document is too large to parse (2 GiB maximum)`. JSON5 has the same bug and is being fixed in #38936. ### Fix - `Source::check_parseable_len(log, what)` in bun_ast (next to `Loc` and `usize2loc`, whose precondition it establishes) logs `<what> is too large to parse (2 GiB maximum)` against the source and returns `Err(SourceTooLarge)`. The error is attributed to the file without a position: computing one would scan the oversized file to find the end of its line (that scan is why the existing JSON check, which reports at offset 0, takes seconds on a single-line file). - Called at the single entry point of each affected parser: `Parser::init` (all JS/TS parsing, including scans and the transpiler API), `TOML::parse`, `YAML::parse` and XML's `parse_units` (both `parse` and `parse_utf16`). The XML indexer's assertion now states the bound it actually relies on, u32: the scanner may transcode UTF-16 or Latin-1 input to UTF-8 after the entry check, which at most doubles it, and positions in transcoded input only ever live in the u32 index (`Scanner::loc` attaches no location to them), so an input under the limit that grows past 2 GiB when transcoded keeps parsing, as it does in release builds today. `SourceTooLarge` converts into each crate's already-logged `SyntaxError` variant, so every existing caller reports it the way it reports any other parse error: `bun build` prints a build error and exits 1, `import` rejects with a `BuildMessage`, `Bun.Transpiler` throws one, bunfig and pnpm report a parse error. - Why the parsers rather than the file loaders: the `i32` limit is the parsers' own precondition, and they are reached from more places than the two file loaders (bunfig, pnpm, S3 XML responses, test snapshots, `Bun.Transpiler`, bundler plugins returning contents, and XML's Latin-1 to UTF-8 re-parse, whose input can be twice the length the API check measured). Checking at the parser covers all of them and matches what the JSON parser already does. The message wording follows the JSON one. - Verified: - test/js/bun/transpiler/source-too-large.test.ts: `Bun.Transpiler` with a 2 GiB buffer for js, ts, toml, yaml, xml (plus json and jsonc to pin the existing behavior), `bun build` of a 2 GiB .xml, and `bun run` of a 2 GiB .js, both sparse files. Passes with `bun bd test`; with `USE_SYSTEM_BUN=1` (1.4.0) all three fail, and the unfixed debug build aborts on the `bun build` case. The fixtures' first line is a syntax error in every format so that a build without the check fails fast instead of scanning 2 GiB; the crash itself needs parseable content past 2 GiB, which is what the repros above use. - `bun bd test` on the toml, xml, yaml, resolve/{toml,yaml,xml,jsonc}, transpiler and bundler_loader suites; `cargo clippy` on bun_ast, bun_parsers, bun_js_parser; the source lints. - Related: #38825 changes the representation of `Loc`; if it lands first, `MAX_PARSEABLE_LEN` and the `Loc::EMPTY` argument in the helper are the only two lines here that need to follow it. Two things found on the way are left for separate changes: the CSS parser has the same class of casts, and the bundler's empty fallback AST for an unparsable JS file presizes its symbol tables from the source length (correct but slow in debug builds, which is why the `bun build` test case uses a data-format file). ### Background - `Loc` (bun_ast) is the position type stored in every AST node and diagnostic: an `i32` byte offset into the source. `usize2loc` is the shared conversion from a parser's `usize` cursor; like the parsers' private equivalents it is an `expect`, and the binary builds with `panic = "abort"`, so an offset past `i32::MAX` is a process abort. - `Source` is the path plus contents handed to every parser, whether the bytes came from a file read, a bundler plugin or a JS string. `Log` collects diagnostics; a parse error is logged and then signalled to the caller with a bare `SyntaxError` value, which is the convention the new error converts into. - A sparse file (`truncateSync` past the end) takes no disk space and reads back as NUL bytes, which is enough to exercise the length check; the tests use that so the 2 GiB fixtures cost only the memory of reading them. <details> <summary>Before and after</summary> Release 1.4.0, sparse `/*` + 2 GiB hole + `*/` JS file: ``` $ bun block.js panic: int cast: TryFromIntError(PosOverflow) Crashed while parsing /tmp/repro/block.js $ bun build block.js --outdir out panic: int cast: TryFromIntError(PosOverflow) Crashed while parsing /tmp/repro/block.js ``` Release 1.4.0, 2 GiB of newlines followed by one line: ``` import('./dense.toml') -> panic: source length is bounded by i32::MAX: TryFromIntError(PosOverflow) import('./dense.yaml') -> panic: int cast: TryFromIntError(PosOverflow) ``` Unfixed debug build, any 2 GiB .xml: `panic: assertion failed: contents.len() <= i32::MAX as usize`. With this change (debug build): ``` $ bun build block.js --outdir out error: File is too large to parse (2 GiB maximum) at /tmp/repro/block.js $ bun build big.toml --outdir out error: TOML document is too large to parse (2 GiB maximum) at /tmp/repro/big.toml $ bun -e "import('./block.js').catch(e => console.log(e.name, e.message))" BuildMessage File is too large to parse (2 GiB maximum) ``` `Bun.Transpiler.transformSync` with a 2 GiB `Uint8Array`, per loader: js/ts `File is too large to parse (2 GiB maximum)`, toml/yaml/xml `<FORMAT> document is too large to parse (2 GiB maximum)`, json/jsonc unchanged (`JSON document is too large to parse (2 GiB maximum)`), each in about a millisecond. </details>
2 participants
Repro
All four abort the process on the release binary with
panic: int cast: TryFromIntError(PosOverflow):Same bug class as #32752 (
Bun.markdown).Why
Every parser behind
Bun.{JSON5,JSONC,TOML,YAML}.parserecords source positions as ani32:ast::usize2loc,start: i32::try_from(loc).expect("int cast")(src/ast/lib.rs)start: i32::try_from(self.pos).expect("int cast")(src/parsers/json5.rs)Pos::loc()doesi32::try_from(self.0).expect("int cast")(src/parsers/yaml.rs)JSON5 and YAML accept a
Blob/Bufferdirectly, so a 2^32-byteUint8Arraygets there in one call. All four accept a string, and JSC strings are Latin-1 or UTF-16 internally, so a string well under 2 GiB can still produce more than 2^31 UTF-8 bytes (the\u00e9repros above). No entry point checked the length.Fix
All four host fns already share one entry point,
with_text_format_source(src/runtime/api.rs), which resolves the argument to the&[u8]the parser will see. It now rejects anything the parsers' offsets cannot represent, beforeSource::init_path_string:Same code and wording as #32752.
Not covered here, same
i32-offset constraint but different entry points:Bun.Transpiler.transformSyncand anything that loads a file into the JS parser or bundler. Those go through the sharedast::Source/ logger machinery used by the whole build pipeline, which is a much larger change than the fourBun.*parse APIs.Test
One test each in
test/js/bun/json5/json5.test.tsandtest/js/bun/yaml/yaml.test.ts, asserting the exact error class, code and message. The check happens before any byte of the input is read, so the 2^31+2-byteUint8Arrayis virtual pages that are never materialized; each test runs in ~6ms.JSONC and TOML only accept a string (
accept_blob_or_bufferis false for them), and the cheapest string whose UTF-8 form exceeds 2^31 bytes commits ~3 GB both before and after the fix, so they have no automated test. They call the identical guard five lines away from the two that do.