markdown: reject inputs of 2^32 bytes or more instead of panicking - #32752
Conversation
Parser::init stored the input length via OFF::try_from(text.len()).expect(), so Bun.markdown.html/ansi/render/react with a 4 GiB typed array crashed with 'panic: int cast: TryFromIntError(PosOverflow)'. Every offset in the parser is a u32, so such inputs cannot be represented at all. Add ParserError::InputTooLarge, return it from Parser::init, and surface it from the Bun.markdown host functions as a RangeError (ERR_OUT_OF_RANGE) naming input.byteLength and the 4294967295-byte limit. The four host functions now share one ParserError -> JS exception mapping, which also stops StackOverflow from being reported as out-of-memory by Bun.markdown.html.
|
Updated 7:51 AM PT - Jun 26th, 2026
❌ @robobun, your commit dd9dae6 has some failures in 🧪 To try this PR locally: bunx bun-pr 32752That installs a local version of the PR into your bun-32752 --bun |
|
Warning Review limit reached
More reviews will be available in 8 minutes and 41 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)
WalkthroughMarkdown parsing now rejects inputs too large for the parser offset type, propagates that error through render entry points, maps it to JS range errors in the runtime API, and adds subprocess tests for 2^32-byte inputs. ChangesOversized markdown input handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/md/md-edge-cases.test.ts`:
- Around line 1199-1231: The boundary test only covers the failing 2^32 case, so
it can miss an off-by-one regression where the max valid size is incorrectly
rejected. Extend the existing Bun.markdown.each coverage in
md-edge-cases.test.ts to add a sibling case for the same entry points (html,
ansi, render, react) that uses Uint8Array(2 ** 32 - 1) and verifies it succeeds,
while keeping the current 2^32 rejection check. Ensure the new assertion sits
alongside the existing expr-based test setup so the limit behavior is covered as
“at the limit succeeds, one past fails.”
- Around line 1220-1231: The subprocess test in the Bun.spawn call is inheriting
stderr, which lets native diagnostics escape without being asserted. Update the
md-edge-cases test to pipe stderr instead of inheriting it, using the existing
Bun.spawn/proc.stdout/proc.exited flow, and add an assertion that stderr remains
empty or is included in the same result check as stdout and exitCode. This keeps
the oversized input regression covered by a stronger invariant while preserving
the current stdout expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fee5378c-f38b-49fe-9223-a3fb77975ba4
📒 Files selected for processing (3)
src/md/parser.rssrc/runtime/api/MarkdownObject.rstest/js/bun/md/md-edge-cases.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…tput space AnsiRenderer::init reserves 1.5x the input length for the output buffer, and ran before Parser::init's length check, so Bun.markdown.ansi could still abort inside Vec::reserve on a host that cannot reserve ~6 GiB. Hoist the check into md::render_to_ansi via a shared parser::input_size helper so nothing is sized from an input the parser is going to reject.
There was a problem hiding this comment.
LGTM — both earlier findings (the ansi pre-reserve ordering and the stale doc comment) are addressed in 231ea41.
Extended reasoning...
Overview
This PR converts a process-aborting panic in Bun.markdown.{html,ansi,render,react} on inputs ≥ 2^32 bytes into a catchable RangeError. It touches four files: src/md/parser.rs (adds ParserError::InputTooLarge, makes Parser::init fallible, extracts input_size()), src/md/ansi_renderer.rs (hoists the length check before the output-buffer pre-reserve), src/runtime/api/MarkdownObject.rs (consolidates the four host functions' ParserError → JS-exception mapping into one parser_err_to_js helper), and a subprocess regression test in md-edge-cases.test.ts.
Security risks
None introduced. The change is strictly defensive: it replaces an unconditional process abort on user input with a catchable JS exception. No new input is accepted that was previously rejected, and the error path does not expose any data.
Level of scrutiny
Moderate. The markdown parser is user-facing runtime code, but the change is narrow error-path plumbing rather than parsing logic. The new enum variant is matched exhaustively everywhere it can surface (Rust enforces this), Parser::init's only two callers are updated to ?-propagate, and the consolidated parser_err_to_js preserves the existing per-variant behavior while fixing the prior over-collapse to OOM in html().
Other factors
A human reviewer (alii) already engaged on the parser.rs design and their concern (avoiding unreachable! arms) was addressed in 5a01093 by switching to process_doc()?. Both of my earlier findings on this PR — the .ansi() path doing a ~6 GiB Vec::reserve before the new guard, and the stale doc-comment enumeration — were fixed exactly as suggested in 231ea41 and the threads are resolved. CodeRabbit's two test-style suggestions were withdrawn after the author explained the repo conventions. The new test covers all four entry points with the exact error string and gracefully skips on hosts that cannot reserve 4 GiB. The musl build failures reported by CI were against the earlier commit 5a01093, not the current head.
…cess Four sequential children, each reserving 4 GiB under a debug ASAN build, can push a loaded runner past the per-test budget. One child exercises html, ansi, render and react and reports all four results; same coverage, one spawn.
…unners The child reserves 4 GiB and runs under a debug+ASAN build: give the test the same explicit timeout its perf-test neighbors in this file use, and let the ASan allocator return null so an allocation failure inside the child surfaces as a catchable error (the SKIP branch) instead of an abort it cannot catch.
There was a problem hiding this comment.
Looks good — both issues from my earlier pass (the ansi-path reserve ordering and the stale ParserError doc comment) are fixed in 231ea41, and the consolidated test covers all four entry points.
Extended reasoning...
Overview
This PR replaces a process-aborting panic in Bun.markdown.{html,ansi,render,react} with a catchable RangeError when the input is ≥ 2^32 bytes. It touches src/md/parser.rs (makes Parser::init fallible via a new ParserError::InputTooLarge variant and an input_size helper), src/md/ansi_renderer.rs (hoists the size check before the output-buffer reserve), src/runtime/api/MarkdownObject.rs (consolidates the four host functions' error mapping into one parser_err_to_js), and adds a subprocess regression test in test/js/bun/md/md-edge-cases.test.ts.
Security risks
None. This is a defensive change that converts an abort-on-user-input into a proper JS exception. No new input parsing, no auth/crypto/permission surface. The only user-visible behavior change beyond the new error is that Bun.markdown.html now reports StackOverflow as a stack-overflow error instead of collapsing it into OOM — a strict improvement.
Level of scrutiny
Moderate. The Rust signature change to Parser::init ripples to both public entry points (render_to_html, render_with_renderer), and the new enum variant required auditing every match site — all of which now go through parser_err_to_js with no wildcard arms. I verified the diff against my earlier finding: md::render_to_ansi now calls parser::input_size(text)? before AnsiRenderer::init, so the ~6 GiB reserve no longer runs for inputs the parser will reject. The doc comment on pub type Error was also updated.
Other factors
All review threads (alii's two on parser.rs, CodeRabbit's two on the test, and my two from the previous run) are resolved with the author's responses and follow-up commits. The bug-hunting pass on the current revision found nothing. The robobun CI comment flags failures on 7970229, but the subsequent commit dd9dae6 ("make the oversized-input test robust on loaded debug+ASAN runners") directly targets that — the test now uses a 30s timeout, JSON-encodes results from one subprocess, appends allocator_may_return_null=1 to ASAN_OPTIONS, and tolerates a SKIP outcome on hosts that cannot reserve 4 GiB.
|
CI status for build 64986 (latest commit): every lane is green except The same agent failed identically on every build it picked up today across unrelated PRs, so it is an infrastructure problem, not a test result. The markdown test lanes (including linux ASAN) are green. |
…e instead of panicking (#32764) ### Repro All four abort the process on the release binary with `panic: int cast: TryFromIntError(PosOverflow)`: ```js Bun.JSON5.parse(new Uint8Array(2 ** 31 + 2).fill(32)); const n = 2 ** 31 + 8, b = new Uint8Array(n).fill(10); b.set([97, 58, 32, 49, 10], n - 5); Bun.YAML.parse(b); Bun.TOML.parse("#" + "\u00e9".repeat(2 ** 30 + 4) + "\na=1"); Bun.JSONC.parse("//" + "\u00e9".repeat(2 ** 30 + 4) + "\n1"); ``` Same bug class as #32752 (`Bun.markdown`). ### Why Every parser behind `Bun.{JSON5,JSONC,TOML,YAML}.parse` records source positions as an `i32`: - JSONC and TOML go through `ast::usize2loc`, `start: i32::try_from(loc).expect("int cast")` (`src/ast/lib.rs`) - JSON5 builds every token with `start: i32::try_from(self.pos).expect("int cast")` (`src/parsers/json5.rs`) - YAML's `Pos::loc()` does `i32::try_from(self.0).expect("int cast")` (`src/parsers/yaml.rs`) JSON5 and YAML accept a `Blob` / `Buffer` directly, so a 2^32-byte `Uint8Array` gets 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 `\u00e9` repros 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, before `Source::init_path_string`: ``` RangeError [ERR_OUT_OF_RANGE]: The value of "input.byteLength" is out of range. It must be <= 2147483647. Received 2147483650 ``` Same code and wording as #32752. Not covered here, same `i32`-offset constraint but different entry points: `Bun.Transpiler.transformSync` and anything that loads a file into the JS parser or bundler. Those go through the shared `ast::Source` / logger machinery used by the whole build pipeline, which is a much larger change than the four `Bun.*` parse APIs. ### Test One test each in `test/js/bun/json5/json5.test.ts` and `test/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-byte `Uint8Array` is virtual pages that are never materialized; each test runs in ~6ms. JSONC and TOML only accept a string (`accept_blob_or_buffer` is 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. ``` bun bd test test/js/bun/json5/json5.test.ts # 321 pass bun bd test test/js/bun/yaml/yaml.test.ts # 609 pass bun bd test test/js/bun/jsonc/ # 14 pass bun bd test test/js/bun/resolve/toml/ # 17 pass USE_SYSTEM_BUN=1 bun test <json5,yaml files> -t "2**31" # 2 fail # json5: gets SyntaxError instead of RangeError # yaml: returns null instead of throwing ```
|
@robobun heads-up: this merged but the fix is incomplete — please open a follow-up PR. It only guards |
|
Thanks for the catch, opening a follow-up now. Plan:
On tests: the accepted-boundary side is covered (a |
|
Follow-up is up: #33078 covers all three (block-metadata cap via a new |
Follow-ups to #32752, requested in review after it merged. The first fix only guarded the input length read by Parser::init; three sibling limits in the same path were still fatal in release builds: - block_bytes growth: every block offset is a u32 (Container.block_byte_off, the casts in push_container and enter_child_containers), but nothing bounded the buffer, so a few hundred MB of nested blockquotes or headings panicked on 'int cast' in push_container. Reject the document at the two sites that grow the buffer (push_container_bytes, start_new_block) with a new ParserError::TooManyBlocks, reported to JS as a RangeError. - lookahead margin: input_size accepted lengths up to u32::MAX, but the parser probes up to 9 bytes past an offset (the <
What
Bun.markdown.html/.ansi/.render/.reactcrash the process on an input of 2^32 bytes or more:Why
Parser::init(src/md/parser.rs) converts the input length withOFF::try_from(text.len()).expect("int cast").OFFisu32, and the largest allocatableArrayBufferis exactly 2^32 bytes, so the cast overflows and the process aborts. Every offset, mark and span boundary in the parser is anOFF, so such inputs cannot be represented at all; they should surface as a catchable error, not a panic on user input.Fix
ParserError::InputTooLarge;Parser::initreturns it instead of panicking (via a sharedparser::input_sizehelper), and both public entry points propagate it, so every consumer ofbun_mdis covered.md::render_to_ansialso checks the length before constructing the renderer:AnsiRenderer::initreserves output space proportional to the input, so nothing may be sized from an input the parser is going to reject.Bun.markdownhost functions report it as aRangeErrorwith codeERR_OUT_OF_RANGE:The value of "input.byteLength" is out of range. It must be <= 4294967295. Received 4294967296. They now share oneParserErrorto JS exception mapping (parser_err_to_js); previouslyBun.markdown.htmlcollapsed every parser error (includingStackOverflow) into an out-of-memory error.bun_mdconsumers (the CLI markdown preview, the bundler loader) already route unrecognized parser errors to their generic error paths.Test
test/js/bun/md/md-edge-cases.test.tsspawns one subprocess that calls all four entry points with a 2^32-byteUint8Array(virtual only, never written, so the allocation is cheap) and asserts the exact RangeError from each. On the unfixed build the child dies with the panic above on the first call.