Repository navigation
When subtyping can switch out trait impls, Pin::new’s check for Target: Unpin becomes insufficient #134407
Description
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
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)
Activity
[-]When subtyping can change switch out trait impls, `Pin::new`’s check for `Target = Unpin` becomes insufficient[/-][+]When subtyping can change switch out trait impls, `Pin::new`’s check for `Target: Unpin` becomes insufficient[/+]compiler-errors commentedon Dec 17, 2024
lol
apiraino commentedon Dec 17, 2024
A bit more on this.
@rustbot label +regression-from-stable-to-beta
lcnr commentedon Dec 17, 2024
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
5 remaining items
danielhenrymantilla commentedon Dec 20, 2024
I have another snippet which pushed things further to cover this "
Unpinchange", while also solving the missingCoerceUnsized(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 commentedon Dec 21, 2024
@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 commentedon Dec 23, 2024
a type wrapping
PinchangesPtrto be invariant as it's used in an associated type (as part of the default arg)RalfJung commentedon Jan 2, 2025
Pin<P>fundamentally relies on reasoning about "the Deref/DerefMut impl ofP", 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 commentedon Jan 2, 2025
Why do higher-ranked function pointers have subtyping anyway? And how much code relies on that?
[-]When subtyping can change switch out trait impls, `Pin::new`’s check for `Target: Unpin` becomes insufficient[/-][+]When subtyping can switch out trait impls, `Pin::new`’s check for `Target: Unpin` becomes insufficient[/+]Jules-Bertholet commentedon Feb 4, 2025
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_checkin 1.84 should be reverted at least temporarily?lcnr commentedon Feb 11, 2025
Doing so would be feasible by adding a flag to
TypingMode::Coherenceto 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.
Jules-Bertholet commentedon Jun 10, 2025
How breaking would it be to require all
Derefimpls to have aTargetcovariant inSelf? Is that something the compiler could realistically enforce?Darksonn commentedon Aug 4, 2025
I believe I solved the equivalent problem with unsizing coercions (ref: #68015) by adding the
PinCoerceUnsizedtrait and only allowing unsizing coercions whenWrap: PinCoerceUnsized. In this case, the problem seems to be the same. We allow a coercion even thoughWraphas not opted in to the coercion.Could we add a requirement that
Pin<T>can only be subtyping coerced ifT: PinCoerceUnsized?danielhenrymantilla commentedon Aug 5, 2025
Subtyping is not a coërcion, is it? It's type-system leniency. If I had to guess, I'd suspect it will either be impossible to implement such a check in all the subtyping logic of the compiler, or that at least it ill be quite convoluted to do so, I'd presume (but I may be wrong).
If we were to go overhaul subtyping like that, I'd rather we instead be given the option to explicitly override the variance of a type.
Rollup merge of rust-lang#156935 - Darksonn:pin-safe-pointer, r=Mark-…
Unrolled build for #156935