From bdf62034bc06e698b7fabf79cc97d66e84c3e009 Mon Sep 17 00:00:00 2001 From: Joshua Liebow-Feeser Date: Thu, 29 Feb 2024 08:36:16 -0800 Subject: [PATCH] Add IntoByteSlice trait, use as bound for into_ref (#966) Add `IntoByteSlice` and `IntoByteSliceMut` traits, which extend `ByteSlice` and `ByteSliceMut`, adding `Into<&[u8]>` and `Into<&mut [u8]>` bounds respectively. Use these new traits as the bounds in the `Ref` methods `into_ref`, `into_mut`, `into_slice`, and `into_mut_slice`. This allows us to remove the post-monomorphization error which was originally added to patch the soundness hole in #716 in a backwards-compatible way. Closes #758 --- src/lib.rs | 167 +++++++++++++++++++++------------------------ src/pointer/ptr.rs | 70 ++++++++++++++++--- 2 files changed, 137 insertions(+), 100 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 9a30422e81..a89ca50112 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -955,7 +955,7 @@ pub unsafe trait KnownLayout { } // SAFETY: Delegates safety to `DstLayout::for_slice`. -unsafe impl KnownLayout for [T] { +unsafe impl KnownLayout for [T] { #[allow(clippy::missing_inline_in_public_items)] fn only_derive_is_allowed_to_implement_this_trait() where @@ -4989,7 +4989,7 @@ where impl<'a, B, T> Ref where - B: 'a + ByteSlice, + B: 'a + IntoByteSlice<'a>, T: FromBytes + NoCell, { /// Converts this `Ref` into a reference. @@ -4997,40 +4997,51 @@ where /// `into_ref` consumes the `Ref`, and returns a reference to `T`. #[inline(always)] pub fn into_ref(self) -> &'a T { - assert!(B::INTO_REF_INTO_MUT_ARE_SOUND); - - // SAFETY: According to the safety preconditions on - // `ByteSlice::INTO_REF_INTO_MUT_ARE_SOUND`, the preceding assert - // ensures that, given `B: 'a`, it is sound to drop `self` and still - // access the underlying memory using reads for `'a`. - unsafe { self.deref_helper() } + let ptr = Ptr::from_ref(self.0.into()); + // SAFETY: By invariant on `Ref`, `self.0`'s size is equal to + // `size_of::()`. + let ptr = unsafe { ptr.cast::() }; + // SAFETY: By invariant on `Ref`, `self.0` is aligned to `T`'s + // alignment. + let ptr = unsafe { ptr.assume_aligned() }; + // SAFETY: `ptr` is derived from `self.0.into()`, which is of type + // `[u8]`, whose bit validity requires that all of its bytes are + // initialized. + let ptr = unsafe { ptr.assume_initialized() }; + let ptr = ptr.bikeshed_recall_valid(); + ptr.as_ref() } } impl<'a, B, T> Ref where - B: 'a + ByteSliceMut, + B: 'a + IntoByteSliceMut<'a>, T: FromBytes + IntoBytes + NoCell, { /// Converts this `Ref` into a mutable reference. /// /// `into_mut` consumes the `Ref`, and returns a mutable reference to `T`. #[inline(always)] - pub fn into_mut(mut self) -> &'a mut T { - assert!(B::INTO_REF_INTO_MUT_ARE_SOUND); - - // SAFETY: According to the safety preconditions on - // `ByteSlice::INTO_REF_INTO_MUT_ARE_SOUND`, the preceding assert - // ensures that, given `B: 'a + ByteSliceMut`, it is sound to drop - // `self` and still access the underlying memory using both reads and - // writes for `'a`. - unsafe { self.deref_mut_helper() } + pub fn into_mut(self) -> &'a mut T { + let ptr = Ptr::from_mut(self.0.into()); + // SAFETY: By invariant on `Ref`, `self.0`'s size is equal to + // `size_of::()`. + let ptr = unsafe { ptr.cast::() }; + // SAFETY: By invariant on `Ref`, `self.0` is aligned to `T`'s + // alignment. + let ptr = unsafe { ptr.assume_aligned() }; + // SAFETY: `ptr` is derived from `self.0.into()`, which is of type + // `[u8]`, whose bit validity requires that all of its bytes are + // initialized. + let ptr = unsafe { ptr.assume_initialized() }; + let ptr = ptr.bikeshed_recall_valid(); + ptr.as_mut() } } impl<'a, B, T> Ref where - B: 'a + ByteSlice, + B: 'a + IntoByteSlice<'a>, T: FromBytes + NoCell, { /// Converts this `Ref` into a slice reference. @@ -5038,19 +5049,19 @@ where /// `into_slice` consumes the `Ref`, and returns a reference to `[T]`. #[inline(always)] pub fn into_slice(self) -> &'a [T] { - assert!(B::INTO_REF_INTO_MUT_ARE_SOUND); - - // SAFETY: According to the safety preconditions on - // `ByteSlice::INTO_REF_INTO_MUT_ARE_SOUND`, the preceding assert - // ensures that, given `B: 'a`, it is sound to drop `self` and still - // access the underlying memory using reads for `'a`. - unsafe { self.deref_slice_helper() } + // PANICS: By invariant on `Ref`, `self.0`'s size and alignment are + // valid for `[T]`, and so this `unwrap` will not panic. + let ptr = Ptr::from_ref(self.0.into()) + .try_cast_into_no_leftover::<[T]>() + .expect("zerocopy internal error: into_slice should be infallible"); + let ptr = ptr.bikeshed_recall_valid(); + ptr.as_ref() } } impl<'a, B, T> Ref where - B: 'a + ByteSliceMut, + B: 'a + IntoByteSliceMut<'a>, T: FromBytes + IntoBytes + NoCell, { /// Converts this `Ref` into a mutable slice reference. @@ -5058,15 +5069,14 @@ where /// `into_mut_slice` consumes the `Ref`, and returns a mutable reference to /// `[T]`. #[inline(always)] - pub fn into_mut_slice(mut self) -> &'a mut [T] { - assert!(B::INTO_REF_INTO_MUT_ARE_SOUND); - - // SAFETY: According to the safety preconditions on - // `ByteSlice::INTO_REF_INTO_MUT_ARE_SOUND`, the preceding assert - // ensures that, given `B: 'a + ByteSliceMut`, it is sound to drop - // `self` and still access the underlying memory using both reads and - // writes for `'a`. - unsafe { self.deref_mut_slice_helper() } + pub fn into_mut_slice(self) -> &'a mut [T] { + // PANICS: By invariant on `Ref`, `self.0`'s size and alignment are + // valid for `[T]`, and so this `unwrap` will not panic. + let ptr = Ptr::from_mut(self.0.into()) + .try_cast_into_no_leftover::<[T]>() + .expect("zerocopy internal error: into_mut_slice should be infallible"); + let ptr = ptr.bikeshed_recall_valid(); + ptr.as_mut() } } @@ -5481,29 +5491,6 @@ mod sealed { pub unsafe trait ByteSlice: Deref + Sized + self::sealed::ByteSliceSealed { - /// Are the [`Ref::into_ref`] and [`Ref::into_mut`] methods sound when used - /// with `Self`? If not, evaluating this constant must panic at compile - /// time. - /// - /// This exists to work around #716 on versions of zerocopy prior to 0.8. - /// - /// # Safety - /// - /// This may only be set to true if the following holds: Given the - /// following: - /// - `Self: 'a` - /// - `bytes: Self` - /// - `let ptr = bytes.as_ptr()` - /// - /// ...then: - /// - Using `ptr` to read the memory previously addressed by `bytes` is - /// sound for `'a` even after `bytes` has been dropped. - /// - If `Self: ByteSliceMut`, using `ptr` to write the memory previously - /// addressed by `bytes` is sound for `'a` even after `bytes` has been - /// dropped. - #[doc(hidden)] - const INTO_REF_INTO_MUT_ARE_SOUND: bool; - /// Gets a raw pointer to the first byte in the slice. #[inline] fn as_ptr(&self) -> *const u8 { @@ -5534,48 +5521,58 @@ pub unsafe trait ByteSliceMut: ByteSlice + DerefMut { } } +/// A [`ByteSlice`] that conveys no ownership, and so can be converted into a +/// byte slice. +/// +/// Some `ByteSlice` types (notably, the standard library's [`Ref`] type) convey +/// ownership, and so they cannot soundly be moved by-value into a byte slice +/// type (`&[u8]`). Some methods in this crate's API (such as [`Ref::into_ref`]) +/// are only compatible with `ByteSlice` types without these ownership +/// semantics. +/// +/// [`Ref`]: core::cell::Ref +pub trait IntoByteSlice<'a>: ByteSlice + Into<&'a [u8]> {} + +/// A [`ByteSliceMut`] that conveys no ownership, and so can be converted into a +/// mutable byte slice. +/// +/// Some `ByteSliceMut` types (notably, the standard library's [`RefMut`] type) +/// convey ownership, and so they cannot soundly be moved by-value into a byte +/// slice type (`&mut [u8]`). Some methods in this crate's API (such as +/// [`Ref::into_mut`]) are only compatible with `ByteSliceMut` types without +/// these ownership semantics. +/// +/// [`RefMut`]: core::cell::RefMut +pub trait IntoByteSliceMut<'a>: ByteSliceMut + Into<&'a mut [u8]> {} + impl<'a> sealed::ByteSliceSealed for &'a [u8] {} // TODO(#429): Add a "SAFETY" comment and remove this `allow`. #[allow(clippy::undocumented_unsafe_blocks)] unsafe impl<'a> ByteSlice for &'a [u8] { - // SAFETY: If `&'b [u8]: 'a`, then the underlying memory is treated as - // borrowed immutably for `'a` even if the slice itself is dropped. - const INTO_REF_INTO_MUT_ARE_SOUND: bool = true; - #[inline] fn split_at(self, mid: usize) -> (Self, Self) { <[u8]>::split_at(self, mid) } } +impl<'a> IntoByteSlice<'a> for &'a [u8] {} + impl<'a> sealed::ByteSliceSealed for &'a mut [u8] {} // TODO(#429): Add a "SAFETY" comment and remove this `allow`. #[allow(clippy::undocumented_unsafe_blocks)] unsafe impl<'a> ByteSlice for &'a mut [u8] { - // SAFETY: If `&'b mut [u8]: 'a`, then the underlying memory is treated as - // borrowed mutably for `'a` even if the slice itself is dropped. - const INTO_REF_INTO_MUT_ARE_SOUND: bool = true; - #[inline] fn split_at(self, mid: usize) -> (Self, Self) { <[u8]>::split_at_mut(self, mid) } } +impl<'a> IntoByteSliceMut<'a> for &'a mut [u8] {} + impl<'a> sealed::ByteSliceSealed for cell::Ref<'a, [u8]> {} // TODO(#429): Add a "SAFETY" comment and remove this `allow`. #[allow(clippy::undocumented_unsafe_blocks)] unsafe impl<'a> ByteSlice for cell::Ref<'a, [u8]> { - const INTO_REF_INTO_MUT_ARE_SOUND: bool = if !cfg!(doc) { - const_panic!("Ref::into_ref and Ref::into_mut are unsound when used with core::cell::Ref; see https://github.com/google/zerocopy/issues/716") - } else { - // When compiling documentation, allow the evaluation of this constant - // to succeed. This doesn't represent a soundness hole - it just delays - // any error to runtime. The reason we need this is that, otherwise, - // `rustdoc` will fail when trying to document this item. - false - }; - #[inline] fn split_at(self, mid: usize) -> (Self, Self) { cell::Ref::map_split(self, |slice| <[u8]>::split_at(slice, mid)) @@ -5586,16 +5583,6 @@ impl<'a> sealed::ByteSliceSealed for RefMut<'a, [u8]> {} // TODO(#429): Add a "SAFETY" comment and remove this `allow`. #[allow(clippy::undocumented_unsafe_blocks)] unsafe impl<'a> ByteSlice for RefMut<'a, [u8]> { - const INTO_REF_INTO_MUT_ARE_SOUND: bool = if !cfg!(doc) { - const_panic!("Ref::into_ref and Ref::into_mut are unsound when used with core::cell::RefMut; see https://github.com/google/zerocopy/issues/716") - } else { - // When compiling documentation, allow the evaluation of this constant - // to succeed. This doesn't represent a soundness hole - it just delays - // any error to runtime. The reason we need this is that, otherwise, - // `rustdoc` will fail when trying to document this item. - false - }; - #[inline] fn split_at(self, mid: usize) -> (Self, Self) { RefMut::map_split(self, |slice| <[u8]>::split_at_mut(slice, mid)) @@ -8911,7 +8898,7 @@ mod tests { // implementation of ` as TryFromBytes>::is_bit_valid`. assert_impls!(ManuallyDrop<[bool]>: KnownLayout, NoCell, TryFromBytes, FromZeros, IntoBytes, Unaligned, !FromBytes); assert_impls!(ManuallyDrop: !NoCell, !TryFromBytes, !KnownLayout, !FromZeros, !FromBytes, !IntoBytes, !Unaligned); - assert_impls!(ManuallyDrop<[NotZerocopy]>: !NoCell, !TryFromBytes, !KnownLayout, !FromZeros, !FromBytes, !IntoBytes, !Unaligned); + assert_impls!(ManuallyDrop<[NotZerocopy]>: KnownLayout, !NoCell, !TryFromBytes, !FromZeros, !FromBytes, !IntoBytes, !Unaligned); assert_impls!(ManuallyDrop>: KnownLayout, TryFromBytes, FromZeros, FromBytes, IntoBytes, Unaligned, !NoCell); assert_impls!(ManuallyDrop<[UnsafeCell]>: KnownLayout, TryFromBytes, FromZeros, FromBytes, IntoBytes, Unaligned, !NoCell); assert_impls!(ManuallyDrop<[UnsafeCell]>: KnownLayout, TryFromBytes, FromZeros, IntoBytes, Unaligned, !NoCell, !FromBytes); @@ -8951,7 +8938,7 @@ mod tests { Unaligned, !FromBytes ); - assert_impls!([NotZerocopy]: !KnownLayout, !NoCell, !TryFromBytes, !FromZeros, !FromBytes, !IntoBytes, !Unaligned); + assert_impls!([NotZerocopy]: KnownLayout, !NoCell, !TryFromBytes, !FromZeros, !FromBytes, !IntoBytes, !Unaligned); assert_impls!( [u8; 0]: KnownLayout, NoCell, diff --git a/src/pointer/ptr.rs b/src/pointer/ptr.rs index 2d8a6013d2..f79d569a01 100644 --- a/src/pointer/ptr.rs +++ b/src/pointer/ptr.rs @@ -765,6 +765,21 @@ mod _transitions { unsafe { Ptr::from_ptr(self) } } + /// Assumes that `Ptr`'s referent is validly-aligned for `T`. + /// + /// # Safety + /// + /// The caller promises that `Ptr`'s referent satisfies the alignment + /// requirement of `T`. + #[inline] + pub(crate) unsafe fn assume_aligned( + self, + ) -> Ptr<'a, T, (I::Aliasing, invariant::Aligned, I::Validity)> { + // SAFETY: The caller promises that `self`'s referent is + // validly-aligned for `T`. + unsafe { self.assume_alignment::() } + } + /// Recalls that `Ptr`'s referent is validly-aligned for `T`. #[inline] // TODO(#859): Reconsider the name of this method before making it @@ -829,6 +844,24 @@ mod _transitions { unsafe { self.assume_validity::() } } + /// Recalls that `Ptr`'s referent is bit-valid for `T`. + #[inline] + // TODO(#859): Reconsider the name of this method before making it + // public. + pub(crate) fn bikeshed_recall_valid( + self, + ) -> Ptr<'a, T, (I::Aliasing, I::Alignment, invariant::Valid)> + where + T: crate::FromBytes, + I: Invariants, + { + // SAFETY: The bound `T: FromBytes` ensures that any initialized + // sequence of bytes is bit-valid for `T`. `I: Invariants` ensures that all of the referent bytes + // are initialized. + unsafe { self.assume_valid() } + } + /// Checks that `Ptr`'s referent is validly initialized for `T`, /// returning a `Ptr` with `invariant::Valid` on success. /// @@ -972,6 +1005,21 @@ mod _casts { // 10. See 9. unsafe { Ptr::new(ptr) } } + + /// Casts to a different target type. + /// + /// # Safety + /// + /// The caller promises that `size_of::()` is not larger than the + /// referent of `self`. + pub(crate) unsafe fn cast( + self, + ) -> Ptr<'a, U, (I::Aliasing, invariant::Any, invariant::Any)> { + // SAFETY: The cast is implemented as required by `cast_unsized`. + // The caller promises that the cast will not result in a larger + // referent. + unsafe { self.cast_unsized(|p| p.cast::()) } + } } impl<'a, T, I> Ptr<'a, T, I> @@ -1074,7 +1122,7 @@ mod _casts { /// alignment of `[u8]` is 1. impl<'a, I> Ptr<'a, [u8], I> where - I: Invariants, + I: Invariants, { /// Attempts to cast `self` to a `U` using the given cast type. /// @@ -1099,7 +1147,7 @@ mod _casts { pub(crate) fn try_cast_into( &self, cast_type: _CastType, - ) -> Option<(Ptr<'a, U, (I::Aliasing, invariant::Aligned, invariant::Any)>, usize)> + ) -> Option<(Ptr<'a, U, (I::Aliasing, invariant::Aligned, invariant::Initialized)>, usize)> where U: NoCell, { @@ -1173,8 +1221,9 @@ mod _casts { // it is derived from `validate_cast_and_convert_metadata` // promises that the object described by `split_at` is validly // aligned for `U`. - // 8. `ptr`, trivially, conforms to the validity invariant of - // `Any`. + // 8. By trait bound, `self` is a bit-valid `[u8]`. All bit-valid + // `[u8]`s have all of their bytes initialized, so `ptr` conforms + // to the validity invariant of `Initialized`. // 9. During the lifetime 'a, no code will reference or load or // store this memory region treating `UnsafeCell`s as existing at // different ranges than they exist in `U`. This is true by @@ -1198,7 +1247,7 @@ mod _casts { #[inline(always)] pub(crate) fn try_cast_into_no_leftover( &self, - ) -> Option> + ) -> Option> where U: NoCell, { @@ -1491,12 +1540,13 @@ mod tests { // SAFETY: The bytes in `slf` must be initialized. unsafe fn validate_and_get_len( - slf: Ptr<'_, T, (invariant::Shared, invariant::Aligned, invariant::Any)>, + slf: Ptr< + '_, + T, + (invariant::Shared, invariant::Aligned, invariant::Initialized), + >, ) -> usize { - // SAFETY: - // - Since all bytes in `slf` are initialized and - // `T: FromBytes`, `slf` contains a valid `T`. - let t = unsafe { slf.as_non_null().as_ref() }; + let t = slf.bikeshed_recall_valid().as_ref(); let bytes = { let len = mem::size_of_val(t);