Skip to content

Add a ratchet for expect("int cast") sites and clear json5, image codecs and elf - #38936

Open
robobun wants to merge 9 commits into
mainfrom
farm/9f8fc102/int-cast-ratchet
Open

Add a ratchet for expect("int cast") sites and clear json5, image codecs and elf#38936
robobun wants to merge 9 commits into
mainfrom
farm/9f8fc102/int-cast-ratchet

Conversation

@robobun

@robobun robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Problem

Fix

  • test/internal/source-lints/int-cast-expects.test.ts + int-cast-expect-limits.json: a per-file ceiling. A file may not gain .expect("int cast") sites and an unlisted file may not have any; a count below its limit passes and prints the --update hint, and --update regenerates the file. It is a ceiling rather than an exact inventory because with ~1100 sites and several removals a week, an exact inventory turns main red whenever two PRs remove sites from the same file, and taxes every removal with a regeneration; mordant-baseline.toml in this repo already uses the ceiling shape. The limits are seeded from main, so every other file is pinned where it is, including h2_frame_parser.rs (38), socket_body.rs (18) and yarn.rs (16), which have open PRs against them (install: migrate workspace packages from yarn.lock #38878 and install: yarn migration fills dep buffers via push, not uninit slices #37539 rewrite the code around every yarn.rs site).
  • The header says what a clear is (a checked conversion returning the caller's error for values from outside; a plain conversion where a check or the type already bounds the value) and that respelling the abort (.unwrap(), another message) or a lossy as is not one. The test counts the canonical spelling so new sites get noticed in review; it does not claim to count every abort (try_from(..).unwrap() and .expect("unreachable") are separate inventories a follow-up can add on the same helper).
  • rust-sources.ts holds the tracked-file walk, src/cli symlink dedupe, comment stripping and inventory sort that dead-code-escapes.test.ts, vm-thread-door.test.ts and this test each carried; the two existing lints now use it, and regenerating all three inventories is a no-op.
  • After this PR: 1142 sites in 272 files. Follow-ups clear a file or a class at a time and run --update.
  • src/exe_format/elf.rs (29 -> 0): every offset read from the template goes through file_range(data, offset, len) -> Result<Range<usize>, ElfError>, which converts and bounds-checks it in one place, with phdr_table / shdr_table / read_phdr / read_shdr on top. write_bun_section and find_bun_section validate the two header tables, e_shstrndx, .shstrtab, the RW segment's file image and the .bun slot before indexing, use checked arithmetic on p_vaddr + p_memsz, the new offsets and the final size, and reject p_filesz > p_memsz; every such template now fails with InvalidElfFile (exit 1, no output). The interpreter rewrite runs in rewrite_store_interpreter() -> Result and normalize_interpreter stays best-effort by logging the reason under BUN_DEBUG_elf=1 instead of returning silently. The layout itself is unchanged: the same synthetic templates compiled with 1.4.0 and with this branch differ only inside the payload page plus the .bun header's sh_size, which tracks the two versions' 2-byte payload length difference (details below).
  • src/exe_format/macho.rs (6 -> 5): the __bun section's offset is kept as the u32 it was read as instead of going through u64 and back.
  • src/parsers/json5.rs (17 -> 0): parse_root rejects documents longer than i32::MAX with a new DocumentTooLarge error ("JSON5 document is too large to parse (2 GiB maximum)"), the scheme json_index.rs already uses, and token and error positions go through bun_ast::usize2loc like the JS lexer and JSON parser. Bun.JSON5.parse already rejected such inputs in api.rs (Bun.{JSON5,JSONC,TOML,YAML}.parse: reject inputs of 2^31 bytes or more instead of panicking #32764); this covers the bundler and module-loader entry points, which read files. Not runtime-testable without a 2 GiB file.
  • src/runtime/image/codecs.rs (17 -> 0) and Image.rs (8 -> 0): probe() folds its <= 0 checks and conversions into positive_dimension(c_int) -> Result<u32, Error>; the kernel calls convert once per function through kernel_dimension(u32) -> Result<i32, Error> (a backstop: decoders and do_resize keep every side far below i32::MAX); rotate takes the u16 both callers store. Image.width/.height kept the decoded u32 sizes in two i32 cells with -1 meaning "nothing has run yet", which is what six of Image.rs's casts were for; they are one Cell<Option<(u32, u32)>> now and the getters produce the documented -1. rotate() keeps its degrees as the u16 the pipeline stores, and the input-size check converts st_size with a fallback. The existing image suites cover all of this; image.test.ts additionally pins the -1 before the first terminal.
  • test/harness.ts gains readElfInterp() and hostLooksNix(), replacing the copies in 24742.test.ts, 29290.test.ts, bun-build-compile.test.ts and the new test, so the runtime's host_uses_nix_store_interpreter() has one test-side mirror.
  • Why one PR rather than the ELF fix on its own: the brief asked for the ratchet and a first pass together, and the first pass is what exercises the clearing rules the header states (a reachable class in elf.rs, bounded values in json5.rs/codecs.rs, a representation fix in Image.rs). The commits are split along those lines; if the ratchet needs more discussion, the elf.rs commits stand alone and I can open them separately.
  • Verified:
    • bun test test/internal/source-lints/int-cast-expects.test.ts: passes on this branch; against main's src/ it fails for exactly elf.rs, macho.rs, json5.rs, codecs.rs and Image.rs. bun test test/internal/source-lints/ passes (424 tests); regenerating all three inventories changes nothing.
    • test/bundler/bundler_compile.test.ts: "rejects an ELF template whose headers point outside the file" (8 corrupt templates -> InvalidElfFile; a consistent template and one with a wrapping PT_INTERP still compile) fails on 1.4.0 with the panic above and passes with bun bd test; "rewrites a Nix store PT_INTERP and the .interp section header" covers the rewrite without patchelf (the existing bun build --compile on NixOS uses store path for ld-linux-x86-64.so.2 #24742/bun build --compile-produced executable does not run in NixOS (bun 1.3.12) #29290 tests skip without it, and CI has none); the Mach-O and short-header template tests still pass.
    • bun bd test test/js/bun/image/{image,image-adversarial,image-kernels}.test.ts: 192 pass. test/js/bun/json5/: 434 pass.
    • cargo clippy on bun_exe_format, bun_parsers, bun_runtime; cargo check --target aarch64-apple-darwin for the macOS-only rotate call; cargo fmt. The mordant job is green after cf19105 (it flagged the first version's swallowed file_range errors).

Background

  • expect("int cast"): the porting convention for Zig @intCast is T::try_from(x).expect("int cast"). In Zig the check only existed in Debug/ReleaseSafe; in the Rust tree it is a real abort in release builds, so each site is either a latent crash (value comes from outside) or noise (value is provably in range), and should become a checked error or a plain conversion respectively.
  • Source lints (test/internal/source-lints/): tests that grep src/ and run on GitHub Actions against a released bun on every PR touching src/**/*.rs, reporting in seconds. dead-code-escape-limits.json and vm-thread-door.inventory.json are the existing per-file inventories there; mordant-baseline.toml is the repo's existing ceiling-style baseline for the Rust lint pack.
  • Loc (bun_ast): AST and diagnostic positions are i32 byte offsets, so every parser has to bound its input to i32::MAX bytes; usize2loc is the shared conversion.
  • --compile-executable-path: bun build --compile normally patches a downloaded bun binary, but this flag lets the user point it at any file. The ELF writer reads that file's program headers, section headers and .shstrtab, appends the bundle after the last mapping, grows the writable PT_LOAD containing .bun to cover it, and relocates whatever followed that segment (non-ALLOC sections and the section header table), so every offset it uses comes from the template.
  • Highway kernels (codecs.rs): the resize/rotate/flip implementations are C++ functions taking i32 dimensions; the Rust side holds dimensions as u32 because that is what the decoders report.
Hostile ELF templates: 1.4.0 vs this branch
case                     before                                                         after
bun-offset-past-eof      panic: range start index 1099511627776 out of range ...        InvalidElfFile
phoff-past-eof           panic: range start index 1099511627776 out of range ...        InvalidElfFile
phoff-wraps              panic: range start index 18446744073709551608 out of range     InvalidElfFile
shoff-wraps              wrapped sum (overflow panic in debug builds)                   InvalidElfFile
shstrndx-past-shnum      panic: range start index 3840152 out of range ...              InvalidElfFile
shstrtab-wraps           panic: range start index 18446744073709551612 out of range     InvalidElfFile
rw-memsz-wraps           wrapped sum (overflow panic in debug builds)                   InvalidElfFile
filesz-over-memsz        panic: slice index starts at 6144 but ends at 4096             InvalidElfFile
interp-wraps             panic: range start index 18446744073709551612 out of range     builds, interpreter left alone (logged)
good / good-interp       builds (8384 / 8448 bytes)                                     builds (8384 / 8448 bytes)

Byte comparison of the good outputs (plain, with PT_INTERP, and with a 0x2000 byte tail): outside the payload page [0x1000, 0x2000) exactly one byte differs per output, the low byte of the .bun section header's sh_size (206 vs 204 = 8 + a 198 vs 196 byte bundle). e_shoff, the relocated section headers, the grown PT_LOAD, the vaddr written into the old .bun slot and the zero fill are identical.

Follow-ups this leaves open
Earlier shape of this PR

The first version was an exact inventory (a count below its limit also failed, as dead-code-escapes.test.ts does) and cleared only json5.rs, codecs.rs and elf.rs (1151 sites in 273 files). Self-review found that shape would go red on main under concurrent removals and that the codecs.rs change left the same conversion in Image.rs; dec551b and 62e21f3 changed both.

…image codecs and elf

test/internal/source-lints/int-cast-expects.test.ts pins the number of
`.expect("int cast")` sites per Rust file (1151 sites in 273 files after
this commit) so new ones fail the source lints and removals have to lower
the limits file.

json5: reject documents longer than i32::MAX up front (DocumentTooLarge,
like the JSON parser) and build token locations with usize2loc.

image codecs: a decoder-reported c_int dimension converts through
positive_dimension (DecodeFailed), kernel dimensions through
kernel_dimension (TooManyPixels), and rotate takes the u16 degrees the
pipeline already stores.

elf: every offset read from a --compile-executable-path template goes
through file_range, which bounds-checks it against the file and returns
InvalidElfFile. The .bun sh_offset, e_phoff, .shstrtab offset and
e_shstrndx were previously used unchecked, and several header sums could
overflow; a corrupt template used to abort the build with a slice-index
panic.
@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 24 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9ddfcae0-abd2-49df-bdd6-393320c7ff13

📥 Commits

Reviewing files that changed from the base of the PR and between df4a040 and c56508c.

📒 Files selected for processing (16)
  • src/exe_format/elf.rs
  • src/exe_format/macho.rs
  • src/parsers/json5.rs
  • src/runtime/image/Image.rs
  • src/runtime/image/codecs.rs
  • test/bundler/bun-build-compile.test.ts
  • test/bundler/bundler_compile.test.ts
  • test/harness.ts
  • test/internal/source-lints/dead-code-escapes.test.ts
  • test/internal/source-lints/int-cast-expect-limits.json
  • test/internal/source-lints/int-cast-expects.test.ts
  • test/internal/source-lints/rust-sources.ts
  • test/internal/source-lints/vm-thread-door.test.ts
  • test/js/bun/image/image.test.ts
  • test/regression/issue/24742.test.ts
  • test/regression/issue/29290.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review (head c56508c). Every bot review finding is addressed and resolved; the PR body describes the current shape, and the comments below record what changed along the way.

Reproduced:

  • grep -rho 'expect("int cast")' src --include=*.rs | wc -l on main (6324a58): 1214 sites in 276 files. With this branch the new lint (bun test test/internal/source-lints/int-cast-expects.test.ts) passes at 1142 sites / 272 files; run against main's src/ it fails for exactly elf.rs, macho.rs, json5.rs, codecs.rs and Image.rs.
  • The ELF half is also a runtime fix: with bun 1.4.0, bun build --compile --target=bun-linux-x64 --compile-executable-path <crafted template> aborts with panic: range start index 1099511627776 out of range for slice of length 8384 (and similar) for each of the 8 templates in the new bundler_compile.test.ts test; with this branch every one fails cleanly with InvalidElfFile, and consistent templates compile to the same layout as before (byte comparison in the PR body).

For the maintainer who picks this up: the one part that deserves a real read is write_bun_section in src/exe_format/elf.rs (it lays out every Linux --compile output; the PR body has the byte comparison against 1.4.0 and the existing compile tests build and run real binaries through it). The ratchet is a ceiling, the shape mordant-baseline.toml already uses. yarn.rs, h2_frame_parser.rs and socket_body.rs are pinned at their current counts because open PRs rewrite them; the remaining follow-ups are listed in the PR body.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline nit, I traced the reordered write_bun_section layout in elf.rs for a few things that looked suspicious and ruled them out: the dropped old_shdr_offset + shdr_table_size > move_src_end check is subsumed by shdr_table()'s file_range bound (since move_src_end == data.len()); the unchecked rw_phdr.p_vaddr + rw_phdr.p_memsz in the NewVaddrCollides guard was already validated by the PT_LOAD loop's checked_add; and new_shdrs.end cannot exceed the resized buffer because old_shdrs.end + tail_shift ≤ total_new_size. checked_next_multiple_of(page_size) matches the old align_up for the power-of-two page sizes used here.

Extended reasoning...

The one reported finding is a developer-machine-only test nit. The substantive risk in this PR is the write_bun_section restructuring, which reorders when offsets are validated relative to when the layout is computed and written. I checked the specific points listed in the message and found them sound, but the change is large enough (and the compiled-binary layout subtle enough — memmove ordering, BSS zero-fill, shdr relocation) that a human should confirm the good-path output is byte-identical, and sign off on the new repo-wide ratchet policy.

Comment thread test/bundler/bundler_compile.test.ts Outdated
…ewrite test

host_uses_nix_store_interpreter() also treats a bun whose own PT_INTERP is a
store path as a Nix host, so the skip condition reads bunExe()'s first page
the same way the sibling patchelf tests do.
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 4:58 AM PT - Aug 15th, 2026

@robobun, your commit 62e21f3 is building: #98010

Comment thread src/exe_format/elf.rs Outdated
Comment thread src/exe_format/elf.rs Outdated
Comment thread src/exe_format/elf.rs Outdated
Comment thread src/exe_format/elf.rs Outdated
Comment thread src/exe_format/elf.rs Outdated
Comment thread src/exe_format/elf.rs Outdated
Comment thread src/parsers/json5.rs Outdated
Comment thread src/parsers/json5.rs Outdated
Comment thread src/runtime/image/codecs.rs Outdated
Comment thread src/runtime/image/codecs.rs Outdated
Comment thread src/exe_format/elf.rs Outdated
Comment thread src/exe_format/elf.rs Outdated
Comment thread src/runtime/image/codecs.rs Outdated
Comment thread test/bundler/bundler_compile.test.ts Outdated
The three patchelf tests and the new template test each carried their own
copy of the Nix host check that mirrors host_uses_nix_store_interpreter();
a change to the runtime's check now has one test-side counterpart to update.
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Review follow-ups, all pushed (head is 6e74012):

  • Nix host predicate on the new PT_INTERP test: fixed in cc3327f, then hoisted in 6e74012. hostLooksNix() and readElfInterp() now live in test/harness.ts, and 24742.test.ts, 29290.test.ts, bun-build-compile.test.ts and bundler_compile.test.ts all import them (net -120 lines), so the runtime's host_uses_nix_store_interpreter() has one test-side mirror to keep in step.
  • The comment-length findings: the comments on the new helpers are one line each now (84a007e, 154526f).
  • On whether the write_bun_section rewrite still produces the same bytes: I compiled the same three synthetic templates (plain, with a PT_INTERP, and with a 0x2000 byte tail between the segment and the section headers) with the released 1.4.0 and with this branch and compared the outputs byte for byte. Outside the payload page ([0x1000, 0x2000), which holds the two versions' different bundles) exactly one byte differs in each output: the low byte of the .bun section header's sh_size, 206 vs 204, which is 8 + payload length for payloads of 198 vs 196 bytes. e_shoff, the relocated section headers, the grown PT_LOAD (p_filesz/p_memsz), the vaddr written into the old .bun slot and the zero fill are identical. The real-binary path is exercised by the existing compile tests in the same file, which build and run executables from the debug binary.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR (including the follow-up commits addressing the two earlier nits) and didn't find any bugs. Because the elf.rs half is a substantial restructuring of write_bun_section — the code path that produces every Linux --compile binary — and the PR also introduces a repo-wide ratchet policy, a human look is worthwhile.

What was reviewed:

  • Traced the reordered write_bun_section layout computation against the old version — checked-arithmetic, the new new_file_offset < move_src_start guard, the removed reserve()/if moved_tail_size != 0 wrappers, and the earlier shdr-table validation are all behaviour-preserving or strictly tighter.
  • codecs.rs: positive_dimension/kernel_dimension fold the existing <= 0 checks; the rotate u32→u16 signature change matches both callers and the macOS system_backend::rotate arm.
  • json5.rs: usize2loc + the DocumentTooLarge guard at parse_root bound every start_token() site; add_to_log/to_error cover the new variant.
  • The harness hostLooksNix()/readElfInterp() hoist and its four call sites now match the runtime's full predicate.
Extended reasoning...

Overview

This PR touches 11 files: three Rust source files (src/exe_format/elf.rs, src/parsers/json5.rs, src/runtime/image/codecs.rs + one caller in Image.rs), a new source-lint test with a 273-entry per-file limits JSON, ~220 lines of new synthetic-ELF-template tests in bundler_compile.test.ts, and a consolidation of four copies of hostLooksNix()/readElfInterp() into test/harness.ts. Net ~+650/−370.

Security risks

The ELF changes harden --compile-executable-path against hostile templates (header offsets past EOF, wrapping arithmetic, p_filesz > p_memsz), converting reachable panics into InvalidElfFile. That is a security improvement, not a new risk. No auth/crypto/permissions code is touched. The json5 and codecs changes replace abort-on-overflow with typed errors on paths whose inputs were already bounded elsewhere.

Level of scrutiny

High. write_bun_section in elf.rs is the code that assembles every Linux bun build --compile output, and the refactor reorders when offsets are computed, changes when validation happens (before vs after resize), removes several guards that are now provably redundant, and switches align_up to checked_next_multiple_of. I traced each of these and believe they are behaviour-preserving on well-formed templates and strictly tighter on hostile ones — the author's byte-for-byte comparison on synthetic templates plus the existing real-binary compile tests support that — but this is exactly the kind of "refactor guilty until proven behaviour-preserving" change the review guide flags for careful human eyes.

Other factors

The PR also introduces a new repo-wide policy mechanism (the per-file .expect("int cast") ratchet). It follows the existing dead-code-escapes pattern in the same directory, but a maintainer should confirm they want the ratchet shape (exact-count, fails on both increase and decrease) before it starts gating every PR that touches the 273 listed files. My two earlier inline nits (Nix-host predicate scope, and hoisting hostLooksNix into harness) were both addressed in cc3327f and 6e74012; the comment-cop findings were addressed in 84a007e and 154526f.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

On the one open design question above, the shape of the ratchet, for whoever picks this up:

  • The limits file is an exact inventory (a count below the limit fails too, with the --update command in the message) because an upper bound goes stale: once a file drops from 16 sites to 2, an upper bound of 16 lets 14 new ones in unnoticed. This is the same trade-off dead-code-escapes.test.ts in the same directory already makes.
  • The cost is that a PR which removes a site also has to run bun ./test/internal/source-lints/int-cast-expects.test.ts --update (the source-lints job fails in seconds with that instruction). If that is more friction than wanted, dropping the count < limit branch is a three line change and the rest of the PR is unaffected; say so and I will switch it.

…rite and log them

normalize_interpreter swallowed a rejected PT_INTERP or .shstrtab range with
a bare return. The rewrite now runs in a function that returns the error and
the best-effort wrapper logs it, so mordant's defaulted_failure no longer
fires and BUN_DEBUG_elf=1 says why a template was left alone.

readElfInterp in the test harness also checks EI_CLASS, as its doc claims.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-reviewed at cf19105 and didn't find any bugs; the earlier nits (Nix-host predicate, hostLooksNix duplication) are addressed. Deferring to a human for two things this PR explicitly leaves open: the exact-count-vs-upper-bound shape of the new ratchet (a repo-wide policy call the author flagged), and a second pair of eyes on the write_bun_section layout reordering in elf.rs, since that path produces every Linux --compile binary.

What was reviewed:

  • elf.rs: traced the reordered offset math — old_shdrs/bun_vaddr_slot are now bounds-checked before resize(), the p_filesz > p_memsz guard replaces the old old_rw_file_end > old_file_size check via new_file_offset < move_src_start, and new_shdrs derivation from old_shdrs + tail_shift matches the old e_shoff + (move_dst_start - move_src_start).
  • json5.rs: confirmed usize2loc is the same helper the JS lexer/JSON parser use and that the > i32::MAX guard covers the EOF token position.
  • codecs.rs: checked both rotate callers already hold a u16, and that kernel_dimension errors before the vec[] allocation would matter.
Extended reasoning...

Overview

This PR touches four Rust source files (elf.rs, json5.rs, codecs.rs, Image.rs), adds a new repo-wide source-lint (int-cast-expects.test.ts + a 273-entry limits JSON), hoists hostLooksNix/readElfInterp into test/harness.ts, refactors four test files to use them, and adds ~220 lines of new ELF-template tests to bundler_compile.test.ts. The elf.rs change is the largest: write_bun_section and its helpers are substantially reorganised around new file_range/phdr_table/shdr_table helpers, with checked arithmetic replacing 29 .expect("int cast") sites and several previously-unchecked slice indexes.

Security risks

The ELF writer now validates header-derived offsets from a user-supplied --compile-executable-path before indexing into the file, which is a strict hardening over the previous panics. I did not find any new unchecked arithmetic or index; file_range bounds-checks against data.len() and uses checked_add. No auth/crypto/permissions surface is touched.

Level of scrutiny

High. write_bun_section is the core of bun build --compile on Linux — a regression there produces broken executables for every user. The computation was reordered (e.g. old_shdrs and bun_vaddr_slot computed and bounds-checked before resize; new_file_offset/move_dst_start converted to usize early; the reserve() before resize() dropped; the shdr-table-in-tail check moved from after the writes to before). The author byte-compared three synthetic templates against 1.4.0 and the existing compile tests build and run real binaries, which is good evidence, but the change is large enough that I would not want it to land without a maintainer reading the diff.

Separately, the new ratchet is a policy decision: it fails when a count goes down as well as up, so every PR that removes an expect("int cast") must also update the limits file. The author explicitly asked for a maintainer call on exact-count vs upper-bound, and it follows the existing dead-code-escapes.test.ts pattern, but it affects every future contributor.

Other factors

Both of my earlier inline comments were addressed (predicate now mirrors all four runtime checks; helper hoisted into harness.ts and the three sibling files updated). The comment-cop findings were all resolved by shortening helper doc-comments. Test coverage is thorough: 8 corrupt ELF templates + 2 good ones, a PT_INTERP-rewrite test that no longer needs patchelf, and the existing image/json5 suites cover the unchanged-behaviour paths. The rotate signature change is small and both call sites already held a u16.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

One more follow-up since the last note: the advisory mordant job flagged two defaulted_failure findings in the first version of elf.rs (let Ok(..) = file_range(..) else { return; } in the interpreter rewrite swallowed a rejected PT_INTERP / .shstrtab range). cf19105 moves the rewrite into rewrite_store_interpreter() -> Result, which propagates with ?, and normalize_interpreter stays best-effort by logging the reason under BUN_DEBUG_elf=1 (leaving PT_INTERP alone: InvalidElfFile) instead of silently returning. Behaviour is otherwise unchanged; the template tests cover both the rewritten and the left-alone cases.

The other Buildkite failures in that run were retries that passed (bun-patch on Windows, a few batch-only timeouts), nothing touching this diff. Head is cf19105; the remaining open points are the two maintainer calls listed above (ratchet shape, and a read of the write_bun_section reordering).

…e walk

A count above its limit fails; a count below it only prints the --update
hint. With ~1100 sites and several PRs a week removing some, an exact
inventory would turn main red whenever two of them touched the same file,
and every removal would need a regeneration; a ceiling needs neither, and
mordant-baseline.toml already works this way.

The tracked-file walk, symlink dedupe and comment stripping that
dead-code-escapes, vm-thread-door and this test each carried now live in
rust-sources.ts. Regenerating all three inventories is a no-op.
…un offset as the u32 it was read as

Image.width/.height kept the decoded u32 dimensions in two i32 cells with -1
meaning "no pipeline has run yet", which is what the six remaining casts in
Image.rs were for. rotate() keeps its degrees as the u16 the pipeline stores
and the input size check converts st_size with a fallback instead of an
abort, so Image.rs leaves the limits file.

macho.rs converted the __bun section's u32 offset to u64 and back.
Comment thread src/runtime/image/Image.rs Outdated
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Self-review of the first version turned up three things worth changing; all are pushed (head c56508c) and the PR body now describes the current shape:

  • The inventory is a ceiling now (dec551b). With ~1100 sites and several removals a week, the exact-count version would have gone red on main whenever two PRs removed sites from the same file, and every removal would have needed a regeneration. A count below its limit passes and prints the --update hint; mordant-baseline.toml already works this way. This supersedes the question in my earlier comment about the ratchet shape.
  • The three lints in test/internal/source-lints/ that walk the Rust tree shared about thirty identical lines; rust-sources.ts holds them now and dead-code-escapes / vm-thread-door use it too. Regenerating all three inventories is a no-op.
  • The codecs.rs change left the same dimension conversion in Image.rs, in the two i32 cells behind .width/.height. They are one Cell<Option<(u32, u32)>> now (62e21f3), which also removes the other two casts in that file; image.test.ts pins the documented -1 before the first terminal. macho.rs had one u32 -> u64 -> u32 round trip on the __bun offset, also gone.

It also pointed out that yaml.rs, toml.rs and the JS lexer share the unguarded 2 GiB position limit that json5.rs now checks locally; the right fix for those is one length check where the bundler and module loader load sources, which is filed separately rather than added here. Remaining total: 1142 sites in 272 files.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and the bug-hunting pass found no issues. Given the scope — a substantial reordering of write_bun_section in elf.rs (which produces every Linux --compile binary) plus a new repo-wide ratchet policy — a human read is still worthwhile.

What was reviewed:

  • elf.rs: traced every offset through the new file_range/shdr_table helpers; the dropped shdr-end check is subsumed by file_range's end > data.len(), and rw_phdr.p_vaddr + rw_phdr.p_memsz was already checked in the loop above.
  • Removed guards (moved_tail_size != 0, move_dst_start > payload_end, the pre-resize reserve) are all no-ops in the empty case.
  • json5.rs start_token, Image.rs last_size, and codecs.rs positive_dimension/kernel_dimension are mechanical and covered by existing suites.
  • rust-sources.ts extraction is behavior-preserving for dead-code-escapes and vm-thread-door (regenerating both inventories is a no-op per the PR body).
Extended reasoning...

Overview

This PR touches 16 files across five distinct concerns: (1) a new source-lint ratchet (int-cast-expects.test.ts + a 272-entry limits file) that pins the count of .expect("int cast") sites per Rust file; (2) a substantial refactor of src/exe_format/elf.rs that replaces 29 unchecked casts and unchecked offset arithmetic with a file_range-based bounds-checking scheme, reordering much of write_bun_section in the process; (3) mechanical clears in macho.rs, json5.rs, Image.rs and codecs.rs; (4) extraction of rust-sources.ts shared by three source-lint tests; (5) hoisting hostLooksNix()/readElfInterp() into test/harness.ts and rewiring four test files.

Security risks

The ELF path parses an arbitrary user-supplied file (--compile-executable-path). The change is strictly a hardening in that direction — every header-derived offset now goes through file_range/checked_add/checked_next_multiple_of, and eight hostile-template cases are added to the test suite. I found no new unchecked indexing; the one remaining unchecked add (rw_phdr.p_vaddr + rw_phdr.p_memsz in the NewVaddrCollides guard) was already checked_add-verified for that same phdr in the PT_LOAD loop above. No auth/crypto/network surface is touched.

Level of scrutiny

High for elf.rs. write_bun_section is what lays out every Linux bun build --compile output; a subtle ordering or off-by-one bug here would corrupt compiled binaries in ways CI on synthetic templates might not catch. The author's byte-for-byte comparison against 1.4.0 on three synthetic templates is compelling, and the existing real-binary compile tests in the same file exercise the debug-bun path, but the refactor moves the shdr-table validity check from after the resize to before it, replaces align_up with checked_next_multiple_of, drops the explicit reserve, and rewrites the section-header relocation loop — enough moving parts that a maintainer should read it through once.

Medium for the ratchet itself. It introduces a repo-wide policy (a ceiling, not an exact count) that every future PR touching Rust will interact with. The design was self-settled (ceiling over exact inventory, matching mordant-baseline.toml), and the reasoning is sound, but it is a process decision a maintainer should sign off on rather than have land via bot approval.

Low for everything else. The json5.rs, Image.rs, codecs.rs and macho.rs changes are small, type-directed, and covered by the existing 192/434-test suites the PR body cites. The test-harness extractions are net-negative-line refactors that I previously asked for and verified.

Other factors

My two earlier inline nits (the under-approximating Nix-host predicate and the fourth hostLooksNix copy) were both addressed in 6e74012 and are resolved. All comment-cop findings are resolved. No human reviewer has weighed in yet. The author's own status comment earlier flagged "a read of the write_bun_section reordering" as an open maintainer call; while later comments suggest the ratchet-shape question is settled, the elf.rs read was never explicitly closed out. Given the combination of a production ELF-writer refactor and a new repo-wide lint policy, deferring to a human is the right call.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Agreed on what needs a human: the write_bun_section read in elf.rs. I have updated the status comment at the top to say so explicitly (it previously said nothing was open); the supporting evidence is the byte comparison against 1.4.0 in the PR body plus the existing compile tests in the same file, which build and run real executables through this code. Everything else raised so far is addressed and the threads are resolved; head is c56508c.

Jarred-Sumner pushed a commit that referenced this pull request Aug 15, 2026
… 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant