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.
  • Loading branch information
joshlf committed Dec 13, 2023
1 parent 961612f commit 4563645
Showing 1 changed file with 50 additions and 9 deletions.
59 changes: 50 additions & 9 deletions src/lib.rs
Expand Up @@ -4644,10 +4644,14 @@ 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
assert!(B::INTO_REF_INTO_MUT_ARE_SOUND);

// SAFETY: TODO
//
// Old safety comment: 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.
unsafe { self.deref_helper() }
Expand All @@ -4664,11 +4668,15 @@ 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
assert!(B::INTO_REF_INTO_MUT_ARE_SOUND);

// SAFETY: TODO
//
// Old safety comment: 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.
unsafe { self.deref_mut_helper() }
}
Expand Down Expand Up @@ -5127,6 +5135,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 +5192,9 @@ 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: TODO
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 +5205,9 @@ 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: TODO
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 +5218,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 +5230,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 4563645

Please sign in to comment.