The safety requirements for using custom smart pointers with `Pin` are insufficient

[Darksonn]

For user-defined smart pointer types P that deref to an !Unpin type, the API of Pin exposes exactly three different ways of creating a Pin<P> value without already having a Pin<P> value:

1. Calling Pin::new_unchecked.
2. Performing an unsizing coercion.
3. Performing a subtyping coercion.

This issue is about methods 1 and 2 only (see #134407 on method 3). Methods 1 and 2 are both gated behind use of unsafe:

1. The Pin::new_unchecked method itself is unsafe.
2. Unsizing coercions are only possible with pointers that implement the unsafe trait PinCoerceUnsized.

However, the safety requirements listed about the pointer type are insufficient.

Pin::new_unchecked (affects stable)

The safety requirements of Pin::new_unchecked says a lot of stuff, but with one exception, everything it says talks about specifically the value behind the pointer, and not the pointer itself. The only thing it says about the pointer is this:

By using this method, you are also making a promise about the Deref, DerefMut, and Drop implementations of Ptr, if they exist. Most importantly, they must not move out of their self arguments: Pin::as_mut and Pin::as_ref will call DerefMut::deref_mut and Deref::deref on the pointer type Ptr and expect these methods to uphold the pinning invariants.

However, there are safe traits other than Deref, DerefMut, and Drop we need to be careful about. Here is an example illustrating why Clone is also important:

use std::ops::Deref;
use std::sync::Arc;
use std::pin::Pin;
use std::marker::PhantomPinned;

struct Malicious<T> {
    inner: Arc<T>,
    extra: Arc<T>,
}

impl<T> Deref for Malicious<T> {
    type Target = T;
    fn deref(&self) -> &T {
        &self.inner
    }
}

impl<T> Clone for Malicious<T> {
    fn clone(&self) -> Malicious<T> {
        Malicious {
            // notice extra is moved to inner
            inner: self.extra.clone(),
            extra: self.extra.clone(),
        }
    }
}

fn main() {
    // we consider arc1 pinned
    let arc1 = Arc::new(PhantomPinned);
    // we do not consider arc2 pinned
    let mut arc2 = Arc::new(PhantomPinned);

    let m: Malicious<PhantomPinned> = Malicious {
        inner: arc1,
        extra: arc2.clone(),
    };

    // SAFETY: `arc1` will not be moved out of again
    let m_pinned = unsafe { Pin::new_unchecked(m) };
    
    let m2 = m_pinned.clone();
    let m2_ref: Pin<&PhantomPinned> = m2.as_ref();
    // somehow we obtained a pinned reference to arc2
    assert!(core::ptr::eq(&*m2_ref, &*arc2));
}

PinCoerceUnsized (unstable only)

The same applies to the PinCoerceUnsized. It makes various requirements about the Deref and DerefMut traits, but it says nothing about Clone.

#![feature(pin_coerce_unsized_trait, derive_coerce_pointee)]
use std::marker::CoercePointee;
use std::sync::Mutex;
use std::ops::Deref;
use std::sync::Arc;
use std::pin::{Pin, PinCoerceUnsized};
use std::marker::PhantomPinned;

static EXTRA: Mutex<Option<Arc<PhantomPinned>>> = Mutex::new(None);

#[derive(CoercePointee)]
#[repr(transparent)]
struct Malicious<T: ?Sized> {
    inner: Arc<T>,
}

// SAFETY: A given value of `Malicious<T>` never changes which object it
// points at.
unsafe impl<T: ?Sized> PinCoerceUnsized for Malicious<T> {}

impl<T: ?Sized> Deref for Malicious<T> {
    type Target = T;
    fn deref(&self) -> &T {
        &self.inner
    }
}

impl Clone for Malicious<dyn MyTrait> {
    fn clone(&self) -> Malicious<dyn MyTrait> {
        Malicious {
            inner: EXTRA.lock().unwrap().clone().unwrap(),
        }
    }
}

trait MyTrait {
    fn to_phantom_pinned(self: Pin<&Self>) -> Option<Pin<&PhantomPinned>>;
}
impl MyTrait for PhantomPinned {
    fn to_phantom_pinned(self: Pin<&Self>) -> Option<Pin<&PhantomPinned>> {
        Some(self)
    }
}
impl MyTrait for i32 {
    fn to_phantom_pinned(self: Pin<&Self>) -> Option<Pin<&PhantomPinned>> {
        None
    }
}

fn main() {
    *EXTRA.lock().unwrap() = Some(Arc::new(PhantomPinned));
    
    let foo = Pin::new(Malicious { inner: Arc::new(42) });
    let bar: Pin<Malicious<dyn MyTrait>> = foo;
    let baz = bar.clone();
    let abc: Pin<&PhantomPinned> = baz.as_ref().to_phantom_pinned().unwrap();
    
    let extra = EXTRA.lock().unwrap().take().unwrap();
    
    // somehow we obtained a pinned reference to extra
    assert!(core::ptr::eq(&*abc, &*extra));
}

Everything in the safety requirements of PinCoerceUnsized talks about the fact that an individual value must not change its identity, and that is true for Malicious.

Not just the Clone trait

There are a bunch of other traits that can potentially be abused in this manner. For example, the implementations of fmt::Debug, fmt::Display, and fmt::Pointer are all called with an &P reference, and creating such a reference is unsound.

Potential solutions

Reword both safety requirements

We can of course reword both Pin::new_unchecked and PinCoerceUnsized to comprehensively list all traits you must not implement maliciously.

Implement dangerous traits in terms of new traits

We could introduce traits like these:

trait PinClone {
    fn clone(self: &Pin<Self>) -> Pin<Self>;
}
impl<Ptr: PinClone> Clone for Pin<Ptr> { ... }

impl PinDebug {
    fn fmt(self: &Pin<Self>, f: &mut Formatter<'_>) -> Result<(), Error>;
}
impl<Ptr: PinDebug> Debug for Pin<Ptr> { ... }
// similarly for Display and Pointer

This would be in addition to the existing unsafe trait PinCoerceUnsized.

The main disadvantage of this is that it's a breaking change.

Expand the scope of PinCoerceUnsized

We already have an unsafe trait guarding unsizing coercions called PinCoerceUnsized. We could expand the scope of this trait. For example, we could rename PinCoerceUnsized to PinSafe and rewrite its documentation to also make requirements about Clone and any other trait like this. Then, we can reword Pin::new_unchecked to require that the pointer type satisfies the safety requirements of PinSafe (we cannot require that it implements PinSafe for backwards compat, but we can require that it could implement it).

Perhaps in a future edition, Pin::new_unchecked could require P: PinSafe to simplify what is listed in its safety requirements.

Click to view sample documentation for PinSafe

Trait indicating that this pointer type is safe to use with Pin.

By implementing this unsafe trait, you are asserting that the API of Pin<_> is sound when Self is the pointer type.

This trait has to do with the pointer type, and should not be confused with the pointee type. For example, given the type Pin<Box<SomeTypeOfFuture>>, the type that must be pin safe is Box, not SomeTypeOfFuture.

This is important because there are certain operations that are not safe to perform on a pinned value in general, but Pin implements various traits by performing those dangerous operations and passing the resulting dangerous values to safe trait implementations on the pointer type. A pointer is considered pin safe if the implementations of these safe traits does not abuse these dangerous values.

A few examples of dangerous operations that Pin may perform when it calls into trait implementations of the pointer type can be found below:

• Calling Pin::new_unchecked. This is dangerous because it assumes the pointed-to value is pinned.
• Creating a &mut T to the pinned value. This is dangerous because it can be used to move the value (e.g. with mem::swap).
• Creating a &P to the pointer type. This is dangerous because reference counted pointer types assume that if one refcounted pointer is wrapped in Pin, then they all are. For example, obtaining an &Arc<T> from a Pin<Arc<T>> is a problem because you can clone the Arc and eventually call Arc::get_mut and move the pinned value.

The below sections explains what a pointer type must satisfy to be pin safe.

No moving out

This pointer type must not provide any API that lets the end-user move the
pointed-to value unless it is !Unpin.

As an example, if this pointer type implements DerefMut or Drop,
then methods such as Pin::as_mut, Pin::set, or the destructor of
Pin<_>, will pass a &mut T to DerefMut::deref_mut or Drop::drop on
the pointer type. The implementation of DerefMut or Drop on the
pointer type must not abuse this &mut T value to move the value.

Object identity

A pin safe pointer type will keep pointing at the same value. That is, the
address and concrete type returned by Deref/DerefMut does not
change.

For example, if you have two values A and B of the same type where only A is
pinned, then it would be illegal for the implementation of Deref or
DerefMut to first return a reference to A, and then later return a
reference to B.

This applies even if the Pin<P> value is moved, or if deref/deref_mut
was called, or if an unsizing coercion was applied to the pointer, or if the
pointer was used with dynamic dispatch. Or any other API the pointer type
may choose to provide.

Clone

The Pin<P> type implements Clone by:

1. Create a &P to the pointer type.
2. Call P::clone with the &P value.
3. Call Pin::new_unchecked on the return value of P::clone.

As explained previously, both operation 1 and 3 are dangerous. If this
pointer type implements Clone, then it must not abuse the &P passed to
clone, and if the original pointer referenced a pinned value, it must
return something that is safe to pass to Pin::new_unchecked.

Formatting traits

The Pin type implements fmt::Debug, fmt::Display, and
fmt::Pointer by calling the fmt method of said trait on the pointer
type, which involves the unsafe operation of creating a &P reference.
If the pointer type implements any of the formatting traits, then the
implementation of said formatting trait must not use the &P reference in a
way that is unsound.

pub unsafe trait PinSafe {}

---

I personally prefer the last solution because it allows for deduplication. We do not need to list the same requirements twice: both for Pin::new_unchecked and PinCoerceUnsized.

[theemathas]

Previous discussion is on Zulip.

See also #148899 (comment) for a related unsoundness, where CoerceUnsized could be implemented directly without going through PinCoerceUnsized.

[BoxyUwU]

is this nightly only? also please make it clearer what the actual unsoundness is, this issue is huge and its hard to immediately find the unsound code example. It should probably go at the very start with explanation afterwards

[theemathas]

@BoxyUwU It is possible to write a custom pointer type that follows the rules for Pin::new_unchecked as written in the documentation (by the letter of the law), but cause UB, in stable rust. It's arguable that it's maybe not quite traditionally "unsound".

[theemathas]

Here's an argument for why, if we strengthen the wording for the safety requirements of Pin::new_unchecked and PinCoerceUnsized, then Pin's Debug/Display/Pointer/Clone impls would be sound (ignoring subtyping coercions and manual CoerceUnsized impls, which are covered in other issues). (This is not an argument on whether this is a good idea. It is intended to merely argue that the status quo isn't "disastrously unsound".)

Pin's Debug, Display, and Pointer impls are suspect only because they convert Pin<Ptr> into &Ptr. To cause UB without unsafe, one would have to construct a Pin<Ptr> for a non-Unpin target type. As per the issue description above, doing so for user-defined pointer types without unsafe is impossible. That leaves the pointer types in the standard library that provide their own ways to construct pinned versions of them. I will now enumerate such pointer types:

• &: Pin<&T> being converted to a &&T is harmless, since the Deref impl already provides access to a &T.
• &mut: Pin<&mut T> being converted to a &&mut T is harmless, since the Deref impl already provides access to a &T, and &&mut T can't really do anything more than a &T can.
• Box: Pin<Box<T>> being converted to a &Box<T> is harmless, since &Box<T> cannot be used to move the T to violate the pin guarantee.
• Rc/Arc: Rc<T> and Arc<T> cannot have user-defined implementations for Debug, Display, and Pointer, since Rc and Arc are not fundamental types. Therefore, Pin<Rc<T>> and Pin<Arc<T>> cannot have their Debug, Display, and Pointer impls be abused.
• Pin: Pin<Pin<Ptr>> being converted to a &Pin<Ptr> is harmless, since in order to construct a Pin<Pin<Ptr>> in the first place, we must first construct a Pin<Ptr>. Therefore, creating a &Pin<Ptr> can't really violate any pin guarantees.

Pin's Clone impl is suspect because it converts a Pin<Ptr> into a &Ptr, and then also converts a Ptr into a Pin<Ptr>. The first conversion can be argued to be sound using the same logic as above. For the conversion from Ptr into a Pin<Ptr>, user-defined pointer types are not an issue, for the same reason as above. Again, this leaves pointer types in the standard library. I will now enumerate such pointer types.

• &: &T already has a Clone impl in the standard library. Therefore, cloning a Pin<&T> will just call that Clone impl, which harmlessly returns a &T that points to the same T, which is therefore fine to re-pin.
• &mut: &mut T cannot ever implement Clone, due to a negative impl in the standard library.
• Box: Converting a Box<T> into a Pin<Box<T>> is harmless, since it already be done in safe code with Box::pin.
• Rc/Arc: Rc<T> and Arc<T> cannot have user-defined implementations for Clone, since Rc and Arc are not fundamental types. (This is true even when involving custom allocators that don't implement Clone.) Therefore, Pin<Rc<T>> and Pin<Arc<T>> cannot have their Clone impls be abused.
• Pin: We need to argue that it's OK to clone a Pin<Pin<Ptr>>. The problematic step is turning a Pin<Ptr> into a Pin<Pin<Ptr>>. This requires the Clone impl of Pin<Ptr> (which might be user-defined) to construct a Pin<Ptr> somehow. This is only possible if Ptr implements Deref (assuming that Make PinCoerceUnsized require Deref #149218 goes through). Therefore, constructing a Pin<Ptr> already pins the pointed-to value. Wrapping it in another layer of Pin just re-pins that same pointed-to value, which is harmless.

(Much of the complexity in this argument isdue to the rules for fundamental types. I believe there is no way to make these arguments that doesn't involve exhaustively listing all the fundamental types. This argument does not mention all the weird impls that a user can make, which require the argument to contort itself to avoid reasoning about such impls.)

[Darksonn]

is this nightly only?

No, Pin::new_unchecked is a stable function.

also please make it clearer what the actual unsoundness is, this issue is huge and its hard to immediately find the unsound code example. It should probably go at the very start with explanation afterwards

I posted two huge code blocks. Those are the two examples. Each example constructs Pin<&PhantomPinned> to a value that was never actually pinned. I did not bother to actually trigger UB using this, but you can do it.

As @theemathas points out, both code blocks contain a single use of unsafe. What I'm getting at with this issue is that the unsafe block follows the rules of said unsafe operation as stated in their docs, but still triggers UB. So this is not a case of safe-api-leads-to-UB, but a case of an unsafe API documenting its requirements incorrectly/incompletely.

[eggyal]

Related #147794