Merge #34

34: BREAKING: Soundness fix for Erasable edge case r=CAD97 a=CAD97



Co-authored-by: CAD97 <[email protected]>

Author: bors[bot]

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = "erasable"
-version = "1.0.0"
+version = "1.1.0"
 edition = "2018"
 
 authors = ["Christopher Durham (cad97) <[email protected]>"]

crates/erasable/Cargo.toml

@@ -16,6 +16,58 @@ There are two main useful reasons to type erase pointers in Rust:
   We provide the [`Thin`](https://cad97.github.io/pointer-utils/erasable/struct.Thin.html)
   wrapper type to provide thin pointer types.
 
+## Changelist
+
+### 1.1.0
+#### Breaking changes
+
+Unfortunately, a subtle problem with the requirements of `Erasable::unerase`
+on implementors was found, and needed to be corrected in this version.
+
+It has always been the intent that a pointer of any provenance should roundtrip
+through `Erasable::erase`/`unerase` without any impact on provenance validity
+of any pointer. Unfortunately, in 1.0.0, `Eraseable::unerase` explicitly suggested
+the use of temporary shared references (`&_`) in its implementation, which do not
+hold up this desired semantics. To quote `Erasable::unerase`'s new documentation:
+
+> No references to the pointee _at all_ should be created,
+> as their mere temporary existence may impact the validity and
+> usable provenance of other pointers to the same location.
+>
+> Creating a shared reference sounds on the surface like it should be ok.
+> After all, you have a known-valid pointer to your type, and you can
+> borrow from whatever pointer was erased. However, in the face of raw
+> pointers with a shared mutable provenance, this is problematic.
+> If a write to the pointee location even potentially races with any
+> invocation of `unerase`, and it creates a reference to the location,
+> we have immediate undefined behavior for writing behind a shared ref.
+>
+> The root issue is that there may be external synchronization that this
+> implementation has no way of knowing about. An implementation of this
+> trait must only read the mimimum amount of data required to re-type the
+> pointer, and must do so with a raw pointer read, or, if and only if
+> there is a known `UnsafeCell` point (such as an atomic), a reference to
+> that `UnsafeCell` point and the safe API of that `UnsafeCell` point.
+
+For more information around the discovery of this issue see
+[this Twitter thread](https://twitter.com/CAD97_/status/1231057021623054336).
+
+To make this update easier, we additionally added `const Erasable::ACK_1_1_0: bool`.
+This value defaults to `false`, but _must_ be overriden to `true` in new
+implementations of `Erasable` as an explicit acknowledgement of the new requirement.
+To make finding impls that do not do so, the `ERASABLE_ENFORCE_1_1_0_SEMANTICS`
+environment variable can be set to remove the default implementation,
+explicitly breaking any implementor that did not override its value.
+
+If you have a use of `unerase` that would be made unsound by `unerase` creating
+a shared reference, then it is recommended to assert the value of `ACK_1_1_0`
+to guard against old impls that may create a shared reference. Also, please tell
+me what it is, because I have not been able to construct a reasonable example
+that would be made unsound by this hole, just a theoretical attack vector.
+Similarly, if you can show that this isn't required, get in touch.
+I'd love to remove this hack if it turns out unnecessary after all
+(but I think I'm confident ruling out that possibility).
+
 ## Related Crates
 
 - [`ptr-union`](https://lib.rs/crates/ptr-union): Pointer unions the size of a pointer.

crates/erasable/README.md

@@ -1,8 +1,19 @@
+use std::env;
+
 fn main() {
     let cfg = autocfg::new();
+
     cfg.emit_expression_cfg("{ extern { type T; } () }", "has_extern_type");
     // NB: Requires this impl to cover `T: ?Sized`, which is not the case as of 2020-09-01.
     // cfg.emit_type_cfg("std::sync::Weak::into_raw", "has_Weak__into_raw");
     cfg.emit_type_cfg("!", "has_never");
+
+    if let Ok(var) = env::var("ERASABLE_ENFORCE_1_1_0_SEMANTICS") {
+        if !var.is_empty() && var != "0" {
+            autocfg::emit("enforce_1_1_0_semantics");
+        }
+    }
+
+    autocfg::rerun_env("ERASABLE_ENFORCE_1_1_0_SEMANTICS");
     autocfg::rerun_path("build.rs");
 }

crates/erasable/build.rs

@@ -201,19 +201,68 @@ pub unsafe trait Erasable {
 
     /// Unerase this erased pointer.
     ///
-    /// Note that this _must_ be sound for erasable pointer types
-    /// with both unique/write provenance and shared/read provenance.
-    /// (In other words, both `&mut _` and `&_` can be unerased with this.)
+    /// Note that this _must_ be sound to roundtrip pointers with any provenance,
+    /// shared, unique, raw, frozen, mutable, and any valid combination thereof.
+    /// (In other words, `&mut _` and `&_` can be safely erased and unerased, and
+    /// any raw pointer should roundtrip without losing the provenance it had.)
     ///
     /// Concretely, this means that the resulting pointer _must_ be derived
     /// from the input pointer without any intervening references manifested.
-    /// Temporary shared references may be used in the implementation,
-    /// so long as the returned pointer is not derived from them.
+    /// Additionally, no references to the pointee _at all_ should be created,
+    /// as their mere temporary existence may impact the validity and
+    /// usable provenance of other pointers to the same location.
+    ///
+    /// Creating a shared reference sounds on the surface like it should be ok.
+    /// After all, you have a known-valid pointer to your type, and you can
+    /// borrow from whatever pointer was erased. However, in the face of raw
+    /// pointers with a shared mutable provenance, this is problematic.
+    /// If a write to the pointee location even potentially races with any
+    /// invocation of `unerase`, and it creates a reference to the location,
+    /// we have immediate undefined behavior for writing behind a shared ref.
+    ///
+    /// The root issue is that there may be external synchronization that this
+    /// implementation has no way of knowing about. An implementation of this
+    /// trait must only read the mimimum amount of data required to re-type the
+    /// pointer, and must do so with a raw pointer read, or, if and only if
+    /// there is a known `UnsafeCell` point (such as an atomic), a reference to
+    /// that `UnsafeCell` point and the safe API of that `UnsafeCell` point.
     ///
     /// # Safety
     ///
     /// The erased pointer must have been created by `erase`ing a valid pointer.
     unsafe fn unerase(this: ErasedPtr) -> ptr::NonNull<Self>;
+
+    /// Whether this implementor has acknowledged the 1.1.0 update to
+    /// `unerase`'s documented implementation requirements.
+    ///
+    /// Prior to 1.0.0, creating a temporary shared reference (`&_`) in
+    /// `unerase` was explicitly listed as allowed, but for the 1.1.0 release
+    /// it was determined that this in fact can cause problems for some use
+    /// cases that `Erasable` is designed to support.
+    ///
+    /// Implementing this as `false` is _not allowed_ and is _not_ permission
+    /// to create references within `unerase`. It only exists as a way to
+    /// make the soundness fix in 1.1.0 disallowing references not breaking.
+    /// You _must_ override this with a value of `true` and follow the current
+    /// documented requirements for `unerase`, and not create references.
+    ///
+    /// If your use of `unerase` would be problematic if it creates a temporary
+    /// shared reference, you should assert that this value is `true`.
+    /// Not doing so will expose you to potentially unsound implementations
+    /// written against 1.0.0 before the reference clarification was made.
+    ///
+    /// The environment variable `ACK_1_1_0` can be set to enforce that all
+    /// implementors have provided an override for this acknowledgement.
+    #[cfg(not(enforce_1_1_0_semantics))]
+    const ACK_1_1_0: bool = false;
+
+    /// Whether this implementor has acknowledged the 1.1.0 update to
+    /// `unerase`'s documented implementation requirements.
+    ///
+    /// Implementing this as `false` is _not allowed_.
+    /// You _must_ override this with a value of `true`.
+    #[cfg(enforce_1_1_0_semantics)]
+    const ACK_1_1_0: bool;
 }
 
 /// Erase a pointer.
@@ -557,6 +606,8 @@ unsafe impl<T: Sized> Erasable for T {
         // SAFETY: must not read the pointer for the safety of the impl directly below.
         this.cast()
     }
+
+    const ACK_1_1_0: bool = true;
 }
 
 // ~~~ impl ErasablePtr ~~~ //

crates/erasable/src/lib.rs

@@ -295,6 +295,8 @@ unsafe impl<Header, Item> Erasable for SliceWithHeader<Header, Item> {
         let raw = ptr::NonNull::new_unchecked(slice_from_raw_parts(this.as_ptr().cast(), len));
         Self::retype(raw)
     }
+
+    const ACK_1_1_0: bool = true;
 }
 
 pub(crate) mod layout_polyfill;