exe_format/macho: validate __BUN filesize before computing growth; reject shrink instead of panicking - #34351
Conversation
…k as InvalidObject
write_section() computed size_diff as i64 then passed it through
usize/u64::try_from(..).expect("int cast") at five sites. A template
whose __BUN.filesize exceeds either the file length or the aligned
bundle size makes size_diff negative, and the first of those casts
(the reserve() call) panics before the existing OffsetOutOfRange
bounds check has a chance to reject the input.
Hoist the bounds check ahead of the size computation so a filesize
that runs past EOF is rejected as OffsetOutOfRange. Then compute
size_diff via aligned_size.checked_sub(original_segsize), rejecting a
would-be-negative result as InvalidObject: the offset-shift logic
below only moves data forward, and real Bun templates ship a minimal
placeholder so growth is the only case that occurs in practice. With
size_diff now u64, the downstream try_from casts collapse to direct
arithmetic.
|
Warning Review limit reached
Next review available in: 2 minutes 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 (2)
Comment |
|
Updated 6:10 AM PT - Jul 16th, 2026
❌ @robobun, your commit 8dacffc has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34351That installs a local version of the PR into your bun-34351 --bun |
There was a problem hiding this comment.
LGTM — hoists the existing bounds check ahead of the growth arithmetic and turns the negative-size_diff panic into a clean InvalidObject error.
What was reviewed:
- Verified
prev_lenis unchanged by the reorder (reserve()doesn't touchlen()), so the hoisted check is semantically identical. - Traced each new test template through
write_section:filesize-past-eof(256-byte file, 224-byte load commands fit exactly) hitsoriginal_bun_end (0x8000) > prev_len (256);filesize-needs-shrinkpasses bounds then hits0x4000.checked_sub(0x8000) = None. - Confirmed the downstream
u64simplifications are mechanical and the unchangedusize::try_from(...).expectcalls remain sound (size_diffnow non-negative, 64-bit targets only). - Checked the "good" template still resolves to the same
__LINKEDITfileoff (0x8000) after the parameter defaults were introduced.
Extended reasoning...
Overview
Two-file change: src/exe_format/macho.rs reorders the __BUN segment bounds check to run before the reserve()/growth arithmetic, and replaces the signed i64 size_diff (with five downstream .expect("int cast") conversions) with a u64 computed via checked_sub that returns InvalidObject on would-be-negative. test/bundler/bundler_compile.test.ts parameterizes the existing Mach-O template-bounds test to add two new failing shapes alongside the original fileoff-past-EOF case and the unchanged in-bounds "good" template.
Security risks
This code processes an untrusted Mach-O template supplied via --compile-executable-path and has unsafe set_len/ptr::copy operations downstream. The change is strictly defensive: it moves an existing validation earlier (so it fires before allocation instead of after) and rejects an additional class of malformed input that previously panicked. No validation is removed or weakened, and the unsafe blocks themselves are untouched. The SAFETY comment on set_len remains accurate — size_diff bytes are still reserved before growing.
Level of scrutiny
Medium. The surrounding code is memory-sensitive, but the diff is a reorder of existing checks plus a signed→unsigned conversion with an explicit error on underflow. I traced the arithmetic for each of the three test templates by hand against the Rust code and confirmed each hits the intended branch. The remaining .expect("int cast") on the reserve() argument is now unreachable-in-practice on 64-bit targets since size_diff is bounded by aligned_size (bundle-derived) and num_of_new_pages * 32 is a small fraction of size_diff.
Other factors
- No CODEOWNERS entry covers
src/exe_format/or the bundler compile tests. - The
debug_assert!(size_diff.is_multiple_of(PAGE_SIZE))was already a debug-only assertion on thei64version; behavior in release is unchanged if a template'sfilesizeisn't page-aligned. - The "good" template's default arguments (
bunFileSize=0x4000, fileSize=0x8100) reproduce the previous hardcoded values, so that regression case is preserved. The__LINKEDITvmaddr bump to0x1_0001_0000is cosmetic (vmaddr isn't part of the file-bounds validation). - The PR description's "why not implement shrinking" rationale is sound:
Shifterand the memmove sequence are forward-only, and real Bun templates always have a minimal placeholder.
|
Self-review pass complete; no concerns survived. CI: the one red lane ( Ready for review. |
Problem
bun build --compile --target=bun-darwin-* --compile-executable-path <template>panics inMachoFile::write_sectionwhen the template's__BUNsegment claims afilesizelarger than the bundle needs:Two shapes reach this:
__BUN.filesizeexceeds the template file's length (e.g. a 256-byte file whose load command claims a 32 KB segment).__BUN.filesizeis in-bounds but larger than the 16 KB-aligned bundle size (e.g. a 48 KB template with a 32 KB__BUNslot and a tiny entry script).Cause
write_sectioncomputessize_diffasi64and then feeds it throughusize::try_from/u64::try_fromwith.expect("int cast")at five sites (reserve,set_len, the memmove dest, the__LINKEDIToffset shift, the code-signature offset, andupdate_load_command_offsets). Whenaligned_size < original_segsize,size_diffis negative and the first of those (thereserve()argument) panics.The
OffsetOutOfRangebounds check added in #31559 would already reject case (1), but it sits after thereserve()call, so the panic fires first. Case (2) is a valid template whose slot would need to shrink; the shift logic only moves data forward.Fix
original_bun_end > prev_len || original_data_end > original_bun_endcheck above the growth arithmetic so afilesizethat runs past EOF is rejected asOffsetOutOfRangebefore anything allocates.size_diffviaaligned_size.checked_sub(original_segsize)and reject a would-be-negative result asInvalidObject. Real Bun templates ship a minimal__BUNplaceholder, so growth is the only case that occurs in practice; a template that would require shrinking is not supported input.size_diff: u64, the four downstreamtry_from(..).expect("int cast")conversions collapse to directu64arithmetic.Why not implement shrinking?
update_load_command_offsets/Shifterare written around unsigned forward shifts, and theset_len+ptr::copysequence assumes growth. Supporting shrink would mean reordering those and makingShiftersign-aware, which is a larger change than the reachable panic warrants:--compile-executable-pathis for pointing at a fresh Bun executable, and those never carry an oversized__BUNslot.Related to #34341, which handles the structural header/load-command validation on the same attack surface; this one is the segment-content case.
Test
Extended the existing Mach-O template-bounds test in
test/bundler/bundler_compile.test.tsto cover both new shapes alongside the originalfileoff-past-EOF case, plus the unchanged in-bounds "good" template. Each bad case asserts the specific error name appears in stderr, no output file is produced, and the build exits 1.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_compile.test.ts