[pointer] Improve soundness of invariant modeling (#2397)

This commit makes the following improvements:
- Removes the `Inaccessible` aliasing mode. This mode was not unsound,
  but it was unnecessary in practice.
- Replaces `Unknown` with `Unaligned` for alignment.
- Replaces `Unknown` with `Uninit` for validity.

Finally, with the exception of `transparent_wrapper_into_inner`, this
commit ensures that all `Ptr` methods which modify the type or validity
of a `Ptr` are sound.

Previously, we modeled validity as "knowledge" about the `Ptr`'s
referent (see #1866 for a more in-depth explanation). In particular, we
assumed that it was always sound to "forget" information about a `Ptr`'s
referent in the same way in which it is sound to "forget" that a `Ptr`
is validly-aligned, converting a `Ptr<T, (_, Aligned, _)>` to a
`Ptr<T, (_, Unaligned, _)>`.

The problem with this approach is that validity doesn't just specify
what bit patterns can be expected to be read from a `Ptr`'s referent, it
also specifies what bit patterns are permitted to be *written* to the
referent. Thus, "forgetting" about validity and expanding the set of
expected bit patterns also expands the set of bit patterns which can be
written.

Consider, for example, "forgetting" that a `Ptr<bool, (_, _, Valid)>` is
bit-valid, and downgrading it to a `Ptr<bool, (_, _, Initialized)>`. If
the aliasing is `Exclusive`, then the resulting `Ptr` would permit
writing arbitrary `u8`s to the referent, violating the bit validity of
the `bool`.

This commit moves us in the direction of a new model, in which changes
to the referent type or the validity of a `Ptr` must abide by the
following rules:
- If the resulting `Ptr` permits mutation of its referent (either via
  interior mutation or `Exclusive` aliasing), then the set of allowed
  bit patterns in the referent may not expand
- If the original `Ptr` permits mutation of its referent while the
  resulting `Ptr` is also live (i.e., via interior mutation on a
  `Shared` `Ptr`), then the set of allowed bit patterns in the referent
  may not shrink

This commit does not fix `transparent_wrapper_into_inner`, which will
require an overhaul or replacement of the `TransparentWrapper` trait.

Makes progress on #2226, #1866


gherrit-pr-id: I95d6c5cd23eb5ea6629cd6e4b99696913b1ded93

Co-authored-by: Jack Wrenn <[email protected]>

src/pointer/invariant.rs

@@ -52,19 +52,6 @@ pub trait Validity: Sealed {}
 /// Exclusive`.
 pub trait Reference: Aliasing + Sealed {}
 
-/// It is unknown whether any invariant holds.
-pub enum Unknown {}
-
-impl Alignment for Unknown {}
-impl Validity for Unknown {}
-
-/// The `Ptr<'a, T>` does not permit any reads or writes from or to its referent.
-pub enum Inaccessible {}
-
-impl Aliasing for Inaccessible {
-    const IS_EXCLUSIVE: bool = false;
-}
-
 /// The `Ptr<'a, T>` adheres to the aliasing rules of a `&'a T`.
 ///
 /// The referent of a shared-aliased `Ptr` may be concurrently referenced by any
@@ -90,11 +77,21 @@ impl Aliasing for Exclusive {
 }
 impl Reference for Exclusive {}
 
+/// It is unknown whether the pointer is aligned.
+pub enum Unaligned {}
+
+impl Alignment for Unaligned {}
+
 /// The referent is aligned: for `Ptr<T>`, the referent's address is a multiple
 /// of the `T`'s alignment.
 pub enum Aligned {}
 impl Alignment for Aligned {}
 
+/// Any bit pattern is allowed in the `Ptr`'s referent, including uninitialized
+/// bytes.
+pub enum Uninit {}
+impl Validity for Uninit {}
+
 /// The byte ranges initialized in `T` are also initialized in the referent.
 ///
 /// Formally: uninitialized bytes may only be present in `Ptr<T>`'s referent
@@ -133,6 +130,17 @@ impl Validity for Initialized {}
 pub enum Valid {}
 impl Validity for Valid {}
 
+/// # Safety
+///
+/// `DT: CastableFrom<ST, SV, DV>` is sound if `SV = DV = Uninit` or `SV = DV =
+/// Initialized`.
+pub unsafe trait CastableFrom<ST: ?Sized, SV, DV> {}
+
+// SAFETY: `SV = DV = Uninit`.
+unsafe impl<ST: ?Sized, DT: ?Sized> CastableFrom<ST, Uninit, Uninit> for DT {}
+// SAFETY: `SV = DV = Initialized`.
+unsafe impl<ST: ?Sized, DT: ?Sized> CastableFrom<ST, Initialized, Initialized> for DT {}
+
 /// [`Ptr`](crate::Ptr) referents that permit unsynchronized read operations.
 ///
 /// `T: Read<A, R>` implies that a pointer to `T` with aliasing `A` permits
@@ -175,14 +183,13 @@ mod sealed {
 
     pub trait Sealed {}
 
-    impl Sealed for Unknown {}
-
-    impl Sealed for Inaccessible {}
     impl Sealed for Shared {}
     impl Sealed for Exclusive {}
 
+    impl Sealed for Unaligned {}
     impl Sealed for Aligned {}
 
+    impl Sealed for Uninit {}
     impl Sealed for AsInitialized {}
     impl Sealed for Initialized {}
     impl Sealed for Valid {}

src/pointer/mod.rs

@@ -24,14 +24,14 @@ use crate::Unaligned;
 /// to [`TryFromBytes::is_bit_valid`].
 ///
 /// [`TryFromBytes::is_bit_valid`]: crate::TryFromBytes::is_bit_valid
-pub type Maybe<'a, T, Aliasing = invariant::Shared, Alignment = invariant::Unknown> =
+pub type Maybe<'a, T, Aliasing = invariant::Shared, Alignment = invariant::Unaligned> =
     Ptr<'a, T, (Aliasing, Alignment, invariant::Initialized)>;
 
 /// A semi-user-facing wrapper type representing a maybe-aligned reference, for
 /// use in [`TryFromBytes::is_bit_valid`].
 ///
 /// [`TryFromBytes::is_bit_valid`]: crate::TryFromBytes::is_bit_valid
-pub type MaybeAligned<'a, T, Aliasing = invariant::Shared, Alignment = invariant::Unknown> =
+pub type MaybeAligned<'a, T, Aliasing = invariant::Shared, Alignment = invariant::Unaligned> =
     Ptr<'a, T, (Aliasing, Alignment, invariant::Valid)>;
 
 // These methods are defined on the type alias, `MaybeAligned`, so as to bring

src/util/macro_util.rs

@@ -26,7 +26,7 @@ use core::ptr::{self, NonNull};
 
 use crate::{
     pointer::invariant::{self, BecauseExclusive, BecauseImmutable, Invariants, ReadReason},
-    FromBytes, Immutable, IntoBytes, Ptr, TryFromBytes, Unalign, ValidityError,
+    FromBytes, Immutable, IntoBytes, Ptr, TryFromBytes, ValidityError,
 };
 
 /// Projects the type of the field at `Index` in `Self`.
@@ -529,10 +529,15 @@ pub unsafe fn transmute_mut<'dst, 'src: 'dst, Src: 'src, Dst: 'dst>(
 ///
 /// # Safety
 ///
-/// Unsafe code may assume that, if `try_cast_or_pme(src)` returns `Some`,
+/// Unsafe code may assume that, if `try_cast_or_pme(src)` returns `Ok`,
 /// `*src` is a bit-valid instance of `Dst`, and that the size of `Src` is
 /// greater than or equal to the size of `Dst`.
 ///
+/// Unsafe code may assume that, if `try_cast_or_pme(src)` returns `Err`, the
+/// encapsulated `Ptr` value is the original `src`. `try_cast_or_pme` cannot
+/// guarantee that the referent has not been modified, as it calls user-defined
+/// code (`TryFromBytes::is_bit_valid`).
+///
 /// # Panics
 ///
 /// `try_cast_or_pme` may either produce a post-monomorphization error or a
@@ -545,15 +550,15 @@ pub unsafe fn transmute_mut<'dst, 'src: 'dst, Src: 'src, Dst: 'dst>(
 fn try_cast_or_pme<Src, Dst, I, R>(
     src: Ptr<'_, Src, I>,
 ) -> Result<
-    Ptr<'_, Dst, (I::Aliasing, invariant::Unknown, invariant::Valid)>,
+    Ptr<'_, Dst, (I::Aliasing, invariant::Unaligned, invariant::Valid)>,
     ValidityError<Ptr<'_, Src, I>, Dst>,
 >
 where
     // TODO(#2226): There should be a `Src: FromBytes` bound here, but doing so
     // requires deeper surgery.
-    Src: IntoBytes + invariant::Read<I::Aliasing, R>,
+    Src: invariant::Read<I::Aliasing, R>,
     Dst: TryFromBytes + invariant::Read<I::Aliasing, R>,
-    I: Invariants<Validity = invariant::Valid>,
+    I: Invariants<Validity = invariant::Initialized>,
     I::Aliasing: invariant::Reference,
     R: ReadReason,
 {
@@ -567,11 +572,6 @@ where
     #[allow(clippy::as_conversions)]
     let c_ptr = unsafe { src.cast_unsized(|p| p as *mut Dst) };
 
-    // SAFETY: `c_ptr` is derived from `src` which is `IntoBytes`. By
-    // invariant on `IntoByte`s, `c_ptr`'s referent consists entirely of
-    // initialized bytes.
-    let c_ptr = unsafe { c_ptr.assume_initialized() };
-
     match c_ptr.try_into_valid() {
         Ok(ptr) => Ok(ptr),
         Err(err) => {
@@ -611,19 +611,44 @@ where
     Src: IntoBytes,
     Dst: TryFromBytes,
 {
-    let mut src = ManuallyDrop::new(src);
-    let ptr = Ptr::from_mut(&mut src);
-    // Wrapping `Dst` in `Unalign` ensures that this cast does not fail due to
-    // alignment requirements.
-    match try_cast_or_pme::<_, ManuallyDrop<Unalign<Dst>>, _, BecauseExclusive>(ptr) {
-        Ok(ptr) => {
-            let dst = ptr.bikeshed_recall_aligned().as_mut();
-            // SAFETY: By shadowing `dst`, we ensure that `dst` is not re-used
-            // after taking its inner value.
-            let dst = unsafe { ManuallyDrop::take(dst) };
-            Ok(dst.into_inner())
-        }
-        Err(_) => Err(ValidityError::new(ManuallyDrop::into_inner(src))),
+    static_assert!(Src, Dst => mem::size_of::<Dst>() == mem::size_of::<Src>());
+
+    let mu_src = mem::MaybeUninit::new(src);
+    // SAFETY: By invariant on `&`, the following are satisfied:
+    // - `&mu_src` is valid for reads
+    // - `&mu_src` is properly aligned
+    // - `&mu_src`'s referent is bit-valid
+    let mu_src_copy = unsafe { core::ptr::read(&mu_src) };
+    // SAFETY: `MaybeUninit` has no validity constraints.
+    let mut mu_dst: mem::MaybeUninit<Dst> =
+        unsafe { crate::util::transmute_unchecked(mu_src_copy) };
+
+    let ptr = Ptr::from_mut(&mut mu_dst);
+
+    // SAFETY: Since `Src: IntoBytes`, and since `size_of::<Src>() ==
+    // size_of::<Dst>()` by the preceding assertion, all of `mu_dst`'s bytes are
+    // initialized.
+    let ptr = unsafe { ptr.assume_validity::<invariant::Initialized>() };
+
+    // SAFETY: `MaybeUninit<T>` and `T` have the same size [1], so this cast
+    // preserves the referent's size. This cast preserves provenance.
+    //
+    // [1] Per https://doc.rust-lang.org/1.81.0/std/mem/union.MaybeUninit.html#layout-1:
+    //
+    //   `MaybeUninit<T>` is guaranteed to have the same size, alignment, and
+    //   ABI as `T`
+    let ptr: Ptr<'_, Dst, _> =
+        unsafe { ptr.cast_unsized(|mu: *mut mem::MaybeUninit<Dst>| mu.cast()) };
+
+    if Dst::is_bit_valid(ptr.forget_aligned()) {
+        // SAFETY: Since `Dst::is_bit_valid`, we know that `ptr`'s referent is
+        // bit-valid for `Dst`. `ptr` points to `mu_dst`, and no intervening
+        // operations have mutated it, so it is a bit-valid `Dst`.
+        Ok(unsafe { mu_dst.assume_init() })
+    } else {
+        // SAFETY: `mu_src` was constructed from `src` and never modified, so it
+        // is still bit-valid.
+        Err(ValidityError::new(unsafe { mu_src.assume_init() }))
     }
 }
 
@@ -645,15 +670,29 @@ where
     Src: IntoBytes + Immutable,
     Dst: TryFromBytes + Immutable,
 {
-    match try_cast_or_pme::<Src, Dst, _, BecauseImmutable>(Ptr::from_ref(src)) {
+    let ptr = Ptr::from_ref(src);
+    let ptr = ptr.bikeshed_recall_initialized_immutable();
+    match try_cast_or_pme::<Src, Dst, _, BecauseImmutable>(ptr) {
         Ok(ptr) => {
             static_assert!(Src, Dst => mem::align_of::<Dst>() <= mem::align_of::<Src>());
             // SAFETY: We have checked that `Dst` does not have a stricter
             // alignment requirement than `Src`.
             let ptr = unsafe { ptr.assume_alignment::<invariant::Aligned>() };
             Ok(ptr.as_ref())
         }
-        Err(err) => Err(err.map_src(Ptr::as_ref)),
+        Err(err) => Err(err.map_src(|ptr| {
+            // SAFETY: Because `Src: Immutable` and we create a `Ptr` via
+            // `Ptr::from_ref`, the resulting `Ptr` is a shared-and-`Immutable`
+            // `Ptr`, which does not permit mutation of its referent. Therefore,
+            // no mutation could have happened during the call to
+            // `try_cast_or_pme` (any such mutation would be unsound).
+            //
+            // `try_cast_or_pme` promises to return its original argument, and
+            // so we know that we are getting back the same `ptr` that we
+            // originally passed, and that `ptr` was a bit-valid `Src`.
+            let ptr = unsafe { ptr.assume_valid() };
+            ptr.as_ref()
+        })),
     }
 }
 
@@ -675,15 +714,17 @@ where
     Src: FromBytes + IntoBytes,
     Dst: TryFromBytes + IntoBytes,
 {
-    match try_cast_or_pme::<Src, Dst, _, BecauseExclusive>(Ptr::from_mut(src)) {
+    let ptr = Ptr::from_mut(src);
+    let ptr = ptr.bikeshed_recall_initialized_from_bytes();
+    match try_cast_or_pme::<Src, Dst, _, BecauseExclusive>(ptr) {
         Ok(ptr) => {
             static_assert!(Src, Dst => mem::align_of::<Dst>() <= mem::align_of::<Src>());
             // SAFETY: We have checked that `Dst` does not have a stricter
             // alignment requirement than `Src`.
             let ptr = unsafe { ptr.assume_alignment::<invariant::Aligned>() };
             Ok(ptr.as_mut())
         }
-        Err(err) => Err(err.map_src(Ptr::as_mut)),
+        Err(err) => Err(err.map_src(|ptr| ptr.bikeshed_recall_valid().as_mut())),
     }
 }
 

src/util/mod.rs

@@ -99,11 +99,11 @@ impl<I: invariant::Validity> ValidityVariance<I> for Covariant {
 pub enum Invariant {}
 
 impl<I: invariant::Alignment> AlignmentVariance<I> for Invariant {
-    type Applied = invariant::Unknown;
+    type Applied = invariant::Unaligned;
 }
 
 impl<I: invariant::Validity> ValidityVariance<I> for Invariant {
-    type Applied = invariant::Unknown;
+    type Applied = invariant::Uninit;
 }
 
 // SAFETY:

tests/ui-msrv/diagnostic-not-implemented-unaligned.stderr

@@ -1,8 +1,8 @@
-error[E0277]: the trait bound `NotZerocopy: Unaligned` is not satisfied
+error[E0277]: the trait bound `NotZerocopy: zerocopy::Unaligned` is not satisfied
   --> tests/ui-msrv/diagnostic-not-implemented-unaligned.rs:18:23
    |
 18 |     takes_unaligned::<NotZerocopy>();
-   |                       ^^^^^^^^^^^ the trait `Unaligned` is not implemented for `NotZerocopy`
+   |                       ^^^^^^^^^^^ the trait `zerocopy::Unaligned` is not implemented for `NotZerocopy`
    |
 note: required by a bound in `takes_unaligned`
   --> tests/ui-msrv/diagnostic-not-implemented-unaligned.rs:21:23

tests/ui-nightly/diagnostic-not-implemented-unaligned.stderr

@@ -1,11 +1,11 @@
-error[E0277]: the trait bound `NotZerocopy: Unaligned` is not satisfied
+error[E0277]: the trait bound `NotZerocopy: zerocopy::Unaligned` is not satisfied
   --> tests/ui-nightly/diagnostic-not-implemented-unaligned.rs:18:23
    |
 18 |     takes_unaligned::<NotZerocopy>();
-   |                       ^^^^^^^^^^^ the trait `Unaligned` is not implemented for `NotZerocopy`
+   |                       ^^^^^^^^^^^ the trait `zerocopy::Unaligned` is not implemented for `NotZerocopy`
    |
    = note: Consider adding `#[derive(Unaligned)]` to `NotZerocopy`
-   = help: the following other types implement trait `Unaligned`:
+   = help: the following other types implement trait `zerocopy::Unaligned`:
              ()
              AtomicBool
              AtomicI8

tests/ui-stable/diagnostic-not-implemented-unaligned.stderr

@@ -1,11 +1,11 @@
-error[E0277]: the trait bound `NotZerocopy: Unaligned` is not satisfied
+error[E0277]: the trait bound `NotZerocopy: zerocopy::Unaligned` is not satisfied
   --> tests/ui-stable/diagnostic-not-implemented-unaligned.rs:18:23
    |
 18 |     takes_unaligned::<NotZerocopy>();
-   |                       ^^^^^^^^^^^ the trait `Unaligned` is not implemented for `NotZerocopy`
+   |                       ^^^^^^^^^^^ the trait `zerocopy::Unaligned` is not implemented for `NotZerocopy`
    |
    = note: Consider adding `#[derive(Unaligned)]` to `NotZerocopy`
-   = help: the following other types implement trait `Unaligned`:
+   = help: the following other types implement trait `zerocopy::Unaligned`:
              ()
              AtomicBool
              AtomicI8

zerocopy-derive/tests/include.rs

@@ -118,11 +118,12 @@ pub mod util {
 
         let buf = super::imp::MaybeUninit::<T>::uninit();
         let ptr = super::imp::Ptr::from_ref(&buf);
+        // SAFETY: This is intentionally unsound; see the preceding comment.
+        let ptr = unsafe { ptr.assume_initialized() };
+
         // SAFETY: `T` and `MaybeUninit<T>` have the same layout, so this is a
         // size-preserving cast. It is also a provenance-preserving cast.
         let ptr = unsafe { ptr.cast_unsized_unchecked(|p| p as *mut T) };
-        // SAFETY: This is intentionally unsound; see the preceding comment.
-        let ptr = unsafe { ptr.assume_initialized() };
         assert!(<T as super::imp::TryFromBytes>::is_bit_valid(ptr));
     }
 }

zerocopy-derive/tests/struct_try_from_bytes.rs

@@ -157,6 +157,7 @@ fn test_maybe_from_bytes() {
     // that we *don't* spuriously do that when generic parameters are present.
 
     let candidate = ::zerocopy::Ptr::from_ref(&[2u8][..]);
+    let candidate = candidate.bikeshed_recall_initialized_from_bytes();
 
     // SAFETY:
     // - The cast preserves address and size. As a result, the cast will address

zerocopy-derive/tests/ui-msrv/derive_transparent.stderr

@@ -70,13 +70,13 @@ note: required by a bound in `_::{closure#0}::_::{closure#0}::assert_impl_all`
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `_::{closure#0}::_::{closure#0}::assert_impl_all`
    = note: this error originates in the macro `::static_assertions::assert_impl_all` (in Nightly builds, run with -Z macro-backtrace for more info)
 
-error[E0277]: the trait bound `NotZerocopy: Unaligned` is not satisfied
+error[E0277]: the trait bound `NotZerocopy: zerocopy::Unaligned` is not satisfied
   --> tests/ui-msrv/derive_transparent.rs:38:1
    |
 38 | util_assert_impl_all!(TransparentStruct<NotZerocopy>: Unaligned);
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Unaligned` is not implemented for `NotZerocopy`
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `zerocopy::Unaligned` is not implemented for `NotZerocopy`
    |
-note: required because of the requirements on the impl of `Unaligned` for `TransparentStruct<NotZerocopy>`
+note: required because of the requirements on the impl of `zerocopy::Unaligned` for `TransparentStruct<NotZerocopy>`
   --> tests/ui-msrv/derive_transparent.rs:24:32
    |
 24 | #[derive(IntoBytes, FromBytes, Unaligned)]

zerocopy-derive/tests/ui-msrv/late_compile_pass.stderr

@@ -69,29 +69,29 @@ error[E0277]: the trait bound `NotZerocopy: zerocopy::IntoBytes` is not satisfie
    = help: see issue #48214
    = note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)
 
-error[E0277]: the trait bound `AU16: Unaligned` is not satisfied
+error[E0277]: the trait bound `AU16: zerocopy::Unaligned` is not satisfied
   --> tests/ui-msrv/late_compile_pass.rs:65:10
    |
 65 | #[derive(Unaligned)]
-   |          ^^^^^^^^^ the trait `Unaligned` is not implemented for `AU16`
+   |          ^^^^^^^^^ the trait `zerocopy::Unaligned` is not implemented for `AU16`
    |
    = help: see issue #48214
    = note: this error originates in the derive macro `Unaligned` (in Nightly builds, run with -Z macro-backtrace for more info)
 
-error[E0277]: the trait bound `AU16: Unaligned` is not satisfied
+error[E0277]: the trait bound `AU16: zerocopy::Unaligned` is not satisfied
   --> tests/ui-msrv/late_compile_pass.rs:73:10
    |
 73 | #[derive(Unaligned)]
-   |          ^^^^^^^^^ the trait `Unaligned` is not implemented for `AU16`
+   |          ^^^^^^^^^ the trait `zerocopy::Unaligned` is not implemented for `AU16`
    |
    = help: see issue #48214
    = note: this error originates in the derive macro `Unaligned` (in Nightly builds, run with -Z macro-backtrace for more info)
 
-error[E0277]: the trait bound `AU16: Unaligned` is not satisfied
+error[E0277]: the trait bound `AU16: zerocopy::Unaligned` is not satisfied
   --> tests/ui-msrv/late_compile_pass.rs:80:10
    |
 80 | #[derive(Unaligned)]
-   |          ^^^^^^^^^ the trait `Unaligned` is not implemented for `AU16`
+   |          ^^^^^^^^^ the trait `zerocopy::Unaligned` is not implemented for `AU16`
    |
    = help: see issue #48214
    = note: this error originates in the derive macro `Unaligned` (in Nightly builds, run with -Z macro-backtrace for more info)