Rollup merge of #140638 - RalfJung:unsafe-pinned-shared-aliased, r=workingjubilee

UnsafePinned: also include the effects of UnsafeCell

This tackles https://github.com/rust-lang/rust/issues/137750 by including an `UnsafeCell` in `UnsafePinned`, thus imbuing it with all the usual properties of interior mutability (no `noalias` nor `dereferenceable` on shared refs, special treatment by Miri's aliasing model). The soundness issue is not fixed yet because coroutine lowering does not use `UnsafePinned`.

The RFC said that `UnsafePinned` would not permit mutability on shared references, but since then, https://github.com/rust-lang/rust/issues/137750 has demonstrated that this is not tenable. In the face of those examples, I propose that we do the "obvious" thing and permit shared mutable state inside `UnsafePinned`. This seems loosely consistent with the fact that we allow going from `Pin<&mut T>` to `&T` (where the former can be aliased with other pointers that perform mutation, and hence the same goes for the latter) -- but the `as_ref` example shows that we in fact would need to add this `UnsafeCell` even if we didn't have a safe conversion to `&T`, since for the compiler and Miri, `&T` and `Pin<&T>` are basically the same type.

To make this possible, I had to remove the `Copy` and `Clone` impls for `UnsafePinned`.

Tracking issue: https://github.com/rust-lang/rust/issues/125735
Cc ``@rust-lang/lang`` ``@rust-lang/opsem``  ``@Sky9x``
I don't think this needs FCP since the type is still unstable -- we'll finally decide whether we like this approach when `UnsafePinned` is moved towards stabilization (IOW, this PR is reversible). However, I'd still like to make sure that the lang team is okay with the direction I am proposing here.

5 files changed, 142 insertions, 22 deletions

diff --git a/library/core/src/pin/unsafe_pinned.rs b/library/core/src/pin/unsafe_pinned.rs
index f65e83662fe..dbcceb807ab 100644
--- a/library/core/src/pin/unsafe_pinned.rs
+++ b/library/core/src/pin/unsafe_pinned.rs
@@ -1,10 +1,13 @@
+use crate::cell::UnsafeCell;
 use crate::marker::{PointerLike, Unpin};
 use crate::ops::{CoerceUnsized, DispatchFromDyn};
 use crate::pin::Pin;
 use crate::{fmt, ptr};
 
-/// This type provides a way to opt-out of typical aliasing rules;
+/// This type provides a way to entirely opt-out of typical aliasing rules;
 /// specifically, `&mut UnsafePinned<T>` is not guaranteed to be a unique pointer.
+/// This also subsumes the effects of `UnsafeCell`, i.e., `&UnsafePinned<T>` may point to data
+/// that is being mutated.
 ///
 /// However, even if you define your type like `pub struct Wrapper(UnsafePinned<...>)`, it is still
 /// very risky to have an `&mut Wrapper` that aliases anything else. Many functions that work
@@ -17,38 +20,24 @@ use crate::{fmt, ptr};
 /// the public API of a library. It is an internal implementation detail of libraries that need to
 /// support aliasing mutable references.
 ///
-/// Further note that this does *not* lift the requirement that shared references must be read-only!
-/// Use `UnsafeCell` for that.
-///
 /// This type blocks niches the same way `UnsafeCell` does.
 #[lang = "unsafe_pinned"]
 #[repr(transparent)]
 #[unstable(feature = "unsafe_pinned", issue = "125735")]
 pub struct UnsafePinned<T: ?Sized> {
-    value: T,
+    value: UnsafeCell<T>,
 }
 
+// Override the manual `!Sync` in `UnsafeCell`.
+#[unstable(feature = "unsafe_pinned", issue = "125735")]
+unsafe impl<T: ?Sized + Sync> Sync for UnsafePinned<T> {}
+
 /// When this type is used, that almost certainly means safe APIs need to use pinning to avoid the
 /// aliases from becoming invalidated. Therefore let's mark this as `!Unpin`. You can always opt
 /// back in to `Unpin` with an `impl` block, provided your API is still sound while unpinned.
 #[unstable(feature = "unsafe_pinned", issue = "125735")]
 impl<T: ?Sized> !Unpin for UnsafePinned<T> {}
 
-/// The type is `Copy` when `T` is to avoid people assuming that `Copy` implies there is no
-/// `UnsafePinned` anywhere. (This is an issue with `UnsafeCell`: people use `Copy` bounds to mean
-/// `Freeze`.) Given that there is no `unsafe impl Copy for ...`, this is also the option that
-/// leaves the user more choices (as they can always wrap this in a `!Copy` type).
-// FIXME(unsafe_pinned): this may be unsound or a footgun?
-#[unstable(feature = "unsafe_pinned", issue = "125735")]
-impl<T: Copy> Copy for UnsafePinned<T> {}
-
-#[unstable(feature = "unsafe_pinned", issue = "125735")]
-impl<T: Copy> Clone for UnsafePinned<T> {
-    fn clone(&self) -> Self {
-        *self
-    }
-}
-
 // `Send` and `Sync` are inherited from `T`. This is similar to `SyncUnsafeCell`, since
 // we eventually concluded that `UnsafeCell` implicitly making things `!Sync` is sometimes
 // unergonomic. A type that needs to be `!Send`/`!Sync` should really have an explicit
@@ -63,7 +52,7 @@ impl<T> UnsafePinned<T> {
     #[must_use]
     #[unstable(feature = "unsafe_pinned", issue = "125735")]
     pub const fn new(value: T) -> Self {
-        UnsafePinned { value }
+        UnsafePinned { value: UnsafeCell::new(value) }
     }
 
     /// Unwraps the value, consuming this `UnsafePinned`.
@@ -72,7 +61,7 @@ impl<T> UnsafePinned<T> {
     #[unstable(feature = "unsafe_pinned", issue = "125735")]
     #[rustc_allow_const_fn_unstable(const_precise_live_drops)]
     pub const fn into_inner(self) -> T {
-        self.value
+        self.value.into_inner()
     }
 }
 
diff --git a/src/tools/miri/tests/fail/async-shared-mutable.rs b/src/tools/miri/tests/fail/async-shared-mutable.rs
new file mode 100644
index 00000000000..62780e7a11c
--- /dev/null
+++ b/src/tools/miri/tests/fail/async-shared-mutable.rs
@@ -0,0 +1,25 @@
+//! FIXME: This test should pass! However, `async fn` does not yet use `UnsafePinned`.
+//! This is a regression test for <https://github.com/rust-lang/rust/issues/137750>:
+//! `UnsafePinned` must include the effects of `UnsafeCell`.
+//@revisions: stack tree
+//@[tree]compile-flags: -Zmiri-tree-borrows
+//@normalize-stderr-test: "\[0x[a-fx\d.]+\]" -> "[OFFSET]"
+
+use core::future::Future;
+use core::pin::{Pin, pin};
+use core::task::{Context, Poll, Waker};
+
+fn main() {
+    let mut f = pin!(async move {
+        let x = &mut 0u8;
+        core::future::poll_fn(move |_| {
+            *x = 1; //~ERROR: write access
+            Poll::<()>::Pending
+        })
+        .await
+    });
+    let mut cx = Context::from_waker(&Waker::noop());
+    assert_eq!(f.as_mut().poll(&mut cx), Poll::Pending);
+    let _: Pin<&_> = f.as_ref(); // Or: `f.as_mut().into_ref()`.
+    assert_eq!(f.as_mut().poll(&mut cx), Poll::Pending);
+}
diff --git a/src/tools/miri/tests/fail/async-shared-mutable.stack.stderr b/src/tools/miri/tests/fail/async-shared-mutable.stack.stderr
new file mode 100644
index 00000000000..8f53a55cc3e
--- /dev/null
+++ b/src/tools/miri/tests/fail/async-shared-mutable.stack.stderr
@@ -0,0 +1,43 @@
+error: Undefined Behavior: attempting a write access using <TAG> at ALLOC[OFFSET], but that tag does not exist in the borrow stack for this location
+  --> tests/fail/async-shared-mutable.rs:LL:CC
+   |
+LL |             *x = 1;
+   |             ^^^^^^
+   |             |
+   |             attempting a write access using <TAG> at ALLOC[OFFSET], but that tag does not exist in the borrow stack for this location
+   |             this error occurs as part of an access at ALLOC[OFFSET]
+   |
+   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
+   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
+help: <TAG> was created by a Unique retag at offsets [OFFSET]
+  --> tests/fail/async-shared-mutable.rs:LL:CC
+   |
+LL | /         core::future::poll_fn(move |_| {
+LL | |             *x = 1;
+LL | |             Poll::<()>::Pending
+LL | |         })
+LL | |         .await
+   | |______________^
+help: <TAG> was later invalidated at offsets [OFFSET] by a SharedReadOnly retag
+  --> tests/fail/async-shared-mutable.rs:LL:CC
+   |
+LL |     let _: Pin<&_> = f.as_ref(); // Or: `f.as_mut().into_ref()`.
+   |                      ^^^^^^^^^^
+   = note: BACKTRACE (of the first span):
+   = note: inside closure at tests/fail/async-shared-mutable.rs:LL:CC
+   = note: inside `<std::future::PollFn<{closure@tests/fail/async-shared-mutable.rs:LL:CC}> as std::future::Future>::poll` at RUSTLIB/core/src/future/poll_fn.rs:LL:CC
+note: inside closure
+  --> tests/fail/async-shared-mutable.rs:LL:CC
+   |
+LL |         .await
+   |          ^^^^^
+note: inside `main`
+  --> tests/fail/async-shared-mutable.rs:LL:CC
+   |
+LL |     assert_eq!(f.as_mut().poll(&mut cx), Poll::Pending);
+   |                ^^^^^^^^^^^^^^^^^^^^^^^^
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+
diff --git a/src/tools/miri/tests/fail/async-shared-mutable.tree.stderr b/src/tools/miri/tests/fail/async-shared-mutable.tree.stderr
new file mode 100644
index 00000000000..d1e66a0d043
--- /dev/null
+++ b/src/tools/miri/tests/fail/async-shared-mutable.tree.stderr
@@ -0,0 +1,47 @@
+error: Undefined Behavior: write access through <TAG> at ALLOC[OFFSET] is forbidden
+  --> tests/fail/async-shared-mutable.rs:LL:CC
+   |
+LL |             *x = 1;
+   |             ^^^^^^ write access through <TAG> at ALLOC[OFFSET] is forbidden
+   |
+   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental
+   = help: the accessed tag <TAG> has state Frozen which forbids this child write access
+help: the accessed tag <TAG> was created here, in the initial state Reserved
+  --> tests/fail/async-shared-mutable.rs:LL:CC
+   |
+LL | /         core::future::poll_fn(move |_| {
+LL | |             *x = 1;
+LL | |             Poll::<()>::Pending
+LL | |         })
+LL | |         .await
+   | |______________^
+help: the accessed tag <TAG> later transitioned to Active due to a child write access at offsets [OFFSET]
+  --> tests/fail/async-shared-mutable.rs:LL:CC
+   |
+LL |             *x = 1;
+   |             ^^^^^^
+   = help: this transition corresponds to the first write to a 2-phase borrowed mutable reference
+help: the accessed tag <TAG> later transitioned to Frozen due to a reborrow (acting as a foreign read access) at offsets [OFFSET]
+  --> tests/fail/async-shared-mutable.rs:LL:CC
+   |
+LL |     let _: Pin<&_> = f.as_ref(); // Or: `f.as_mut().into_ref()`.
+   |                      ^^^^^^^^^^
+   = help: this transition corresponds to a loss of write permissions
+   = note: BACKTRACE (of the first span):
+   = note: inside closure at tests/fail/async-shared-mutable.rs:LL:CC
+   = note: inside `<std::future::PollFn<{closure@tests/fail/async-shared-mutable.rs:LL:CC}> as std::future::Future>::poll` at RUSTLIB/core/src/future/poll_fn.rs:LL:CC
+note: inside closure
+  --> tests/fail/async-shared-mutable.rs:LL:CC
+   |
+LL |         .await
+   |          ^^^^^
+note: inside `main`
+  --> tests/fail/async-shared-mutable.rs:LL:CC
+   |
+LL |     assert_eq!(f.as_mut().poll(&mut cx), Poll::Pending);
+   |                ^^^^^^^^^^^^^^^^^^^^^^^^
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+
diff --git a/src/tools/miri/tests/pass/both_borrows/unsafe_pinned.rs b/src/tools/miri/tests/pass/both_borrows/unsafe_pinned.rs
new file mode 100644
index 00000000000..0c75a07bfa2
--- /dev/null
+++ b/src/tools/miri/tests/pass/both_borrows/unsafe_pinned.rs
@@ -0,0 +1,16 @@
+//@revisions: stack tree
+//@[tree]compile-flags: -Zmiri-tree-borrows
+#![feature(unsafe_pinned)]
+
+use std::pin::UnsafePinned;
+
+fn mutate(x: &UnsafePinned<i32>) {
+    let ptr = x as *const _ as *mut i32;
+    unsafe { ptr.write(42) };
+}
+
+fn main() {
+    let x = UnsafePinned::new(0);
+    mutate(&x);
+    assert_eq!(x.into_inner(), 42);
+}