Skip to content

Commit

Permalink
Fix soundness hole in Ref::into_ref and into_mut
Browse files Browse the repository at this point in the history
This commit implements the fix for #716 which will be released as a new
version in version trains 0.2, 0.3, 0.4, 0.5, 0.6, and 0.7. See #716 for
a description of the soundness hole and an explanation of why this fix
is chosen.

Unfortunately, due to dtolnay/trybuild#241, there is no way for us to
write a UI test that will detect a failure post-monomorphization, which
is when the code implemented in this change is designed to fail. I have
manually verified that unsound uses of these APIs now fail to compile.
  • Loading branch information
joshlf committed Dec 13, 2023
1 parent 961612f commit 9040e22
Showing 1 changed file with 61 additions and 25 deletions.
86 changes: 61 additions & 25 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4644,12 +4644,12 @@ where
/// `into_ref` consumes the `Ref`, and returns a reference to `T`.
#[inline(always)]
pub fn into_ref(self) -> &'a T {
// SAFETY: This is sound because `B` is guaranteed to live for the
// lifetime `'a`, meaning that a) the returned reference cannot outlive
// the `B` from which `self` was constructed and, b) no mutable methods
// on that `B` can be called during the lifetime of the returned
// reference. See the documentation on `deref_helper` for what
// invariants we are required to uphold.
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() }
}
}
Expand All @@ -4664,12 +4664,13 @@ where
/// `into_mut` consumes the `Ref`, and returns a mutable reference to `T`.
#[inline(always)]
pub fn into_mut(mut self) -> &'a mut T {
// SAFETY: This is sound because `B` is guaranteed to live for the
// lifetime `'a`, meaning that a) the returned reference cannot outlive
// the `B` from which `self` was constructed and, b) no other methods -
// mutable or immutable - on that `B` can be called during the lifetime
// of the returned reference. See the documentation on
// `deref_mut_helper` for what invariants we are required to uphold.
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() }
}
}
Expand All @@ -4684,12 +4685,12 @@ where
/// `into_slice` consumes the `Ref`, and returns a reference to `[T]`.
#[inline(always)]
pub fn into_slice(self) -> &'a [T] {
// SAFETY: This is sound because `B` is guaranteed to live for the
// lifetime `'a`, meaning that a) the returned reference cannot outlive
// the `B` from which `self` was constructed and, b) no mutable methods
// on that `B` can be called during the lifetime of the returned
// reference. See the documentation on `deref_slice_helper` for what
// invariants we are required to uphold.
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() }
}
}
Expand All @@ -4705,13 +4706,13 @@ where
/// `[T]`.
#[inline(always)]
pub fn into_mut_slice(mut self) -> &'a mut [T] {
// SAFETY: This is sound because `B` is guaranteed to live for the
// lifetime `'a`, meaning that a) the returned reference cannot outlive
// the `B` from which `self` was constructed and, b) no other methods -
// mutable or immutable - on that `B` can be called during the lifetime
// of the returned reference. See the documentation on
// `deref_mut_slice_helper` for what invariants we are required to
// uphold.
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() }
}
}
Expand Down Expand Up @@ -5127,6 +5128,29 @@ mod sealed {
pub unsafe trait ByteSlice:
Deref<Target = [u8]> + 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 {
Expand Down Expand Up @@ -5161,6 +5185,10 @@ 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)
Expand All @@ -5171,6 +5199,10 @@ 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)
Expand All @@ -5181,6 +5213,8 @@ 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 = panic!("Ref::into_ref and Ref::into_mut are unsound when used with core::cell::Ref; see https://github.com/google/zerocopy/issues/716");

#[inline]
fn split_at(self, mid: usize) -> (Self, Self) {
cell::Ref::map_split(self, |slice| <[u8]>::split_at(slice, mid))
Expand All @@ -5191,6 +5225,8 @@ 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 = panic!("Ref::into_ref and Ref::into_mut are unsound when used with core::cell::RefMut; see https://github.com/google/zerocopy/issues/716");

#[inline]
fn split_at(self, mid: usize) -> (Self, Self) {
RefMut::map_split(self, |slice| <[u8]>::split_at_mut(slice, mid))
Expand Down

0 comments on commit 9040e22

Please sign in to comment.