When subtyping can switch out trait impls, Pin::new’s check for Target: Unpin becomes insufficient

[steffahn]

Below, Marker1 is a subtype of Marker2; this means we can also coerce Pin<CustomType<Marker1>> to Pin<CustomType<Marker2>> for any covariant CustomType. If we now write different Deref implementations for CustomType for Marker1 vs. Marker2, then we can switch out the Target underneath a Pin, breaking Pin::new’s soundness.

This does run into the coherence_leak_check warning easily, though as far as I understand that warning is supposed to go away eventually, and already was weakened recently [the below code doesn’t hit any warning at all anymore, since 1.84].

use std::{
    marker::PhantomData,
    ops::{Deref, DerefMut},
    pin::Pin,
};

struct S<'a, T, Marker>(&'a mut T, PhantomData<Marker>);

type Marker1 = for<'a> fn(&'a ());
type Marker2 = fn(&'static ());

impl<T> Deref for S<'_, T, Marker1> {
    type Target = ();

    fn deref(&self) -> &() {
        &()
    }
}

struct Wrap<T>(T);
impl<T: Deref> Deref for Wrap<T> {
    type Target = T::Target;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

// with this difference in the `Wrap<…>`-nesting,
// it seems that [since Rust 1.84], this impl will
// even avoid the `coherence_leak_check` lint
impl<T> Deref for Wrap<S<'_, T, Marker2>> {
    type Target = T;

    fn deref(&self) -> &T {
        self.0 .0
    }
}
impl<T> DerefMut for Wrap<S<'_, T, Marker2>> {
    fn deref_mut(&mut self) -> &mut T {
        self.0 .0
    }
}

// obviously a “working” function with this signature is problematic
pub fn unsound_pin<T>(x: T, callback: impl FnOnce(Pin<&mut T>)) -> T {
    let mut x = x;
    let w = Wrap(S(&mut x, PhantomData));
    let ps: Pin<Wrap<S<'_, T, Marker1>>> = Pin::new(w); // accepted, since Target = (): Unpin
    let ps: Pin<Wrap<S<'_, T, Marker2>>> = ps; // subtyping coercion; but now Target = T
    callback(Pin::as_mut(&mut { ps }));
    x
}

////////////////////////////////////////////////////////////////////////////////////////////////////
// everything below here just exploitation of `unsound_pin` to get a segfault

use std::{
    future::Future,
    mem,
    sync::Arc,
    task::{Context, Poll, Wake},
};

fn yield_now() -> impl Future<Output = ()> {
    struct Yield(bool);
    impl Future for Yield {
        type Output = ();

        fn poll(mut self: Pin<&mut Self>, _cx: &mut std::task::Context<'_>) -> Poll<Self::Output> {
            if matches!(mem::replace(&mut *self, Yield(true)), Yield(true)) {
                Poll::Ready(())
            } else {
                Poll::Pending
            }
        }
    }
    Yield(false)
}

fn main() {
    let fut = async {
        let x = &&0;
        let reference = &x;
        yield_now().await; // future will be moved here
        dbg!(reference);
    };

    struct Waker;
    impl Wake for Waker {
        fn wake(self: std::sync::Arc<Self>) {}
    }
    let waker = Arc::new(Waker).into();
    let mut cx = Context::from_waker(&waker);

    let fut = unsound_pin(fut, |fut| {
        let _ = fut.poll(&mut cx);
    });
    // moving `fut`  vvv  after the first poll above, then polling again
    let _ = Box::pin(fut).as_mut().poll(&mut cx);
}

$ cargo run
   Compiling play v0.1.0 (/home/…/play)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.09s
     Running `target/debug/play`
[src/main.rs:85:9] reference = [1]    1834892 segmentation fault (core dumped)  cargo run

(playground)

Unlike #85099, this one has nothing to do with unsizing.

@rustbot label A-pin, I-unsound, T-types, T-libs-api
(I’m roughly guessing appropriate T-… labels; feel free to adjust these, like e.g. if T-compiler seems relevant)

[compiler-errors]

lol

[apiraino]

[the below code doesn’t hit any warning at all anymore, since 1.84]

A bit more on this.

• On rust 1.84.0-beta.4 (66221abde 2024-11-19) does not return any warning and just segfaults.
• On rust 1.83 stable still returns a warning (then segfaults):

$ rustc +stable --edition 2021 main.rs
warning: conflicting implementations of trait `Deref` for type `Wrap<S<'_, _, fn(&'static ())>>`
  --> src/main.rs:32:1
   |
21 | impl<T: Deref> Deref for Wrap<T> {
   | -------------------------------- first implementation here
...
32 | impl<T> Deref for Wrap<S<'_, T, Marker2>> {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `Wrap<S<'_, _, fn(&'static ())>>`
   |
   = warning: the behavior may change in a future release
   = note: for more information, see issue #56105 <https://github.com/rust-lang/rust/issues/56105>
   = note: `#[warn(coherence_leak_check)]` on by default

@rustbot label +regression-from-stable-to-beta

[lcnr]

It's highly likely that this warning will never get moved to a hard error and be allowed instead. I therefore do not consider this to be a regression

[lcnr]

While also requiring Deref::Target to only change via subtyping would fix the 'practical' part of this unsoundness, it would not fix the theoretical issue:

You could have MyType<for<'a> fn(&'a ())>: Unpin while MyType<fn(&'static ())> does not. This way even using Pin::<Box<MyType<for<'a> fn(&'a ())>>>::new is broken.

That can only cause unsoundness if MyType itself has a library invariant which relies on it, e.g. see #89948. So 🤷 we could update the documentation of Unpin to explicitly state that the guarantees provided need to extend to all subtypes of the type or whatever

I do really like this idea though 😁 been thinking of "add a Ptr: SensiblePtrTy" where-bound to Pin instead, but that seems like far more churn, even if it is lintable.

I agree that the temporary? fix uses specialization in an acceptable and sound way

[danielhenrymantilla]

I have another snippet which pushed things further to cover this "Unpin change", while also solving the missing CoerceUnsized (the latter in a wave-handed manner): https://rust.godbolt.org/z/r9ofG8jPc.
But while discussing with @lcnr, we realized that the "second default type param" trick to avoid running into the (lack-of-co)variance issue does not scale: any user type wrapping, e.g., a Pin<Box<T>>, would end up invariant 😿

I have thus hidden my previous post (too large for a currently unviable solution), but if rustc were to develop the ability to force/override the variance of a type, then it might be worth revisiting it.

[theemathas]

@danielhenrymantilla What issue involving "any user type wrapping" are you talking about? This seems to work fine. https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=bbf50ce4d24a72448b78b214f47e7085

[lcnr]

@danielhenrymantilla What issue involving "any user type wrapping" are you talking about? This seems to work fine. https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=bbf50ce4d24a72448b78b214f47e7085

a type wrapping Pin changes Ptr to be invariant as it's used in an associated type (as part of the default arg)

struct Foo<T>(Pin<T>);

fn foo<'a>(x: Foo<Pin<(&'static i32, PhantomPinned)>>) -> Foo<Pin<(&'a i32, PhantomPinned)>> {
    x
}

[RalfJung]

Pin<P> fundamentally relies on reasoning about "the Deref/DerefMut impl of P", so yeah if subtyping changes which impl is picked then everything is fundamentally broken. Given that we do have orphan rules, it doesn't seem so far-fetched to assume that other unsafe code may also rely on trait impl coherence in a similar way, though I am not aware of such code.

What is the long-term plan here -- to fix traits/subtyping such that coherence actually becomes useful to unsafe code, or to tell unsafe code authors that this kind of reasoning it not sound?

[theemathas]

Why do higher-ranked function pointers have subtyping anyway? And how much code relies on that?

[Jules-Bertholet]

It's highly likely that this warning will never get moved to a hard error and be allowed instead. I therefore do not consider this to be a regression

Perhaps the existence of this issue suggests that this “highly likely” outcome should in fact be reconsidered, and that the reduction in scope of coherence_leak_check in 1.84 should be reverted at least temporarily?

[lcnr]

Perhaps the existence of this issue suggests that this “highly likely” outcome should in fact be reconsidered, and that the reduction in scope of coherence_leak_check in 1.84 should be reverted at least temporarily?

Doing so would be feasible by adding a flag to TypingMode::Coherence to optionally disable the leak check inside of the trait solver. I would review and approve such a PR, don't think that the outcome of "we end up actually removing the leak check here" is likely, so I am not too motivated to do so myself.

I am confident that fully removing the leak check won't happen, given that the fallout of slightly (depending on perspective) weakening it in #119820 ended up getting reverted again due to its impact.

Only disabling it in coherence (or only doing it when for binders from types or sth) ends up giving u different types for which u can't write separate impls. It pretty much results in higher ranked function pointers never being able to implement user defined traits.