Merge pull request #177 from rust-osdev/cstring-follow-up

phip1611 · web-flow · commit d055b41cf1e7 · 2023-09-21T17:13:53.000+02:00
string parsing followup: introduce better fitting error type + raise MSRV
diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml
@@ -20,7 +20,7 @@ jobs:
     name: build (msrv)
     uses: ./.github/workflows/_build-rust.yml
     with:
-      rust-version: 1.68.0 # MSRV
+      rust-version: 1.69.0 # MSRV
       do-style-check: false
       features: builder
 
@@ -46,7 +46,7 @@ jobs:
     needs: build_msrv
     uses: ./.github/workflows/_build-rust.yml
     with:
-      rust-version: 1.68.0 # MSRV
+      rust-version: 1.69.0 # MSRV
       do-style-check: false
       rust-target: thumbv7em-none-eabihf
       features: builder
@@ -103,7 +103,7 @@ jobs:
     needs: build_msrv
     uses: ./.github/workflows/_build-rust.yml
     with:
-      rust-version: 1.68.0 # MSRV
+      rust-version: 1.69.0 # MSRV
       do-style-check: true
       do-test: false
       features: builder
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/integration-test/bins/Cargo.lock b/integration-test/bins/Cargo.lock
diff --git a/multiboot2/Cargo.toml b/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>",
diff --git a/multiboot2/Changelog.md b/multiboot2/Changelog.md
@@ -1,6 +1,15 @@
 # CHANGELOG for crate `multiboot2`
 
-## 0.19.0 (2023-07-14)
+## 0.19.0 (2023-09-XX)
+- **BREAKING** MSRV is 1.69.0
+- **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.
 
diff --git a/multiboot2/README.md b/multiboot2/README.md
@@ -38,7 +38,7 @@ other fields  | variable
 All tags and the mbi itself are 8-byte aligned. The last tag must be the _end tag_, which is a tag of type `0` and size `8`.
 
 ## MSRV
-The MSRV is 1.68.0 stable.
+The MSRV is 1.69.0 stable.
 
 ## License & Contribution
 
diff --git a/multiboot2/src/boot_loader_name.rs b/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)
     }
 }
 
diff --git a/multiboot2/src/builder/boxed_dst.rs b/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));
     }
 }
diff --git a/multiboot2/src/command_line.rs b/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)
     }
 }
 
diff --git a/multiboot2/src/lib.rs b/multiboot2/src/lib.rs
@@ -33,7 +33,7 @@
 //! ```
 //!
 //! ## MSRV
-//! The MSRV is 1.68.0 stable.
+//! The MSRV is 1.69.0 stable.
 
 #[cfg(feature = "builder")]
 extern crate alloc;
@@ -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)
             }
         }
 
diff --git a/multiboot2/src/module.rs b/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.
diff --git a/multiboot2/src/tag.rs b/multiboot2/src/tag.rs
@@ -4,10 +4,36 @@
 
 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 NUL character, although the specification
+    /// requires one.
+    MissingNul(core::ffi::FromBytesUntilNulError),
+    /// The sequence until the first NUL character 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::MissingNul(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 +64,12 @@ 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::MissingNul)?;
+
+        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::MissingNul(_))
+        ));
+        // 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::MissingNul(_))
         ));
+        // 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"));
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -103,6 +103,7 @@ impl<T: ?Sized + PartialEq> PartialEq for BoxedDst<T> {`
`103`	`103`	`#[cfg(test)]`
`104`	`104`	`mod tests {`
`105`	`105`	`use super::*;`
	`106`	`+ use crate::tag::StringError;`
`106`	`107`	`use crate::TagType;`
`107`	`108`
`108`	`109`	`const METADATA_SIZE: usize = 8;`
`@@ -116,8 +117,8 @@ mod tests {`
`116`	`117`	`}`
`117`	`118`
`118`	`119`	`impl CustomTag {`
`119`		`- fn string(&self) -> Result<&str, core::str::Utf8Error> {`
`120`		`- Tag::get_dst_str_slice(&self.string)`
	`120`	`+ fn string(&self) -> Result<&str, StringError> {`
	`121`	`+ Tag::parse_slice_as_string(&self.string)`
`121`	`122`	`}`
`122`	`123`	`}`
`123`	`124`
`@@ -132,11 +133,12 @@ mod tests {`
`132`	`133`
`133`	`134`	`#[test]`
`134`	`135`	`fn test_boxed_dst_tag() {`
`135`		`- let content = "hallo";`
	`136`	`+ let content = b"hallo\0";`
	`137`	`+ let content_rust_str = "hallo";`
`136`	`138`
`137`		`- let tag = BoxedDst::<CustomTag>::new(content.as_bytes());`
	`139`	`+ let tag = BoxedDst::<CustomTag>::new(content);`
`138`	`140`	`assert_eq!(tag.typ, CustomTag::ID);`
`139`	`141`	`assert_eq!(tag.size as usize, METADATA_SIZE + content.len());`
`140`		`- assert_eq!(tag.string(), Ok(content));`
	`142`	`+ assert_eq!(tag.string(), Ok(content_rust_str));`
`141`	`143`	`}`
`142`	`144`	`}`