Repository navigation
Reject sources of 2 GiB or more before parsing instead of aborting in usize2loc - #39095
Conversation
Every parser records positions as an i32 Loc, and usize2loc aborts on the first position past i32::MAX. Bun.TOML.parse and the other JS entry points already rejected such inputs, but the parsers themselves did not, so a file of 2 GiB or more reached through bun build, bun run, import or Bun.Transpiler crashed the process (JS, TOML, YAML) or failed the XML indexer's length assertion. Add Source::check_parseable_len, which logs "<what> is too large to parse (2 GiB maximum)" against the source (without a position, so the oversized file is not scanned) and call it at the entry of the JS parser, TOML, YAML and XML, so every caller of those parsers gets an error instead of a crash. JSON and JSONC already had an equivalent check in the structural indexer.
|
Warning Review limit reached
Next review available in: 31 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
Comment |
|
Updated 11:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit 6fe9599 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 39095That installs a local version of the PR into your bun-39095 --bun |
|
Status: fix and tests pushed, waiting on CI. Reproduced on 1.4.0 with sparse files ( test/js/bun/transpiler/source-too-large.test.ts fails on 1.4.0 ( Review so far: the XML transcoding edge case from the review is handled in f82bb18 (the indexer asserts the u32 bound it relies on); the new comments were trimmed to one line each. |
There was a problem hiding this comment.
LGTM. The one inline nit is a pre-existing debug-only edge case (XML in-parser transcoding of >1 GiB inputs), not a regression introduced here, and doesn't block — the change is a strict improvement over the current abort.
Extended reasoning...
Overview
Adds a single shared guard, Source::check_parseable_len, in bun_ast next to Loc/usize2loc (whose i32 precondition it enforces), and calls it once at the entry of each affected parser: JS/TS Parser::init, TOML::parse, YAML::parse, and XML parse_units. A new zero-size SourceTooLarge error converts via From into each crate's existing already-logged SyntaxError variant, so every caller — bun build, bun run, import, Bun.Transpiler, bunfig, pnpm — surfaces it exactly as it surfaces any other parse error today. A new test file covers every loader through Bun.Transpiler plus bun build/bun run via sparse 2 GiB files.
Security risks
None. This is a length guard that turns a process abort on adversarial-size input into a recoverable, logged error. No new input parsing, no new trust boundaries, no allocation driven by the oversized length.
Level of scrutiny
The JS parser entry point is hot-path production code, but the added line is a branch that's trivially false for every input under 2 GiB — behavior for real workloads is byte-identical. The error propagation reuses the existing SyntaxError variant that every caller already handles, so no caller needs updating. add_error_fmt(Some(self), Loc::EMPTY, ...) matches how other position-less diagnostics are logged (e.g. lib.rs:1804). The message wording mirrors the JSON parser's existing check. This is a small, mechanical, well-scoped defensive fix.
Other factors
- The inline nit about XML transcoding (
transcode_utf16/ Latin-1 → UTF-8 expandingself.srcpast 2 GiB after the check) is real but pre-existing, debug-only, and requires >1 GiB of pathological all-high-byte content. The PR description already names the Latin-1 re-parse case explicitly; release builds are unaffected becauseScanner::locreturnsLoc::EMPTYwhentranscoded. It's a reasonable follow-up, not a blocker. - Test design is careful: subprocess with drained pipes, sparse files to avoid disk cost,
totalmem()skip gate copied fromfs-oom.test.ts, first line is a syntax error in every format so a build without the check fails fast rather than scanning 2 GiB, and json/jsonc are included to pin existing behavior. The 120s per-test timeout on the two file-based cases is justified in-comment (debug allocator bookkeeping on a 2 GiB read). - No CODEOWNERS cover the touched files.
- PR description documents
USE_SYSTEM_BUN=1failure andbun bd testpass, plus clippy on the three crates.
The parser bounds its input to i32::MAX units before building the index, but transcoding UTF-16 or Latin-1 input to UTF-8 can grow it by up to 2x; positions are stored as u32, so that is the bound the index's assertion should state.
There was a problem hiding this comment.
LGTM — the XML transcoding concern from the earlier review is addressed by relaxing the indexer's debug_assert to u32::MAX (f82bb18), and the comment-cop lints are resolved (6fe9599).
Checked that check_parseable_len sits at the single entry point of each parser and is a no-op fast-path for inputs under 2 GiB.
Verified parse_utf16's caller casts source.contents to &[u16], so the byte-length check bounds the unit count too, and InputEncoding::Text skips transcoding on that path.
Confirmed 2×(i32::MAX) < u32::MAX and Scanner::loc gates i32 conversion on !transcoded, so the relaxed assertion matches the actual invariant.
The three From<SourceTooLarge> impls route to each crate's already-logged SyntaxError variant, so callers report it like any other parse error.
Extended reasoning...
Overview
Adds Source::check_parseable_len and SourceTooLarge to bun_ast (next to Loc/usize2loc, whose i32 precondition it establishes), calls it at the single entry point of each affected parser (Parser::init for JS/TS, TOML::parse, YAML::parse, XML's parse_units), converts SourceTooLarge into each crate's existing SyntaxError variant, and relaxes the XML structural indexer's debug_assert from i32::MAX to u32::MAX to reflect the bound it actually relies on after in-parser transcoding. New test file exercises all seven loaders through Bun.Transpiler, plus bun build and bun run with sparse 2 GiB files.
Security risks
None. This adds an early length rejection; it cannot expose data or bypass checks. The change strictly tightens behavior (abort → recoverable error) and only fires on inputs ≥ 2 GiB.
Level of scrutiny
Moderate. The change touches the JS parser entry point (as hot as it gets), but the added code is a single usize comparison that returns Ok(()) on every input under 2 GiB — no observable change to normal parsing. For oversized inputs, a process abort becomes a logged SyntaxError routed through existing error machinery, which every caller already handles. The XML debug_assert relaxation was the only subtle piece; the earlier review examined it in detail and the author's fix (assert u32::MAX, since transcoded positions never reach i32 conversion) is the correct one of the two options I proposed.
Other factors
- My earlier inline finding (XML transcoding could grow past the checked length) was addressed exactly as suggested; the thread is resolved.
- The comment-cop bot flagged multi-line comments; those were trimmed in 6fe9599 and the threads are resolved.
- Tests cover the full loader matrix including json/jsonc (pinning existing behavior), and both file-based entry points. The 2 GiB file cases are gated on
totalmem() >= 10 GiB(matchingfs-oom.test.ts) and use sparse files to avoid disk cost. - The
From<SourceTooLarge>impls in three separate error enums are mechanical and each maps to the existingSyntaxErrorvariant, preserving the "already logged" contract. - CSS parser and JSON5 are noted as having the same bug class but explicitly scoped out (JSON5 is #38936); that's a stated exclusion, not an oversight.
ast/lib.rs: add_formatted_msg lost its clone flag on main while this branch made the range optional, so the four callers take both. #39095's new Source::check_parseable_len follows this branch as its description said it would: the error carries no location, and the 2 GiB limit stays, now because Range::len and reported columns are still i32 rather than Loc itself.
Problem
bun build,bun run,importorBun.Transpiler:panic: int cast: TryFromIntError(PosOverflow)/Crashed while parsing big.js(Lexer::loc->bun_ast::usize2loc, src/ast/lib.rs)panic: source length is bounded by i32::MAX: TryFromIntError(PosOverflow)(loc_of, src/parsers/toml.rs); nothing enforced that boundpanic: int cast: TryFromIntError(PosOverflow)(Pos::loc, src/parsers/yaml.rs)panic: assertion failed: contents.len() <= i32::MAX as usizein debug builds (src/parsers/xml_index.rs:40); release builds silently saturate positionsi32Loc, and Bun.{JSON5,JSONC,TOML,YAML}.parse: reject inputs of 2^31 bytes or more instead of panicking #32764 only added the length check to theBun.*.parseJS 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.tomlloading, pnpm migration andBun.Transpilerhand them whatever they read.writeFileSync(f, "/*"); truncateSync(f, 2 ** 31); appendFileSync(f, "*/\nx"), thenbun f.jsorbun build f.js), and with 2 GiB of newlines followed bya = 1/a: 1for TOML and YAML. JSON and JSONC are not affected: their structural indexer already reportsJSON document is too large to parse (2 GiB maximum). JSON5 has the same bug and is being fixed in Add a ratchet for expect("int cast") sites and clear json5, image codecs and elf #38936.Fix
Source::check_parseable_len(log, what)in bun_ast (next toLocandusize2loc, whose precondition it establishes) logs<what> is too large to parse (2 GiB maximum)against the source and returnsErr(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).Parser::init(all JS/TS parsing, including scans and the transpiler API),TOML::parse,YAML::parseand XML'sparse_units(bothparseandparse_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::locattaches 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.SourceTooLargeconverts into each crate's already-loggedSyntaxErrorvariant, so every existing caller reports it the way it reports any other parse error:bun buildprints a build error and exits 1,importrejects with aBuildMessage,Bun.Transpilerthrows one, bunfig and pnpm report a parse error.i32limit 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.Bun.Transpilerwith a 2 GiB buffer for js, ts, toml, yaml, xml (plus json and jsonc to pin the existing behavior),bun buildof a 2 GiB .xml, andbun runof a 2 GiB .js, both sparse files. Passes withbun bd test; withUSE_SYSTEM_BUN=1(1.4.0) all three fail, and the unfixed debug build aborts on thebun buildcase. 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 teston the toml, xml, yaml, resolve/{toml,yaml,xml,jsonc}, transpiler and bundler_loader suites;cargo clippyon bun_ast, bun_parsers, bun_js_parser; the source lints.Loc; if it lands first,MAX_PARSEABLE_LENand theLoc::EMPTYargument 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 thebun buildtest case uses a data-format file).Background
Loc(bun_ast) is the position type stored in every AST node and diagnostic: ani32byte offset into the source.usize2locis the shared conversion from a parser'susizecursor; like the parsers' private equivalents it is anexpect, and the binary builds withpanic = "abort", so an offset pasti32::MAXis a process abort.Sourceis the path plus contents handed to every parser, whether the bytes came from a file read, a bundler plugin or a JS string.Logcollects diagnostics; a parse error is logged and then signalled to the caller with a bareSyntaxErrorvalue, which is the convention the new error converts into.truncateSyncpast 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.Before and after
Release 1.4.0, sparse
/*+ 2 GiB hole +*/JS file:Release 1.4.0, 2 GiB of newlines followed by one line:
Unfixed debug build, any 2 GiB .xml:
panic: assertion failed: contents.len() <= i32::MAX as usize.With this change (debug build):
Bun.Transpiler.transformSyncwith a 2 GiBUint8Array, per loader: js/tsFile 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.