From 535e8b1eabdce29e32f326f536d92ad8b8cf4d98 Mon Sep 17 00:00:00 2001 From: Joshua Liebow-Feeser Date: Wed, 13 Dec 2023 15:46:17 -0800 Subject: [PATCH] Fix soundness hole in Ref::into_ref and into_mut (#721) 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.6.6. --- Cargo.toml | 4 +- src/lib.rs | 103 +++++++++++---- ...ost_monomorphization_compile_fail_tests.rs | 118 ++++++++++++++++++ zerocopy-derive/Cargo.toml | 2 +- 4 files changed, 199 insertions(+), 28 deletions(-) create mode 100644 src/post_monomorphization_compile_fail_tests.rs diff --git a/Cargo.toml b/Cargo.toml index 6950b914a0..4f55dbb82d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,7 +11,7 @@ [package] edition = "2021" name = "zerocopy" -version = "0.6.5" +version = "0.6.6" authors = ["Joshua Liebow-Feeser "] description = "Utilities for zero-copy parsing and serialization" license = "BSD-2-Clause" @@ -38,7 +38,7 @@ simd-nightly = ["simd"] __internal_use_only_features_that_work_on_stable = ["alloc", "simd"] [dependencies] -zerocopy-derive = { version = "=0.6.5", path = "zerocopy-derive" } +zerocopy-derive = { version = "=0.6.6", path = "zerocopy-derive" } [dependencies.byteorder] version = "1.3" diff --git a/src/lib.rs b/src/lib.rs index 801de6e79a..b2af1a86ba 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -128,6 +128,7 @@ pub mod byteorder; #[doc(hidden)] pub mod derive_util; +mod post_monomorphization_compile_fail_tests; pub use crate::byteorder::*; pub use zerocopy_derive::*; @@ -2081,12 +2082,12 @@ where /// `into_ref` consumes the `LayoutVerified`, and returns a reference to /// `T`. 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() } } } @@ -2101,12 +2102,13 @@ where /// `into_mut` consumes the `LayoutVerified`, and returns a mutable /// reference to `T`. 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() } } } @@ -2121,12 +2123,12 @@ where /// `into_slice` consumes the `LayoutVerified`, and returns a reference to /// `[T]`. 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() } } } @@ -2141,13 +2143,13 @@ where /// `into_mut_slice` consumes the `LayoutVerified`, and returns a mutable /// reference to `[T]`. 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() } } } @@ -2627,6 +2629,29 @@ mod sealed { /// [`Vec`]: alloc::vec::Vec /// [`split_at`]: crate::ByteSlice::split_at pub unsafe trait ByteSlice: Deref + Sized + self::sealed::Sealed { + /// 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 { @@ -2660,6 +2685,10 @@ pub unsafe trait ByteSliceMut: ByteSlice + DerefMut { // TODO(#61): 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) @@ -2669,6 +2698,10 @@ unsafe impl<'a> ByteSlice for &'a [u8] { // TODO(#61): 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) @@ -2678,6 +2711,16 @@ unsafe impl<'a> ByteSlice for &'a mut [u8] { // TODO(#61): Add a "SAFETY" comment and remove this `allow`. #[allow(clippy::undocumented_unsafe_blocks)] unsafe impl<'a> ByteSlice for 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) { Ref::map_split(self, |slice| <[u8]>::split_at(slice, mid)) @@ -2687,6 +2730,16 @@ unsafe impl<'a> ByteSlice for Ref<'a, [u8]> { // TODO(#61): 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)) diff --git a/src/post_monomorphization_compile_fail_tests.rs b/src/post_monomorphization_compile_fail_tests.rs new file mode 100644 index 0000000000..32505b6693 --- /dev/null +++ b/src/post_monomorphization_compile_fail_tests.rs @@ -0,0 +1,118 @@ +// Copyright 2018 The Fuchsia Authors +// +// Licensed under the 2-Clause BSD License , Apache License, Version 2.0 +// , or the MIT +// license , 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: () = (); diff --git a/zerocopy-derive/Cargo.toml b/zerocopy-derive/Cargo.toml index c830efd1ec..471634ba20 100644 --- a/zerocopy-derive/Cargo.toml +++ b/zerocopy-derive/Cargo.toml @@ -5,7 +5,7 @@ [package] edition = "2021" name = "zerocopy-derive" -version = "0.6.5" +version = "0.6.6" authors = ["Joshua Liebow-Feeser "] description = "Custom derive for traits from the zerocopy crate" license = "BSD-2-Clause"