From ca494733bf98b4636e9c82d4132e8991f12612f5 Mon Sep 17 00:00:00 2001 From: Joshua Liebow-Feeser Date: Thu, 22 Feb 2024 11:02:51 -0800 Subject: [PATCH] Support testing TryFromBytes for !NoCell types (#924) Previously, our test harness for `TryFromBytes` types assumed that those types also implemented `NoCell`. This commit introduces machinery that allows us to conditionally execute certain steps of our tests depending on whether or not the type under test implements `NoCell`. This commit also lifts the `T: NoCell` bound in `TryFromBytes for MaybeUninit` in order to test this machinery. Makes progress on #5 Co-authored-by: Jack Wrenn --- src/lib.rs | 255 +++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 226 insertions(+), 29 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 8f14ce5d35..e981c77c79 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -3577,7 +3577,7 @@ safety_comment! { /// [1] TODO(https://github.com/rust-lang/rust/pull/121215) /// [2] https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#layout-1 unsafe_impl!(T: NoCell => NoCell for MaybeUninit); - unsafe_impl!(T: NoCell => TryFromBytes for MaybeUninit); + unsafe_impl!(T => TryFromBytes for MaybeUninit); unsafe_impl!(T => FromZeros for MaybeUninit); unsafe_impl!(T => FromBytes for MaybeUninit); unsafe_impl!(T: Unaligned => Unaligned for MaybeUninit); @@ -8167,48 +8167,245 @@ mod tests { @failure [0x01; mem::size_of::<*mut NotZerocopy>()]; ); + // Use the trick described in [1] to allow us to call methods + // conditional on certain trait bounds. + // + // In all of these cases, methods return `Option`, where `R` is the + // return type of the method we're conditionally calling. The "real" + // implementations (the ones defined in traits using `&self`) return + // `Some`, and the default implementations (the ones defined as inherent + // methods using `&mut self`) return `None`. + // + // [1] https://github.com/dtolnay/case-studies/blob/master/autoref-specialization/README.md + mod autoref_trick { + use super::*; + + pub(super) struct AutorefWrapper(pub(super) PhantomData); + + pub(super) trait TestTryFromRef { + fn test_try_from_ref<'bytes>( + &self, + bytes: &'bytes [u8], + ) -> Option>; + + fn test_try_from_mut<'bytes>( + &self, + bytes: &'bytes mut [u8], + ) -> Option>; + } + + impl TestTryFromRef for AutorefWrapper { + fn test_try_from_ref<'bytes>( + &self, + bytes: &'bytes [u8], + ) -> Option> { + Some(T::try_from_ref(bytes)) + } + + fn test_try_from_mut<'bytes>( + &self, + bytes: &'bytes mut [u8], + ) -> Option> { + Some(T::try_from_mut(bytes)) + } + } + + pub(super) trait TestTryReadFrom { + fn test_try_read_from(&self, bytes: &[u8]) -> Option>; + } + + // TODO(#5): Remove the `NoCell` bound once `try_read_from` no + // longer requires that bound. + impl TestTryReadFrom for AutorefWrapper { + fn test_try_read_from(&self, bytes: &[u8]) -> Option> { + Some(T::try_read_from(bytes)) + } + } + + pub(super) trait TestAsBytes { + fn test_as_bytes<'slf, 't>(&'slf self, t: &'t T) -> Option<&'t [u8]>; + } + + impl TestAsBytes for AutorefWrapper { + fn test_as_bytes<'slf, 't>(&'slf self, t: &'t T) -> Option<&'t [u8]> { + Some(t.as_bytes()) + } + } + } + + use autoref_trick::*; + + // Asserts that `$ty` is one of a list of types which are allowed to not + // provide a "real" implementation for `$fn_name`. Since the + // `autoref_trick` machinery fails silently, this allows us to ensure + // that the "default" impls are only being used for types which we + // expect. + // + // Note that, since this is a runtime test, it is possible to have an + // allowlist which is too restrictive if the function in question is + // never called for a particular type. For example, if `as_bytes` is not + // supported for a particular type, and so `test_as_bytes` returns + // `None`, methods such as `test_try_from_ref` may never be called for + // that type. As a result, it's possible that, for example, adding + // `as_bytes` support for a type would cause other allowlist assertions + // to fail. This means that allowlist assertion failures should not + // automatically be taken as a sign of a bug. + macro_rules! assert_on_allowlist { + ($fn_name:ident($ty:ty) $(: $($tys:ty),*)?) => {{ + use core::any::TypeId; + + let allowlist: &[TypeId] = &[ $($(TypeId::of::<$tys>()),*)? ]; + let allowlist_names: &[&str] = &[ $($(stringify!($tys)),*)? ]; + + let id = TypeId::of::<$ty>(); + assert!(allowlist.contains(&id), "{} is not on allowlist for {}: {:?}", stringify!($ty), stringify!($fn_name), allowlist_names); + }}; + } + // Asserts that `$ty` implements any `$trait` and doesn't implement any // `!$trait`. Note that all `$trait`s must come before any `!$trait`s. // // For `T: TryFromBytes`, uses `TryFromBytesTestable` to test success - // and failure cases for `TryFromBytes::is_bit_valid`. + // and failure cases. macro_rules! assert_impls { ($ty:ty: TryFromBytes) => { + // "Default" implementations that match the "real" + // implementations defined in the `autoref_trick` module above. + #[allow(unused)] + impl AutorefWrapper<$ty> { + fn test_try_from_ref<'bytes>(&mut self, _bytes: &'bytes [u8]) -> Option> { + assert_on_allowlist!(test_try_from_ref($ty)); + + None + } + + fn test_try_from_mut<'bytes>(&mut self, _bytes: &'bytes mut [u8]) -> Option> { + assert_on_allowlist!(test_try_from_mut($ty)); + + None + } + + fn test_try_read_from(&mut self, _bytes: &[u8]) -> Option> { + assert_on_allowlist!( + test_try_read_from($ty): + str, + ManuallyDrop<[u8]>, + ManuallyDrop<[bool]>, + [u8], + [bool] + ); + + None + } + + fn test_as_bytes(&mut self, _t: &$ty) -> Option<&[u8]> { + assert_on_allowlist!( + test_as_bytes($ty): + Option<&'static UnsafeCell>, + Option<&'static mut UnsafeCell>, + Option>>, + Option>>, + Option, + Option, + Option, + Option, + MaybeUninit, + MaybeUninit, + MaybeUninit>, + *const NotZerocopy, + *mut NotZerocopy + ); + + None + } + } + <$ty as TryFromBytesTestable>::with_passing_test_cases(|val| { let c = Ptr::from_ref(val); let c = c.forget_aligned(); - // SAFETY: - // TODO(#899): This is unsound. `$ty` is not necessarily - // `IntoBytes`, but that's the corner we've backed ourselves - // into by using `Ptr::from_ref`. + // Test `is_bit_valid` directly. NOTE: It's very important + // that we test this directly in addition to testing other + // methods below. Those methods rely on the `autoref_trick` + // module, which is very brittle, and fails silently + // (namely, bugs manifest as code simply not running and + // thus tests succeeding). While we have + // `assert_on_allowlist!` to help prevent regressions, we + // shouldn't rely on it. `is_bit_valid` is the most + // important thing to test, and so it's crucial that we test + // this directly in a way that will reliably be tested even + // if we have an undiscovered bug in our `autoref_trick` + // machinery. + // + // SAFETY: TODO(#899): This is unsound. `$ty` is not + // necessarily `IntoBytes`, but that's the corner we've + // backed ourselves into by using `Ptr::from_ref`. let c = unsafe { c.assume_initialized() }; let res = <$ty as TryFromBytes>::is_bit_valid(c); assert!(res, "{}::is_bit_valid({:?}): got false, expected true", stringify!($ty), val); - // TODO(#5): In addition to testing `is_bit_valid`, test the - // methods built on top of it. This would both allow us to - // test their implementations and actually convert the bytes - // to `$ty`, giving Miri a chance to catch if this is - // unsound (ie, if our `is_bit_valid` impl is buggy). - // - // The following code was tried, but it doesn't work because - // a) some types are not `IntoBytes` and, b) some types are - // not `Sized`. + // TODO(#494): These tests only get exercised for types + // which are `IntoBytes`. Once we implement #494, we should + // be able to support non-`IntoBytes` types by zeroing + // padding. + + // We define `w` and `ww` since, in the case of the inherent + // methods, Rust thinks they're both borrowed mutably at the + // same time (given how we use them below). If we just + // defined a single `w` and used it for multiple operations, + // this would conflict. // - // let r = <$ty as TryFromBytes>::try_from_ref(val.as_bytes()).unwrap(); - // assert_eq!(r, &val); - // let r = <$ty as TryFromBytes>::try_from_mut(val.as_mut_bytes()).unwrap(); - // assert_eq!(r, &mut val); - // let v = <$ty as TryFromBytes>::try_read_from(val.as_bytes()).unwrap(); - // assert_eq!(v, val); + // We `#[allow(unused_mut]` for the cases where the "real" + // impls are used, which take `&self`. + #[allow(unused_mut)] + let (mut w, mut ww) = (AutorefWrapper::<$ty>(PhantomData), AutorefWrapper::<$ty>(PhantomData)); + + // `bytes` is `Some(val.as_bytes())` if `$ty: IntoBytes + + // NoCell` and `None` otherwise. + let bytes = w.test_as_bytes(val); + + // The inner closure returns + // `Some($ty::try_from_ref(bytes))` if `$ty: NoCell` and + // `None` otherwise. + let res = bytes.and_then(|bytes| ww.test_try_from_ref(bytes)); + if let Some(res) = res { + assert!(res.is_some(), "{}::try_from_ref({:?}): got `None`, expected `Some`", stringify!($ty), val); + } + + if let Some(bytes) = bytes { + let mut bytes = bytes.to_vec(); + let res = ww.test_try_from_mut(bytes.as_mut_slice()); + if let Some(res) = res { + assert!(res.is_some(), "{}::try_from_mut({:?}): got `None`, expected `Some`", stringify!($ty), val); + } + } + + let res = bytes.and_then(|bytes| ww.test_try_read_from(bytes)); + if let Some(res) = res { + assert!(res.is_some(), "{}::try_read_from({:?}): got `None`, expected `Some`", stringify!($ty), val); + } }); #[allow(clippy::as_conversions)] <$ty as TryFromBytesTestable>::with_failing_test_cases(|c| { - let res = <$ty as TryFromBytes>::try_from_ref(c); - assert!(res.is_none(), "{}::try_from_ref({:?}): got Some, expected None", stringify!($ty), c); - let res = <$ty as TryFromBytes>::try_from_mut(c); - assert!(res.is_none(), "{}::try_from_mut({:?}): got Some, expected None", stringify!($ty), c); + #[allow(unused_mut)] // For cases where the "real" impls are used, which take `&self`. + let mut w = AutorefWrapper::<$ty>(PhantomData); + + // This is `Some($ty::try_from_ref(c))` if `$ty: NoCell` and + // `None` otherwise. + let res = w.test_try_from_ref(c); + if let Some(res) = res { + assert!(res.is_none(), "{}::try_from_ref({:?}): got Some, expected None", stringify!($ty), c); + } + + let res = w.test_try_from_mut(c); + if let Some(res) = res { + assert!(res.is_none(), "{}::try_from_mut({:?}): got Some, expected None", stringify!($ty), c); + } + + let res = w.test_try_read_from(c); + if let Some(res) = res { + assert!(res.is_none(), "{}::try_read_from({:?}): got Some, expected None", stringify!($ty), c); + } }); #[allow(dead_code)] @@ -8562,8 +8759,8 @@ mod tests { assert_impls!(ManuallyDrop<[UnsafeCell<()>]>: KnownLayout, FromZeros, FromBytes, IntoBytes, Unaligned, !NoCell, !TryFromBytes); assert_impls!(MaybeUninit: KnownLayout, NoCell, TryFromBytes, FromZeros, FromBytes, Unaligned, !IntoBytes); - assert_impls!(MaybeUninit: KnownLayout, FromZeros, FromBytes, !NoCell, !TryFromBytes, !IntoBytes, !Unaligned); - assert_impls!(MaybeUninit>: KnownLayout, FromZeros, FromBytes, Unaligned, !TryFromBytes, !NoCell, !IntoBytes); + assert_impls!(MaybeUninit: KnownLayout, TryFromBytes, FromZeros, FromBytes, !NoCell, !IntoBytes, !Unaligned); + assert_impls!(MaybeUninit>: KnownLayout, TryFromBytes, FromZeros, FromBytes, Unaligned, !NoCell, !IntoBytes); assert_impls!(Wrapping: KnownLayout, NoCell, TryFromBytes, FromZeros, FromBytes, IntoBytes, Unaligned); // This test is important because it allows us to test our hand-rolled @@ -8572,10 +8769,10 @@ mod tests { assert_impls!(Wrapping: KnownLayout, !NoCell, !TryFromBytes, !FromZeros, !FromBytes, !IntoBytes, !Unaligned); assert_impls!(Wrapping>: KnownLayout, FromZeros, FromBytes, IntoBytes, Unaligned, !NoCell, !TryFromBytes); - assert_impls!(Unalign: KnownLayout, NoCell, TryFromBytes, FromZeros, FromBytes, IntoBytes, Unaligned, TryFromBytes); + assert_impls!(Unalign: KnownLayout, NoCell, TryFromBytes, FromZeros, FromBytes, IntoBytes, Unaligned); // This test is important because it allows us to test our hand-rolled // implementation of ` as TryFromBytes>::is_bit_valid`. - assert_impls!(Unalign: KnownLayout, NoCell, TryFromBytes, FromZeros, IntoBytes, Unaligned, TryFromBytes, !FromBytes); + assert_impls!(Unalign: KnownLayout, NoCell, TryFromBytes, FromZeros, IntoBytes, Unaligned, !FromBytes); assert_impls!(Unalign: Unaligned, !NoCell, !KnownLayout, !TryFromBytes, !FromZeros, !FromBytes, !IntoBytes); assert_impls!(