Skip to content

Commit

Permalink
Fix soundness hole in Ref::into_ref and into_mut (#721) (#755)
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.

Release 0.8.0-alpha.2.
  • Loading branch information
joshlf authored Dec 28, 2023
1 parent e56a42b commit aaac106
Show file tree
Hide file tree
Showing 4 changed files with 201 additions and 30 deletions.
8 changes: 4 additions & 4 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand All @@ -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"
Expand All @@ -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"
103 changes: 78 additions & 25 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() }
}
}
Expand All @@ -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() }
}
}
Expand All @@ -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() }
}
}
Expand All @@ -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() }
}
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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))
Expand All @@ -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))
Expand Down
118 changes: 118 additions & 0 deletions src/post_monomorphization_compile_fail_tests.rs
Original file line number Diff line number Diff line change
@@ -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: () = ();
2 changes: 1 addition & 1 deletion zerocopy-derive/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit aaac106

Please sign in to comment.