multiboot2: new_boxed() can be used with less allocations on callee side

Note that Miri runs significantly longer with this change. More memory accesses
that need to be tracked.

Author: phip1611

multiboot2/src/boot_loader_name.rs

@@ -2,10 +2,10 @@
 
 use crate::tag::TagHeader;
 use crate::{new_boxed, parse_slice_as_string, StringError, TagTrait, TagType};
+#[cfg(feature = "builder")]
+use alloc::boxed::Box;
 use core::fmt::{Debug, Formatter};
 use core::mem;
-#[cfg(feature = "builder")]
-use {alloc::boxed::Box, alloc::vec::Vec};
 
 const METADATA_SIZE: usize = mem::size_of::<TagHeader>();
 
@@ -23,12 +23,12 @@ impl BootLoaderNameTag {
     #[cfg(feature = "builder")]
     #[must_use]
     pub fn new(name: &str) -> Box<Self> {
-        let mut bytes: Vec<_> = name.bytes().collect();
-        if !bytes.ends_with(&[0]) {
-            // terminating null-byte
-            bytes.push(0);
+        let bytes = name.as_bytes();
+        if bytes.ends_with(&[0]) {
+            new_boxed(&[bytes])
+        } else {
+            new_boxed(&[bytes, &[0]])
         }
-        new_boxed(&bytes)
     }
 
     /// Returns the underlying [`TagType`].

multiboot2/src/command_line.rs

@@ -27,12 +27,12 @@ impl CommandLineTag {
     #[cfg(feature = "builder")]
     #[must_use]
     pub fn new(command_line: &str) -> Box<Self> {
-        let mut bytes: Vec<_> = command_line.bytes().collect();
-        if !bytes.ends_with(&[0]) {
-            // terminating null-byte
-            bytes.push(0);
+        let bytes = command_line.as_bytes();
+        if bytes.ends_with(&[0]) {
+            new_boxed(&[bytes])
+        } else {
+            new_boxed(&[bytes, &[0]])
         }
-        new_boxed(&bytes)
     }
 
     /// Reads the command line of the kernel as Rust string slice without

multiboot2/src/elf_sections.rs

@@ -27,14 +27,10 @@ impl ElfSectionsTag {
     #[cfg(feature = "builder")]
     #[must_use]
     pub fn new(number_of_sections: u32, entry_size: u32, shndx: u32, sections: &[u8]) -> Box<Self> {
-        let mut bytes = [
-            number_of_sections.to_le_bytes(),
-            entry_size.to_le_bytes(),
-            shndx.to_le_bytes(),
-        ]
-        .concat();
-        bytes.extend_from_slice(sections);
-        new_boxed(&bytes)
+        let number_of_sections = number_of_sections.to_ne_bytes();
+        let entry_size = entry_size.to_ne_bytes();
+        let shndx = shndx.to_ne_bytes();
+        new_boxed(&[&number_of_sections, &entry_size, &shndx, sections])
     }
 
     /// Get an iterator of loaded ELF sections.

multiboot2/src/framebuffer.rs

@@ -95,13 +95,13 @@ impl FramebufferTag {
         bpp: u8,
         buffer_type: FramebufferType,
     ) -> Box<Self> {
-        let mut bytes: Vec<u8> = address.to_le_bytes().into();
-        bytes.extend(pitch.to_le_bytes());
-        bytes.extend(width.to_le_bytes());
-        bytes.extend(height.to_le_bytes());
-        bytes.extend(bpp.to_le_bytes());
-        bytes.extend(buffer_type.to_bytes());
-        new_boxed(&bytes)
+        let address = address.to_ne_bytes();
+        let pitch = pitch.to_ne_bytes();
+        let width = width.to_ne_bytes();
+        let height = height.to_ne_bytes();
+        let bpp = bpp.to_ne_bytes();
+        let buffer_type = buffer_type.to_bytes();
+        new_boxed(&[&address, &pitch, &width, &height, &bpp, &buffer_type])
     }
 
     /// Contains framebuffer physical address.
@@ -145,6 +145,7 @@ impl FramebufferTag {
         match typ {
             FramebufferTypeId::Indexed => {
                 let num_colors = reader.read_u32();
+                // TODO static cast looks like UB?
                 let palette = unsafe {
                     slice::from_raw_parts(
                         reader.current_address() as *const FramebufferColor,
@@ -274,23 +275,23 @@ impl<'a> FramebufferType<'a> {
         let mut v = Vec::new();
         match self {
             FramebufferType::Indexed { palette } => {
-                v.extend(0u8.to_le_bytes()); // type
-                v.extend(0u16.to_le_bytes()); // reserved
-                v.extend((palette.len() as u32).to_le_bytes());
+                v.extend(0u8.to_ne_bytes()); // type
+                v.extend(0u16.to_ne_bytes()); // reserved
+                v.extend((palette.len() as u32).to_ne_bytes());
                 for color in palette.iter() {
                     v.extend(color.as_bytes());
                 }
             }
             FramebufferType::RGB { red, green, blue } => {
-                v.extend(1u8.to_le_bytes()); // type
-                v.extend(0u16.to_le_bytes()); // reserved
+                v.extend(1u8.to_ne_bytes()); // type
+                v.extend(0u16.to_ne_bytes()); // reserved
                 v.extend(red.as_bytes());
                 v.extend(green.as_bytes());
                 v.extend(blue.as_bytes());
             }
             FramebufferType::Text => {
-                v.extend(2u8.to_le_bytes()); // type
-                v.extend(0u16.to_le_bytes()); // reserved
+                v.extend(2u8.to_ne_bytes()); // type
+                v.extend(0u16.to_ne_bytes()); // reserved
             }
         }
         v

multiboot2/src/memory_map.rs

@@ -11,7 +11,7 @@ use core::fmt::{Debug, Formatter};
 use core::marker::PhantomData;
 use core::mem;
 #[cfg(feature = "builder")]
-use {crate::builder::AsBytes, crate::new_boxed, alloc::boxed::Box};
+use {crate::new_boxed, alloc::boxed::Box, core::slice};
 
 const METADATA_SIZE: usize = mem::size_of::<TagHeader>() + 2 * mem::size_of::<u32>();
 
@@ -39,13 +39,14 @@ impl MemoryMapTag {
     #[cfg(feature = "builder")]
     #[must_use]
     pub fn new(areas: &[MemoryArea]) -> Box<Self> {
-        let entry_size: u32 = mem::size_of::<MemoryArea>().try_into().unwrap();
-        let entry_version: u32 = 0;
-        let mut bytes = [entry_size.to_le_bytes(), entry_version.to_le_bytes()].concat();
-        for area in areas {
-            bytes.extend(area.as_bytes());
-        }
-        new_boxed(bytes.as_slice())
+        let entry_size = mem::size_of::<MemoryArea>().to_ne_bytes();
+        let entry_version = 0_u32.to_ne_bytes();
+        let areas = {
+            let ptr = areas.as_ptr().cast::<u8>();
+            let len = areas.len() * size_of::<MemoryArea>();
+            unsafe { slice::from_raw_parts(ptr, len) }
+        };
+        new_boxed(&[&entry_size, &entry_version, areas])
     }
 
     /// Returns the entry size.
@@ -139,9 +140,6 @@ impl Debug for MemoryArea {
     }
 }
 
-#[cfg(feature = "builder")]
-impl AsBytes for MemoryArea {}
-
 /// ABI-friendly version of [`MemoryAreaType`].
 #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 #[repr(C)]
@@ -292,9 +290,6 @@ impl TagTrait for BasicMemoryInfoTag {
 
 const EFI_METADATA_SIZE: usize = mem::size_of::<TagTypeId>() + 3 * mem::size_of::<u32>();
 
-#[cfg(feature = "builder")]
-impl AsBytes for EFIMemoryDesc {}
-
 /// EFI memory map tag. The embedded [`EFIMemoryDesc`]s follows the EFI
 /// specification.
 #[derive(ptr_meta::Pointee, PartialEq, Eq, PartialOrd, Ord, Hash)]
@@ -322,54 +317,30 @@ pub struct EFIMemoryMapTag {
 
 impl EFIMemoryMapTag {
     /// Create a new EFI memory map tag with the given memory descriptors.
-    /// Version and size can't be set because you're passing a slice of
-    /// EFIMemoryDescs, not the ones you might have gotten from the firmware.
     #[cfg(feature = "builder")]
     #[must_use]
     pub fn new_from_descs(descs: &[EFIMemoryDesc]) -> Box<Self> {
-        // TODO replace this EfiMemorydesc::uefi_desc_size() in the next uefi_raw
-        // release.
-
-        let size_base = mem::size_of::<EFIMemoryDesc>();
-        // Taken from https://github.com/tianocore/edk2/blob/7142e648416ff5d3eac6c6d607874805f5de0ca8/MdeModulePkg/Core/PiSmmCore/Page.c#L1059
-        let desc_size_diff = mem::size_of::<u64>() - size_base % mem::size_of::<u64>();
-        let desc_size = size_base + desc_size_diff;
-
-        assert!(desc_size >= size_base);
-
-        let mut efi_mmap = alloc::vec::Vec::with_capacity(descs.len() * desc_size);
-        for desc in descs {
-            efi_mmap.extend(desc.as_bytes());
-            // fill with zeroes
-            efi_mmap.extend([0].repeat(desc_size_diff));
-        }
+        let efi_mmap = {
+            let ptr = descs.as_ptr().cast::<u8>();
+            let len = descs.len() * size_of::<EFIMemoryDesc>();
+            unsafe { slice::from_raw_parts(ptr, len) }
+        };
 
         Self::new_from_map(
-            desc_size as u32,
+            mem::size_of::<EFIMemoryDesc>() as u32,
             EFIMemoryDesc::VERSION,
-            efi_mmap.as_slice(),
+            efi_mmap,
         )
     }
 
     /// Create a new EFI memory map tag from the given EFI memory map.
     #[cfg(feature = "builder")]
     #[must_use]
     pub fn new_from_map(desc_size: u32, desc_version: u32, efi_mmap: &[u8]) -> Box<Self> {
-        assert!(desc_size > 0);
-        assert_eq!(efi_mmap.len() % desc_size as usize, 0);
-        assert_eq!(
-            efi_mmap
-                .as_ptr()
-                .align_offset(mem::align_of::<EFIMemoryDesc>()),
-            0
-        );
-        let bytes = [
-            &desc_size.to_le_bytes(),
-            &desc_version.to_le_bytes(),
-            efi_mmap,
-        ]
-        .concat();
-        new_boxed(&bytes)
+        assert_ne!(desc_size, 0);
+        let desc_size = desc_size.to_ne_bytes();
+        let desc_version = desc_version.to_ne_bytes();
+        new_boxed(&[&desc_size, &desc_version, efi_mmap])
     }
 
     /// Returns an iterator over the provided memory areas.
@@ -491,8 +462,6 @@ mod tests {
         ];
         let efi_mmap_tag = EFIMemoryMapTag::new_from_descs(&descs);
 
-        assert_eq!(efi_mmap_tag.desc_size, 48 /* 40 + 8 */);
-
         let mut iter = efi_mmap_tag.memory_areas();
 
         assert_eq!(iter.next(), Some(&descs[0]));

multiboot2/src/module.rs

@@ -29,16 +29,15 @@ impl ModuleTag {
     pub fn new(start: u32, end: u32, cmdline: &str) -> Box<Self> {
         assert!(end > start, "must have a size");
 
-        let mut cmdline_bytes: Vec<_> = cmdline.bytes().collect();
-        if !cmdline_bytes.ends_with(&[0]) {
-            // terminating null-byte
-            cmdline_bytes.push(0);
+        let start = start.to_ne_bytes();
+        let end = end.to_ne_bytes();
+        let cmdline = cmdline.as_bytes();
+
+        if cmdline.ends_with(&[0]) {
+            new_boxed(&[&start, &end, cmdline])
+        } else {
+            new_boxed(&[&start, &end, cmdline, &[0]])
         }
-        let start_bytes = start.to_le_bytes();
-        let end_bytes = end.to_le_bytes();
-        let mut content_bytes = [start_bytes, end_bytes].concat();
-        content_bytes.extend_from_slice(&cmdline_bytes);
-        new_boxed(&content_bytes)
     }
 
     /// Reads the command line of the boot module as Rust string slice without

multiboot2/src/smbios.rs

@@ -25,9 +25,8 @@ impl SmbiosTag {
     #[cfg(feature = "builder")]
     #[must_use]
     pub fn new(major: u8, minor: u8, tables: &[u8]) -> Box<Self> {
-        let mut bytes = [major, minor, 0, 0, 0, 0, 0, 0].to_vec();
-        bytes.extend(tables);
-        new_boxed(&bytes)
+        let reserved = [0, 0, 0, 0, 0, 0];
+        new_boxed(&[&[major, minor], &reserved, tables])
     }
 
     /// Returns the major number.

multiboot2/src/util.rs

@@ -41,11 +41,17 @@ impl core::error::Error for StringError {
 /// constructor and box the result.
 ///
 /// # Parameters
-/// - `additional_bytes`: All bytes apart from the default [`TagHeader`] that
-///                       are included into the tag.
+/// - `additional_bytes_slices`: Array of byte slices that should be included
+///   without additional padding in-between. You don't need to add the bytes
+///   for [`TagHeader`], but only additional ones.
 #[cfg(feature = "alloc")]
-pub fn new_boxed<T: TagTrait + ?Sized>(additional_bytes: &[u8]) -> Box<T> {
-    let size = size_of::<TagHeader>() + additional_bytes.iter().len();
+pub fn new_boxed<T: TagTrait + ?Sized>(additional_bytes_slices: &[&[u8]]) -> Box<T> {
+    let additional_size = additional_bytes_slices
+        .iter()
+        .map(|b| b.len())
+        .sum::<usize>();
+
+    let size = size_of::<TagHeader>() + additional_size;
     let alloc_size = increase_to_alignment(size);
     let layout = Layout::from_size_align(alloc_size, ALIGNMENT).unwrap();
     let heap_ptr = unsafe { alloc::alloc::alloc(layout) };
@@ -55,9 +61,16 @@ pub fn new_boxed<T: TagTrait + ?Sized>(additional_bytes: &[u8]) -> Box<T> {
         heap_ptr.cast::<u32>().write(T::ID.val());
         heap_ptr.cast::<u32>().add(1).write(size as u32);
     }
-    unsafe {
-        let ptr = heap_ptr.add(size_of::<TagHeader>());
-        ptr::copy_nonoverlapping(additional_bytes.as_ptr(), ptr, additional_bytes.len());
+
+    let mut write_offset = size_of::<TagHeader>();
+    for &bytes in additional_bytes_slices {
+        unsafe {
+            let len = bytes.len();
+            let src = bytes.as_ptr();
+            let dst = heap_ptr.add(write_offset);
+            ptr::copy_nonoverlapping(src, dst, len);
+            write_offset += len;
+        }
     }
 
     let header = unsafe { heap_ptr.cast::<TagHeader>().as_ref() }.unwrap();
@@ -128,10 +141,16 @@ mod tests {
 
     #[test]
     fn test_new_boxed() {
-        let tag = new_boxed::<GenericTag>(&[0, 1, 2, 3]);
+        let tag = new_boxed::<GenericTag>(&[&[0, 1, 2, 3]]);
         assert_eq!(tag.header().typ, GenericTag::ID);
-        {}
-        let tag = new_boxed::<CommandLineTag>("hello\0".as_bytes());
+        assert_eq!(tag.payload(), &[0, 1, 2, 3]);
+
+        // Test that bytes are added consecutively without gaps.
+        let tag = new_boxed::<GenericTag>(&[&[0], &[1], &[2, 3]]);
+        assert_eq!(tag.header().typ, GenericTag::ID);
+        assert_eq!(tag.payload(), &[0, 1, 2, 3]);
+
+        let tag = new_boxed::<CommandLineTag>(&["hello\0".as_bytes()]);
         assert_eq!(tag.cmdline(), Ok("hello"));
     }
 }