From dad47d5ca87595f11881657d377f595be471e65b Mon Sep 17 00:00:00 2001 From: Joshua Liebow-Feeser Date: Thu, 28 Dec 2023 13:13:48 -0800 Subject: [PATCH] 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 | 8 +- src/lib.rs | 103 +++++++++++---- ...ost_monomorphization_compile_fail_tests.rs | 118 ++++++++++++++++++ zerocopy-derive/Cargo.toml | 2 +- 4 files changed, 201 insertions(+), 30 deletions(-) create mode 100644 src/post_monomorphization_compile_fail_tests.rs diff --git a/Cargo.toml b/Cargo.toml index 248dee18f3..33c4aa5d70 100644 --- a/Cargo.toml +++ b/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 "] 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" diff --git a/src/lib.rs b/src/lib.rs index 94b7362cc2..4e4508c1b0 100644 --- a/src/lib.rs +++ b/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 + 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)) 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 9c69f0c379..1f1788234a 100644 --- a/zerocopy-derive/Cargo.toml +++ b/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 "] description = "Custom derive for traits from the zerocopy crate" license = "BSD-2-Clause OR Apache-2.0 OR MIT"