Merge pull request #127 from rust-osdev/iterator-fix

multiboot2: convenience and safety fixes (0.15.1)

Author: phip1611

multiboot2/Cargo.toml

@@ -6,7 +6,7 @@ Multiboot2-compliant bootloaders, like GRUB. It supports all tags from the speci
 including full support for the sections of ELF-64. This library is `no_std` and can be
 used in a Multiboot2-kernel.
 """
-version = "0.15.0"
+version = "0.15.1"
 authors = [
     "Philipp Oppermann <[email protected]>",
     "Calvin Lee <[email protected]>",
@@ -38,4 +38,5 @@ unstable = []
 
 [dependencies]
 bitflags = "1"
-derive_more = { version = "0.99.17", default-features = false, features = ["display"] }
+derive_more = { version = "0.99", default-features = false, features = ["display"] }
+log = { version = "0.4", default-features = false }

multiboot2/Changelog.md

@@ -1,5 +1,19 @@
 # CHANGELOG for crate `multiboot2`
 
+## 0.15.1 (2023-03-18)
+- **BREAKING** `MemoryMapTag::all_memory_areas()` was renamed to `memory_areas`
+  and now returns `MemoryAreaIter` instead of
+  `impl Iterator<Item = &MemoryArea>`. Experience showed that its better to
+  return the specific iterator whenever possible.
+- **BREAKING** `MemoryMapTag::memory_areas()` was renamed to
+  `available_memory_areas`
+  (_Sorry for the breaking changes in a minor release, but I just stumbled upon
+  this und since the last breaking release was just yesterday, users have to
+  deal with changes anyway._)
+- **BREAKING** `ElfSection::name()` now returns a Result instead of just the
+  value. This prevents possible panics.
+- fix: prevent a possible panic in `ElfSection::section_type()`
+
 ## 0.15.0 (2023-03-17)
 - **BREAKING** MSRV is 1.56.1
 - **BREAKING** fixed lifetime issues: `VBEInfoTag` is no longer `&static`

multiboot2/src/elf_sections.rs

@@ -1,6 +1,7 @@
 use crate::tag_type::Tag;
 use crate::TagType;
 use core::fmt::{Debug, Formatter};
+use core::str::Utf8Error;
 
 /// This tag contains section header table from an ELF kernel.
 ///
@@ -178,7 +179,13 @@ impl ElfSection {
             11 => ElfSectionType::DynamicLoaderSymbolTable,
             0x6000_0000..=0x6FFF_FFFF => ElfSectionType::EnvironmentSpecific,
             0x7000_0000..=0x7FFF_FFFF => ElfSectionType::ProcessorSpecific,
-            _ => panic!(),
+            e => {
+                log::warn!(
+                    "Unknown section type {:x}. Treating as ElfSectionType::Unused",
+                    e
+                );
+                ElfSectionType::Unused
+            }
         }
     }
 
@@ -188,7 +195,7 @@ impl ElfSection {
     }
 
     /// Read the name of the section.
-    pub fn name(&self) -> &str {
+    pub fn name(&self) -> Result<&str, Utf8Error> {
         use core::{slice, str};
 
         let name_ptr = unsafe { self.string_table().offset(self.get().name_index() as isize) };
@@ -202,7 +209,7 @@ impl ElfSection {
             len as usize
         };
 
-        str::from_utf8(unsafe { slice::from_raw_parts(name_ptr, strlen) }).unwrap()
+        str::from_utf8(unsafe { slice::from_raw_parts(name_ptr, strlen) })
     }
 
     /// Get the physical start address of the section.
@@ -246,15 +253,15 @@ impl ElfSection {
         match self.entry_size {
             40 => unsafe { &*(self.inner as *const ElfSectionInner32) },
             64 => unsafe { &*(self.inner as *const ElfSectionInner64) },
-            _ => panic!(),
+            s => panic!("Unexpected entry size: {}", s),
         }
     }
 
     unsafe fn string_table(&self) -> *const u8 {
         let addr = match self.entry_size {
             40 => (*(self.string_section as *const ElfSectionInner32)).addr as usize,
             64 => (*(self.string_section as *const ElfSectionInner64)).addr as usize,
-            _ => panic!(),
+            s => panic!("Unexpected entry size: {}", s),
         };
         (addr + self.offset) as *const _
     }

multiboot2/src/framebuffer.rs

@@ -93,7 +93,7 @@ pub struct FramebufferColor {
 
 /// Error when an unknown [`FramebufferTypeId`] is found.
 #[derive(Debug, Copy, Clone, Display, PartialEq, Eq)]
-#[display(fmt = "Unknown framebuffer type _0")]
+#[display(fmt = "Unknown framebuffer type {}", _0)]
 pub struct UnknownFramebufferType(u8);
 
 #[cfg(feature = "unstable")]

multiboot2/src/lib.rs

@@ -1188,15 +1188,15 @@ mod tests {
         assert_eq!(bytes.len(), bi.total_size());
         let es = bi.elf_sections_tag().unwrap();
         let mut s = es.sections();
-        let s1 = s.next().unwrap();
-        assert_eq!(".rodata", s1.name());
+        let s1 = s.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());
         assert_eq!(0xFFFF_8000_0010_3000, s1.end_address());
         assert_eq!(0x0000_0000_0000_3000, s1.size());
         assert_eq!(ElfSectionFlags::ALLOCATED, s1.flags());
         assert_eq!(ElfSectionType::ProgramSection, s1.section_type());
-        let s2 = s.next().unwrap();
-        assert_eq!(".text", s2.name());
+        let s2 = s.next().expect("Should have one more section");
+        assert_eq!(".text", s2.name().expect("Should be valid utf-8"));
         assert_eq!(0xFFFF_8000_0010_3000, s2.start_address());
         assert_eq!(0xFFFF_8000_0010_C000, s2.end_address());
         assert_eq!(0x0000_0000_0000_9000, s2.size());
@@ -1205,8 +1205,8 @@ mod tests {
             s2.flags()
         );
         assert_eq!(ElfSectionType::ProgramSection, s2.section_type());
-        let s3 = s.next().unwrap();
-        assert_eq!(".data", s3.name());
+        let s3 = s.next().expect("Should have one more section");
+        assert_eq!(".data", s3.name().expect("Should be valid utf-8"));
         assert_eq!(0xFFFF_8000_0010_C000, s3.start_address());
         assert_eq!(0xFFFF_8000_0010_E000, s3.end_address());
         assert_eq!(0x0000_0000_0000_2000, s3.size());
@@ -1215,8 +1215,8 @@ mod tests {
             s3.flags()
         );
         assert_eq!(ElfSectionType::ProgramSection, s3.section_type());
-        let s4 = s.next().unwrap();
-        assert_eq!(".bss", s4.name());
+        let s4 = s.next().expect("Should have one more section");
+        assert_eq!(".bss", s4.name().expect("Should be valid utf-8"));
         assert_eq!(0xFFFF_8000_0010_E000, s4.start_address());
         assert_eq!(0xFFFF_8000_0011_3000, s4.end_address());
         assert_eq!(0x0000_0000_0000_5000, s4.size());
@@ -1225,8 +1225,8 @@ mod tests {
             s4.flags()
         );
         assert_eq!(ElfSectionType::Uninitialized, s4.section_type());
-        let s5 = s.next().unwrap();
-        assert_eq!(".data.rel.ro", s5.name());
+        let s5 = s.next().expect("Should have one more section");
+        assert_eq!(".data.rel.ro", s5.name().expect("Should be valid utf-8"));
         assert_eq!(0xFFFF_8000_0011_3000, s5.start_address());
         assert_eq!(0xFFFF_8000_0011_3000, s5.end_address());
         assert_eq!(0x0000_0000_0000_0000, s5.size());
@@ -1235,29 +1235,29 @@ mod tests {
             s5.flags()
         );
         assert_eq!(ElfSectionType::ProgramSection, s5.section_type());
-        let s6 = s.next().unwrap();
-        assert_eq!(".symtab", s6.name());
+        let s6 = s.next().expect("Should have one more section");
+        assert_eq!(".symtab", s6.name().expect("Should be valid utf-8"));
         assert_eq!(0x0000_0000_0011_3000, s6.start_address());
         assert_eq!(0x0000_0000_0011_5BE0, s6.end_address());
         assert_eq!(0x0000_0000_0000_2BE0, s6.size());
         assert_eq!(ElfSectionFlags::empty(), s6.flags());
         assert_eq!(ElfSectionType::LinkerSymbolTable, s6.section_type());
-        let s7 = s.next().unwrap();
-        assert_eq!(".strtab", s7.name());
+        let s7 = s.next().expect("Should have one more section");
+        assert_eq!(".strtab", s7.name().expect("Should be valid utf-8"));
         assert_eq!(0x0000_0000_0011_5BE0, s7.start_address());
         assert_eq!(0x0000_0000_0011_9371, s7.end_address());
         assert_eq!(0x0000_0000_0000_3791, s7.size());
         assert_eq!(ElfSectionFlags::empty(), s7.flags());
         assert_eq!(ElfSectionType::StringTable, s7.section_type());
-        let s8 = s.next().unwrap();
-        assert_eq!(".shstrtab", s8.name());
+        let s8 = s.next().expect("Should have one more section");
+        assert_eq!(".shstrtab", s8.name().expect("Should be valid utf-8"));
         assert_eq!(string_addr, s8.start_address());
         assert_eq!(string_addr + string_bytes.len() as u64, s8.end_address());
         assert_eq!(string_bytes.len() as u64, s8.size());
         assert_eq!(ElfSectionFlags::empty(), s8.flags());
         assert_eq!(ElfSectionType::StringTable, s8.section_type());
         assert!(s.next().is_none());
-        let mut mm = bi.memory_map_tag().unwrap().memory_areas();
+        let mut mm = bi.memory_map_tag().unwrap().available_memory_areas();
         let mm1 = mm.next().unwrap();
         assert_eq!(0x00000000, mm1.start_address());
         assert_eq!(0x009_FC00, mm1.end_address());
@@ -1373,8 +1373,8 @@ mod tests {
         assert_eq!(bytes.0.len(), bi.total_size());
         let es = bi.elf_sections_tag().unwrap();
         let mut s = es.sections();
-        let s1 = s.next().unwrap();
-        assert_eq!(".shstrtab", s1.name());
+        let s1 = s.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());
         assert_eq!(string_addr + string_bytes.0.len() as u64, s1.end_address());
         assert_eq!(string_bytes.0.len() as u64, s1.size());

multiboot2/src/memory_map.rs

@@ -22,14 +22,15 @@ pub struct MemoryMapTag {
 }
 
 impl MemoryMapTag {
-    /// Return an iterator over all AVAILABLE marked memory areas.
-    pub fn memory_areas(&self) -> impl Iterator<Item = &MemoryArea> {
-        self.all_memory_areas()
+    /// Return an iterator over all memory areas that have the type
+    /// [`MemoryAreaType::Available`].
+    pub fn available_memory_areas(&self) -> impl Iterator<Item = &MemoryArea> {
+        self.memory_areas()
             .filter(|entry| matches!(entry.typ, MemoryAreaType::Available))
     }
 
-    /// Return an iterator over all marked memory areas.
-    pub fn all_memory_areas(&self) -> impl Iterator<Item = &MemoryArea> {
+    /// Return an iterator over all memory areas.
+    pub fn memory_areas(&self) -> MemoryAreaIter {
         let self_ptr = self as *const MemoryMapTag;
         let start_area = self.first_area.as_ptr();