From 9a97d10a72fbc57efcbc6c9e77ca92a565be9d59 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Mon, 3 Jun 2024 11:24:01 +0200 Subject: [PATCH] Make panicking Path/PathBuf conversions fallible To avoid unintended panics and to make it clear which conversions can fail and which cannot, this patch changes all panicking Path/PathBuf conversions to return a Result instead. Fixes: https://github.com/trussed-dev/littlefs2/issues/63 --- CHANGELOG.md | 3 ++ src/fs.rs | 7 ++-- src/lib.rs | 9 +++-- src/path.rs | 101 ++++++++++++++++++++++++++------------------------- src/tests.rs | 8 ++-- 5 files changed, 69 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e9735808f..8e8a6522c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Enforced const evaluation for `path!`. - Removed `cstr_core` and `cty` dependencies. - Updated `littlefs2-sys` dependency to 0.2.0. +- Replace all panicking `Path`/`PathBuf` conversions with fallible alternatives: + - Return a `Result` from `Path::from_str_with_nul`. + - Replace the `From<_>` implementations for `Path` and `PathBuf` with `TryFrom<_>`, except for `From<&Path> for PathBuf`. ### Removed diff --git a/src/fs.rs b/src/fs.rs index 8fa16d8d1..2f8be1706 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -1254,7 +1254,7 @@ impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> { let path_slice = path.as_ref().as_bytes(); for i in 0..path_slice.len() { if path_slice[i] == b'/' { - let dir = PathBuf::from(&path_slice[..i]); + let dir = PathBuf::try_from(&path_slice[..i])?; #[cfg(test)] println!("generated PathBuf dir {:?} using i = {}", &dir, i); match self.create_dir(&dir) { @@ -1362,6 +1362,7 @@ impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> { #[cfg(test)] mod tests { use super::*; + use crate::path; use core::convert::TryInto; use driver::Storage as LfsStorage; use io::Result as LfsResult; @@ -1483,7 +1484,7 @@ mod tests { // (...) // fs.remove_dir_all(&PathBuf::from(b"/tmp\0"))?; // fs.remove_dir_all(&PathBuf::from(b"/tmp"))?; - fs.remove_dir_all(&PathBuf::from("/tmp"))?; + fs.remove_dir_all(path!("/tmp"))?; } Ok(()) @@ -1493,7 +1494,7 @@ mod tests { let mut alloc = Allocation::new(); let fs = Filesystem::mount(&mut alloc, &mut test_storage).unwrap(); // fs.write(b"/z.txt\0".try_into().unwrap(), &jackson5).unwrap(); - fs.write(&PathBuf::from("z.txt"), jackson5).unwrap(); + fs.write(path!("z.txt"), jackson5).unwrap(); } #[cfg(feature = "dir-entry-path")] diff --git a/src/lib.rs b/src/lib.rs index 8186606d6..4b336f483 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -93,7 +93,7 @@ Separately, keeping track of the allocations is a chore, we hope that ``` # use littlefs2::fs::{Filesystem, File, OpenOptions}; # use littlefs2::io::prelude::*; -# use littlefs2::path::PathBuf; +# use littlefs2::path; # # use littlefs2::{consts, ram_storage, driver, io::Result}; # @@ -113,7 +113,7 @@ let mut fs = Filesystem::mount(&mut alloc, &mut storage).unwrap(); let mut buf = [0u8; 11]; fs.open_file_with_options_and_then( |options| options.read(true).write(true).create(true), - &PathBuf::from(b"example.txt"), + path!("example.txt"), |file| { file.write(b"Why is black smoke coming out?!")?; file.seek(SeekFrom::End(-24)).unwrap(); @@ -199,7 +199,10 @@ pub struct Version { macro_rules! path { ($path:literal) => {{ const _PATH: &$crate::path::Path = - $crate::path::Path::from_str_with_nul(::core::concat!($path, "\0")); + match $crate::path::Path::from_str_with_nul(::core::concat!($path, "\0")) { + Ok(path) => path, + Err(_) => panic!("invalid littlefs2 path"), + }; _PATH }}; } diff --git a/src/path.rs b/src/path.rs index 4ff29f4ce..396070dcb 100644 --- a/src/path.rs +++ b/src/path.rs @@ -9,7 +9,7 @@ use core::{ ops, ptr, slice, str, }; -use crate::consts; +use crate::{consts, path}; /// A path /// @@ -94,14 +94,14 @@ impl<'a> Iterator for Ancestors<'a> { return None; } else if self.path == "/" { self.path = ""; - return Some("/".into()); + return Some(path!("/").into()); } let item = self.path; let Some((rem, item_name)) = self.path.rsplit_once('/') else { self.path = ""; - return Some(item.into()); + return item.try_into().ok(); }; if self.path.starts_with('/') && rem.is_empty() { @@ -114,7 +114,7 @@ impl<'a> Iterator for Ancestors<'a> { if item_name.is_empty() { self.next(); } - Some(item.into()) + item.try_into().ok() } } @@ -135,17 +135,17 @@ impl<'a> Iterator for Iter<'a> { } if self.path.starts_with('/') { self.path = &self.path[1..]; - return Some("/".into()); + return Some(path!("/").into()); } let Some((path, rem)) = self.path.split_once('/') else { - let ret_val = Some(self.path.into()); + let ret_val = self.path.try_into().ok(); self.path = ""; return ret_val; }; self.path = rem; - Some(path.into()) + path.try_into().ok() } } @@ -236,21 +236,9 @@ impl Path { /// /// The string must only consist of ASCII characters. The last character must be null. It /// must contain at most [`PATH_MAX`](`consts::PATH_MAX`) bytes, not including the trailing - /// null. If these conditions are not met, this function panics. - pub const fn from_str_with_nul(s: &str) -> &Self { - let bytes = s.as_bytes(); - let mut i = 0; - while i < bytes.len().saturating_sub(1) { - assert!(bytes[i] != 0, "must not contain null"); - assert!(bytes[i].is_ascii(), "must be ASCII"); - i += 1; - } - assert!(!bytes.is_empty(), "must not be empty"); - assert!(bytes[i] == 0, "last byte must be null"); - unsafe { - let cstr = CStr::from_bytes_with_nul_unchecked(bytes); - Self::from_cstr_unchecked(cstr) - } + /// null. If these conditions are not met, this function returns an error. + pub const fn from_str_with_nul(s: &str) -> Result<&Self> { + Self::from_bytes_with_nul(s.as_bytes()) } /// Creates a path from a byte buffer. @@ -322,14 +310,16 @@ impl Path { pub fn parent(&self) -> Option { let rk_path_bytes = self.as_ref()[..].as_bytes(); match rk_path_bytes.iter().rposition(|x| *x == b'/') { - Some(0) if rk_path_bytes.len() != 1 => Some(PathBuf::from("/")), + Some(0) if rk_path_bytes.len() != 1 => Some(path!("/").into()), Some(slash_index) => { // if we have a directory that ends with `/`, // still need to "go up" one parent if slash_index + 1 == rk_path_bytes.len() { - PathBuf::from(&rk_path_bytes[..slash_index]).parent() + PathBuf::try_from(&rk_path_bytes[..slash_index]) + .ok()? + .parent() } else { - Some(PathBuf::from(&rk_path_bytes[..slash_index])) + PathBuf::try_from(&rk_path_bytes[..slash_index]).ok() } } None => None, @@ -382,9 +372,11 @@ macro_rules! array_impls { } } - impl From<&[u8; $N]> for PathBuf { - fn from(bytes: &[u8; $N]) -> Self { - Self::from(&bytes[..]) + impl TryFrom<&[u8; $N]> for PathBuf { + type Error = Error; + + fn try_from(bytes: &[u8; $N]) -> Result { + Self::try_from(&bytes[..]) } } @@ -521,11 +513,11 @@ impl From<&Path> for PathBuf { } } -impl From<&[u8]> for PathBuf { - /// Accepts byte string, with or without trailing nul. - /// - /// PANICS: when there are embedded nuls - fn from(bytes: &[u8]) -> Self { +/// Accepts byte strings, with or without trailing nul. +impl TryFrom<&[u8]> for PathBuf { + type Error = Error; + + fn try_from(bytes: &[u8]) -> Result { // NB: This needs to set the final NUL byte, unless it already has one // It also checks that there are no inner NUL bytes let bytes = if !bytes.is_empty() && bytes[bytes.len() - 1] == b'\0' { @@ -533,21 +525,31 @@ impl From<&[u8]> for PathBuf { } else { bytes }; - let has_no_embedded_nul = !bytes.iter().any(|&byte| byte == b'\0'); - assert!(has_no_embedded_nul); + if bytes.len() > consts::PATH_MAX { + return Err(Error::TooLarge); + } + for byte in bytes { + if *byte == 0 { + return Err(Error::NotCStr); + } + if !byte.is_ascii() { + return Err(Error::NotAscii); + } + } let mut buf = [0; consts::PATH_MAX_PLUS_ONE]; let len = bytes.len(); - assert!(len <= consts::PATH_MAX); - assert!(bytes.is_ascii()); unsafe { ptr::copy_nonoverlapping(bytes.as_ptr(), buf.as_mut_ptr().cast(), len) } - Self { buf, len: len + 1 } + Ok(Self { buf, len: len + 1 }) } } -impl From<&str> for PathBuf { - fn from(s: &str) -> Self { - PathBuf::from(s.as_bytes()) +/// Accepts strings, with or without trailing nul. +impl TryFrom<&str> for PathBuf { + type Error = Error; + + fn try_from(s: &str) -> Result { + PathBuf::try_from(s.as_bytes()) } } @@ -597,7 +599,7 @@ impl<'de> serde::Deserialize<'de> for PathBuf { if v.len() > consts::PATH_MAX { return Err(E::invalid_length(v.len(), &self)); } - Ok(PathBuf::from(v)) + PathBuf::try_from(v).map_err(|_| E::custom("invalid path buffer")) } } @@ -670,8 +672,8 @@ mod tests { #[test] fn path_macro() { - assert_eq!(EMPTY, &*PathBuf::from("")); - assert_eq!(SLASH, &*PathBuf::from("/")); + assert_eq!(EMPTY, &*PathBuf::try_from("").unwrap()); + assert_eq!(SLASH, &*PathBuf::try_from("/").unwrap()); } // does not compile: @@ -679,15 +681,13 @@ mod tests { // const NULL: &Path = path!("ub\0er"); #[test] - #[should_panic] fn nul_in_from_str_with_nul() { - Path::from_str_with_nul("ub\0er"); + assert!(Path::from_str_with_nul("ub\0er").is_err()); } #[test] - #[should_panic] fn non_ascii_in_from_str_with_nul() { - Path::from_str_with_nul("über"); + assert!(Path::from_str_with_nul("über").is_err()); } #[test] @@ -725,7 +725,10 @@ mod tests { #[test] fn trailing_nuls() { - assert_eq!(PathBuf::from("abc"), PathBuf::from("abc\0")); + assert_eq!( + PathBuf::try_from("abc").unwrap(), + PathBuf::try_from("abc\0").unwrap() + ); } #[test] diff --git a/src/tests.rs b/src/tests.rs index 7e7498d23..1e32e5fcf 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -195,20 +195,20 @@ fn test_create() { assert_eq!(fs.available_blocks().unwrap(), 512 - 2); assert_eq!(fs.available_space().unwrap(), 130_560); - assert!(!crate::path::PathBuf::from(b"/test_open.txt").exists(fs)); + assert!(!path!("/test_open.txt").exists(fs)); assert_eq!( File::open_and_then(fs, b"/test_open.txt\0".try_into().unwrap(), |_| { Ok(()) }) .map(drop) .unwrap_err(), // "real" contains_err is experimental Error::NoSuchEntry ); - assert!(!crate::path::PathBuf::from(b"/test_open.txt").exists(fs)); + assert!(!path!("/test_open.txt").exists(fs)); fs.create_dir(b"/tmp\0".try_into().unwrap()).unwrap(); assert_eq!(fs.available_blocks().unwrap(), 512 - 2 - 2); // can create new files - assert!(!crate::path::PathBuf::from(b"/tmp/test_open.txt").exists(fs)); + assert!(!path!("/tmp/test_open.txt").exists(fs)); fs.create_file_and_then(b"/tmp/test_open.txt\0".try_into().unwrap(), |file| { // can write to files assert!(file.write(&[0u8, 1, 2]).unwrap() == 3); @@ -219,7 +219,7 @@ fn test_create() { // file.close()?; Ok(()) })?; - assert!(crate::path::PathBuf::from(b"/tmp/test_open.txt").exists(fs)); + assert!(path!("/tmp/test_open.txt").exists(fs)); // // cannot remove non-empty directories assert_eq!(