Fix soundness hole in Ref::into_ref and into_mut (#721) (#755)

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.

Release 0.8.0-alpha.2.

Cargo.toml

@@ -15,7 +15,7 @@
 [package]
 edition = "2018"
 name = "zerocopy"
-version = "0.8.0-alpha.1"
+version = "0.8.0-alpha.2"
 authors = ["Joshua Liebow-Feeser <joshlf@google.com>"]
 description = "Utilities for zero-copy parsing and serialization"
 license = "BSD-2-Clause OR Apache-2.0 OR MIT"
@@ -49,7 +49,7 @@ simd-nightly = ["simd"]
 __internal_use_only_features_that_work_on_stable = ["alloc", "derive", "simd"]
 
 [dependencies]
-zerocopy-derive = { version = "=0.8.0-alpha.1", path = "zerocopy-derive", optional = true }
+zerocopy-derive = { version = "=0.8.0-alpha.2", path = "zerocopy-derive", optional = true }
 
 [dependencies.byteorder]
 version = "1.3"
@@ -60,7 +60,7 @@ optional = true
 # zerocopy-derive remain equal, even if the 'derive' feature isn't used.
 # See: https://github.com/matklad/macro-dep-test
 [target.'cfg(any())'.dependencies]
-zerocopy-derive = { version = "=0.8.0-alpha.1", path = "zerocopy-derive" }
+zerocopy-derive = { version = "=0.8.0-alpha.2", path = "zerocopy-derive" }
 
 [dev-dependencies]
 assert_matches = "1.5"
@@ -75,6 +75,6 @@ testutil = { path = "testutil" }
 # CI test failures.
 trybuild = { version = "=1.0.85", features = ["diff"] }
 # In tests, unlike in production, zerocopy-derive is not optional
-zerocopy-derive = { version = "=0.8.0-alpha.1", path = "zerocopy-derive" }
+zerocopy-derive = { version = "=0.8.0-alpha.2", path = "zerocopy-derive" }
 # TODO(#381) Remove this dependency once we have our own layout gadgets.
 elain = "0.3.0"

src/lib.rs

@@ -249,6 +249,7 @@ mod macros;
 pub mod byteorder;
 #[doc(hidden)]
 pub mod macro_util;
+mod post_monomorphization_compile_fail_tests;
 mod util;
 // TODO(#252): If we make this pub, come up with a better name.
 mod wrappers;
@@ -4801,12 +4802,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() }
     }
 }
@@ -4821,12 +4822,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() }
     }
 }
@@ -4841,12 +4843,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() }
     }
 }
@@ -4862,13 +4864,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() }
     }
 }
@@ -5339,6 +5341,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 {
@@ -5373,6 +5398,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)
@@ -5383,6 +5412,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)
@@ -5393,6 +5426,16 @@ 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) {
+        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))
@@ -5403,6 +5446,16 @@ 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) {
+        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))

src/post_monomorphization_compile_fail_tests.rs

@@ -0,0 +1,118 @@
+// Copyright 2018 The Fuchsia Authors
+//
+// Licensed under the 2-Clause BSD License <LICENSE-BSD or
+// https://opensource.org/license/bsd-2-clause>, Apache License, Version 2.0
+// <LICENSE-APACHE or https://www.apache.org/licenses/LICENSE-2.0>, or the MIT
+// license <LICENSE-MIT or https://opensource.org/licenses/MIT>, at your option.
+// This file may not be copied, modified, or distributed except according to
+// those terms.
+
+//! Code that should fail to compile during the post-monomorphization compiler
+//! pass.
+//!
+//! Due to [a limitation with the `trybuild` crate][trybuild-issue], we cannot
+//! use our UI testing framework to test compilation failures that are
+//! encountered after monomorphization has complated. This module has one item
+//! for each such test we would prefer to have as a UI test, with the code in
+//! question appearing as a rustdoc example which is marked with `compile_fail`.
+//! This has the effect of causing doctests to fail if any of these examples
+//! compile successfully.
+//!
+//! This is very much a hack and not a complete replacement for UI tests - most
+//! notably because this only provides a single "compile vs fail" bit of
+//! information, but does not allow us to depend upon the specific error that
+//! causes compilation to fail.
+//!
+//! [trybuild-issue]: https://github.com/dtolnay/trybuild/issues/241
+
+// Miri doesn't detect post-monimorphization failures as compile-time failures,
+// but instead as runtime failures.
+#![cfg(not(miri))]
+
+/// ```compile_fail
+/// use core::cell::{Ref, RefCell};
+///
+/// let refcell = RefCell::new([0u8, 1, 2, 3]);
+/// let core_ref = refcell.borrow();
+/// let core_ref = Ref::map(core_ref, |bytes| &bytes[..]);
+///
+/// // `zc_ref` now stores `core_ref` internally.
+/// let zc_ref = zerocopy::Ref::<_, u32>::new(core_ref).unwrap();
+///
+/// // This causes `core_ref` to get dropped and synthesizes a Rust
+/// // reference to the memory `core_ref` was pointing at.
+/// let rust_ref = zc_ref.into_ref();
+///
+/// // UB!!! This mutates `rust_ref`'s referent while it's alive.
+/// *refcell.borrow_mut() = [0, 0, 0, 0];
+///
+/// println!("{}", rust_ref);
+/// ```
+#[allow(unused)]
+const REFCELL_REF_INTO_REF: () = ();
+
+/// ```compile_fail
+/// use core::cell::{RefCell, RefMut};
+///
+/// let refcell = RefCell::new([0u8, 1, 2, 3]);
+/// let core_ref_mut = refcell.borrow_mut();
+/// let core_ref_mut = RefMut::map(core_ref_mut, |bytes| &mut bytes[..]);
+///
+/// // `zc_ref` now stores `core_ref_mut` internally.
+/// let zc_ref = zerocopy::Ref::<_, u32>::new(core_ref_mut).unwrap();
+///
+/// // This causes `core_ref_mut` to get dropped and synthesizes a Rust
+/// // reference to the memory `core_ref` was pointing at.
+/// let rust_ref_mut = zc_ref.into_mut();
+///
+/// // UB!!! This mutates `rust_ref_mut`'s referent while it's alive.
+/// *refcell.borrow_mut() = [0, 0, 0, 0];
+///
+/// println!("{}", rust_ref_mut);
+/// ```
+#[allow(unused)]
+const REFCELL_REFMUT_INTO_MUT: () = ();
+
+/// ```compile_fail
+/// use core::cell::{Ref, RefCell};
+///
+/// let refcell = RefCell::new([0u8, 1, 2, 3]);
+/// let core_ref = refcell.borrow();
+/// let core_ref = Ref::map(core_ref, |bytes| &bytes[..]);
+///
+/// // `zc_ref` now stores `core_ref` internally.
+/// let zc_ref = zerocopy::Ref::<_, [u16]>::new_slice(core_ref).unwrap();
+///
+/// // This causes `core_ref` to get dropped and synthesizes a Rust
+/// // reference to the memory `core_ref` was pointing at.
+/// let rust_ref = zc_ref.into_slice();
+///
+/// // UB!!! This mutates `rust_ref`'s referent while it's alive.
+/// *refcell.borrow_mut() = [0, 0, 0, 0];
+///
+/// println!("{:?}", rust_ref);
+/// ```
+#[allow(unused)]
+const REFCELL_REFMUT_INTO_SLICE: () = ();
+
+/// ```compile_fail
+/// use core::cell::{RefCell, RefMut};
+///
+/// let refcell = RefCell::new([0u8, 1, 2, 3]);
+/// let core_ref_mut = refcell.borrow_mut();
+/// let core_ref_mut = RefMut::map(core_ref_mut, |bytes| &mut bytes[..]);
+///
+/// // `zc_ref` now stores `core_ref_mut` internally.
+/// let zc_ref = zerocopy::Ref::<_, [u16]>::new_slice(core_ref_mut).unwrap();
+///
+/// // This causes `core_ref_mut` to get dropped and synthesizes a Rust
+/// // reference to the memory `core_ref` was pointing at.
+/// let rust_ref_mut = zc_ref.into_mut_slice();
+///
+/// // UB!!! This mutates `rust_ref_mut`'s referent while it's alive.
+/// *refcell.borrow_mut() = [0, 0, 0, 0];
+///
+/// println!("{:?}", rust_ref_mut);
+/// ```
+#[allow(unused)]
+const REFCELL_REFMUT_INTO_MUT_SLICE: () = ();

zerocopy-derive/Cargo.toml

@@ -9,7 +9,7 @@
 [package]
 edition = "2018"
 name = "zerocopy-derive"
-version = "0.8.0-alpha.1"
+version = "0.8.0-alpha.2"
 authors = ["Joshua Liebow-Feeser <joshlf@google.com>"]
 description = "Custom derive for traits from the zerocopy crate"
 license = "BSD-2-Clause OR Apache-2.0 OR MIT"