markdown: harden the parser's remaining 32-bit limits - #33078
Conversation
|
Updated 3:33 AM PT - Jun 30th, 2026
❌ @robobun, your commit fc66f1f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33078That installs a local version of the PR into your bun-33078 --bun |
WalkthroughThe markdown parser gains explicit size limit constants ( ChangesParser OOM/limit hardening
Possibly related PRs
🚥 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: 3
🤖 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 `@src/md/ansi_renderer.rs`:
- Around line 272-286: The OOM guard in AnsiRenderer only covers self.out, while
side buffers like image_alt, code_buf, heading_buf, table_cell_buf, table_cells,
and table_rows can still abort on allocation failure. Update the renderer paths
that write to these buffers to use fallible helpers that set the same oom state,
or change the relevant callbacks in AnsiRenderer to return Result so allocation
failures flow into to_owned_slice() as AllocError instead of panicking/aborting.
In `@src/md/blocks.rs`:
- Around line 850-854: `end_current_block` and the header-write path only guard
`block_bytes` growth in one place, but `VerbatimLine` appends can still overflow
the u32-addressable range before that check runs. Move the `MAX_BLOCK_BYTES`
validation into a shared helper used by both `end_current_block` and the
container/header write path, so every `block_bytes` growth is checked before
appending. Make sure `end_current_block` returns `parser::Error::TooManyBlocks`
on failure and avoid any panic-prone size arithmetic on external input.
In `@test/js/bun/md/md-edge-cases.test.ts`:
- Around line 1248-1254: The subprocess test in md-edge-cases.test.ts only
checks stdout via JSON.parse(stdout.trim()) and ignores the captured exitCode
from proc.exited, so a failing child process could still pass. Update the test
around the Promise.all(proc.stdout.text(), proc.exited) result to assert the
subprocess exits successfully before validating stdout, and keep the stdout
assertion in place using the existing message helper and SKIP expectation.
🪄 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: 2597bbe4-daca-4122-bfda-7176cb6cd111
📒 Files selected for processing (6)
src/md/ansi_renderer.rssrc/md/blocks.rssrc/md/containers.rssrc/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}
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
CI status for build 66810 (latest commit): every failure is on a lane or file this PR does not touch, and
Nothing markdown-related is red. All review feedback is addressed and the threads are resolved. |
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 <
b61c9ad to
6561c6a
Compare
…er can meet The 'documents with many reference definitions and references render in linear time' test spawns a debug+ASAN child over a 220k-line document with a 30s kill timer. On a loaded runner that child needs 24-45s (observed: SIGKILLed at 30.1s), which made it the flakiest test in this file. Raise the child's hard stop to 75s, still under the test's own 90s timeout; the workload and every assertion are unchanged.
|
CI status for build 67052 (latest commit, finished): every failure is in a lane or file this PR does not touch, and
Unrelated to markdown. The last commit also deflakes the one test in this file that was genuinely flaky under load (the reference-definition perf test's 30s child timeout; its debug+ASAN child needs 24-45s on a loaded runner), which is what kept intermittently failing earlier runs of this file. |
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
Adversarial design-review pass on this PR. The integer-overflow analysis is solid — I went after the MAX_BLOCK_BYTES headroom proof cast-by-cast and the MAX_LOOKAHEAD bound with a crate-wide grep and could not break either. But two of the three headline claims need work before this merges, so: requesting changes (in substance; left as a comment-review).
The 5 inline comments are ordered by severity: #1 (ANSI staging buffers + duplicated OutputBuffer) and #2 (zero coverage on TooManyBlocks when bun:internal-for-testing makes it a 1 KB test) are the request-changes. #3 (MAX_LOOKAHEAD proof is wrong and unenforced) and #4 (the new 20s inner SIGKILL) are should-fix. #5 is non-blocking notes from verifying the headroom proof.
What was probed and held up — no action needed
- Entry-point coverage is even. All four JS entry points (html / ansi / render / react, MarkdownObject.rs:201/168/318/418) reach the single
input_sizecall inParser::init(parser.rs:188-189) and route everyParserErrorthroughparser_err_to_js. No mode is missing the InputTooLarge/TooManyBlocks mapping. - The boundary test is not vacuous — the SKIP branch, the both-allocations-in-one-try shape, and the "fails for the wrong reason" angle were all run to ground.
indentaccumulation (helpers.rs:123) cannot overflow u32 in release given the now-enforced input bound;[profile.release]never setsoverflow-checks.- The
TooManyBlockserror shape and the hardcoded "(4 GiB)" are fine — the message states a different fact thanMAX_BLOCK_BYTES, so it's not a duplicated constant. - The CLI / bundler
.mdconsumers (run_command.rs:3565, transpiler.rs:2162, ParseTask.rs:867) only seeErr(_), so the new variants change nothing for them. - Widening the lookahead arithmetic instead of shrinking the input was considered; shrinking
MAX_INPUT_LENis the right design.
(Separately: I rebased the branch onto main earlier today to drop the stale [autofix.ci] commit that was reverting part of #33112's docs pass — the diff is now src/md + MarkdownObject.rs + the test only.)
… staging buffers The HTML and ANSI renderers each carried a byte-identical OutputBuffer; hoist it (plus try_extend/try_push helpers) into src/md/output.rs. The ANSI renderer routed code-block, heading, image-alt, and table-cell bytes through staging Vecs that still aborted on allocation failure one branch above the guarded output sink; those sites (and the table_rows/table_cells pushes) now record the failure on the same oom flag that render_to_ansi already checks, matching how the HTML renderer guards its heading buffer. resolve_href can no longer fail, so it returns Box<[u8]> directly.
MAX_LOOKAHEAD is now derived from the CDATA probe it exists for (line_analysis::CDATA_OPEN), and its doc states the real invariant: the longest OFF-typed fixed addition, not the longest lookahead. A const assert next to MAX_BLOCK_BYTES encodes the headroom proof so a larger BlockHeader fails to compile instead of silently shrinking the margin. The align/check/grow/write-header sequence that start_new_block and push_container_bytes duplicated moves into Parser::append_block_header, so a new header site cannot skip check_block_bytes_len; the remaining u32 casts of block_bytes.len() are annotated with the invariant that makes them safe. check_block_bytes_len now reads an AtomicUsize initialized to MAX_BLOCK_BYTES and lowerable (never raisable) through setMaxMarkdownBlockBytesForTesting in bun:internal-for-testing. The new test shrinks the cap to ~1 KB and proves the exact boundary: a document whose metadata lands on the cap renders, one more block (and a nested blockquote's container headers) throws the catchable ERR_OUT_OF_RANGE, and restoring the cap makes both render again. Also drops the redundant inner timeout/SIGKILL from the addressable limit test; the outer test timeout already bounds and reaps the child.
BlockHeader.data carries block-type payloads (heading level, list start, fenced-code info offset into the source text), never an offset into block_bytes, so it does not justify the cap.
|
@alii all five taken, in 240c35a and b5b7ffc (plus fc66f1f for one doc line). Per thread:
|
|
CI status for build 67192 (fc66f1f, finished): 283 jobs passed and 3 failed.
I am not pushing a retrigger for those, so this stays red until a maintainer re-runs or merges. Review feedback is addressed: all five of @alii's comments plus the later bot nit are replied to and resolved, and the PR body is rewritten. |
|
@robobun I left a review with 5 inline comments: #33078 (review) — please address them on this branch. Required before merge:
Should-fix:
Non-blocking but take it if it's clean:
When you're done, reply summarizing what you changed vs. deliberately skipped, per comment. |
|
@alii all five were addressed this morning in 240c35a, b5b7ffc, and fc66f1f, which are all in the squash Jarred merged (52a1ddf). Each inline thread has a specific reply and is resolved; here is the changed vs. deliberately-skipped summary per comment. 1. 2. 3. 4. 5. Verification on the final head: |
3 participants
Follow-up to #32752, requested by @alii after it merged (#32752 (comment) has the plan). That fix only guarded the input length that
Parser::initreads; three sibling limits in the same path were still fatal in release builds.What
Block metadata overflow. A block's offset into
block_bytesis stored as au32(Container.block_byte_off, and the casts inpush_container/enter_child_containersthat produce it), but nothing boundedblock_bytes, which grows by one 16-byte header per block plus 12 bytes per accumulated line. A few hundred MB of nested>or#(16x amplification) therefore still aborted in release:block_bytesis now capped atMAX_BLOCK_BYTESthrough one sharedparser::check_block_bytes_len, called by the only two growers:Parser::append_block_header(the align/check/grow/write sequence thatstart_new_blockandpush_container_bytesused to duplicate) andend_current_block's line append. Aconst _: () = assert!(..)next to the constant encodes the headroom proof, so enlargingBlockHeaderis a compile error instead of a silently shrunk margin, and the survivingu32casts are annotated with the invariant that makes them safe. JS sees aRangeError(ERR_OUT_OF_RANGE) saying the input requires more block metadata than the parser can address, because aninput.byteLengthbound would be wrong for a 300 MB input.Lookahead overflow at the accepted boundary.
input_sizeaccepted lengths up tou32::MAX, butis_html_block_start_conditionprobes 9 bytes past an in-bounds offset inOFF(u32) arithmetic, sooff + 9could wrap at the very end of a maximum-length input, pass the<= sizetest, and index out of bounds. The accepted maximum is nowMAX_INPUT_LEN = u32::MAX - MAX_LOOKAHEAD = 4294967286, whereMAX_LOOKAHEADis derived from the probe's own constant (line_analysis::CDATA_OPEN) rather than restated as a literal, so the two cannot drift. Its doc states the real invariant: the longestOFF-typed fixed addition (longer probes such asmatch_html_tag's add inusizeand cannot wrap).Uncatchable OOM in the ANSI renderer.
AnsiRenderer::initreserved 1.5x the input with the abortingVec::reserve, and its output path used plainextend_from_slice/push, so a failed allocation aborted the process even though the renderer already carries anoomflag thatrender_to_ansiconverts into an error. The two renderers' byte-identicalOutputBufferstructs are now one shared type insrc/md/output.rsusingtry_reserve, and the ANSI staging buffers that hold output-scale data before it reaches the sink (code_buf,heading_buf,image_alt,table_cell_buf, plus thetable_rows/table_cellsvectors) record their failure on the same flag, matching what the HTML renderer already did for its heading buffer. Scope, to be explicit: that flag covers the output sink and those staging buffers; the parser proper and the renderer's remaining per-node scratch keep the runtime's global abort-on-OOM policy, as before this PR.Tests
test/js/bun/md/md-edge-cases.test.ts, in subprocesses so thebun:internal-for-testingknob and the huge virtual buffers cannot leak into other tests:2^32 - 1, the accepted-boundary side) and asserts the new maximum in the message. On the previous code that length is accepted and the child grinds through a 4 GiB scan until the test times out.setMaxMarkdownBlockBytesForTesting(n)inbun:internal-for-testing(modeled onsetSyntheticAllocationLimitForTesting): lowers the block-metadata cap, never raises it past the real one, returns the previous value. The new test shrinks it to exactly 40 single-line paragraphs of metadata (28 bytes each) and proves the boundary: 40 paragraphs render, 41 throw, a nested blockquote throws through the container-header path, and both render again once the cap is restored. TheERR_OUT_OF_RANGEcode and message are asserted verbatim, so the guard, the error mapping, and the?propagation through the growers are all load-bearing.Not covered by a test: the
try_reserveconversion itself, since an allocation failure cannot be triggered deterministically in CI.