Merge pull request #246 from rust-osdev/dev

multiboot2: bug fixes

Author: phip1611

integration-test/bins/multiboot2_payload/src/verify/mod.rs

@@ -48,8 +48,9 @@ pub(self) fn print_memory_map(mbi: &BootInformation) -> anyhow::Result<()> {
 
 pub(self) fn print_elf_info(mbi: &BootInformation) -> anyhow::Result<()> {
     let sections_iter = mbi
-        .elf_sections()
+        .elf_sections_tag()
         .ok_or("Should have elf sections")
+        .map(|tag| tag.sections())
         .map_err(anyhow::Error::msg)?;
     println!("ELF sections:");
     for s in sections_iter {

multiboot2/CHANGELOG.md

@@ -2,6 +2,14 @@
 
 ## Unreleased
 
+## v0.23.1 (2024-10-21)
+
+- Fix wrong tag ID when using `BootdevTag::new`
+- `BootInformation::elf_sections` is now deprecated and replaced by the newly
+- added `BootInformation::elf_sections_tag`. On the returned type, you can call
+  `.sections()` to iterate the sections
+- Fixed the debug output of `BootInformation`
+
 ## v0.23.0 (2024-09-17)
 
 - dependency updates

multiboot2/src/apm.rs

@@ -36,7 +36,7 @@ impl ApmTag {
         dseg_len: u16,
     ) -> Self {
         Self {
-            header: TagHeader::new(TagType::Apm, mem::size_of::<Self>() as u32),
+            header: TagHeader::new(Self::ID, mem::size_of::<Self>() as u32),
             version,
             cseg,
             offset,

multiboot2/src/boot_information.rs

@@ -254,7 +254,7 @@ impl<'a> BootInformation<'a> {
     /// # use multiboot2::{BootInformation, BootInformationHeader};
     /// # let ptr = 0xdeadbeef as *const BootInformationHeader;
     /// # let boot_info = unsafe { BootInformation::load(ptr).unwrap() };
-    /// if let Some(sections) = boot_info.elf_sections() {
+    /// if let Some(sections) = boot_info.elf_sections_tag().map(|tag| tag.sections()) {
     ///     let mut total = 0;
     ///     for section in sections {
     ///         println!("Section: {:?}", section);
@@ -263,14 +263,21 @@ impl<'a> BootInformation<'a> {
     /// }
     /// ```
     #[must_use]
+    #[deprecated = "Use elf_sections_tag() instead and corresponding getters"]
     pub fn elf_sections(&self) -> Option<ElfSectionIter> {
         let tag = self.get_tag::<ElfSectionsTag>();
         tag.map(|t| {
             assert!((t.entry_size() * t.shndx()) <= t.header().size);
-            t.sections_iter()
+            t.sections()
         })
     }
 
+    /// Search for the [`ElfSectionsTag`].
+    #[must_use]
+    pub fn elf_sections_tag(&self) -> Option<&ElfSectionsTag> {
+        self.get_tag()
+    }
+
     /// Search for the [`FramebufferTag`]. The result is `Some(Err(e))`, if the
     /// framebuffer type is unknown, while the framebuffer tag is present.
     #[must_use]
@@ -421,50 +428,42 @@ impl<'a> BootInformation<'a> {
 
 impl fmt::Debug for BootInformation<'_> {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        /// Limit how many Elf-Sections should be debug-formatted.
-        /// Can be thousands of sections for a Rust binary => this is useless output.
-        /// If the user really wants this, they should debug-format the field directly.
-        const ELF_SECTIONS_LIMIT: usize = 7;
-
         let mut debug = f.debug_struct("Multiboot2BootInformation");
         debug
             .field("start_address", &self.start_address())
             .field("end_address", &self.end_address())
             .field("total_size", &self.total_size())
             // now tags in alphabetical order
+            .field("apm", &self.apm_tag())
             .field("basic_memory_info", &(self.basic_memory_info_tag()))
             .field("boot_loader_name", &self.boot_loader_name_tag())
-            // .field("bootdev", &self.bootdev_tag())
+            .field("bootdev", &self.bootdev_tag())
             .field("command_line", &self.command_line_tag())
             .field("efi_bs_not_exited", &self.efi_bs_not_exited_tag())
+            .field("efi_ih32", &self.efi_ih32_tag())
+            .field("efi_ih64", &self.efi_ih64_tag())
             .field("efi_memory_map", &self.efi_memory_map_tag())
             .field("efi_sdt32", &self.efi_sdt32_tag())
             .field("efi_sdt64", &self.efi_sdt64_tag())
-            .field("efi_ih32", &self.efi_ih32_tag())
-            .field("efi_ih64", &self.efi_ih64_tag());
-
-        // usually this is REALLY big (thousands of tags) => skip it here
-        {
-            let elf_sections_tag_entries_count =
-                self.elf_sections().map(|x| x.count()).unwrap_or(0);
-
-            if elf_sections_tag_entries_count > ELF_SECTIONS_LIMIT {
-                debug.field("elf_sections (count)", &elf_sections_tag_entries_count);
-            } else {
-                debug.field("elf_sections", &self.elf_sections());
-            }
-        }
-
-        debug
+            .field("elf_sections", &self.elf_sections_tag())
             .field("framebuffer", &self.framebuffer_tag())
             .field("load_base_addr", &self.load_base_addr_tag())
             .field("memory_map", &self.memory_map_tag())
             .field("modules", &self.module_tags())
-            // .field("network", &self.network_tag())
+            .field("network", &self.network_tag())
             .field("rsdp_v1", &self.rsdp_v1_tag())
             .field("rsdp_v2", &self.rsdp_v2_tag())
-            .field("smbios_tag", &self.smbios_tag())
-            .field("vbe_info_tag", &self.vbe_info_tag())
+            .field("smbios", &self.smbios_tag())
+            .field("vbe_info", &self.vbe_info_tag())
+            // computed fields
+            .field("custom_tags_count", &{
+                self.tags()
+                    .filter(|tag| {
+                        let id: TagType = tag.header().typ.into();
+                        matches!(id, TagType::Custom(_))
+                    })
+                    .count()
+            })
             .finish()
     }
 }

multiboot2/src/bootdev.rs

@@ -19,7 +19,7 @@ impl BootdevTag {
     #[must_use]
     pub fn new(biosdev: u32, slice: u32, part: u32) -> Self {
         Self {
-            header: TagHeader::new(TagType::Apm, mem::size_of::<Self>() as u32),
+            header: TagHeader::new(Self::ID, mem::size_of::<Self>() as u32),
             biosdev,
             slice,
             part,

multiboot2/src/builder.rs

@@ -378,5 +378,6 @@ mod tests {
             // Mainly a test for Miri.
             dbg!(tag.header(), tag.payload().len());
         }
+        eprintln!("{info:#x?}")
     }
 }

multiboot2/src/elf_sections.rs

@@ -37,9 +37,9 @@ impl ElfSectionsTag {
         )
     }
 
-    /// Get an iterator of loaded ELF sections.
+    /// Get an iterator over the ELF sections.
     #[must_use]
-    pub(crate) const fn sections_iter(&self) -> ElfSectionIter {
+    pub const fn sections(&self) -> ElfSectionIter {
         let string_section_offset = (self.shndx * self.entry_size) as isize;
         let string_section_ptr =
             unsafe { self.sections.as_ptr().offset(string_section_offset) as *const _ };
@@ -96,12 +96,12 @@ impl Debug for ElfSectionsTag {
             .field("number_of_sections", &self.number_of_sections)
             .field("entry_size", &self.entry_size)
             .field("shndx", &self.shndx)
-            .field("sections", &self.sections_iter())
+            .field("sections", &self.sections())
             .finish()
     }
 }
 
-/// An iterator over some ELF sections.
+/// An iterator over [`ElfSection`]s.
 #[derive(Clone)]
 pub struct ElfSectionIter<'a> {
     current_section: *const u8,
@@ -132,27 +132,62 @@ impl<'a> Iterator for ElfSectionIter<'a> {
         }
         None
     }
+
+    fn size_hint(&self) -> (usize, Option<usize>) {
+        (
+            self.remaining_sections as usize,
+            Some(self.remaining_sections as usize),
+        )
+    }
+}
+
+impl ExactSizeIterator for ElfSectionIter<'_> {
+    fn len(&self) -> usize {
+        self.remaining_sections as usize
+    }
 }
 
 impl Debug for ElfSectionIter<'_> {
     fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result {
+        /// Limit how many Elf-Sections should be debug-formatted.
+        /// Can be thousands of sections for a Rust binary => this is useless output.
+        /// If the user really wants this, they should debug-format the field directly.
+        const ELF_SECTIONS_LIMIT: usize = 7;
+
         let mut debug = f.debug_list();
-        self.clone().for_each(|ref e| {
+
+        self.clone().take(ELF_SECTIONS_LIMIT).for_each(|ref e| {
             debug.entry(e);
         });
+
+        if self.clone().len() > ELF_SECTIONS_LIMIT {
+            debug.entry(&"...");
+        }
+
         debug.finish()
     }
 }
 
 /// A single generic ELF Section.
-#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
+// TODO Shouldn't this be called ElfSectionPtrs, ElfSectionWrapper or so?
+#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct ElfSection<'a> {
     inner: *const u8,
     string_section: *const u8,
     entry_size: u32,
     _phantom: PhantomData<&'a ()>,
 }
 
+impl Debug for ElfSection<'_> {
+    fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result {
+        let inner = self.get();
+        f.debug_struct("ElfSection")
+            .field("inner", &inner)
+            .field("string_section_ptr", &self.string_section)
+            .finish()
+    }
+}
+
 #[derive(Clone, Copy, Debug)]
 #[repr(C, packed)]
 struct ElfSectionInner32 {
@@ -297,7 +332,7 @@ impl ElfSection<'_> {
     }
 }
 
-trait ElfSectionInner {
+trait ElfSectionInner: Debug {
     fn name_index(&self) -> u32;
 
     fn typ(&self) -> u32;

multiboot2/src/end.rs

@@ -14,7 +14,7 @@ pub struct EndTag {
 impl Default for EndTag {
     fn default() -> Self {
         Self {
-            header: TagHeader::new(TagType::End, mem::size_of::<Self>() as u32),
+            header: TagHeader::new(Self::ID, mem::size_of::<Self>() as u32),
         }
     }
 }

multiboot2/src/lib.rs

@@ -168,7 +168,7 @@ mod tests {
         assert_eq!(addr, bi.start_address());
         assert_eq!(addr + bytes.0.len(), bi.end_address());
         assert_eq!(bytes.0.len(), bi.total_size());
-        assert!(bi.elf_sections().is_none());
+        assert!(bi.elf_sections_tag().is_none());
         assert!(bi.memory_map_tag().is_none());
         assert!(bi.module_tags().next().is_none());
         assert!(bi.boot_loader_name_tag().is_none());
@@ -191,7 +191,7 @@ mod tests {
         assert_eq!(addr, bi.start_address());
         assert_eq!(addr + bytes.0.len(), bi.end_address());
         assert_eq!(bytes.0.len(), bi.total_size());
-        assert!(bi.elf_sections().is_none());
+        assert!(bi.elf_sections_tag().is_none());
         assert!(bi.memory_map_tag().is_none());
         assert!(bi.module_tags().next().is_none());
         assert!(bi.boot_loader_name_tag().is_none());
@@ -214,7 +214,7 @@ mod tests {
         assert_eq!(addr, bi.start_address());
         assert_eq!(addr + bytes.0.len(), bi.end_address());
         assert_eq!(bytes.0.len(), bi.total_size());
-        assert!(bi.elf_sections().is_none());
+        assert!(bi.elf_sections_tag().is_none());
         assert!(bi.memory_map_tag().is_none());
         assert!(bi.module_tags().next().is_none());
         assert!(bi.boot_loader_name_tag().is_none());
@@ -240,7 +240,7 @@ mod tests {
         assert_eq!(addr, bi.start_address());
         assert_eq!(addr + bytes.0.len(), bi.end_address());
         assert_eq!(bytes.0.len(), bi.total_size());
-        assert!(bi.elf_sections().is_none());
+        assert!(bi.elf_sections_tag().is_none());
         assert!(bi.memory_map_tag().is_none());
         assert!(bi.module_tags().next().is_none());
         assert_eq!(
@@ -834,7 +834,7 @@ mod tests {
         assert_eq!(addr, bi.start_address());
         assert_eq!(addr + bytes.len(), bi.end_address());
         assert_eq!(bytes.len(), bi.total_size());
-        let mut es = bi.elf_sections().unwrap();
+        let mut es = bi.elf_sections_tag().unwrap().sections();
         let s1 = es.next().expect("Should have one more section");
         assert_eq!(".rodata", s1.name().expect("Should be valid utf-8"));
         assert_eq!(0xFFFF_8000_0010_0000, s1.start_address());
@@ -1007,8 +1007,7 @@ mod tests {
         ]);
         #[repr(C, align(8))]
         struct StringBytes([u8; 11]);
-        let string_bytes: StringBytes =
-            StringBytes([0, 46, 115, 104, 115, 116, 114, 116, 97, 98, 0]);
+        let string_bytes: StringBytes = StringBytes(*b"\0.shstrtab\0");
         let string_addr = string_bytes.0.as_ptr() as u64;
         for i in 0..8 {
             let offset = 108;
@@ -1019,10 +1018,13 @@ mod tests {
         let addr = ptr as usize;
         let bi = unsafe { BootInformation::load(ptr.cast()) };
         let bi = bi.unwrap();
+
+        eprintln!("boot information with elf sections: {bi:#x?}");
+
         assert_eq!(addr, bi.start_address());
         assert_eq!(addr + bytes.0.len(), bi.end_address());
         assert_eq!(bytes.0.len(), bi.total_size());
-        let mut es = bi.elf_sections().unwrap();
+        let mut es = bi.elf_sections_tag().unwrap().sections();
         let s1 = es.next().expect("Should have one more section");
         assert_eq!(".shstrtab", s1.name().expect("Should be valid utf-8"));
         assert_eq!(string_addr, s1.start_address());