standalone: check the enum bytes of each module record before reading it

scripts/rust-miri.ts

@@ -50,6 +50,7 @@ const MIRI_CRATES = [
   "bun_ptr",
   "bun_resolve_builtins",
   "bun_shell_parser",
+  "bun_standalone_graph",
   "bun_threading",
   "bun_wyhash",
 ];

src/ast/loader.rs

@@ -23,6 +23,7 @@ use enum_map::Enum;
     Enum,
     strum::IntoStaticStr,
     strum::VariantNames,
+    strum::FromRepr,
 )]
 // The lower_snake names are exposed to JS (HTMLImportManifest
 // `"loader":`, BuildArtifact.loader) so the strum serialization must match exactly.

src/standalone_graph/StandaloneModuleGraph.rs

@@ -2,7 +2,7 @@
 //! But this incurred a fixed 350ms overhead on every build, which is unacceptable
 //! so we give up on codesigning support on macOS for now until we can find a better solution
 
-use core::mem::size_of;
+use core::mem::{offset_of, size_of};
 use core::ptr::NonNull;
 use std::io::Write as _;
 use std::sync::Arc;
@@ -321,7 +321,7 @@ impl bun_resolver::StandaloneModuleGraph for StandaloneModuleGraph {
 }
 
 #[repr(C)]
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub(crate) struct CompiledModuleGraphFile {
     pub name: StringPointer,
     pub contents: StringPointer,
@@ -337,16 +337,47 @@ pub(crate) struct CompiledModuleGraphFile {
     pub side: FileSide,
 }
 
+impl CompiledModuleGraphFile {
+    /// Decodes one record as `to_bytes` laid it out. The record is not read as a
+    /// whole because a byte outside one of its enums would make that read
+    /// undefined behavior, so each enum byte is checked first.
+    fn read(record: &[u8; size_of::<Self>()]) -> crate::Result<Self> {
+        let pointer = |field: usize| -> StringPointer {
+            // SAFETY: `field` is the offset of a `StringPointer` field of `Self`, so
+            // it lies inside `record`, which holds a whole `Self`; every bit pattern
+            // is a valid `StringPointer` (two `u32`s), and `read_unaligned` does not
+            // need `record` to be 4-byte aligned.
+            unsafe { core::ptr::read_unaligned(record.as_ptr().add(field).cast::<StringPointer>()) }
+        };
+        Ok(Self {
+            name: pointer(offset_of!(Self, name)),
+            contents: pointer(offset_of!(Self, contents)),
+            sourcemap: pointer(offset_of!(Self, sourcemap)),
+            bytecode: pointer(offset_of!(Self, bytecode)),
+            module_info: pointer(offset_of!(Self, module_info)),
+            bytecode_origin_path: pointer(offset_of!(Self, bytecode_origin_path)),
+            encoding: Encoding::from_repr(record[offset_of!(Self, encoding)])
+                .ok_or(Corruption::ModuleEncoding)?,
+            loader: Loader::from_repr(record[offset_of!(Self, loader)])
+                .ok_or(Corruption::ModuleLoader)?,
+            module_format: ModuleFormat::from_repr(record[offset_of!(Self, module_format)])
+                .ok_or(Corruption::ModuleFormat)?,
+            side: FileSide::from_repr(record[offset_of!(Self, side)])
+                .ok_or(Corruption::ModuleSide)?,
+        })
+    }
+}
+
 #[repr(u8)]
-#[derive(Clone, Copy, PartialEq, Eq, Default)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Default, strum::FromRepr)]
 pub enum FileSide {
     #[default]
     Server = 0,
     Client = 1,
 }
 
 #[repr(u8)]
-#[derive(Clone, Copy, PartialEq, Eq, Default)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Default, strum::FromRepr)]
 pub enum Encoding {
     Binary = 0,
     #[default]
@@ -356,7 +387,7 @@ pub enum Encoding {
 }
 
 #[repr(u8)]
-#[derive(Clone, Copy, PartialEq, Eq, Default)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Default, strum::FromRepr)]
 pub enum ModuleFormat {
     #[default]
     None = 0,
@@ -675,32 +706,23 @@ impl StandaloneModuleGraph {
                 Corruption::ModuleList,
             )?
         };
-        // Note: the modules blob sits at an arbitrary byte offset in the section, and
-        // `&[CompiledModuleGraphFile]` would require natural alignment (StringPointer's u32 fields
-        // → 4-byte). We instead iterate by index and `read_unaligned` each fixed-size record into a
-        // local (`CompiledModuleGraphFile` is `Copy`/POD), so no `&T` ever points at unaligned memory.
-        let modules_list_count = modules_list_bytes.len() / size_of::<CompiledModuleGraphFile>();
-        let modules_list_base = modules_list_bytes.as_ptr();
+        // The blob sits at an arbitrary byte offset, so it is split into byte records for
+        // `CompiledModuleGraphFile::read` rather than viewed as a `&[CompiledModuleGraphFile]`,
+        // which would need 4-byte alignment.
+        let (records, _) =
+            modules_list_bytes.as_chunks::<{ size_of::<CompiledModuleGraphFile>() }>();
 
         // `entry_point()` indexes `files` by this id. `to_bytes` writes one file per
         // record with distinct names, and a repeated name is rejected below, so the
         // record count is the file count.
-        if offsets.entry_point_id as usize >= modules_list_count {
+        if offsets.entry_point_id as usize >= records.len() {
             return Err(Corruption::EntryPointId.into());
         }
 
         let mut modules = StringArrayHashMap::<File>::new();
-        modules.reserve(modules_list_count);
-        for i in 0..modules_list_count {
-            // SAFETY: index < count derived from byte length above; bytes live for 'static.
-            let module: CompiledModuleGraphFile = unsafe {
-                core::ptr::read_unaligned(
-                    modules_list_base
-                        .add(i * size_of::<CompiledModuleGraphFile>())
-                        .cast::<CompiledModuleGraphFile>(),
-                )
-            };
-            let module = &module;
+        modules.reserve(records.len());
+        for record in records {
+            let module = CompiledModuleGraphFile::read(record)?;
             // SAFETY: each name/contents/sourcemap/bytecode_origin_path subrange is checked
             // against `raw_len` and is disjoint from the writable bytecode/module_info
             // subranges (serialized by `to_bytes`); section bytes are a live 'static allocation.
@@ -2599,3 +2621,91 @@ pub(crate) fn serialize_json_source_map_for_standalone(
     debug_assert!(header_list.len() == string_payload_start_location);
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// A record with a distinct value in every field, and its bytes as `to_bytes` writes them.
+    fn record() -> (
+        CompiledModuleGraphFile,
+        [u8; size_of::<CompiledModuleGraphFile>()],
+    ) {
+        let pointer = |offset, length| StringPointer { offset, length };
+        let module = CompiledModuleGraphFile {
+            name: pointer(1, 2),
+            contents: pointer(3, 4),
+            sourcemap: pointer(5, 6),
+            bytecode: pointer(7, 8),
+            module_info: pointer(9, 10),
+            bytecode_origin_path: pointer(11, 12),
+            encoding: Encoding::Utf8,
+            loader: Loader::Xml,
+            module_format: ModuleFormat::Cjs,
+            side: FileSide::Client,
+        };
+        // SAFETY: `CompiledModuleGraphFile` is `repr(C)` without padding (six 8-byte
+        // pointers and then four bytes, 4-byte aligned), so every byte of it is
+        // initialized, and a byte array has no alignment requirement.
+        let bytes = unsafe {
+            core::ptr::from_ref(&module)
+                .cast::<[u8; size_of::<CompiledModuleGraphFile>()]>()
+                .read()
+        };
+        (module, bytes)
+    }
+
+    #[test]
+    fn read_decodes_what_to_bytes_writes() {
+        let (module, bytes) = record();
+        assert_eq!(CompiledModuleGraphFile::read(&bytes), Ok(module));
+    }
+
+    #[test]
+    fn read_checks_each_enum_byte_against_its_own_enum() {
+        let (_, valid) = record();
+        let enum_bytes: [(usize, fn(u8) -> bool, Corruption); 4] = [
+            (
+                offset_of!(CompiledModuleGraphFile, encoding),
+                |byte| Encoding::from_repr(byte).is_some(),
+                Corruption::ModuleEncoding,
+            ),
+            (
+                offset_of!(CompiledModuleGraphFile, loader),
+                |byte| Loader::from_repr(byte).is_some(),
+                Corruption::ModuleLoader,
+            ),
+            (
+                offset_of!(CompiledModuleGraphFile, module_format),
+                |byte| ModuleFormat::from_repr(byte).is_some(),
+                Corruption::ModuleFormat,
+            ),
+            (
+                offset_of!(CompiledModuleGraphFile, side),
+                |byte| FileSide::from_repr(byte).is_some(),
+                Corruption::ModuleSide,
+            ),
+        ];
+        // The enums are the whole tail of the record, so a new one needs an entry above.
+        assert_eq!(
+            enum_bytes.len(),
+            size_of::<CompiledModuleGraphFile>() - offset_of!(CompiledModuleGraphFile, encoding)
+        );
+        for (at, is_discriminant, what) in enum_bytes {
+            for byte in 0..=u8::MAX {
+                let mut record = valid;
+                record[at] = byte;
+                let expected: crate::Result<()> = if is_discriminant(byte) {
+                    Ok(())
+                } else {
+                    Err(what.into())
+                };
+                assert_eq!(
+                    CompiledModuleGraphFile::read(&record).map(|_| ()),
+                    expected,
+                    "byte {byte} at offset {at}"
+                );
+            }
+        }
+    }
+}

src/standalone_graph/error.rs

@@ -1,6 +1,6 @@
 /// What `StandaloneModuleGraph::from_bytes` found wrong with the graph embedded
-/// in the executable. Every offset it reads comes from the file, so each one is
-/// checked before it is dereferenced.
+/// in the executable. Every offset and enum byte it reads comes from the file,
+/// so each one is checked before it is used.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum Corruption {
     ByteCount,
@@ -13,6 +13,10 @@ pub enum Corruption {
     ModuleBytecode,
     ModuleInfo,
     BytecodeOriginPath,
+    ModuleEncoding,
+    ModuleLoader,
+    ModuleFormat,
+    ModuleSide,
     CompileExecArgv,
 }
 
@@ -33,6 +37,10 @@ impl Corruption {
             Self::BytecodeOriginPath => {
                 "Corrupted module graph: bytecode origin path is out of range"
             }
+            Self::ModuleEncoding => "Corrupted module graph: module encoding is out of range",
+            Self::ModuleLoader => "Corrupted module graph: module loader is out of range",
+            Self::ModuleFormat => "Corrupted module graph: module format is out of range",
+            Self::ModuleSide => "Corrupted module graph: module side is out of range",
             Self::CompileExecArgv => "Corrupted module graph: compile exec argv is out of range",
         }
     }

test/bundler/bun-build-compile.test.ts

@@ -150,7 +150,8 @@ describe("compiled binary validity", () => {
   // The embedded module graph ends with `Offsets` (repr(C): byte_count u64, modules_ptr {offset u32, length u32},
   // entry_point_id u32, compile_exec_argv_ptr, flags) followed by the trailer. `byte_count` is the size of the
   // graph before `Offsets`, and `modules_ptr` points into it at the 52-byte `CompiledModuleGraphFile` records,
-  // each of which starts with the module name as {offset u32, length u32} into the graph.
+  // each of which starts with the module name as {offset u32, length u32} into the graph and ends with four
+  // one-byte enums: encoding, loader, module format, side.
   const GRAPH_TRAILER = "\n---- Bun! ----\n";
   const OFFSETS_SIZE = 32;
   const MODULE_RECORD_SIZE = 52;
@@ -168,11 +169,12 @@ describe("compiled binary validity", () => {
   }
   type ModuleGraphLayout = ReturnType<typeof locateModuleGraph>;
 
+  const oneFile = { "app.js": `console.log("should not run");` };
   const twoFiles = { "app.js": `console.log("should not run");`, "worker.js": `console.log("should not run");` };
   const corruptedGraphs = [
     {
       label: "entry_point_id equal to the module count",
-      files: { "app.js": `console.log("should not run");` },
+      files: oneFile,
       error: "entry point ID is out of range for the module list",
       corrupt(executable: Buffer, { moduleRecordCount, entryPointIdAt }: ModuleGraphLayout) {
         executable.writeUInt32LE(moduleRecordCount, entryPointIdAt);
@@ -205,6 +207,20 @@ describe("compiled binary validity", () => {
         executable.writeUInt32LE(0x7fffffff, modulesAt + MODULE_RECORD_SIZE + 12);
       },
     },
+    // The enum bytes at the end of the first record. 0xff is not a value of any of the four enums.
+    ...[
+      { field: "encoding", at: 48 },
+      { field: "loader", at: 49 },
+      { field: "format", at: 50 },
+      { field: "side", at: 51 },
+    ].map(({ field, at }) => ({
+      label: `an out-of-range module ${field} byte`,
+      files: oneFile,
+      error: `module ${field} is out of range`,
+      corrupt(executable: Buffer, { modulesAt }: ModuleGraphLayout) {
+        executable[modulesAt + at] = 0xff;
+      },
+    })),
   ];
 
   // A damaged graph must surface as an error, not an out-of-bounds panic in