sys/exe_format: LoadCommand borrows its bytes; patch commands by offset

[robobun]

What

bun_sys::macho::LoadCommand stored the bytes of a load command as RawSlice { ptr: *const u8, len }, and LoadCommandIterator::new(ncmds, buffer: &[u8]) copied the slice into a *const u8 plus length, so "the buffer must stay live while the iterator or any command is in use" was a doc-comment contract, and the safe cast<T: Copy>() was a read_unaligned of whatever T the caller named. Its one writing consumer, MachoFile::update_load_command_offsets in src/exe_format/macho.rs, took entry.data.as_ptr().cast_mut() on a pointer obtained through a shared borrow of self.data and wrote through it in four arms, each with its own read_unaligned/write_unaligned block.

The command now borrows its bytes and the cursor borrows nothing: the region is passed to each next call, so a yielded command only borrows the file for as long as the caller uses it.

// before
pub struct LoadCommand { pub hdr: load_command, pub data: RawSlice, pub offset: usize }
impl LoadCommand { pub fn cast<T: Copy>(&self) -> Option<T> }            // unsafe read_unaligned
pub struct LoadCommandIterator { ncmds, index, buf_ptr: *const u8, buf_len, offset }
impl LoadCommandIterator { pub fn new(ncmds: u32, buffer: &[u8]) -> Self; pub fn next(&mut self) -> Option<LoadCommand> }

// after
pub struct LoadCommand<'a> { pub hdr: load_command, pub data: &'a [u8], pub offset: usize }
impl LoadCommand<'_> { pub fn cast<T: bytemuck::AnyBitPattern>(&self) -> Option<T> }   // pod_read_unaligned
pub struct LoadCommandIterator { ncmds, index, offset }
impl LoadCommandIterator { pub fn new(ncmds: u32) -> Self; pub fn next<'a>(&mut self, cmds: &'a [u8]) -> Option<LoadCommand<'a>> }

load_command and segment_command_64 (in bun_sys) and linkedit_data_command (in macho_types.rs) get bytemuck::Zeroable + Pod impls; they are the three types cast is instantiated with, all #[repr(C)] integer structs without padding. RawSlice and the macho_types.rs re-export of it are deleted along with the module comment that explained the raw-pointer design.

Call sites: MachoFile::iterator() becomes load_commands() (the sizeofcmds slice after the header), and the three loops in write_section, update_load_command_offsets and validate_segments plus the two passes in MachoSigner::init call iter.next(region); the crash handler (src/crash_handler/lib.rs, macOS only) keeps its one from_raw_parts over the dyld image and passes the resulting slice to next. In update_load_command_offsets each iteration copies hdr, offset and data.len() out of the yielded command, takes &mut self.data[lc_base + offset..][..len], and the four arms go through one patch::<T>(bytes, |cmd| ..) helper built on the crate's existing read_struct/write_struct, which also replaces the per-arm require(size) closure (a command shorter than T is still MachoError::InvalidObject, and a failed shift still leaves the command unwritten). Seven unsafe blocks are removed (three in bun_sys, four in macho.rs); the six added lines are the unsafe impl markers.

Why

The lifetime of the bytes a LoadCommand points at is now checked by the compiler instead of stated in comments, and the write in update_load_command_offsets goes through a real &mut into the Vec rather than through a pointer derived from a shared borrow. cast can no longer be instantiated with a type for which arbitrary bytes are not a valid value. &[u8] is the same pointer and length pair RawSlice held, the cursor shrinks from 32 to 16 bytes, pod_read_unaligned on an exactly sized subslice is the same unaligned copy as before, and the per-command work is the same header checks plus the slice bounds checks the surrounding read_struct/write_struct call sites already use, on a loop that runs once per load command while writing a compiled single-file executable or formatting a crash report.

Part of a series of small type-system hardening changes; each PR stands alone.

Verification

cargo check and cargo clippy are clean for the touched crates. Debug build succeeds. bun bd test test/bundler/bundler_compile.test.ts (60 pass, 1 fail), test/regression/issue/29120.test.ts (2 skipped, darwin-only), test/cli/run/run-crash-handler.test.ts (14 pass, 9 skipped platform-gated): 74 pass, 11 skip, 1 fail total.

The single failure, compile/HelloWorldWithProcessVersionsBun, fails identically on main (debug build reports process.versions.bun as "1.4.0-debug" while the test compares against the version with "-debug" stripped); pre-existing and unrelated to the Mach-O load-command changes.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 1 minute

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: 9593bfd9-b0df-48c0-b80a-de74734b324e

📥 Commits

Reviewing files that changed from the base of the PR and between 7d0ca0c and bfd5d3f.

📒 Files selected for processing (1)

• src/sys/lib.rs

Walkthrough

Changes

The Mach-O load-command iterator now borrows an explicit command slice and validates command boundaries. Mach-O updates use safe patching, and callers use the new iterator API.

Mach-O load-command handling

Layer / File(s)	Summary
Borrowed iterator and typed command contract src/sys/lib.rs, src/exe_format/macho_types.rs	LoadCommandIterator receives command slices during iteration. Parsing uses bounded unaligned reads and borrowed command data. Mach-O POD types and re-exports were updated.
Mach-O command updates and signing src/exe_format/macho.rs	Mach-O writing, validation, offset updates, and signing pass explicit command slices. Command mutations use the validated patch helper.
Crash-handler iterator integration src/crash_handler/lib.rs	The macOS crash handler creates the command slice separately and passes it to each iterator call.

Suggested reviewers: jarred-sumner, dylan-conway

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary change to borrowed load-command bytes and offset-based patching.
Description check	✅ Passed	The description explains the changes, rationale, affected call sites, safety improvements, and verification results, including the known unrelated test failure.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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

[coderabbitai]

Actionable comments posted: 1

🤖 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/sys/lib.rs`:
- Around line 6074-6088: Change LoadCommandIterator::next in
src/sys/lib.rs:6074-6088 to return a typed parsing error for malformed headers,
invalid command sizes, and truncated bodies, while retaining None only for
normal exhaustion. Propagate that error through write_section
(src/exe_format/macho.rs:94-96), update_load_command_offsets (364-368),
validate_segments (430-432), and both MachoSigner::init passes (441-444 and
551-555). In load_commands (577-580) and MachoSigner::init (441-444), validate
sizeofcmds using checked slice bounds so truncated input returns the typed error
instead of panicking.

🪄 Autofix

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: 1868ec01-4493-40e4-94bd-9076f0b25cd8

📥 Commits

Reviewing files that changed from the base of the PR and between 97e21e5 and 7d0ca0c.

📒 Files selected for processing (4)

• src/crash_handler/lib.rs
• src/exe_format/macho.rs
• src/exe_format/macho_types.rs
• src/sys/lib.rs

[claude]

LGTM — mechanical refactor replacing the raw-pointer LoadCommandIterator/RawSlice with a lifetime-checked cursor and bytemuck reads.

What was reviewed:

• Traced all six call sites (write_section, update_load_command_offsets, validate_segments, two MachoSigner::init passes, crash handler) — each copies hdr/offset/len out before taking &mut self.data, so NLL ends the shared borrow before the write.
• next() early-return on malformed headers still stays stopped: offset/index are unchanged when .get() returns None, so the next call hits the same guard.
• patch preserves the old require(size) contract via .get_mut(..size_of::<T>()) → InvalidObject, and skips the write when the closure errors (matching the old read-then-write ordering).
• Verified the three new Pod impls have no padding (load_command 8B, linkedit_data_command 16B, segment_command_64 72B) and cover every cast::<T>() instantiation.

Extended reasoning...

Overview

This PR replaces bun_sys::macho::LoadCommandIterator's raw *const u8 storage with a stateless cursor whose next() takes the &[u8] region on each call, and gives LoadCommand a real &'a [u8] instead of a RawSlice { *const u8, usize }. cast<T> moves from an unchecked read_unaligned bounded only by T: Copy to bytemuck::pod_read_unaligned bounded by AnyBitPattern. In macho.rs, the four write arms in update_load_command_offsets are collapsed into one patch::<T>(bytes, |cmd| ..) helper built on the crate's existing read_struct/write_struct, replacing per-arm cast_mut() writes through a shared-borrow-derived pointer. RawSlice and its re-export are deleted; the module comment explaining the raw-pointer workaround is removed. Net: seven unsafe blocks removed, six unsafe impl Zeroable/Pod markers added with accurate SAFETY comments.

Security risks

None. This is internal Mach-O header parsing for bun build --compile and the crash reporter. The change strictly tightens invariants: cast can no longer be instantiated with a type for which arbitrary bytes are invalid, and the write path now goes through a real &mut slice with a checked .get_mut(..size_of::<T>()) in release builds instead of a debug-only require closure plus raw-pointer write. Bounds checking on untrusted cmdsize is preserved (rest.get(..cmdsize)? in next, .get_mut(..size_of::<T>()) in patch).

Level of scrutiny

Medium. The old code was already sound in practice (the buffer is a Vec<u8> that isn't reallocated during iteration), so this is a type-system hardening pass rather than a bug fix. I verified: (1) all consumers of LoadCommandIterator/LoadCommand/RawSlice are in the four touched files (the other RawSlice hits repo-wide are bun_ptr::RawSlice, unrelated); (2) the NLL borrow pattern in update_load_command_offsets and write_section — the yielded entry's last use (entry.data.len()) precedes the &mut self.data[...] borrow, so it compiles and is sound; (3) the malformed-header "stays stopped" contract holds without the explicit self.index = self.ncmds because state is unchanged on the ? early returns; (4) the three Pod structs are genuinely padding-free (segment_command_64: 2×u32 + [u8;16] + 4×u64 + 2×i32 + 2×u32 = 72 bytes, u64 fields land at offset 24).

Other factors

No new tests, which is acceptable for a pure refactor with no observable behavior change — the PR ran bundler_compile.test.ts (60 pass, 1 pre-existing unrelated failure also on main) which exercises the Mach-O write/sign path end-to-end. bytemuck is already a bun_sys dependency. The iterator() → load_commands() visibility narrowing (pub(crate) → private) is fine; no other crate-internal callers exist. Part of a stated series of small type-system hardening PRs.

[claude]

I reviewed this PR and didn't find any bugs. It's a well-scoped safety refactor that removes seven unsafe blocks and replaces the raw-pointer RawSlice with a borrowed slice, but because it restructures the borrow pattern in the Mach-O load-command writer used by bun build --compile and the crash handler, a human look would still be worthwhile. Note also the two open comment-cop notes on src/sys/lib.rs.

What was reviewed:

• LoadCommandIterator::next / parse_at — bounds checks and the malformed-header stop match the old raw-pointer path (including index = ncmds latch).
• update_load_command_offsets — the new patch<T> helper preserves the old require(size) reject and the no-write-on-shift-error semantics; NLL ends the shared borrow from load_commands() before &mut self.data[..] is taken.
• bytemuck::Pod impls — field layouts checked for load_command (8B), segment_command_64 (72B), linkedit_data_command (16B); no padding.
• cast<T> bound tightened from Copy to AnyBitPattern — all three call sites (segment_command_64, linkedit_data_command) now have the required impls.

Extended reasoning...

Overview

This PR replaces bun_sys::macho::LoadCommand's raw-pointer RawSlice { ptr, len } storage with a borrowed &'a [u8], and reworks LoadCommandIterator so the command region is passed to each next() call rather than stored in the cursor. In src/exe_format/macho.rs, update_load_command_offsets no longer writes through entry.data.as_ptr().cast_mut() (a mutable pointer derived from a shared borrow — a stacked-borrows violation); instead it copies hdr/offset/len out of the yielded command, drops the shared borrow, and takes a real &mut self.data[..] slice into a new patch<T> helper built on the crate's existing read_struct/write_struct. cast<T> now requires bytemuck::AnyBitPattern and uses pod_read_unaligned. RawSlice and its re-export are deleted. Net: seven unsafe blocks removed, six unsafe impl markers added for the three POD structs.

Security risks

None identified. The input is Bun's own executable image (crash handler reads the dyld-mapped image; --compile patches a Bun-built template), not attacker-controlled data. The change strictly tightens safety: lifetimes are now compiler-checked, cast can no longer be instantiated with a type where arbitrary bytes are invalid, and the mutable write goes through a real &mut instead of a cast-from-shared pointer. The pre-existing silent-stop-on-malformed-header and unchecked [..sizeofcmds] slicing are deliberately preserved (CodeRabbit raised and withdrew this).

Level of scrutiny

Medium-high. This is not a mechanical change: it restructures the borrow pattern around interleaved reads and in-place mutation of the same Vec<u8>, which was the explicit reason the old code used raw pointers (per the deleted module comment). The new design relies on NLL ending the entry borrow after hdr/offset/data.len() are copied out and before &mut self.data[..] is taken — correct, but subtle enough that a maintainer should confirm the pattern. The code paths affected (bun build --compile Mach-O patching, macOS crash-handler backtrace) are user-visible and platform-specific; the darwin-only regression test (29120) was skipped on this Linux run.

Other factors

• The bug-hunting system found no issues.
• Tests: 74 pass / 11 skip / 1 fail; the single failure is confirmed pre-existing on main and unrelated (version-string mismatch in debug builds).
• CodeRabbit's one finding was withdrawn after the author explained it's pre-existing behavior intentionally preserved.
• Two github-actions comment-cop notes remain open on src/sys/lib.rs:6056,6073 flagging comment length — these appear to target the new one-line doc comments and look like linter false positives, but they are unresolved.
• I verified the Pod layout claims by hand (segment_command_64: 2×u32 + [u8;16] + 4×u64 + 2×i32 + 2×u32 = 72 bytes, u64 fields at offset 24, no padding), and that patch<T> preserves the old error-before-write ordering.

Given the subtlety of the borrow restructuring in code that ships user binaries, and the open bot comments, I'm deferring rather than approving.

[robobun]

^{Updated 12:58 PM PT - Aug 11th, 2026}

✅ @robobun, your commit bfd5d3f4c60dea148a2555d03efb035667496023 passed in Build #92396! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37569

That installs a local version of the PR into your bun-37569 executable, so you can run:

bun-37569 --bun