multiboot2: refactored parsing of embedded strings in tags (name, cmdline)

Cargo.lock

@@ -40,7 +40,7 @@ dependencies = [
 
 [[package]]
 name = "multiboot2"
-version = "0.18.1"
+version = "0.19.0"
 dependencies = [
  "bitflags",
  "derive_more",

integration-test/bins/Cargo.lock

@@ -109,7 +109,7 @@ dependencies = [
 
 [[package]]
 name = "multiboot2"
-version = "0.18.1"
+version = "0.19.0"
 dependencies = [
  "bitflags 2.4.0",
  "derive_more",
@@ -135,7 +135,7 @@ dependencies = [
  "good_memory_allocator",
  "log",
  "multiboot",
- "multiboot2 0.18.1",
+ "multiboot2 0.19.0",
  "multiboot2-header",
  "util",
 ]
@@ -147,7 +147,7 @@ dependencies = [
  "anyhow",
  "good_memory_allocator",
  "log",
- "multiboot2 0.18.1",
+ "multiboot2 0.19.0",
  "util",
  "x86",
 ]

multiboot2/Cargo.toml

@@ -6,7 +6,7 @@ Multiboot2-compliant bootloaders, such as GRUB. It supports all tags from the
 specification including full support for the sections of ELF files. This library
 is `no_std` and can be used in a Multiboot2-kernel.
 """
-version = "0.18.1"
+version = "0.19.0"
 authors = [
     "Philipp Oppermann <dev@phil-opp.com>",
     "Calvin Lee <cyrus296@gmail.com>",

multiboot2/Changelog.md

@@ -1,6 +1,14 @@
 # CHANGELOG for crate `multiboot2`
 
-## 0.19.0 (2023-07-14)
+## 0.19.0 (2023-09-XX)
+- **BREAKING** `Tag::get_dst_str_slice` renamed to
+  `Tag::parse_slice_as_string` and now returns `Result<&str, StringError>`
+- **BREAKING** `BootLoaderNameTag::name` now returns `Result<&str, StringError>`
+- **BREAKING** `CommandLineTag::cmdline` now returns `Result<&str, StringError>`
+- **BREAKING** `ModuleTag::cmdline` now returns `Result<&str, StringError>`
+- Introduced new enum type `StringError`
+- Additionally, a bug was fixed in `parse_slice_as_string` which now parses
+  multiboot2 strings as expected: as null-terminated UTF-8 strings.
 - `InformationBuilder` now also allows to add custom tags. The new public method
   `add_tag` was introduced for that.
 

multiboot2/src/boot_loader_name.rs

@@ -1,9 +1,9 @@
 //! Module for [`BootLoaderNameTag`].
 
+use crate::tag::StringError;
 use crate::{Tag, TagTrait, TagType, TagTypeId};
 use core::fmt::{Debug, Formatter};
 use core::mem::size_of;
-use core::str::Utf8Error;
 #[cfg(feature = "builder")]
 use {crate::builder::BoxedDst, alloc::vec::Vec};
 
@@ -47,8 +47,8 @@ impl BootLoaderNameTag {
     ///     assert_eq!(Ok("GRUB 2.02~beta3-5"), tag.name());
     /// }
     /// ```
-    pub fn name(&self) -> Result<&str, Utf8Error> {
-        Tag::get_dst_str_slice(&self.name)
+    pub fn name(&self) -> Result<&str, StringError> {
+        Tag::parse_slice_as_string(&self.name)
     }
 }
 

multiboot2/src/builder/boxed_dst.rs

@@ -103,6 +103,7 @@ impl<T: ?Sized + PartialEq> PartialEq for BoxedDst<T> {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::tag::StringError;
     use crate::TagType;
 
     const METADATA_SIZE: usize = 8;
@@ -116,8 +117,8 @@ mod tests {
     }
 
     impl CustomTag {
-        fn string(&self) -> Result<&str, core::str::Utf8Error> {
-            Tag::get_dst_str_slice(&self.string)
+        fn string(&self) -> Result<&str, StringError> {
+            Tag::parse_slice_as_string(&self.string)
         }
     }
 
@@ -132,11 +133,12 @@ mod tests {
 
     #[test]
     fn test_boxed_dst_tag() {
-        let content = "hallo";
+        let content = b"hallo\0";
+        let content_rust_str = "hallo";
 
-        let tag = BoxedDst::<CustomTag>::new(content.as_bytes());
+        let tag = BoxedDst::<CustomTag>::new(content);
         assert_eq!(tag.typ, CustomTag::ID);
         assert_eq!(tag.size as usize, METADATA_SIZE + content.len());
-        assert_eq!(tag.string(), Ok(content));
+        assert_eq!(tag.string(), Ok(content_rust_str));
     }
 }

multiboot2/src/command_line.rs

@@ -2,10 +2,10 @@
 
 use crate::{Tag, TagTrait, TagType, TagTypeId};
 
+use crate::tag::StringError;
 use core::fmt::{Debug, Formatter};
 use core::mem;
 use core::str;
-
 #[cfg(feature = "builder")]
 use {crate::builder::BoxedDst, alloc::vec::Vec};
 
@@ -55,8 +55,8 @@ impl CommandLineTag {
     ///     assert_eq!(Ok("/bootarg"), command_line);
     /// }
     /// ```
-    pub fn cmdline(&self) -> Result<&str, str::Utf8Error> {
-        Tag::get_dst_str_slice(&self.cmdline)
+    pub fn cmdline(&self) -> Result<&str, StringError> {
+        Tag::parse_slice_as_string(&self.cmdline)
     }
 }
 

multiboot2/src/lib.rs

@@ -84,7 +84,7 @@ pub use module::{ModuleIter, ModuleTag};
 pub use ptr_meta::Pointee;
 pub use rsdp::{RsdpV1Tag, RsdpV2Tag};
 pub use smbios::SmbiosTag;
-pub use tag::Tag;
+pub use tag::{StringError, Tag};
 pub use tag_trait::TagTrait;
 pub use tag_type::{TagType, TagTypeId};
 pub use vbe_info::{
@@ -447,7 +447,7 @@ impl<'a> BootInformation<'a> {
     ///
     /// impl CustomTag {
     ///     fn name(&self) -> Result<&str, Utf8Error> {
-    ///         Tag::get_dst_str_slice(&self.name)
+    ///         Tag::parse_slice_as_string(&self.name)
     ///     }
     /// }
     /// let mbi_ptr = 0xdeadbeef as *const BootInformationHeader;
@@ -524,7 +524,7 @@ impl fmt::Debug for BootInformation<'_> {
 mod tests {
     use super::*;
     use crate::memory_map::MemoryAreaType;
-    use core::str::Utf8Error;
+    use crate::tag::StringError;
 
     /// Compile time test to check if the boot information is Send and Sync.
     /// This test is relevant to give library users flexebility in passing the
@@ -542,7 +542,6 @@ mod tests {
             8, 0, 0, 0, // end tag size
         ]);
         let ptr = bytes.0.as_ptr();
-        let addr = ptr as usize;
         let bi = unsafe { BootInformation::load(ptr.cast()) };
         let bi = bi.unwrap();
 
@@ -1604,8 +1603,8 @@ mod tests {
         }
 
         impl CustomTag {
-            fn name(&self) -> Result<&str, Utf8Error> {
-                Tag::get_dst_str_slice(&self.name)
+            fn name(&self) -> Result<&str, StringError> {
+                Tag::parse_slice_as_string(&self.name)
             }
         }
 

multiboot2/src/module.rs

@@ -1,10 +1,9 @@
 //! Module for [`ModuleTag`].
 
+use crate::tag::StringError;
 use crate::{Tag, TagIter, TagTrait, TagType, TagTypeId};
 use core::fmt::{Debug, Formatter};
 use core::mem::size_of;
-use core::str::Utf8Error;
-
 #[cfg(feature = "builder")]
 use {crate::builder::BoxedDst, alloc::vec::Vec};
 
@@ -48,8 +47,8 @@ impl ModuleTag {
     /// contains  `"module2 /some_boot_module --test cmdline-option"`.
     ///
     /// If the function returns `Err` then perhaps the memory is invalid.
-    pub fn cmdline(&self) -> Result<&str, Utf8Error> {
-        Tag::get_dst_str_slice(&self.cmdline)
+    pub fn cmdline(&self) -> Result<&str, StringError> {
+        Tag::parse_slice_as_string(&self.cmdline)
     }
 
     /// Start address of the module.

multiboot2/src/tag.rs

@@ -4,10 +4,35 @@
 
 use crate::{TagTrait, TagType, TagTypeId};
 use core::fmt;
-use core::fmt::{Debug, Formatter};
+use core::fmt::{Debug, Display, Formatter};
 use core::marker::PhantomData;
 use core::str::Utf8Error;
 
+/// Error type describing failures when parsing the string from a tag.
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum StringError {
+    /// There is no terminating null-byte, although the spec requires one.
+    MissingNull(core::ffi::FromBytesUntilNulError),
+    /// The sequence until the first NULL byte is not valid UTF-8.
+    Utf8(Utf8Error),
+}
+
+impl Display for StringError {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        write!(f, "{:?}", self)
+    }
+}
+
+#[cfg(feature = "unstable")]
+impl core::error::Error for StringError {
+    fn source(&self) -> Option<&(dyn core::error::Error + 'static)> {
+        match self {
+            StringError::MissingNull(e) => Some(e),
+            StringError::Utf8(e) => Some(e),
+        }
+    }
+}
+
 /// Common base structure for all tags that can be passed via the Multiboot2
 /// Information Structure (MBI) to a Multiboot2 payload/program/kernel.
 ///
@@ -38,22 +63,13 @@ impl Tag {
         unsafe { TagTrait::from_base_tag(self) }
     }
 
-    /// Some multiboot2 tags are a DST as they end with a dynamically sized byte
-    /// slice. This function parses this slice as [`str`] so that either a valid
-    /// UTF-8 Rust string slice without a terminating null byte or an error is
-    /// returned.
-    pub fn get_dst_str_slice(bytes: &[u8]) -> Result<&str, Utf8Error> {
-        // Determine length of string before first NUL character
-
-        let length = match bytes.iter().position(|ch| (*ch) == 0x00) {
-            // Unterminated string case:
-            None => bytes.len(),
-
-            // Terminated case:
-            Some(idx) => idx,
-        };
-        // Convert to Rust string:
-        core::str::from_utf8(&bytes[..length])
+    /// Parses the provided byte sequence as Multiboot string, which maps to a
+    /// [`str`].
+    pub fn parse_slice_as_string(bytes: &[u8]) -> Result<&str, StringError> {
+        let cstr =
+            core::ffi::CStr::from_bytes_until_nul(bytes).map_err(StringError::MissingNull)?;
+
+        cstr.to_str().map_err(StringError::Utf8)
     }
 }
 
@@ -128,21 +144,28 @@ mod tests {
     use super::*;
 
     #[test]
-    fn test_get_dst_str_slice() {
-        // unlikely case
-        assert_eq!(Ok(""), Tag::get_dst_str_slice(&[]));
-        // also unlikely case
-        assert_eq!(Ok(""), Tag::get_dst_str_slice(&[b'\0']));
-        // unlikely case: missing null byte. but the lib can cope with that
-        assert_eq!(Ok("foobar"), Tag::get_dst_str_slice(b"foobar"));
-        // test that the null bytes is not included in the string slice
-        assert_eq!(Ok("foobar"), Tag::get_dst_str_slice(b"foobar\0"));
-        // test that C-style null string termination works as expected
-        assert_eq!(Ok("foo"), Tag::get_dst_str_slice(b"foo\0bar"));
-        // test invalid utf8
+    fn parse_slice_as_string() {
+        // empty slice is invalid
+        assert!(matches!(
+            Tag::parse_slice_as_string(&[]),
+            Err(StringError::MissingNull(_))
+        ));
+        // empty string is fine
+        assert_eq!(Tag::parse_slice_as_string(&[0x00]), Ok(""));
+        // reject invalid utf8
+        assert!(matches!(
+            Tag::parse_slice_as_string(&[0xff, 0x00]),
+            Err(StringError::Utf8(_))
+        ));
+        // reject missing null
         assert!(matches!(
-            Tag::get_dst_str_slice(&[0xff, 0xff]),
-            Err(Utf8Error { .. })
+            Tag::parse_slice_as_string(b"hello"),
+            Err(StringError::MissingNull(_))
         ));
+        // must not include final null
+        assert_eq!(Tag::parse_slice_as_string(b"hello\0"), Ok("hello"));
+        assert_eq!(Tag::parse_slice_as_string(b"hello\0\0"), Ok("hello"));
+        // must skip everytihng after first null
+        assert_eq!(Tag::parse_slice_as_string(b"hello\0foo"), Ok("hello"));
     }
 }