From 61f17d6c2bd115c75cf572266789a9c6b01ebc08 Mon Sep 17 00:00:00 2001 From: Alex Saveau Date: Wed, 20 Nov 2024 21:35:52 -0500 Subject: [PATCH] Use MaybeUninit in DecInt (#1201) * Use MaybeUninit in DecInt * Fix DecInt::new panicking on integers greater than 64 bits and optimize out any panics in DecInt::new * Add tests so people messing with DecInt can catch UB with miri * Use new itoa --------- Co-authored-by: Alex Saveau --- Cargo.toml | 2 +- src/path/dec_int.rs | 66 +++++++++++++++++++++++-------------------- tests/path/dec_int.rs | 55 ++++++++++++++++++++++++++---------- 3 files changed, 76 insertions(+), 47 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index fbda86c5e..7a8a4560a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,7 +17,7 @@ rust-version = "1.63" [dependencies] bitflags = { version = "2.4.0", default-features = false } -itoa = { version = "1.0.1", default-features = false, optional = true } +itoa = { version = "1.0.13", default-features = false, optional = true } # Special dependencies used in rustc-dep-of-std mode. core = { version = "1.0.0", optional = true, package = "rustc-std-workspace-core" } diff --git a/src/path/dec_int.rs b/src/path/dec_int.rs index 73907c5b2..63b12229b 100644 --- a/src/path/dec_int.rs +++ b/src/path/dec_int.rs @@ -8,7 +8,7 @@ use crate::backend::fd::{AsFd, AsRawFd}; use crate::ffi::CStr; -use core::fmt::Write; +use core::mem::{self, MaybeUninit}; use itoa::{Buffer, Integer}; #[cfg(all(feature = "std", unix))] use std::os::unix::ffi::OsStrExt; @@ -36,23 +36,36 @@ use {core::fmt, std::ffi::OsStr, std::path::Path}; /// ``` #[derive(Clone)] pub struct DecInt { - // 20 `u8`s is enough to hold the decimal ASCII representation of any - // `u64`, and we add one for a NUL terminator for `as_c_str`. - buf: [u8; 20 + 1], + // Enough to hold an {u,i}64 and NUL terminator. + buf: [MaybeUninit; u64::MAX_STR_LEN + 1], len: usize, } +const _: () = assert!(u64::MAX_STR_LEN == i64::MAX_STR_LEN); impl DecInt { /// Construct a new path component from an integer. #[inline] - pub fn new(i: Int) -> Self { - let mut me = DecIntWriter(Self { - buf: [0; 20 + 1], - len: 0, + pub fn new(i: Int) -> Self { + let mut buf = [MaybeUninit::uninit(); 21]; + + let mut str_buf = Buffer::new(); + let str_buf = str_buf.format(i); + assert!( + str_buf.len() < buf.len(), + "{str_buf}{} unsupported.", + core::any::type_name::() + ); + + buf[..str_buf.len()].copy_from_slice(unsafe { + // SAFETY: you can always go from init to uninit + mem::transmute::<&[u8], &[MaybeUninit]>(str_buf.as_bytes()) }); - let mut buf = Buffer::new(); - me.write_str(buf.format(i)).unwrap(); - me.0 + buf[str_buf.len()] = MaybeUninit::new(0); + + Self { + buf, + len: str_buf.len(), + } } /// Construct a new path component from a file descriptor. @@ -72,7 +85,7 @@ impl DecInt { /// Return the raw byte buffer as a `&CStr`. #[inline] pub fn as_c_str(&self) -> &CStr { - let bytes_with_nul = &self.buf[..=self.len]; + let bytes_with_nul = self.as_bytes_with_nul(); debug_assert!(CStr::from_bytes_with_nul(bytes_with_nul).is_ok()); // SAFETY: `self.buf` holds a single decimal ASCII representation and @@ -80,28 +93,19 @@ impl DecInt { unsafe { CStr::from_bytes_with_nul_unchecked(bytes_with_nul) } } - /// Return the raw byte buffer. + /// Return the raw byte buffer including the NUL byte. #[inline] - pub fn as_bytes(&self) -> &[u8] { - &self.buf[..self.len] + pub fn as_bytes_with_nul(&self) -> &[u8] { + let init = &self.buf[..=self.len]; + // SAFETY: we're guaranteed to have initialized len+1 bytes. + unsafe { mem::transmute::<&[MaybeUninit], &[u8]>(init) } } -} - -/// A wrapper around `DecInt` that implements `Write` without exposing this -/// implementation to `DecInt`'s public API. -struct DecIntWriter(DecInt); -impl core::fmt::Write for DecIntWriter { + /// Return the raw byte buffer. #[inline] - fn write_str(&mut self, s: &str) -> core::fmt::Result { - match self.0.buf.get_mut(self.0.len..self.0.len + s.len()) { - Some(slice) => { - slice.copy_from_slice(s.as_bytes()); - self.0.len += s.len(); - Ok(()) - } - None => Err(core::fmt::Error), - } + pub fn as_bytes(&self) -> &[u8] { + let bytes = self.as_bytes_with_nul(); + &bytes[..bytes.len() - 1] } } @@ -109,7 +113,7 @@ impl core::fmt::Write for DecIntWriter { impl AsRef for DecInt { #[inline] fn as_ref(&self) -> &Path { - let as_os_str: &OsStr = OsStrExt::from_bytes(&self.buf[..self.len]); + let as_os_str: &OsStr = OsStrExt::from_bytes(self.as_bytes()); Path::new(as_os_str) } } diff --git a/tests/path/dec_int.rs b/tests/path/dec_int.rs index 6ce1a5123..80f95126f 100644 --- a/tests/path/dec_int.rs +++ b/tests/path/dec_int.rs @@ -1,20 +1,45 @@ use rustix::path::DecInt; +macro_rules! check { + ($i:expr) => { + let i = $i; + assert_eq!(DecInt::new(i).as_ref().to_str().unwrap(), i.to_string()); + }; +} + #[test] fn test_dec_int() { - assert_eq!(DecInt::new(0).as_ref().to_str().unwrap(), "0"); - assert_eq!(DecInt::new(-1).as_ref().to_str().unwrap(), "-1"); - assert_eq!(DecInt::new(789).as_ref().to_str().unwrap(), "789"); - assert_eq!( - DecInt::new(i64::MIN).as_ref().to_str().unwrap(), - i64::MIN.to_string() - ); - assert_eq!( - DecInt::new(i64::MAX).as_ref().to_str().unwrap(), - i64::MAX.to_string() - ); - assert_eq!( - DecInt::new(u64::MAX).as_ref().to_str().unwrap(), - u64::MAX.to_string() - ); + check!(0); + check!(-1); + check!(789); + + check!(u8::MAX); + check!(i8::MIN); + check!(u16::MAX); + check!(i16::MIN); + check!(u32::MAX); + check!(i32::MIN); + check!(u64::MAX); + check!(i64::MIN); + #[cfg(any( + target_pointer_width = "16", + target_pointer_width = "32", + target_pointer_width = "64" + ))] + { + check!(usize::MAX); + check!(isize::MIN); + } +} + +#[test] +#[should_panic] +fn test_unsupported_max_u128_dec_int() { + check!(u128::MAX); +} + +#[test] +#[should_panic] +fn test_unsupported_min_u128_dec_int() { + check!(i128::MIN); }