[pointer] Relax UnsafeCell requirements (#1211)

Previously, we required there to always be "`UnsafeCell` agreement"
between any `Ptr`s or references referencing a given region of memory.
This was based on an overly-strict interpretation of the semantics of
`UnsafeCell`.

In this commit, we relax `Ptr` to only require "`UnsafeCell` agreement"
when the aliasing model is `Shared`. All of the places that our internal
invariants are "consumed" - ie, where we use them as safety
preconditions for calling unsafe functions defined outside our crate -
this relaxation is sufficient.

This is based on what we (@jswrenn and I) believe to be a more accurate
model of the semantics of `UnsafeCell`s. In particular, `UnsafeCell`s do
not affect the semantics of loads or stores in Rust. All they do is
affect the semantics of shared references. In particular, Rust assumes
that the referent of a shared reference will not be stored to during the
lifetime of any shared reference, but this assumption is not made for
bytes which are covered by an `UnsafeCell`.

This is entirely a runtime property. If two references refer to the same
memory, but disagree on whether that memory is covered by an
`UnsafeCell`, this results in UB if the `UnsafeCell` is used to store to
the memory, violating the expectations of the non-`UnsafeCell` shared
reference. This commit is consistent with the runtime nature of this
property, but is inconsistent with Stacked Borrows
(rust-lang/unsafe-code-guidelines#455). However, this is considered to
be a bug in Stacked Borrows.

src/lib.rs

@@ -4428,12 +4428,17 @@ unsafe impl<T: TryFromBytes> TryFromBytes for UnsafeCell<T> {
         // We wrap in `Unalign` here so that we can get a vanilla Rust reference
         // below, which in turn allows us to call `UnsafeCell::get_mut`.
         //
-        // SAFETY: `Unalign` and `MaybeUninit` both have the same size as the
-        // types they wrap [1]. Thus, this cast will preserve the size of the
-        // pointer. Since both the source and destination types are wrapped in
-        // `UnsafeCell`, all bytes of both types are inside of `UnsafeCell`s,
-        // and so the byte ranges covered by `UnsafeCell`s are identical in both
-        // types.
+        // SAFETY:
+        // - `.cast` preserves address. `Unalign` and `MaybeUninit` both have
+        //   the same size as the types they wrap [1]. Thus, this cast will
+        //   preserve the size of the pointer. As a result, the cast will
+        //   address the same bytes as `c`.
+        // - `.cast` preserves provenance.
+        // - Since both the source and destination types are wrapped in
+        //   `UnsafeCell`, all bytes of both types are inside of `UnsafeCell`s,
+        //   and so the byte ranges covered by `UnsafeCell`s are identical in
+        //   both types. Since the pointers refer to the same byte ranges,
+        //   the same is true of the pointers' referents as well.
         //
         // [1] Per https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#layout-1:
         //

src/macros.rs

@@ -142,10 +142,11 @@ macro_rules! unsafe_impl {
         #[inline]
         fn is_bit_valid<AA: invariant::Aliasing + invariant::AtLeast<invariant::Shared>>(candidate: Maybe<'_, Self, AA>) -> bool {
             // SAFETY:
-            // - The argument to `cast_unsized` is `|p| p as *mut _` as required
-            //   by that method's safety precondition.
-            // - The caller has promised that the cast results in an object of
-            //   equal or lesser size.
+            // - The cast preserves address. The caller has promised that the
+            //   cast results in an object of equal or lesser size, and so the
+            //   cast returns a pointer which references a subset of the bytes
+            //   of `p`.
+            // - The cast preserves provenance.
             // - The caller has promised that the destination type has
             //   `UnsafeCell`s at the same byte ranges as the source type.
             #[allow(clippy::as_conversions)]
@@ -164,10 +165,11 @@ macro_rules! unsafe_impl {
         #[inline]
         fn is_bit_valid<AA: invariant::Aliasing + invariant::AtLeast<invariant::Shared>>(candidate: Maybe<'_, Self, AA>) -> bool {
             // SAFETY:
-            // - The argument to `cast_unsized` is `|p| p as *mut _` as required
-            //   by that method's safety precondition.
-            // - The caller has promised that the cast results in an object of
-            //   equal or lesser size.
+            // - The cast preserves address. The caller has promised that the
+            //   cast results in an object of equal or lesser size, and so the
+            //   cast returns a pointer which references a subset of the bytes
+            //   of `p`.
+            // - The cast preserves provenance.
             // - The caller has promised that the destination type has
             //   `UnsafeCell`s at the same byte ranges as the source type.
             #[allow(clippy::as_conversions)]

src/util.rs

@@ -28,10 +28,8 @@ use crate::{
 ///
 /// `T: TransparentWrapper` implies that `T` has the same size as [`T::Inner`].
 /// Further, `T: TransparentWrapper<I>` implies that:
-/// - If `T::UnsafeCellVariance = Covariant`, then:
-///   - `T` has `UnsafeCell`s covering the same byte ranges as `T::Inner`
-///   - `T` has zero-sized `UnsafeCell`s (e.g., `UnsafeCell<()>`,
-///     `[UnsafeCell<u8>; 0]`, etc) at the same byte offsets as `T::Inner`
+/// - If `T::UnsafeCellVariance = Covariant`, then `T` has `UnsafeCell`s
+///   covering the same byte ranges as `T::Inner`.
 /// - If a `T` pointer satisfies the alignment invariant `I::Alignment`, then
 ///   that same pointer, cast to `T::Inner`, satisfies the alignment invariant
 ///   `<T::AlignmentVariance as AlignmentVariance<I::Alignment>>::Applied`.
@@ -112,8 +110,8 @@ impl<I: invariant::Validity> ValidityVariance<I> for Invariant {
 unsafe impl<T, I: Invariants> TransparentWrapper<I> for MaybeUninit<T> {
     type Inner = T;
 
-    // SAFETY: Per [1], `MaybeUninit<T>` has `UnsafeCell`s at the same byte
-    // ranges as `Inner = T`, and `UnsafeCell`s at the same byte offsets as `T`.
+    // SAFETY: Per [1], `MaybeUninit<T>` has `UnsafeCell`s covering the same
+    // byte ranges as `Inner = T`.
     //
     // [1] TODO(#896): Write a safety proof before the next stable release.
     type UnsafeCellVariance = Covariant;
@@ -154,8 +152,8 @@ unsafe impl<T, I: Invariants> TransparentWrapper<I> for MaybeUninit<T> {
 unsafe impl<T: ?Sized, I: Invariants> TransparentWrapper<I> for ManuallyDrop<T> {
     type Inner = T;
 
-    // SAFETY: Per [1], `ManuallyDrop<T>` has `UnsafeCell`s at the same byte
-    // ranges as `Inner = T`, and `UnsafeCell`s at the same byte offsets as `T`.
+    // SAFETY: Per [1], `ManuallyDrop<T>` has `UnsafeCell`s covering the same
+    // byte ranges as `Inner = T`.
     //
     // [1] TODO(#896): Write a safety proof before the next stable release.
     type UnsafeCellVariance = Covariant;
@@ -199,8 +197,8 @@ unsafe impl<T: ?Sized, I: Invariants> TransparentWrapper<I> for ManuallyDrop<T>
 unsafe impl<T, I: Invariants> TransparentWrapper<I> for Wrapping<T> {
     type Inner = T;
 
-    // SAFETY: Per [1], `Wrapping<T>` has `UnsafeCell`s at the same byte ranges
-    // as `Inner = T`, and `UnsafeCell`s at the same byte offsets as `T`.
+    // SAFETY: Per [1], `Wrapping<T>` has `UnsafeCell`s covering the same byte
+    // ranges as `Inner = T`.
     //
     // [1] TODO(#896): Write a safety proof before the next stable release.
     type UnsafeCellVariance = Covariant;
@@ -249,8 +247,8 @@ unsafe impl<T, I: Invariants> TransparentWrapper<I> for Wrapping<T> {
 //
 // [1] Per https://doc.rust-lang.org/core/cell/struct.UnsafeCell.html#memory-layout:
 //
-//   `UnsafeCell<T>`` has the same in-memory representation as its inner
-//   type `T`.
+//   `UnsafeCell<T>` has the same in-memory representation as its inner type
+//   `T`.
 unsafe impl<T: ?Sized, I: Invariants> TransparentWrapper<I> for UnsafeCell<T> {
     type Inner = T;
 
@@ -268,9 +266,9 @@ unsafe impl<T: ?Sized, I: Invariants> TransparentWrapper<I> for UnsafeCell<T> {
     //
     // [1] Per https://doc.rust-lang.org/std/cell/struct.UnsafeCell.html#memory-layout:
     //
-    //   `UnsafeCell<T>`` has the same in-memory representation as its inner
-    //   type `T`. A consequence of this guarantee is that it is possible to
-    //   convert between `T` and `UnsafeCell<T>`.
+    //   `UnsafeCell<T>` has the same in-memory representation as its inner type
+    //   `T`. A consequence of this guarantee is that it is possible to convert
+    //   between `T` and `UnsafeCell<T>`.
     type ValidityVariance = Covariant;
 
     fn cast_into_inner(ptr: *mut UnsafeCell<T>) -> *mut T {
@@ -292,25 +290,22 @@ unsafe impl<T: ?Sized, I: Invariants> TransparentWrapper<I> for UnsafeCell<T> {
     }
 }
 
-// SAFETY: We define `Unalign<T>` to be a `#[repr(C, packed)]` type wrapping a
-// single `T` field. Thus, `Unalign<T>` has the same size as `T`.
+// SAFETY: `Unalign<T>` promises to have the same size as `T`.
 //
 // See inline comments for other safety justifications.
 unsafe impl<T, I: Invariants> TransparentWrapper<I> for Unalign<T> {
     type Inner = T;
 
-    // SAFETY: `Unalign<T>` is a `#[repr(C, packed)]` type wrapping a single `T`
-    // field, and so has `UnsafeCell`s at the same byte ranges as `Inner = T`,
-    // and `UnsafeCell`s at the same byte offsets as `T`.
+    // SAFETY: `Unalign<T>` promises to have `UnsafeCell`s covering the same
+    // byte ranges as `Inner = T`.
     type UnsafeCellVariance = Covariant;
 
-    // SAFETY: Since `Unalign<T>` is `repr(packed)`, it has the alignment 1
-    // regardless of `T`'s alignment. Thus, an aligned pointer to `Unalign<T>`
-    // is not necessarily an aligned pointer to `T`.
+    // SAFETY: Since `Unalign<T>` promises to have alignment 1 regardless of
+    // `T`'s alignment. Thus, an aligned pointer to `Unalign<T>` is not
+    // necessarily an aligned pointer to `T`.
     type AlignmentVariance = Invariant;
 
-    // SAFETY: `Unalign<T>` is a `#[repr(C, packed)]` type wrapping a single `T`
-    // field, and so has the same validity as `T`.
+    // SAFETY: `Unalign<T>` promises to have the same validity as `T`.
     type ValidityVariance = Covariant;
 
     fn cast_into_inner(ptr: *mut Unalign<T>) -> *mut T {

src/wrappers.rs

@@ -39,6 +39,12 @@ use super::*;
 /// [`try_deref_mut`]: Unalign::try_deref_mut
 /// [`deref_unchecked`]: Unalign::deref_unchecked
 /// [`deref_mut_unchecked`]: Unalign::deref_mut_unchecked
+///
+/// # Safety
+///
+/// `Unalign<T>` is guaranteed to have the same size and bit validity as `T`,
+/// and to have [`UnsafeCell`]s covering the same byte ranges as `T`.
+/// `Unalign<T>` is guaranteed to have alignment 1.
 // NOTE: This type is sound to use with types that need to be dropped. The
 // reason is that the compiler-generated drop code automatically moves all
 // values to aligned memory slots before dropping them in-place. This is not
@@ -63,8 +69,8 @@ impl_known_layout!(T => Unalign<T>);
 
 safety_comment! {
     /// SAFETY:
-    /// - `Unalign<T>` is `repr(packed)`, so it is unaligned regardless of the
-    ///   alignment of `T`, and so we don't require that `T: Unaligned`
+    /// - `Unalign<T>` promises to have alignment 1, and so we don't require
+    ///   that `T: Unaligned`.
     /// - `Unalign<T>` has the same bit validity as `T`, and so it is
     ///   `FromZeros`, `FromBytes`, or `IntoBytes` exactly when `T` is as well.
     /// - `Immutable`: `Unalign<T>` has the same fields as `T`, so it contains

zerocopy-derive/src/lib.rs

@@ -395,11 +395,13 @@ fn derive_try_from_bytes_struct(ast: &DeriveInput, strct: &DataStruct) -> proc_m
                 mut candidate: ::zerocopy::Maybe<Self, A>
             ) -> bool {
                 true #(&& {
-                    // SAFETY: `project` is a field projection of `candidate`,
-                    // and `Self` is a struct type. The candidate will have
-                    // `UnsafeCell`s at exactly the same ranges as its
-                    // projection, because the projection is a field of the
-                    // candidate struct.
+                    // SAFETY:
+                    // - `project` is a field projection, and so it addresses a
+                    //   subset of the bytes addressed by `slf`
+                    // - ..., and so it preserves provenance
+                    // - ..., and `*slf` is a struct, so `UnsafeCell`s exist at
+                    //   the same byte ranges in the returned pointer's referent
+                    //   as they do in `*slf`
                     let field_candidate = unsafe {
                         let project = |slf: *mut Self|
                             ::zerocopy::macro_util::core_reexport::ptr::addr_of_mut!((*slf).#field_names);
@@ -444,11 +446,13 @@ fn derive_try_from_bytes_union(ast: &DeriveInput, unn: &DataUnion) -> proc_macro
                 mut candidate: ::zerocopy::Maybe<Self, A>
             ) -> bool {
                 false #(|| {
-                    // SAFETY: `project` is a field projection of `candidate`,
-                    // and `Self` is a union type. The candidate and projection
-                    // agree exactly on where their `UnsafeCell` ranges are,
-                    // because `Self: Immutable` is enforced by
-                    // `self_type_trait_bounds`.
+                    // SAFETY:
+                    // - `project` is a field projection, and so it addresses a
+                    //   subset of the bytes addressed by `slf`
+                    // - ..., and so it preserves provenance
+                    // - Since `Self: Immutable` is enforced by
+                    //   `self_type_trait_bounds`, neither `*slf` nor the
+                    //   returned pointer's referent contain any `UnsafeCell`s
                     let field_candidate = unsafe {
                         let project = |slf: *mut Self|
                             ::zerocopy::macro_util::core_reexport::ptr::addr_of_mut!((*slf).#field_names);
@@ -514,9 +518,11 @@ fn derive_try_from_bytes_enum(ast: &DeriveInput, enm: &DataEnum) -> proc_macro2:
         quote!(
             use ::zerocopy::macro_util::core_reexport;
             // SAFETY:
-            // - `cast` is implemented as required.
-            // - By definition, `*mut Self` and `*mut [u8; size_of::<Self>()]`
-            //   are types of the same size.
+            // - The closure is a pointer cast, and `Self` and `[u8;
+            //   size_of::<Self>()]` have the same size, so the returned pointer
+            //   addresses the same bytes as `p` subset of the bytes addressed
+            //   by `slf`
+            // - ..., and so it preserves provenance
             // - Since we validate that this type is a field-less enum, it
             //   cannot contain any `UnsafeCell`s. Neither does `[u8; N]`.
             let discriminant = unsafe { candidate.cast_unsized(|p: *mut Self| p as *mut [core_reexport::primitive::u8; core_reexport::mem::size_of::<Self>()]) };

zerocopy-derive/tests/struct_try_from_bytes.rs

@@ -74,11 +74,10 @@ fn two_bad() {
     let candidate = unsafe { candidate.assume_initialized() };
 
     // SAFETY:
-    // - The cast `cast(p)` is implemented exactly as follows: `|p: *mut T| p as
-    //   *mut U`.
-    // - The size of the object referenced by the resulting pointer is equal to
-    //   the size of the object referenced by `self`.
-    // - `Two` does not contain any `UnsafeCell`s.
+    // - The cast preserves address and size. As a result, the cast will address
+    //   the same bytes as `c`.
+    // - The cast preserves provenance.
+    // - Neither the input nor output types contain any `UnsafeCell`s.
     let candidate = unsafe { candidate.cast_unsized(|p| p as *mut Two) };
 
     // SAFETY: `candidate`'s referent is as-initialized as `Two`.
@@ -105,12 +104,10 @@ fn un_sized() {
     let candidate = unsafe { candidate.assume_initialized() };
 
     // SAFETY:
-    // - The cast `cast(p)` is implemented exactly as follows: `|p: *mut T| p as
-    //   *mut U`.
-    // - The size of the object referenced by the resulting pointer is equal to
-    //   the size of the object referenced by `self`.
-    // - The alignment of `Unsized` is equal to the alignment of `[u8]`.
-    // - `Unsized` does not contain any `UnsafeCell`s.
+    // - The cast preserves address and size. As a result, the cast will address
+    //   the same bytes as `c`.
+    // - The cast preserves provenance.
+    // - Neither the input nor output types contain any `UnsafeCell`s.
     let candidate = unsafe { candidate.cast_unsized(|p| p as *mut Unsized) };
 
     // SAFETY: `candidate`'s referent is as-initialized as `Two`.

zerocopy-derive/tests/union_try_from_bytes.rs

@@ -69,11 +69,10 @@ fn two_bad() {
     let candidate = unsafe { candidate.assume_initialized() };
 
     // SAFETY:
-    // - The cast `cast(p)` is implemented exactly as follows: `|p: *mut T| p as
-    //   *mut U`.
-    // - The size of the object referenced by the resulting pointer is equal to
-    //   the size of the object referenced by `self`.
-    // - `Two` does not contain any `UnsafeCell`s.
+    // - The cast preserves address and size. As a result, the cast will address
+    //   the same bytes as `c`.
+    // - The cast preserves provenance.
+    // - Neither the input nor output types contain any `UnsafeCell`s.
     let candidate = unsafe { candidate.cast_unsized(|p| p as *mut Two) };
 
     // SAFETY: `candidate`'s referent is as-initialized as `Two`.
@@ -99,11 +98,10 @@ fn bool_and_zst() {
     let candidate = unsafe { candidate.assume_initialized() };
 
     // SAFETY:
-    // - The cast `cast(p)` is implemented exactly as follows: `|p: *mut T| p as
-    //   *mut U`.
-    // - The size of the object referenced by the resulting pointer is equal to
-    //   the size of the object referenced by `self`.
-    // - `BoolAndZst` does not contain any `UnsafeCell`s.
+    // - The cast preserves address and size. As a result, the cast will address
+    //   the same bytes as `c`.
+    // - The cast preserves provenance.
+    // - Neither the input nor output types contain any `UnsafeCell`s.
     let candidate = unsafe { candidate.cast_unsized(|p| p as *mut BoolAndZst) };
 
     // SAFETY: `candidate`'s referent is fully initialized.