Unsoundness in `Pin`

language design
73

Getting &Ptr<T> is safe for most pointer types, but dangerous for Rc. If you could go from Pin<Rc<T>> to &Rc<T>, you could clone it to get a new unpinned Rc<T>, drop the pinned version, then (if there were no other references) use Rc::get_mut to get &mut T.

However, in most cases you can't add your own impls to Rc<T> because it's not marked #[fundamental]. And the existing impls of the relevant traits for Rc<T> are safe because they delegate to the impls for T, passing on only &T rather than &Rc<T>.

But the coherence rules are more flexible for traits that have type parameters! And that includes the comparison traits, like PartialEq. You can write an impl like:

impl PartialEq<Rc<RemoteType>> for LocalType

or even

impl PartialEq<LocalType> for Rc<RemoteType>

though the latter is legal only if RemoteType is non-generic (but that's due to change in the future).

Combine that with the impl for Pin:

impl<P, Q> PartialEq<Pin<Q>> for Pin<P>
where
    P: PartialEq<Q>,
{
    fn eq(&self, other: &Pin<Q>) -> bool {
        self.pointer == other.pointer
    }
    // [...]
}

And sure enough... here is a new exploit, for what I'll call Method 4:

Playground link

This exploit cheats in one respect. There's nothing in the standard library that actually depends on the pin guarantee of Pin<&T>; there's only futures, which require a mutable reference. So the exploit includes a copied-and-pasted version of @withoutboats' pin-cell crate. This crate exposes a variant of RefCell which works with Pin: given a Pin<&PinCell<T>>, you can get Pin<&mut T>, with uniqueness being checked at runtime. The implementation uses unsafe code, but the interface is intended to be compatible with the design of Pin.

Without pin-cell, you can still observe move-before-Drop, but no dramatic crashes.

(The reason I copied and pasted pin-cell instead of just depending on it is that the playground doesn't have it available. The exploit does not depend on PinCell being crate-local.)

I'm not sure how to fix this hole. A PartialEq impl between an Rc<T> and a non-Rc type is a little strange, but it's not obviously insane, unlike the impls involved in the other exploits. It seems considerably more likely that banning such impls altogether would break real code. So I think we should go with the unsafe-marker-trait approach, at least when it comes to PartialEq and friends (we could still ban crazy impls of DerefMut and Clone). We'd have to add some sort of bound to either Rc::pin, which is currently implemented for all T: Sized, or to Pin's PartialEq impl. Not sure which is better.

14 Likes