Skip to content

Commit 04cfcf2

Browse files
authored
Unrolled build for #140638
Rollup merge of #140638 - RalfJung:unsafe-pinned-shared-aliased, r=workingjubilee UnsafePinned: also include the effects of UnsafeCell This tackles #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, #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: #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.
2 parents c360e21 + 259d6f4 commit 04cfcf2

File tree

5 files changed

+142
-22
lines changed

5 files changed

+142
-22
lines changed

library/core/src/pin/unsafe_pinned.rs

Lines changed: 11 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
1+
use crate::cell::UnsafeCell;
12
use crate::marker::{PointerLike, Unpin};
23
use crate::ops::{CoerceUnsized, DispatchFromDyn};
34
use crate::pin::Pin;
45
use crate::{fmt, ptr};
56

6-
/// This type provides a way to opt-out of typical aliasing rules;
7+
/// This type provides a way to entirely opt-out of typical aliasing rules;
78
/// specifically, `&mut UnsafePinned<T>` is not guaranteed to be a unique pointer.
9+
/// This also subsumes the effects of `UnsafeCell`, i.e., `&UnsafePinned<T>` may point to data
10+
/// that is being mutated.
811
///
912
/// However, even if you define your type like `pub struct Wrapper(UnsafePinned<...>)`, it is still
1013
/// very risky to have an `&mut Wrapper` that aliases anything else. Many functions that work
@@ -17,38 +20,24 @@ use crate::{fmt, ptr};
1720
/// the public API of a library. It is an internal implementation detail of libraries that need to
1821
/// support aliasing mutable references.
1922
///
20-
/// Further note that this does *not* lift the requirement that shared references must be read-only!
21-
/// Use `UnsafeCell` for that.
22-
///
2323
/// This type blocks niches the same way `UnsafeCell` does.
2424
#[lang = "unsafe_pinned"]
2525
#[repr(transparent)]
2626
#[unstable(feature = "unsafe_pinned", issue = "125735")]
2727
pub struct UnsafePinned<T: ?Sized> {
28-
value: T,
28+
value: UnsafeCell<T>,
2929
}
3030

31+
// Override the manual `!Sync` in `UnsafeCell`.
32+
#[unstable(feature = "unsafe_pinned", issue = "125735")]
33+
unsafe impl<T: ?Sized + Sync> Sync for UnsafePinned<T> {}
34+
3135
/// When this type is used, that almost certainly means safe APIs need to use pinning to avoid the
3236
/// aliases from becoming invalidated. Therefore let's mark this as `!Unpin`. You can always opt
3337
/// back in to `Unpin` with an `impl` block, provided your API is still sound while unpinned.
3438
#[unstable(feature = "unsafe_pinned", issue = "125735")]
3539
impl<T: ?Sized> !Unpin for UnsafePinned<T> {}
3640

37-
/// The type is `Copy` when `T` is to avoid people assuming that `Copy` implies there is no
38-
/// `UnsafePinned` anywhere. (This is an issue with `UnsafeCell`: people use `Copy` bounds to mean
39-
/// `Freeze`.) Given that there is no `unsafe impl Copy for ...`, this is also the option that
40-
/// leaves the user more choices (as they can always wrap this in a `!Copy` type).
41-
// FIXME(unsafe_pinned): this may be unsound or a footgun?
42-
#[unstable(feature = "unsafe_pinned", issue = "125735")]
43-
impl<T: Copy> Copy for UnsafePinned<T> {}
44-
45-
#[unstable(feature = "unsafe_pinned", issue = "125735")]
46-
impl<T: Copy> Clone for UnsafePinned<T> {
47-
fn clone(&self) -> Self {
48-
*self
49-
}
50-
}
51-
5241
// `Send` and `Sync` are inherited from `T`. This is similar to `SyncUnsafeCell`, since
5342
// we eventually concluded that `UnsafeCell` implicitly making things `!Sync` is sometimes
5443
// unergonomic. A type that needs to be `!Send`/`!Sync` should really have an explicit
@@ -63,7 +52,7 @@ impl<T> UnsafePinned<T> {
6352
#[must_use]
6453
#[unstable(feature = "unsafe_pinned", issue = "125735")]
6554
pub const fn new(value: T) -> Self {
66-
UnsafePinned { value }
55+
UnsafePinned { value: UnsafeCell::new(value) }
6756
}
6857

6958
/// Unwraps the value, consuming this `UnsafePinned`.
@@ -72,7 +61,7 @@ impl<T> UnsafePinned<T> {
7261
#[unstable(feature = "unsafe_pinned", issue = "125735")]
7362
#[rustc_allow_const_fn_unstable(const_precise_live_drops)]
7463
pub const fn into_inner(self) -> T {
75-
self.value
64+
self.value.into_inner()
7665
}
7766
}
7867

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
//! FIXME: This test should pass! However, `async fn` does not yet use `UnsafePinned`.
2+
//! This is a regression test for <https://github.com/rust-lang/rust/issues/137750>:
3+
//! `UnsafePinned` must include the effects of `UnsafeCell`.
4+
//@revisions: stack tree
5+
//@[tree]compile-flags: -Zmiri-tree-borrows
6+
//@normalize-stderr-test: "\[0x[a-fx\d.]+\]" -> "[OFFSET]"
7+
8+
use core::future::Future;
9+
use core::pin::{Pin, pin};
10+
use core::task::{Context, Poll, Waker};
11+
12+
fn main() {
13+
let mut f = pin!(async move {
14+
let x = &mut 0u8;
15+
core::future::poll_fn(move |_| {
16+
*x = 1; //~ERROR: write access
17+
Poll::<()>::Pending
18+
})
19+
.await
20+
});
21+
let mut cx = Context::from_waker(&Waker::noop());
22+
assert_eq!(f.as_mut().poll(&mut cx), Poll::Pending);
23+
let _: Pin<&_> = f.as_ref(); // Or: `f.as_mut().into_ref()`.
24+
assert_eq!(f.as_mut().poll(&mut cx), Poll::Pending);
25+
}
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
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
2+
--> tests/fail/async-shared-mutable.rs:LL:CC
3+
|
4+
LL | *x = 1;
5+
| ^^^^^^
6+
| |
7+
| attempting a write access using <TAG> at ALLOC[OFFSET], but that tag does not exist in the borrow stack for this location
8+
| this error occurs as part of an access at ALLOC[OFFSET]
9+
|
10+
= help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
11+
= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
12+
help: <TAG> was created by a Unique retag at offsets [OFFSET]
13+
--> tests/fail/async-shared-mutable.rs:LL:CC
14+
|
15+
LL | / core::future::poll_fn(move |_| {
16+
LL | | *x = 1;
17+
LL | | Poll::<()>::Pending
18+
LL | | })
19+
LL | | .await
20+
| |______________^
21+
help: <TAG> was later invalidated at offsets [OFFSET] by a SharedReadOnly retag
22+
--> tests/fail/async-shared-mutable.rs:LL:CC
23+
|
24+
LL | let _: Pin<&_> = f.as_ref(); // Or: `f.as_mut().into_ref()`.
25+
| ^^^^^^^^^^
26+
= note: BACKTRACE (of the first span):
27+
= note: inside closure at tests/fail/async-shared-mutable.rs:LL:CC
28+
= 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
29+
note: inside closure
30+
--> tests/fail/async-shared-mutable.rs:LL:CC
31+
|
32+
LL | .await
33+
| ^^^^^
34+
note: inside `main`
35+
--> tests/fail/async-shared-mutable.rs:LL:CC
36+
|
37+
LL | assert_eq!(f.as_mut().poll(&mut cx), Poll::Pending);
38+
| ^^^^^^^^^^^^^^^^^^^^^^^^
39+
40+
note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
41+
42+
error: aborting due to 1 previous error
43+
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
error: Undefined Behavior: write access through <TAG> at ALLOC[OFFSET] is forbidden
2+
--> tests/fail/async-shared-mutable.rs:LL:CC
3+
|
4+
LL | *x = 1;
5+
| ^^^^^^ write access through <TAG> at ALLOC[OFFSET] is forbidden
6+
|
7+
= help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental
8+
= help: the accessed tag <TAG> has state Frozen which forbids this child write access
9+
help: the accessed tag <TAG> was created here, in the initial state Reserved
10+
--> tests/fail/async-shared-mutable.rs:LL:CC
11+
|
12+
LL | / core::future::poll_fn(move |_| {
13+
LL | | *x = 1;
14+
LL | | Poll::<()>::Pending
15+
LL | | })
16+
LL | | .await
17+
| |______________^
18+
help: the accessed tag <TAG> later transitioned to Active due to a child write access at offsets [OFFSET]
19+
--> tests/fail/async-shared-mutable.rs:LL:CC
20+
|
21+
LL | *x = 1;
22+
| ^^^^^^
23+
= help: this transition corresponds to the first write to a 2-phase borrowed mutable reference
24+
help: the accessed tag <TAG> later transitioned to Frozen due to a reborrow (acting as a foreign read access) at offsets [OFFSET]
25+
--> tests/fail/async-shared-mutable.rs:LL:CC
26+
|
27+
LL | let _: Pin<&_> = f.as_ref(); // Or: `f.as_mut().into_ref()`.
28+
| ^^^^^^^^^^
29+
= help: this transition corresponds to a loss of write permissions
30+
= note: BACKTRACE (of the first span):
31+
= note: inside closure at tests/fail/async-shared-mutable.rs:LL:CC
32+
= 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
33+
note: inside closure
34+
--> tests/fail/async-shared-mutable.rs:LL:CC
35+
|
36+
LL | .await
37+
| ^^^^^
38+
note: inside `main`
39+
--> tests/fail/async-shared-mutable.rs:LL:CC
40+
|
41+
LL | assert_eq!(f.as_mut().poll(&mut cx), Poll::Pending);
42+
| ^^^^^^^^^^^^^^^^^^^^^^^^
43+
44+
note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
45+
46+
error: aborting due to 1 previous error
47+
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
//@revisions: stack tree
2+
//@[tree]compile-flags: -Zmiri-tree-borrows
3+
#![feature(unsafe_pinned)]
4+
5+
use std::pin::UnsafePinned;
6+
7+
fn mutate(x: &UnsafePinned<i32>) {
8+
let ptr = x as *const _ as *mut i32;
9+
unsafe { ptr.write(42) };
10+
}
11+
12+
fn main() {
13+
let x = UnsafePinned::new(0);
14+
mutate(&x);
15+
assert_eq!(x.into_inner(), 42);
16+
}

0 commit comments

Comments
 (0)