Merge pull request #158 from rust-osdev/fixes2

multiboot2: create DSTs: hopefully better memory fix

Author: phip1611

multiboot2-header/src/builder/header.rs

@@ -8,7 +8,6 @@ use crate::{
     EntryAddressHeaderTag, EntryEfi32HeaderTag, EntryEfi64HeaderTag, FramebufferHeaderTag,
     ModuleAlignHeaderTag, Multiboot2BasicHeader, RelocatableHeaderTag,
 };
-use alloc::boxed::Box;
 use alloc::vec::Vec;
 use core::mem::size_of;
 use core::ops::Deref;
@@ -22,7 +21,7 @@ pub struct HeaderBytes {
     // guarantee alignment at the moment.
     offset: usize,
     structure_len: usize,
-    bytes: Box<[u8]>,
+    bytes: Vec<u8>,
 }
 
 impl HeaderBytes {
@@ -163,7 +162,7 @@ impl HeaderBuilder {
     pub fn build(mut self) -> HeaderBytes {
         const ALIGN: usize = 8;
 
-        // PHASE 1/3: Prepare Vector
+        // PHASE 1/2: Prepare Vector
 
         // We allocate more than necessary so that we can ensure an correct
         // alignment within this data.
@@ -184,17 +183,9 @@ impl HeaderBuilder {
         bytes.extend([0].repeat(offset));
 
         // -----------------------------------------------
-        // PHASE 2/3: Add Tags
+        // PHASE 2/2: Add Tags
         self.build_add_tags(&mut bytes);
 
-        // -----------------------------------------------
-        // PHASE 3/3: Finalize Vector
-
-        // Ensure that the vector has the same length as it's capacity. This is
-        // important so that miri doesn't complain that the boxed memory is
-        // smaller than the original allocation.
-        bytes.extend([0].repeat(bytes.capacity() - bytes.len()));
-
         assert_eq!(
             alloc_ptr,
             bytes.as_ptr(),
@@ -206,11 +197,6 @@ impl HeaderBuilder {
             "The offset to alignment area should be zero."
         );
 
-        // Construct a box from a vec without `into_boxed_slice`. The latter
-        // calls `shrink` on the allocator, which might reallocate this memory.
-        // We don't want that!
-        let bytes = unsafe { Box::from_raw(bytes.leak()) };
-
         HeaderBytes {
             offset,
             bytes,
@@ -316,6 +302,42 @@ mod tests {
         RelocatableHeaderTagPreference,
     };
 
+    fn create_builder() -> HeaderBuilder {
+        let builder = HeaderBuilder::new(HeaderTagISA::I386);
+        // Multiboot2 basic header + end tag
+        let mut expected_len = 16 + 8;
+        assert_eq!(builder.expected_len(), expected_len);
+
+        // add information request tag
+        let ifr_builder =
+            InformationRequestHeaderTagBuilder::new(HeaderTagFlag::Required).add_irs(&[
+                MbiTagType::EfiMmap,
+                MbiTagType::Cmdline,
+                MbiTagType::ElfSections,
+            ]);
+        let ifr_tag_size_with_padding = ifr_builder.expected_len() + 4;
+        assert_eq!(
+            ifr_tag_size_with_padding % 8,
+            0,
+            "the length of the IFR tag with padding must be a multiple of 8"
+        );
+        expected_len += ifr_tag_size_with_padding;
+        let builder = builder.information_request_tag(ifr_builder);
+        assert_eq!(builder.expected_len(), expected_len);
+
+        let builder = builder.relocatable_tag(RelocatableHeaderTag::new(
+            HeaderTagFlag::Required,
+            0x1337,
+            0xdeadbeef,
+            4096,
+            RelocatableHeaderTagPreference::None,
+        ));
+        expected_len += 0x18;
+        assert_eq!(builder.expected_len(), expected_len);
+
+        builder
+    }
+
     #[test]
     fn test_size_or_up_aligned() {
         assert_eq!(0, HeaderBuilder::size_or_up_aligned(0));
@@ -324,73 +346,44 @@ mod tests {
         assert_eq!(16, HeaderBuilder::size_or_up_aligned(9));
     }
 
+    /// Test of the `build` method in isolation specifically for miri to check
+    /// for memory issues.
+    #[test]
+    fn test_builder_miri() {
+        let builder = create_builder();
+        let expected_len = builder.expected_len();
+        assert_eq!(builder.build().as_bytes().len(), expected_len);
+    }
+
     #[test]
     fn test_builder() {
         // Step 1/2: Build Header
-        let (mb2_hdr_data, expected_len) = {
-            let builder = HeaderBuilder::new(HeaderTagISA::I386);
-            // Multiboot2 basic header + end tag
-            let mut expected_len = 16 + 8;
-            assert_eq!(builder.expected_len(), expected_len);
-
-            // add information request tag
-            let ifr_builder = InformationRequestHeaderTagBuilder::new(HeaderTagFlag::Required)
-                .add_irs(&[
-                    MbiTagType::EfiMmap,
-                    MbiTagType::Cmdline,
-                    MbiTagType::ElfSections,
-                ]);
-            let ifr_tag_size_with_padding = ifr_builder.expected_len() + 4;
-            assert_eq!(
-                ifr_tag_size_with_padding % 8,
-                0,
-                "the length of the IFR tag with padding must be a multiple of 8"
-            );
-            expected_len += ifr_tag_size_with_padding;
-            let builder = builder.information_request_tag(ifr_builder);
-            assert_eq!(builder.expected_len(), expected_len);
-
-            let builder = builder.relocatable_tag(RelocatableHeaderTag::new(
-                HeaderTagFlag::Required,
-                0x1337,
-                0xdeadbeef,
-                4096,
-                RelocatableHeaderTagPreference::None,
-            ));
-            expected_len += 0x18;
-            assert_eq!(builder.expected_len(), expected_len);
-
-            println!("builder: {:#?}", builder);
-            println!("expected_len: {} bytes", builder.expected_len());
-
-            (builder.build(), expected_len)
-        };
-
-        assert_eq!(mb2_hdr_data.as_bytes().len(), expected_len);
+        let mb2_hdr_data = create_builder().build();
 
         // Step 2/2: Test the built Header
-        {
-            let mb2_hdr = mb2_hdr_data.as_ptr().cast();
-            let mb2_hdr = unsafe { Multiboot2Header::load(mb2_hdr) }
-                .expect("the generated header to be loadable");
-            println!("{:#?}", mb2_hdr);
-            assert_eq!(
-                mb2_hdr.relocatable_tag().unwrap().flags(),
-                HeaderTagFlag::Required
-            );
-            assert_eq!(mb2_hdr.relocatable_tag().unwrap().min_addr(), 0x1337);
-            assert_eq!(mb2_hdr.relocatable_tag().unwrap().max_addr(), 0xdeadbeef);
-            assert_eq!(mb2_hdr.relocatable_tag().unwrap().align(), 4096);
-            assert_eq!(
-                mb2_hdr.relocatable_tag().unwrap().preference(),
-                RelocatableHeaderTagPreference::None
-            );
-
-            /* you can write the binary to a file and a tool such as crate "bootinfo"
-               will be able to fully parse the MB2 header
-            let mut file = std::file::File::create("mb2_hdr.bin").unwrap();
-            use std::io::Write;
-            file.write_all(mb2_hdr_data.as_slice()).unwrap();*/
-        }
+        let mb2_hdr = mb2_hdr_data.as_ptr().cast();
+        let mb2_hdr = unsafe { Multiboot2Header::load(mb2_hdr) }
+            .expect("the generated header to be loadable");
+        println!("{:#?}", mb2_hdr);
+        assert_eq!(
+            mb2_hdr.relocatable_tag().unwrap().flags(),
+            HeaderTagFlag::Required
+        );
+        assert_eq!(mb2_hdr.relocatable_tag().unwrap().min_addr(), 0x1337);
+        assert_eq!(mb2_hdr.relocatable_tag().unwrap().max_addr(), 0xdeadbeef);
+        assert_eq!(mb2_hdr.relocatable_tag().unwrap().align(), 4096);
+        assert_eq!(
+            mb2_hdr.relocatable_tag().unwrap().preference(),
+            RelocatableHeaderTagPreference::None
+        );
+
+        // Printing the header transitively ensures that a lot of stuff works.
+        println!("{:#?}", mb2_hdr);
+
+        /* you can write the binary to a file and a tool such as crate "bootinfo"
+           will be able to fully parse the MB2 header
+        let mut file = std::file::File::create("mb2_hdr.bin").unwrap();
+        use std::io::Write;
+        file.write_all(mb2_hdr_data.as_slice()).unwrap();*/
     }
 }

multiboot2/src/boot_loader_name.rs

@@ -5,8 +5,8 @@ use core::str::Utf8Error;
 
 #[cfg(feature = "builder")]
 use {
-    crate::builder::boxed_dst_tag, crate::builder::traits::StructAsBytes, crate::TagType,
-    alloc::boxed::Box, alloc::vec::Vec,
+    crate::builder::traits::StructAsBytes, crate::builder::BoxedDst, crate::TagType,
+    alloc::vec::Vec,
 };
 
 const METADATA_SIZE: usize = size_of::<TagTypeId>() + size_of::<u32>();
@@ -23,13 +23,13 @@ pub struct BootLoaderNameTag {
 
 impl BootLoaderNameTag {
     #[cfg(feature = "builder")]
-    pub fn new(name: &str) -> Box<Self> {
+    pub fn new(name: &str) -> BoxedDst<Self> {
         let mut bytes: Vec<_> = name.bytes().collect();
         if !bytes.ends_with(&[0]) {
             // terminating null-byte
             bytes.push(0);
         }
-        boxed_dst_tag(TagType::BootLoaderName, &bytes)
+        BoxedDst::new(TagType::BootLoaderName, &bytes)
     }
 
     /// Reads the name of the bootloader that is booting the kernel as Rust
@@ -119,5 +119,11 @@ mod tests {
         let bytes = tag.struct_as_bytes();
         assert_eq!(bytes, get_bytes());
         assert_eq!(tag.name(), Ok(MSG));
+
+        // test also some bigger message
+        let tag = BootLoaderNameTag::new("AbCdEfGhUjK YEAH");
+        assert_eq!(tag.name(), Ok("AbCdEfGhUjK YEAH"));
+        let tag = BootLoaderNameTag::new("AbCdEfGhUjK YEAH".repeat(42).as_str());
+        assert_eq!(tag.name(), Ok("AbCdEfGhUjK YEAH".repeat(42).as_str()));
     }
 }