From bd81c69c2fbafbf91194235445fe9270bb068800 Mon Sep 17 00:00:00 2001 From: Joshua Liebow-Feeser Date: Sat, 18 May 2024 09:53:18 -0700 Subject: [PATCH] [error] Implement std::error::Error on errors While we're here, also relax `Dispaly for AlignmentError` to permit `Dst: ?Sized` in exchange for `Dst: KnownLayout`. This is an important relaxation since our APIs permit performing conversions into unsized destination types with runtime alignment checking. Also make all errors `Send + Sync` regardless of `Dst`, which only exists at the type level, but is never instantiated. Finally, make `Display`'s verbosity conditional on `debug_assertions`. Makes progress on #1297 Co-authored-by: John Wrenn --- Cargo.toml | 3 +- README.md | 5 + src/error.rs | 328 ++++++++++++++++++++++++++++++++++++++++++-------- src/lib.rs | 7 +- src/macros.rs | 1 + src/util.rs | 26 ++++ 6 files changed, 320 insertions(+), 50 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index aa71206d62e..5921969225c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,10 +60,11 @@ alloc = [] derive = ["zerocopy-derive"] simd = [] simd-nightly = ["simd"] +std = ["alloc"] # This feature depends on all other features that work on the stable compiler. # We make no stability guarantees about this feature; it may be modified or # removed at any time. -__internal_use_only_features_that_work_on_stable = ["alloc", "derive", "simd"] +__internal_use_only_features_that_work_on_stable = ["alloc", "derive", "simd", "std"] [dependencies] zerocopy-derive = { version = "=0.8.0-alpha.14", path = "zerocopy-derive", optional = true } diff --git a/README.md b/README.md index bf9974882ad..0664c998cda 100644 --- a/README.md +++ b/README.md @@ -82,6 +82,11 @@ for network parsing. the `alloc` crate is added as a dependency, and some allocation-related functionality is added. +- **`std`** + By default, `zerocopy` is `no_std`. When the `std` feature is enabled, the + `std` crate is added as a dependency (ie, `no_std` is disabled), and + support for some `std` types is added. `std` implies `alloc`. + - **`derive`** Provides derives for the core marker traits via the `zerocopy-derive` crate. These derives are re-exported from `zerocopy`, so it is not diff --git a/src/error.rs b/src/error.rs index e32f9eb92cf..bf3b88da802 100644 --- a/src/error.rs +++ b/src/error.rs @@ -32,15 +32,34 @@ //! All error types provide an `into_src` method that converts the error into //! the source value underlying the failed conversion. //! +//! ## Display formatting +//! +//! All error types provide a `Display` implementation that produces a +//! human-readable error message. When `debug_assertions` are enabled, these +//! error messages are verbose and may include potentially sensitive +//! information, including: +//! +//! - the names of the involved types +//! - the sizes of the involved types +//! - the addresses of the involved types +//! - the contents of the involved types +//! +//! When `debug_assertions` are disabled (as is default for `release` builds), +//! such potentially sensitive information is excluded. +//! //! ## Validation order //! //! Our conversion methods typically check alignment, then size, then bit //! validity. However, we do not guarantee that this is always the case, and //! this behavior may change between releases. -use core::{convert::Infallible, fmt, marker::PhantomData, ops::Deref}; +use core::{ + convert::Infallible, + fmt::{self, Debug, Write}, + ops::Deref, +}; -use crate::TryFromBytes; +use crate::{util::SendSyncPhantomData, KnownLayout, TryFromBytes}; #[cfg(doc)] use crate::{FromBytes, Ref}; @@ -86,6 +105,10 @@ impl fmt::Debug for ConvertError fmt::Display for ConvertError { #[inline] fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -97,18 +120,28 @@ impl fmt::Display for Convert } } +#[cfg(any(feature = "std", test))] +#[allow(clippy::std_instead_of_core)] +impl std::error::Error for ConvertError +where + A: fmt::Display + fmt::Debug, + S: fmt::Display + fmt::Debug, + V: fmt::Display + fmt::Debug, +{ +} + /// The error emitted if the conversion source is improperly aligned. #[derive(PartialEq, Eq)] pub struct AlignmentError { /// The source value involved in the conversion. src: Src, /// The inner destination type inolved in the conversion. - dst: PhantomData, + dst: SendSyncPhantomData, } impl AlignmentError { pub(crate) fn new(src: Src) -> Self { - Self { src, dst: PhantomData } + Self { src, dst: SendSyncPhantomData::default() } } /// Produces the source underlying the failed conversion. @@ -118,16 +151,46 @@ impl AlignmentError { } pub(crate) fn with_src(self, new_src: NewSrc) -> AlignmentError { - AlignmentError { src: new_src, dst: PhantomData } + AlignmentError { src: new_src, dst: SendSyncPhantomData::default() } } pub(crate) fn map_src(self, f: impl Fn(Src) -> NewSrc) -> AlignmentError { - AlignmentError { src: f(self.src), dst: PhantomData } + AlignmentError { src: f(self.src), dst: SendSyncPhantomData::default() } } pub(crate) fn into(self) -> ConvertError { ConvertError::Alignment(self) } + + /// Format extra details for a verbose, human-readable error message. + /// + /// This formatting may include potentially sensitive information. + fn display_verbose_extras(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result + where + Src: Deref, + Dst: KnownLayout, + { + #[allow(clippy::as_conversions)] + let addr = self.src.deref() as *const _ as *const (); + let addr_align = 2usize.pow((crate::util::AsAddress::addr(addr)).trailing_zeros()); + + f.write_str("\n\nSource type: ")?; + f.write_str(core::any::type_name::())?; + + f.write_str("\nSource address: ")?; + addr.fmt(f)?; + f.write_str(" (a multiple of ")?; + addr_align.fmt(f)?; + f.write_str(")")?; + + f.write_str("\nDestination type: ")?; + f.write_str(core::any::type_name::())?; + + f.write_str("\nDestination alignment: ")?; + ::LAYOUT.align.get().fmt(f)?; + + Ok(()) + } } impl fmt::Debug for AlignmentError { @@ -138,30 +201,37 @@ impl fmt::Debug for AlignmentError { } /// Produces a human-readable error message. -// The bounds on this impl are intentionally conservative, and can be relaxed -// either once a `?Sized` alignment accessor is stabilized, or by storing the -// alignment as a runtime value. -impl fmt::Display for AlignmentError +/// +/// The message differs between debug and release builds. When +/// `debug_assertions` are enabled, this message is verbose and includes +/// potentially sensitive information. +impl fmt::Display for AlignmentError where Src: Deref, + Dst: KnownLayout, { #[inline] fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - #[cfg_attr(__INTERNAL_USE_ONLY_NIGHTLY_FEATURES_IN_TESTS, allow(lossy_provenance_casts))] - #[allow(clippy::as_conversions)] - let addr = self.src.deref() as *const _ as *const () as usize; - let addr_align = 2usize.pow(addr.trailing_zeros()); - f.write_str("the conversion failed because the address of the source (a multiple of ")?; - addr_align.fmt(f)?; - f.write_str(") is not a multiple of the alignment (")?; - core::mem::align_of::().fmt(f)?; - f.write_str(") of the destination type: ")?; - f.write_str(core::any::type_name::())?; - Ok(()) + f.write_str("The conversion failed because the address of the source is not a multiple of the alignment of the destination type.")?; + + if cfg!(debug_assertions) { + self.display_verbose_extras(f) + } else { + Ok(()) + } } } -impl From> +#[cfg(any(feature = "std", test))] +#[allow(clippy::std_instead_of_core)] +impl std::error::Error for AlignmentError +where + Src: Deref, + Dst: KnownLayout, +{ +} + +impl From> for ConvertError, S, V> { #[inline] @@ -176,12 +246,12 @@ pub struct SizeError { /// The source value involved in the conversion. src: Src, /// The inner destination type inolved in the conversion. - dst: PhantomData, + dst: SendSyncPhantomData, } impl SizeError { pub(crate) fn new(src: Src) -> Self { - Self { src, dst: PhantomData } + Self { src, dst: SendSyncPhantomData::default() } } /// Produces the source underlying the failed conversion. @@ -192,23 +262,61 @@ impl SizeError { /// Sets the source value associated with the conversion error. pub(crate) fn with_src(self, new_src: NewSrc) -> SizeError { - SizeError { src: new_src, dst: PhantomData } + SizeError { src: new_src, dst: SendSyncPhantomData::default() } } /// Maps the source value associated with the conversion error. pub(crate) fn map_src(self, f: impl Fn(Src) -> NewSrc) -> SizeError { - SizeError { src: f(self.src), dst: PhantomData } + SizeError { src: f(self.src), dst: SendSyncPhantomData::default() } } /// Sets the destination type associated with the conversion error. pub(crate) fn with_dst(self) -> SizeError { - SizeError { src: self.src, dst: PhantomData } + SizeError { src: self.src, dst: SendSyncPhantomData::default() } } /// Converts the error into a general [`ConvertError`]. pub(crate) fn into(self) -> ConvertError { ConvertError::Size(self) } + + /// Format extra details for a verbose, human-readable error message. + /// + /// This formatting may include potentially sensitive information. + fn display_verbose_extras(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result + where + Src: Deref, + Dst: KnownLayout, + { + // include the source type + f.write_str("\nSource type: ")?; + f.write_str(core::any::type_name::())?; + + // include the source.deref() size + let src_size = core::mem::size_of_val(&*self.src); + f.write_str("\nSource size: ")?; + src_size.fmt(f)?; + f.write_str(" byte")?; + if src_size != 1 { + f.write_char('s')?; + } + + // if `Dst` is `Sized`, include the `Dst` size + if let crate::SizeInfo::Sized { size } = Dst::LAYOUT.size_info { + f.write_str("\nDestination size: ")?; + size.fmt(f)?; + f.write_str(" byte")?; + if size != 1 { + f.write_char('s')?; + } + } + + // include the destination type + f.write_str("\nDestination type: ")?; + f.write_str(core::any::type_name::())?; + + Ok(()) + } } impl fmt::Debug for SizeError { @@ -219,19 +327,36 @@ impl fmt::Debug for SizeError { } /// Produces a human-readable error message. +/// +/// The message differs between debug and release builds. When +/// `debug_assertions` are enabled, this message is verbose and includes +/// potentially sensitive information. impl fmt::Display for SizeError where Src: Deref, + Dst: KnownLayout, { #[inline] fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str("the conversion failed because the source was incorrectly sized to complete the conversion into the destination type: ")?; - f.write_str(core::any::type_name::())?; + f.write_str("The conversion failed because the source was incorrectly sized to complete the conversion into the destination type.")?; + if cfg!(debug_assertions) { + f.write_str("\n")?; + self.display_verbose_extras(f)?; + } Ok(()) } } -impl From> for ConvertError, V> { +#[cfg(any(feature = "std", test))] +#[allow(clippy::std_instead_of_core)] +impl std::error::Error for SizeError +where + Src: Deref, + Dst: KnownLayout, +{ +} + +impl From> for ConvertError, V> { #[inline] fn from(err: SizeError) -> Self { Self::Size(err) @@ -244,12 +369,12 @@ pub struct ValidityError { /// The source value involved in the conversion. pub(crate) src: Src, /// The inner destination type inolved in the conversion. - dst: PhantomData, + dst: SendSyncPhantomData, } impl ValidityError { pub(crate) fn new(src: Src) -> Self { - Self { src, dst: PhantomData } + Self { src, dst: SendSyncPhantomData::default() } } /// Produces the source underlying the failed conversion. @@ -260,13 +385,26 @@ impl ValidityError { /// Maps the source value associated with the conversion error. pub(crate) fn map_src(self, f: impl Fn(Src) -> NewSrc) -> ValidityError { - ValidityError { src: f(self.src), dst: PhantomData } + ValidityError { src: f(self.src), dst: SendSyncPhantomData::default() } } /// Converts the error into a general [`ConvertError`]. pub(crate) fn into(self) -> ConvertError { ConvertError::Validity(self) } + + /// Format extra details for a verbose, human-readable error message. + /// + /// This formatting may include potentially sensitive information. + fn display_verbose_extras(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result + where + Src: Deref, + Dst: KnownLayout, + { + f.write_str("Destination type: ")?; + f.write_str(core::any::type_name::())?; + Ok(()) + } } impl fmt::Debug for ValidityError { @@ -277,18 +415,35 @@ impl fmt::Debug for ValidityError { } /// Produces a human-readable error message. -impl fmt::Display for ValidityError +/// +/// The message differs between debug and release builds. When +/// `debug_assertions` are enabled, this message is verbose and includes +/// potentially sensitive information. +impl fmt::Display for ValidityError where Src: Deref, + Dst: KnownLayout + TryFromBytes, { #[inline] fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str("the conversion failed because the source bytes are not a valid value of the destination type: ")?; - f.write_str(core::any::type_name::())?; + f.write_str("The conversion failed because the source bytes are not a valid value of the destination type.")?; + if cfg!(debug_assertions) { + f.write_str("\n\n")?; + self.display_verbose_extras(f)?; + } Ok(()) } } +#[cfg(any(feature = "std", test))] +#[allow(clippy::std_instead_of_core)] +impl std::error::Error for ValidityError +where + Src: Deref, + Dst: KnownLayout + TryFromBytes, +{ +} + impl From> for ConvertError> { @@ -410,9 +565,46 @@ impl TryReadError { } #[cfg(test)] -mod test { +mod tests { use super::*; + #[test] + fn test_send_sync() { + // Test that all error types are `Send + Sync` even if `Dst: !Send + + // !Sync`. + + #[allow(dead_code)] + fn is_send_sync(_t: T) {} + + #[allow(dead_code)] + fn alignment_err_is_send_sync(err: AlignmentError) { + is_send_sync(err) + } + + #[allow(dead_code)] + fn size_err_is_send_sync(err: SizeError) { + is_send_sync(err) + } + + #[allow(dead_code)] + fn validity_err_is_send_sync( + err: ValidityError, + ) { + is_send_sync(err) + } + + #[allow(dead_code)] + fn convert_error_is_send_sync( + err: ConvertError< + AlignmentError, + SizeError, + ValidityError, + >, + ) { + is_send_sync(err) + } + } + #[test] fn alignment_display() { #[repr(C, align(128))] @@ -420,34 +612,72 @@ mod test { bytes: [u8; 128], } + impl_known_layout!(elain::Align::<8>); + let aligned = Aligned { bytes: [0; 128] }; + let bytes = &aligned.bytes[1..]; + let addr = crate::util::AsAddress::addr(bytes); assert_eq!( - AlignmentError::<_, elain::Align::<8>>::new(&aligned.bytes[1..]).to_string(), - "the conversion failed because the address of the source (a multiple of 1) is not a multiple of the alignment (8) of the destination type: elain::Align<8>" + AlignmentError::<_, elain::Align::<8>>::new(bytes).to_string(), + format!("The conversion failed because the address of the source is not a multiple of the alignment of the destination type.\n\ + \nSource type: &[u8]\ + \nSource address: 0x{:x} (a multiple of 1)\ + \nDestination type: elain::Align<8>\ + \nDestination alignment: 8", addr) ); + let bytes = &aligned.bytes[2..]; + let addr = crate::util::AsAddress::addr(bytes); assert_eq!( - AlignmentError::<_, elain::Align::<8>>::new(&aligned.bytes[2..]).to_string(), - "the conversion failed because the address of the source (a multiple of 2) is not a multiple of the alignment (8) of the destination type: elain::Align<8>" + AlignmentError::<_, elain::Align::<8>>::new(bytes).to_string(), + format!("The conversion failed because the address of the source is not a multiple of the alignment of the destination type.\n\ + \nSource type: &[u8]\ + \nSource address: 0x{:x} (a multiple of 2)\ + \nDestination type: elain::Align<8>\ + \nDestination alignment: 8", addr) ); + let bytes = &aligned.bytes[3..]; + let addr = crate::util::AsAddress::addr(bytes); assert_eq!( - AlignmentError::<_, elain::Align::<8>>::new(&aligned.bytes[3..]).to_string(), - "the conversion failed because the address of the source (a multiple of 1) is not a multiple of the alignment (8) of the destination type: elain::Align<8>" + AlignmentError::<_, elain::Align::<8>>::new(bytes).to_string(), + format!("The conversion failed because the address of the source is not a multiple of the alignment of the destination type.\n\ + \nSource type: &[u8]\ + \nSource address: 0x{:x} (a multiple of 1)\ + \nDestination type: elain::Align<8>\ + \nDestination alignment: 8", addr) ); + let bytes = &aligned.bytes[4..]; + let addr = crate::util::AsAddress::addr(bytes); assert_eq!( - AlignmentError::<_, elain::Align::<8>>::new(&aligned.bytes[4..]).to_string(), - "the conversion failed because the address of the source (a multiple of 4) is not a multiple of the alignment (8) of the destination type: elain::Align<8>" + AlignmentError::<_, elain::Align::<8>>::new(bytes).to_string(), + format!("The conversion failed because the address of the source is not a multiple of the alignment of the destination type.\n\ + \nSource type: &[u8]\ + \nSource address: 0x{:x} (a multiple of 4)\ + \nDestination type: elain::Align<8>\ + \nDestination alignment: 8", addr) ); } #[test] fn size_display() { assert_eq!( - SizeError::<_, [u8]>::new(&[0u8; 1][..]).to_string(), - "the conversion failed because the source was incorrectly sized to complete the conversion into the destination type: [u8]" + SizeError::<_, [u8]>::new(&[0u8; 2][..]).to_string(), + "The conversion failed because the source was incorrectly sized to complete the conversion into the destination type.\n\ + \nSource type: &[u8]\ + \nSource size: 2 bytes\ + \nDestination type: [u8]" + ); + + assert_eq!( + SizeError::<_, [u8; 2]>::new(&[0u8; 1][..]).to_string(), + "The conversion failed because the source was incorrectly sized to complete the conversion into the destination type.\n\ + \nSource type: &[u8]\ + \nSource size: 1 byte\ + \nDestination size: 2 bytes\ + \nDestination type: [u8; 2]" ); } @@ -455,7 +685,9 @@ mod test { fn validity_display() { assert_eq!( ValidityError::<_, bool>::new(&[2u8; 1][..]).to_string(), - "the conversion failed because the source bytes are not a valid value of the destination type: bool" + "The conversion failed because the source bytes are not a valid value of the destination type.\n\ + \n\ + Destination type: bool" ); } } diff --git a/src/lib.rs b/src/lib.rs index f16f35200e8..52357ab5d77 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -82,6 +82,11 @@ //! the `alloc` crate is added as a dependency, and some allocation-related //! functionality is added. //! +//! - **`std`** +//! By default, `zerocopy` is `no_std`. When the `std` feature is enabled, the +//! `std` crate is added as a dependency (ie, `no_std` is disabled), and +//! support for some `std` types is added. `std` implies `alloc`. +//! //! - **`derive`** //! Provides derives for the core marker traits via the `zerocopy-derive` //! crate. These derives are re-exported from `zerocopy`, so it is not @@ -264,7 +269,7 @@ clippy::arithmetic_side_effects, clippy::indexing_slicing, ))] -#![cfg_attr(not(test), no_std)] +#![cfg_attr(not(any(test, feature = "std")), no_std)] #![cfg_attr( all(feature = "simd-nightly", any(target_arch = "x86", target_arch = "x86_64")), feature(stdarch_x86_avx512) diff --git a/src/macros.rs b/src/macros.rs index a732af225d9..f1d4f50d4d7 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -553,6 +553,7 @@ macro_rules! impl_known_layout { const _: () = { use core::ptr::NonNull; + #[allow(non_local_definitions)] // SAFETY: Delegates safety to `DstLayout::for_type`. unsafe impl<$($tyvar $(: ?$optbound)?)? $(, const $constvar : $constty)?> KnownLayout for $ty { #[allow(clippy::missing_inline_in_public_items)] diff --git a/src/util.rs b/src/util.rs index f91e9475f1f..2f069fc872f 100644 --- a/src/util.rs +++ b/src/util.rs @@ -8,6 +8,7 @@ use core::{ cell::UnsafeCell, + marker::PhantomData, mem::{self, ManuallyDrop, MaybeUninit}, num::{NonZeroUsize, Wrapping}, ptr::NonNull, @@ -431,6 +432,31 @@ safety_comment! { ); } +/// Like [`PhantomData`], but [`Send`] and [`Sync`] regardless of whether the +/// wrapped `T` is. +pub(crate) struct SendSyncPhantomData(PhantomData); + +// SAFETY: `SendSyncPhantomData` does not enable any behavior which isn't sound +// to be called from multiple threads. +unsafe impl Send for SendSyncPhantomData {} +// SAFETY: `SendSyncPhantomData` does not enable any behavior which isn't sound +// to be called from multiple threads. +unsafe impl Sync for SendSyncPhantomData {} + +impl Default for SendSyncPhantomData { + fn default() -> SendSyncPhantomData { + SendSyncPhantomData(PhantomData) + } +} + +impl PartialEq for SendSyncPhantomData { + fn eq(&self, other: &Self) -> bool { + self.0.eq(&other.0) + } +} + +impl Eq for SendSyncPhantomData {} + pub(crate) trait AsAddress { fn addr(self) -> usize; }