Conversation
from_bytes read every 52-byte CompiledModuleGraphFile record out of the executable with read_unaligned, which also turned its four repr(u8) enum bytes (encoding, loader, module_format, side) into enum values. A byte outside an enum's discriminants is not a value of that enum, so a damaged executable produced undefined behavior before any of the offset checks ran; in practice it ran as if the byte were valid. Decode each record field by field instead: the StringPointers are read at their offsets and the four enum bytes go through from_repr, returning a Corruption naming the field for a byte that is not a discriminant, so the executable exits with "Corrupted module graph: module <field> is out of range" like the other damaged fields of the record.
|
Status: fix and test pushed, stacked on #37639 (this PR's base branch is that PR's branch; it should retarget to Reproduced by compiling a one-file app and writing 0xff into each of the four enum bytes (offsets 48..51) of the first module record: bun 1.4.0 and a debug build both run the program and exit 0. With this change each one exits 1 with The four new cases in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — closes the last unchecked field of the module record by validating each enum byte through from_repr before it becomes a value.
What was reviewed:
CompiledModuleGraphFile::read:offset_of!StringPointer reads and per-bytefrom_repron the four enums;as_chunkspreserves the old record count (integer division of the blob length).- Downstream loop:
moduleis now owned instead of&module; all field accesses areCopy, no behavior change. - Test offsets 48..51 line up with the
#[repr(C)]field order (6×StringPointer + 4×u8 = 52), and error strings match the newCorruptionvariants.
Extended reasoning...
Overview
Follow-up to #37639 in the same loop of StandaloneModuleGraph::from_bytes. The prior PR added bounds checks on every StringPointer offset in each 52-byte module record; this PR covers the remaining four bytes — the #[repr(u8)] enums encoding, loader, module_format, side — which were previously materialized directly via read_unaligned::<CompiledModuleGraphFile>(). Materializing a byte outside a #[repr(u8)] enum's discriminant set as that enum is immediate UB, so the check has to happen on the byte. The fix introduces CompiledModuleGraphFile::read which reads each StringPointer at its offset_of! position and routes each enum byte through strum::FromRepr::from_repr, returning a new Corruption::Module{Encoding,Loader,Format,Side} variant on miss. strum::FromRepr is derived on Encoding, ModuleFormat, FileSide and bun_ast::Loader. Four table-driven test cases write 0xff into each byte of the first record and assert exit 1 with the specific message.
Security risks
None. This is defensive parsing of the executable's own embedded section — it turns UB on damaged bytes into a clean error exit. No new attack surface; the input was already read, just not validated. The unsafe block in read is a read_unaligned of two u32s at an in-bounds offset of a fixed-size array, with an accurate SAFETY comment.
Level of scrutiny
Medium. The record decoder touches an unsafe read path, but it's a strict narrowing of the prior read_unaligned on the whole struct: the new code reads the same bytes at the same offsets, just with validation on the four that need it. The as_chunks::<52>() split yields the same record count as the old len / size_of (both discard any trailing remainder), and records.len() replaces modules_list_count in the entry-point-id check and reserve. The dropped let module = &module; re-borrow is fine — every downstream field access is on a Copy field.
Other factors
- Tests extend the existing
corruptedGraphstable exactly as the prior four cases do; the offsets (48–51) match the#[repr(C)]layout (6 × 8-byte StringPointer + 4 × u8), and the error substrings match the newCorruption::messagearms verbatim (including "module format" formodule_format). from_repris derived from the variant list, so the append-onlyLoaderenum stays in sync automatically — the same pattern already used for errno per the PR description.- The PR description states the four new tests fail without the src change and pass with it, and that adjacent compile suites still pass, satisfying the "test fails for the right reason" bar.
|
Nothing to change from the review above: it confirms the record count (as_chunks vs the old len / size_of), the Copy field accesses after dropping the reborrow, and the 48..51 offsets the tests patch. No open threads on the PR; waiting on CI. |
… under Miri Two tests in the crate: a record with a distinct value in every field round-trips through read, and every byte value in each enum slot is accepted exactly when it is a discriminant of that slot's enum, with the Corruption naming the slot otherwise. The second test also asserts that the enum slots are the whole tail of the record, so a field added later needs an entry. bun_standalone_graph joins the Miri crate set so the sweep runs under Miri, which reports the old whole-record read as UB at the first byte outside an enum. The FromRepr derive on Loader moves after VariantNames, the position another open change adds it in, so either landing order merges to one derive.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Since the first push (539f028):
The description is updated to match. The mordant check is red on the base branch as well (a finding in #37639's |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it rewrites the unsafe record-decode path in the standalone module graph loader (runs at every compiled-binary startup) and is stacked on #37639, a human look would still be worthwhile. Note that comment-cop fired again after the latest push (lines ~342 and ~709), though the flagged blocks are the required SAFETY comment and a two-line alignment note.
What was reviewed:
CompiledModuleGraphFile::read: verified the sixoffset_of!StringPointer reads stay in-bounds of the 52-byte array and thatStringPointeris valid for all bit patterns (#[repr(C)]twou32s).as_chunksvs the oldlen / size_ofyields the same record count and discards the same remainder; downstream field accesses (module.name,.loader, etc.) are Copy so dropping the&modulereborrow is fine.- Test offsets 48..51 match the
repr(C)layout (6×8-byte pointers then fouru8enums, no padding); the unit test's no-padding claim inrecord()holds (52 bytes, align 4).
Extended reasoning...
Overview
This PR closes a UB hole in StandaloneModuleGraph::from_bytes: the loop over 52-byte CompiledModuleGraphFile records previously did a single read_unaligned::<CompiledModuleGraphFile>(), which materializes four #[repr(u8)] enum fields (encoding, loader, module_format, side) directly from disk bytes. A byte outside an enum's discriminant set is UB at the read. The fix adds CompiledModuleGraphFile::read() which reads the six StringPointer fields via read_unaligned at their offset_of! positions and each enum byte via array indexing + strum::FromRepr::from_repr, returning a new Corruption::Module{Encoding,Loader,Format,Side} error on an invalid byte. The record loop now uses as_chunks::<52>() instead of manual pointer arithmetic. Also adds Debug/PartialEq/Eq derives (for the tests), four new Corruption variants with messages, an exhaustive unit test (all 256 byte values × 4 enum positions), four integration-test cases in bun-build-compile.test.ts, and bun_standalone_graph in the Miri crate list.
Security risks
The path parses bytes from an on-disk executable that may be corrupted or tampered with. The change strictly reduces risk: it replaces UB-on-bad-input with a checked error and removes a whole-struct read_unaligned in favor of per-field reads whose safety argument is simpler (StringPointer accepts any bit pattern; enum bytes go through a checked from_repr). No new attack surface.
Level of scrutiny
Medium-high. This is startup-critical code for every bun build --compile binary and contains unsafe. The change is small and net-negative in unsafe complexity, but the layout assumptions (offsets 48..51, no padding, 52-byte records) and the switch from a reborrowed &module to an owned value both deserve a second pair of eyes even though I verified them against the #[repr(C)] definition and downstream uses.
Other factors
- Stacked on #37639; base branch is that PR's branch, so merge order matters.
- The comment-cop bot fired at 14:24:19–20 UTC, ~17s after the latest commit
f1d517e4(14:24:02), so two flags are still outstanding on the current diff. They target the SAFETY block insidepointerand the two-lineas_chunkscomment — the SAFETY comment is required by REVIEW.md, so this may just need a manual dismissal, but the author should confirm. - Test coverage is strong: the unit test exhaustively covers every byte value at every enum offset and asserts the enum-tail length so a new enum field forces a new table entry; the integration tests write
0xffat each offset and check for exit 1 with the field-specific message. as_chunksis already used across the codebase (encoding.rs, Blob.rs, npm.rs, etc.), so no MSRV concern.strumwas already a dependency ofbun_standalone_graph.
|
On the two comment-cop flags from the latest push: both threads are answered and resolved. The block at line 342 is the doc comment of On this push the |
This branch has not been deployed
2 participants
Stacked on #37639: the base branch is that PR's branch, so this diff is only the follow-up its review asked for. When #37639 lands I rebase this onto
main.Problem
StandaloneModuleGraph::from_bytesread each 52-byteCompiledModuleGraphFilerecord out of the executable withread_unaligned, so the record's four#[repr(u8)]enum bytes (encoding,loader,module_format,side, offsets 48..51) became enum values without a check.bun build --compile --target=bun-<os>-<arch>-v<x.y.z>andexecutablePathembed a graph written by this Bun into a runtime of another version, so a loader this Bun knows can be a byte the runtime does not.Fix
CompiledModuleGraphFile::readdecodes a record field by field. The sixStringPointers are read at theiroffset_of!positions (any bytes are a validStringPointer). Each enum byte goes throughfrom_repr(strum::FromRepr, derived onEncoding,ModuleFormat,FileSideandbun_ast::Loader). A byte that is not a discriminant returnsCorruption::ModuleEncoding/ModuleLoader/ModuleFormat/ModuleSide, and the executable exits 1 with "Corrupted module graph: module is out of range".from_bytessplits the module list withas_chunksand builds eachFilefrom the decoded record, so no enum value is made from an unchecked byte.to_byteswrites these bytes from the enums, so an executable whose runtime matches decodes to the same values as before (the debug round-trip at the end ofto_bytesnow runs the decoder on every record it writes). Any other byte is damage or a version mismatch, and standalone: check every offset in the embedded module graph and reject a repeated module name #37639 made a bad field of this record an error rather than something to act on.from_repris derived from the variant list, so an appendedLoadervariant keeps the check correct. Delete the schema::api mirror types and bun_api; one loader numbering across Rust/C++/JS #37095 adds the same derive toLoaderfor the plugin ABI, in the same position, so either landing order merges to one derive.bun_errnousesfrom_reprthe same way for untrusted errno values.test/bundler/bun-build-compile.test.ts: four new cases in thecorruptedGraphstable, one per enum byte. Each writes 0xff into the first record and expects exit 1 with the message that names the field. Without thesrc/change all four fail (the executable runs, stdoutshould not run, exit 0). With it the whole file passes.src/standalone_graph/StandaloneModuleGraph.rs: a record with a distinct value in every field round-trips throughread, and every byte value in each enum slot is accepted exactly when it is a discriminant of that slot's enum. The test also asserts that the enum slots are the whole tail of the record, so a later field needs an entry.bun_standalone_graphjoins the Miri crate set inscripts/rust-miri.ts(5 s to run, the whole set still passes), so the sweep runs under Miri, which is the tool that sees this class of bug.bun-build-compile-sourcemap.test.ts,compile-asset-bunfs.test.ts,bundler_html_server.test.ts(client-side records), the bytecode and embedded-file subset ofbundler_compile.test.ts,cargo clippyon both crates, andbun run rust:miri.Background
bun build --compileappends to a copy of bun.from_bytesparses it at startup. Its module list is an array of#[repr(C)]CompiledModuleGraphFilerecords: sixStringPointers ({offset: u32, length: u32}into the section) followed by the four one-byte enums.#[repr(u8)]enum is one byte wide, but only its declared discriminants are values of the type. Materializing any other byte as the enum (read_unaligned,transmute) is undefined behavior at that point, not at a latermatch, so the check has to look at the byte. Nothing at runtime detects it (ASAN does not, Miri does), which is why the unfixed executable appears to work.strum::FromReprderivesfn from_repr(u8) -> Option<Self>from the variant list.Corruption(src/standalone_graph/error.rs, from standalone: check every offset in the embedded module graph and reject a repeated module name #37639) has one variant per field of the graph that can be bad.Error::CorruptedModuleGraph(Corruption)is what prints "Corrupted module graph: ..." and exits 1.checked_range, which standalone: check every offset in the embedded module graph and reject a repeated module name #37639 adds). This PR adds no finding.Miri on the old shape, and the repro before and after
The new sweep test, run against a
readthat reads the whole record the way the old loop did (read_unaligned(record.as_ptr().cast::<Self>())), undercargo miri test -p bun_standalone_graph:A compiled one-file app with 0xff written into each enum byte of its first record. Before (bun 1.4.0 release and the debug build, the same for all four bytes):
After (debug build with this change):
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/bun-build-compile.test.ts