Decide what to do about UnsafePinned and safe Pin::deref

[traviscross]

As context, this entirely safe code is reported as UB by Miri under both Stacked Borrows and Tree Borrows:

use core::{ops::Deref, pin::{pin, Pin}, 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 is forbidden
            Poll::<()>::Pending
        }).await
    });
    let mut cx = Context::from_waker(&Waker::noop());
    assert_eq!(f.as_mut().poll(&mut cx), Poll::Pending);
    _ = <Pin<&mut _> as Deref>::deref(&f);
    assert_eq!(f.as_mut().poll(&mut cx), Poll::Pending);
}

Playground link

Output

Under TB:

error: Undefined Behavior: write access through <1666> at alloc969[0x8] is forbidden
  --> src/main.rs:7:13
   |
7  |             *x = 1;
   |             ^^^^^^ write access through <1666> at alloc969[0x8] 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 <1666> has state Frozen which forbids this child write access
help: the accessed tag <1666> was created here, in the initial state Reserved
  --> src/main.rs:6:9
   |
6  | /         core::future::poll_fn(move |_| {
7  | |             *x = 1;
8  | |             Poll::<()>::Pending
9  | |         }).await
   | |________________^
help: the accessed tag <1666> later transitioned to Active due to a child write access at offsets [0x8..0x9]
  --> src/main.rs:7:13
   |
7  |             *x = 1;
   |             ^^^^^^
   = help: this transition corresponds to the first write to a 2-phase borrowed mutable reference
help: the accessed tag <1666> later transitioned to Frozen due to a reborrow (acting as a foreign read access) at offsets [0x0..0x10]
  --> src/main.rs:13:9
   |
13 |     _ = <Pin<&mut _> as Deref>::deref(&f);
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = help: this transition corresponds to a loss of write permissions
   = note: BACKTRACE (of the first span):
   = note: inside closure at src/main.rs:7:13: 7:19
  --> src/main.rs:9:12
   |
9  |         }).await
   |            ^^^^^
note: inside `main`
  --> src/main.rs:14:16
   |
14 |     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

This isn't a bug in Miri really. It's a bug... elsewhere. Part of that is described by:

• Resolve unsound interaction between noalias and self-referential data (incl. generators, async fn) #63818

But at the same time, this is relevant to the post-RFC work on UnsafePinned:

• Tracking Issue for RFC 3467: UnsafePinned #125735

The idea of UnsafePinned is that we use it to wrap the fields subject to self-borrows so that given a mutable reference to such fields, the compiler (and our model) won't assume that a foreign write can't occur.

But the RFC suggested that while &mut UnsafePinned<T> was special, &UnsafePinned<T> wasn't, and would therefore act like &T. This is where we've hit problems due to safe Pin::deref, as @RalfJung has described:

• Tracking Issue for RFC 3467: UnsafePinned #125735 (comment)
• Initial UnsafePinned implementation [Part 1: Libs] #137043 (comment)
• !Send Future violates stacked borrow rules miri#3796 (comment)

In the above example, if the compiler were to insert:

let x: &mut UnsafePinned<0u8> = ..;

Then we still end up having a problem, under the RFC's model and our current model behavior, which is (quoting @RalfJung):

when a shared reference gets created, do a read of everything the reference points to (except UnsafeCell) to ensure things are in a sane state

What happens in the example is that:

• We create and store a mutable self-reference to the u8 owned by the future.
• We activate that self-reference with a write, marking it Unique under TB.
• Then, due to the deref creating a reference to the future and our behavior to treat that as a read of the entire future, a foreign read occurs, marking the mutable reference as Frozen.
• Next time we try to write, we get UB.

About this, @RalfJung has said:

In the RFC we had the question whether UnsafePinned should also act like an UnsafeCell. I am now fairly sure that the answer is "yes, it must". The reason is this: due to Pin::deref being safe, it is at any moment possible to create a shared reference to a future that is halted at a suspension point. If there is no UnsafeCell, this acts like a read of the entire future data, and that read conflicts with mutable references that the future may hold to its own data. This is not a necessary property of aliasing with mutable reference, but it is a necessary consequence of the decision that Pin::deref should be safe.

And also that:

One could imagine aliasing models that don't do such a read, though then dereferenceable becomes more questionable and I don't know how this would impact optimizations.

Anyway, it seems we should have a canonical issue to use to decide about how to proceed about this and how either our model or RFC 3467 would need to be adjusted before the stabilization of UnsafePinned, so here is that issue.

cc @RalfJung @WaffleLapkin @rust-lang/opsem @rust-lang/lang

@rustbot labels +T-lang +T-opsem +C-discussion +F-unsafe_pinned

[traviscross]

Regarding this bit:

One could imagine aliasing models that don't do such a read, though then dereferenceable becomes more questionable and I don't know how this would impact optimizations.

@RalfJung: Does it work at all, and if so, how ugly would it be to otherwise treat &UnsafePinned<T> like &T in the model but to not do this read in this one particular case? (So that, e.g., if it affects optimizations, it affects fewer places.)

[CAD97]

It would defer the unsoundness, but safe code would still be allowed to do real source reads through the &{async block} (e.g. by casting to &[MaybeUninit<u8>]). The borrow relaxations required are almost exactly the same as with UnsafeCell, as we have aliasing &Outer and &mut Inner simultaneously.

The difference is that the &mut in the async block is "inactive" when the &Outer exists and gets reactivated by passing &mut Outer to Future::poll… but this difference is only skin deep. Pinning that exposes &mut-derived access outside of Future::poll is difficult to write soundly, but it should still be considered valid, as long as dropping the containing future still terminates the access. Scoped async tasks would be a canonical instance of such¹, if they could be restricted to borrowing from the parent task.

I believe that theoretically it would be coherent to add a TempFrozen retag that is invalidated by sibling Unique (unless unprotected) instead of downgrading sibling Unique when created, but the model and reasoning complexity costs that requires don't seem all that beneficial. (The only potential "benefit" I can really see is adding new ways in which scoped async spawn and borrowing completion based async IO are both fundamentally unsound in the face of careless but reasonable seeming safe code¹.)

Footnotes

1. Scoped async tasks are believed to be sound if all borrowed memory is owned by the parent task and drop-cancelling the parent task waits on drop-cancelling the child tasks. With TempFrozen semantics, however, this is not true. ↩ ↩²

[RalfJung]

Does it work at all, and if so, how ugly would it be to otherwise treat &UnsafePinned like &T in the model but to not do this read in this one particular case? (So that, e.g., if it affects optimizations, it affects fewer places.)

In SB, this is just plain impossible.

In TB, we could treat such a reference like &(), which reads nothing but as TB allows accessing pointers "outside" the range of memory their type indicates, could still be used to access the underlying future. We'd have to remove the dereferenceable attribute on such a reference to match that.

But... honestly it doesn't seem worth it. Allowing aliasing of &mut in particular for Pin<&mut T> is the entire point of UnsafePinned. We decided a long time ago that Pin<&mut T> gives safe access to &T. We should just accept the consequence that this means &UnsafePinned is part of subtle aliasing games, and stop having noalias on that memory.