Unsoundness in `Pin`

Recently, @withoutboats challenged me to demonstrate a hypothetical version of Pin with different guarantees, and show how it would be sound. However, while working on that, I stumbled upon unsoundness in the actual Pin. I haven't seen this reported before, though I could have missed it.

Playground link: proof of concept (methods 1 and 2)

This code demonstrates two related exploits to obtain a Pin<&mut T> from &mut T, ultimately resulting in a segfault. It compiles on stable and has no unsafe blocks. The only non-standard-library imports are two things from futures; these are just for convenience and are not crucial to the exploit.

TL;DR:

  • Method 1: impl DerefMut for &dyn Trait
  • Method 2: impl Clone for &mut dyn Trait
  • Unstable-feature-dependent method 3 (not in the PoC): CoerceUnsized to type with different Deref impl
  • Edit: Method 4 from later in the thread

Method 1: DerefMut

Here's the definition of Pin::as_mut (abridged):

impl<P: DerefMut> Pin<P> {
    pub fn as_mut(&mut self) -> Pin<&mut P::Target> {
        unsafe { Pin::new_unchecked(&mut *self.pointer) }
    }
}

According to a comment, this is safe because:

/// "Malicious" implementations of `Pointer::DerefMut` are likewise
/// ruled out by the contract of `Pin::new_unchecked`.

This is mostly true. In general, you cannot safely obtain instances of Pin<P> for an arbitrary self-defined type P. If P implements Deref<Target: Unpin>, you can use the safe function Pin::new, but the Unpin condition makes the ability to get Pin<&mut P::Target> uninteresting by definition. Otherwise, you're left with either Pin::new_unchecked, which is unsafe, or various safe wrappers that return Pin<P> for specific choices of P, such as Pin<Box<T>>, Pin<&mut T>, and Pin<&T>. (T itself is arbitrary, but it has to be wrapped in one of those specific pointer types.)

However, just because we can't use our own types doesn't mean we can't use our own impls!

For reference: Coherence rules mean that you can only implement foreign traits, such as DerefMut, for what I'll call "local-ish" types. Local-ish types are normally structs, enums, and trait objects defined in the current crate. But if T is local-ish, then &T, &mut T, Box<T>, and Pin<T> are also treated as local-ish. These wrappers are called "fundamental", as described in RFC 1023.

Thus, for some local type Foo, we could try to add a DerefMut impl to any of &Foo, &mut Foo, Box<Foo>, or Pin<Foo>. Out of these, we can rule out &mut Foo and Box<Foo> because they already impl DerefMut. And we can rule out Pin<Foo> because it's not useful for our purposes, as there's no way to get a pinned version of it (i.e. Pin<Pin<Foo>> for arbitrary Foo).

That leaves just &Foo – and it works! It doesn't impl DerefMut already, and we can get a pinned version of it (i.e. Pin<&Foo> for arbitrary Foo) by just using Pin::as_ref.

So:

impl<'a> DerefMut for &'a Foo {
    // ...
}

We don't get to choose what type to deref to. DerefMut reuses the associated type Target from Deref, and immutable references already have an impl of Deref. In this case, Target will be Foo, so we must provide a function with the signature:

    fn deref_mut(self: &mut &'a Foo) -> &mut Foo

We then have to obtain a legitimately pinned Pin<&Foo> and call as_mut on it, which delegates to our custom deref_mut. self will be that legitimately pinned reference, but we can return a completely different reference, and as_mut() will then unsafely wrap it in a Pin.

The fact that both input and output have to point to Foo is somewhat constraining, but not fatal. To be able to return a reference without unsafe code*, the reference needs to be stored inside the input Foo, while the output Foo has to be some !Unpin type that's actually dangerous to move after pinning. The most straightforward approach (though not the only one) is to make Foo a trait object, so that the input and output can be different concrete types entirely. For more details, please look at the playground link.

* Well, we could safely return &'static mut references using Box::leak, but that wouldn't be exploitable: there would be no way to recover the reference afterwards.

Method 2: Clone

Pin derives Clone:

#[derive(Copy, Clone, Hash, Eq, Ord)]
pub struct Pin<P>

Similarly to how Pin::as_mut calls deref_mut on the pointer and pins the result, the derived Pin::clone calls clone on the pointer and pins the result.

Like before, the only way to exploit this is to add a Clone impl to an existing fundamental pointer type. Once again, the options are &Foo, &mut Foo, Box<Foo>, and Pin<Foo>. This time, &Foo is out because it impls Clone (along with Copy). Pin<Foo> is still not useful.

Box<Foo> impls Clone if Foo does. We could add a custom impl Clone for Box<Foo> if Foo doesn't impl Clone, but it wouldn't be useful. If we hand out a Box<Foo>, Pin::clone will convert it to Pin<Box<Foo>>... but there's already a way to do that: the standard library's impl<T> From<Box<T>> for Pin<Box<T>>. Pinning existing boxes is perfectly safe, because once a Box is wrapped in a Pin, there's no way to take it back out.

That leaves &mut Foo, which has no built-in Clone impl. In contrast to boxes, pinning existing references is dangerous, because we could pin a reborrowed reference, then effectively "take it back out" by letting the lifetime expire and accessing the reference we originally reborrowed from. That's how method 1 works, and we can do the same here.

The required type signature is strange, but similar to the one in method 1. Last time we had to implement

    fn deref_mut(self: &mut &'a Foo) -> &mut Foo

This time we have to implement

    fn clone(self: &&'a mut Foo) -> &'a mut Foo

Instead of a mutable reference to an immutable reference, we have an immutable reference to a mutable reference. And the lifetimes are in different places. But otherwise it's the same, so the exploits look very similar.

Unstable-feature-dependent method 3: CoerceUnsized

Well, I can't say I discovered this one, since it's more or less explained already in a comment:

// Note: this means that any impl of `CoerceUnsized` that allows coercing from
// a type that impls `Deref<Target=impl !Unpin>` to a type that impls
// `Deref<Target=Unpin>` is unsound. Any such impl would probably be unsound
// for other reasons, though, so we just need to take care not to allow such
// impls to land in std.
impl<P, U> CoerceUnsized<Pin<U>> for Pin<P>
where
    P: CoerceUnsized<U>,
{}

This impl is what lets you do an unsize coercion from, e.g., Pin<&T> to Pin<&Trait>, in conjunction with the CoerceUnsized impl for &T itself.

Contrary to the comment, I'd say the most immediate danger is the other way around, from Unpin to !Unpin. Specifically, an impl that allows coercing from a type P that impls Deref with Target: Unpin, to a type U that impls Deref with Target: !Unpin. That way, you can use Pin::new to create a Pin<P>, then coerce it to a Pin<U>. This impl wouldn't necessarily be "unsound for other reasons". In particular, it wouldn't require unsafe code, since there's no requirement that the Deref impls of P and U have anything to do with each other: they can return different references of completely different types.

For now, CoerceUnsized is behind a feature gate, so the comment's admonition to "take care not to allow such impls to land in std" makes sense. Without such impls, the issue is not exploitable on stable. But for the record, it is exploitable on nightly by activating that feature gate:

Playground link: proof of concept

If it's not exploitable on stable, why am I mentioning it? Because sooner or later we will want to stabilize CoerceUnsized in some form. Even if you're not trying to write your own standard library, it's common enough to implement your own smart pointer types. But while the std smart pointers all have CoerceUnsized implementations, allowing you to, e.g., coerce Rc<T> to Rc<Trait>, there's currently no way to get the same ability for your own types. This clearly should be fixed.

See also: Tracking issue for DST coercions (coerce_unsized, unsize) stabilization

Based on that thread, it seems like a stabilized version will look somewhat different from the current CoerceUnsized, and it may have soundness concerns all on its own. But without Pin, I don't think there would be any reason for soundness to be affected by Deref impls. Now it is, and I'm not sure whether the fix should be to change coercions or to change Pin. See below for more discussion of fixes.

What about existing CoerceUnsized impls?

Is there some way to exploit Pin with one of the existing standard library impls of CoerceUnsized? The goal would be have a type that impls CoerceUnsized where we can arbitrarily control what it derefs to: either by adding our own Deref implementation, or by taking advantage of the existing implementation somehow.

TL;DR: No.

Here is the list of CoerceUnsized impls:

Pointer-like impls:

(of the form impl<T, U> CoerceUnsized<Foo<U>> for Foo<T> where T: Unsize<U>)

  • Ref<T>
  • RefMut<T>
  • Unique<T>
  • NonNull<T>
  • &T
  • &mut T
  • *const T
  • *mut T

Of these, Ref<T>, RefMut<T>, &T, and &mut T all impl Deref already, but they're "boring": they all simply cast the self pointer to a different type. There's nothing we can control. For the rest, we could try to add our own Deref impl, but none of them are fundamental.

(We can add a DerefMut implementation to &T, but that's just method 1, which doesn't require using coercion.)

It's somewhat surprising that raw pointers are not fundamental when references are – in other words, you can't implement arbitrary traits for *const MyType. It feels like an oversight that should be fixed, but doing so would make this exploitable.

Wrapper-like impls:

(of the form impl<T, U> CoerceUnsized<Foo<U>> for Foo<T> where T: CoerceUnsized<U>)

  • Cell<T>
  • RefCell<T>
  • UnsafeCell<T>
  • Pin<T>

The first three don't impl Deref.

Pin has impl<P: Deref> Deref for Pin<P>. It's also fundamental, so we could impl Deref for Pin<Foo<T>> – but only if Foo is local-ish and doesn't impl Deref, which is the same condition for being able to add a Deref impl. So we're left needing exactly what we wanted in the first place: a type whose Deref impl we can either control or write ourselves, which also impls CoerceUnsized. This doesn't help.

Non-method: Subtyping

Another way to change the type of a Pin is using variance, but that only lets you change lifetimes.

In theory you could have a type that's only conditionally Unpin depending on lifetimes, so that after changing lifetimes, you're left with a Pin to a !Unpin type:

struct Foo<'a>(&'a ());
impl Unpin for Foo<'static> {}

fn test<'a>(p: Pin<&'a Foo<'a>>) {
    Pin::into_inner(p); // error: does not impl Unpin
}
fn main() {
    test(Pin::new(&Foo(&())));
}

But I don't think there's any way to actually exploit this.

Potential fixes

Since the proof-of-concept works on stable, fixing it requires a breaking change by definition.

The most straightforward fix seems to be to prevent users from implementing DerefMut for immutable references or Clone for mutable references. I believe this could be done by adding dummy blanket impls to the standard library:

pub trait DummyTrait {}
pub struct DummyStruct<T>(T);
impl<'a, T> DerefMut for &'a T where DummyStruct<T>: DummyTrait { ... }
impl<'a, T> Clone for &'a mut T where DummyStruct<T>: DummyTrait { ... }

[Edit: Changed to something that actually works.]
[Edit2: @CAD97 pointed out that there's an unstable feature specifically for reserved impls that could be used instead.]

I'm curious to learn whether this would break any actual code.

What about when CoerceUnsized is stabilized? How do we make it safe to implement CoerceUnsized on your own types? I can think of two basic approaches:

  1. We could require CoerceUnsized to be "coherent" with respect to Deref and DerefMut, in the sense that calling deref() and deref_mut() gives you the same value before and after coercion.

    It sounds hard for the compiler to enforce that, though, especially considering that we don't even require deref and deref_mut to be pure. We could make CoerceUnsized unsafe and offload enforcement to the programmer, but that seems unfortunate if it doesn't fundamentally need to be unsafe.

  2. Or we could simply condition Pin's CoerceUnsized impl on an unsafe marker trait. So if you define a struct Foo that coerces to Bar, you still couldn't coerce Pin<Foo> to Pin<Bar>, without implementing the unsafe trait for Foo.

However, it's quite possible that I'm missing a nicer solution, especially considering that the stable version of CoerceUnsized isn't even fully designed yet.

53 Likes

Forgive me if this is a gross misunderstanding of the situation, but I get the impression from looking at your code that exploits 1 and 2, which are the most urgent concern, essentially rely on shared mutability:

  • You cannot do anything "interesting" inside of impl DerefMut for &T unless T has some form of shared mutability, because all you can do otherwise is swap a shared reference with another.
  • You cannot safely impl Clone for &mut T unless you have a way to invalidate the old &mut T that was "cloned", which in turn you have to do via shared mutability as otherwise &&mut T does not allow mutating T per Rust's inherited mutability model.

In this sense, I believe the most fundamental problem here is that the safe Pin::new() function accepts (reference) types with shared mutability, while it's pretty clear that those types violate the spirit of the contract of the underlying Pin::new_unchecked constructor, even if they may not strictly violate its rule as written:

This constructor is unsafe because we cannot guarantee that the data pointed to by pointer is pinned, meaning that the data will not be moved or its storage invalidated until it gets dropped. If the constructed Pin<P> does not guarantee that the data P points to is pinned, that is a violation of the API contract and may lead to undefined behavior in later (safe) operations.


This sort of validates a persistent feeling I've had that Pin's strategy of exposing &T but not &mut T as a protection against data invalidation is very fragile in the presence of shared mutability. So far, we settled on the weak "only the object directly pointed by Pin is protected from moving" interface contract compromise as a response to this concern, which had the sole effect of making Pin hard to use correctly, but from your study it seems that even this unsatisfactory contract is not fulfilled by the current Pin implementation.

Maybe we do need to have a stable "no transitive shared mutability" trait, after all? And then Pin::as_ref() would become unsafe when that trait is not implemented, much like Pin::as_mut() already is when Unpin is not implemented? In addition to fixing both problems described above, this would also have the effect of generally making Pin a much more ergonomic abstraction, as it would then follow the expectations of people used to Rust references' inherited mutability rules.


As an aside, I'm not a fan of the DummyTrait solution, because that totally sounds like something that will be forgotten and silently break if coherence checking ever gets just a tiny bit smarter about blanket impls, which I'm sure many people wish would happen someday.

Here's to hoping that this trick is not already used to prevent implementation of some traits in the wild...


The CoerceUnsized issue seems different though. Maybe CoerceUnsized should actually be unsafe to implement? I have not spent enough time thinking about it, but reading through the CorceUnsized documentation it intuitively feels unsound that you can just safely write impl<U, T: CoerceUnsized<U>> CoerceUnsized<StrangeWrapper<U>> for StrangeWrapper<T> {}, which is morally equivalent to impl<U, T: CoerceUnsized<U>> CoerceUnsized<U> for T {}, without getting some sort of typechecker error message. Doesn't that open soundness holes in other areas?

EDIT: Let me try to clarify that feeeling a bit. CoerceUnsized is normally used to allow coercion of pointer-to-sized types to pointer-to-unsized types, such as &[T; N] -> &[T], which is generally a safe thing to do. However the reverse operation is quite clearly not safe: allowing a coercion from &[T] to &[T; N] without any form of run-time checking would trivially break memory safety if N were greater than the input slice's length. So why can we seemingly write a trait impl that allows that in safe code?

4 Likes

@comex Wow, good catch! What makes this particularly interesting is how it interacts with the trait system -- Pin::new/Pin::as_mut cannot really be verified inside the scope of the first RustBelt paper as that does not model the trait system.

So you are saying both of these exploits rely on Pin::as_ref()? For the first that's clearly true; the second does not seem to call that method but indeed the Pin implementation of Clone basically re-implements as_ref to be able to call T::clone.

I do not see how Pin::as_ref makes Pin harder to use correctly, could you elaborate? In fact, the main motivation for adding it was making it simpler to use as you could use all existing &self-based APIs, to e.g. Debug print something pinned or Clone it or whatnot, without having to re-implement everything. Not having as_ref would arguably make Pin much more painful than it currently is.

3 Likes

If you spend enough time on Rust forums reading through topics written by people new to the Pin API, you will probably end up sharing my impression that one recurring pain point for people learning the API and trying to figure out whether it can address their particular self-referential struct problem, is getting to grips with the rule that only the direct target of the pinned pointer is protected from moving, not the target of any other pointer recursively owned by that object.

I believe this learning pain is partially caused by the fact that Rust's collection APIs try very hard to make owned heap pointers look and feel exactly like owned stack values, which Just Works most of the time thanks to RAII-based deallocation and inherited mutability. Unfortunately, however, Pin breaks this illusion and forces you to carefully consider what is a value and what is an owned pointer in your data model, which is not something that people are used to doing in Rust. Ergo, Pin feels unintuitive.

(To be honest, this is not specific to owned pointers, and applies to any other construct that allows you to move part of a struct's owned data without moving the whole struct, such as Option<T> or mem::replace. But owned pointers are where people seem to encounter this issue most often in practice...)

And this is part of how we end up with silly situations like struct field accesses becoming a scary and complex topic worthy of about 3 A4 pages of discussion, though admittedly Drop also plays a role here.

I am not advocating against Pin::as_ref() being a safe method in general, only against it being a safe method for types with shared mutability, whether direct or transitive. That should at least be opt-in, much like implementing Sync for a type which contains raw pointers is.

2 Likes

Why would this be enough?

The compiler knows that DummyTrait is not implemented for the type the user provides due to coherence (assuming the type is defined in the crate with the exploit impl), and so that impl doesn't prevent the exploit impl, no?

Seems like the simplest thing would be to use specialization to have Pin::as_mut and Pin::clone panic in the exploitable cases, which would be backwards compatible (assuming no code relies on this behavior).

Even better, make them unavailable at compile time, although that requires to support specialized negative impls.

1 Like

Here's just the first method without the macro to switch between both; I found that easier to follow.


As you said yourself in the next paragraph, this is not at all about stack vs. heap, and it also has nothing inherently to do with pointers. It is about containers in general and that you can pin a container without pinning its content (whether that content is stored in-line or behind a ptr indirection is irrelevant). And that is a core feature of pinning.

No, I disagree on the causal connection you are drawing here. The proper fix for this complex discussion is support for pinning in the compiler.


I have noticed that as a common point of confusion, but I see no strong connection between that as the safety of as_ref. Back when the API got discussed, I was worried about safe as_ref (mostly because it made the formalism more tricky), but my proposed alternative still would make pinning a non-structural property. I think changing pinning the way you suggested (that everything recursively is pinned) would make it basically useless -- after all, self-referential async fn still want to have the ability to move out of their unpinned local variables.

Whether pinning propagates into inner data and whether as_ref is safe are distinct considerations, the only connection is that one of the four possible combinations (pinning always propagates + as_ref is safe) is unsound. But propagating pinning was never a reasonable option for us (AFAIK).

I also don't see the soundness issue as being caused by not propagating pinning. The soundness issue was introduced when we moved from Pin+PinMut+PinBox (dedicated pinned pointer types) to the generic Pin wrapper, and made that rely on existing safe traits only (Deref+DerefMut).

So, I agree with all of your factual statements but with basically none of the conclusions and causal relationships you are deducing from those. :wink:


The proper fix IMO is to require some unsafe trait in Pin::new and Pin::deref/Pin::deref_mut that is used as an explicit marker that the type and Deref/DerefMut impl are well-behaved:

unsafe trait PinDeref: Deref {}
unsafe trait PinDerefMut: PinDeref+DerefMut {}

Then these two should be used everywhere that Deref/DerefMut is used in the Pin API. In fact I had a bad feeling about this exact point when the API landed, but was unable to actually construct a tangible example... maybe I should trust my feelings more. :wink:

Alas, now it is probably too late for the proper fix.

15 Likes

Talking about our stability guarantees, aren't soundness problems exempt?

25 Likes

As a soundness fix, I'd argue adding an unsafe trait bound to Pin::new/deref/deref_mut is about as uninvasive as a breaking change can get. The only requirement is to add the marker trait impl, which is about as trivial as a fix can get.

The other "morally correct" fix of preventing "lying" deref/mut implementations on #[fundamental] types is more theoretically breaking, as it removes potentially correct code (in the absence of pin) without providing a simple alternative. Although, this one has the advantage of not breaking anything unless it actually has a chance of being unsound, and there's a decent chance it's not even used for legitimate cases.

9 Likes

You're right, it wouldn't be enough as written. I misunderstood the coherence rules around blanket impls.

For the record, I'm somewhat persuaded by the suggestions by others to use a different approach entirely, like @RalfJung's PinDeref and PinDerefMut. But it is possible to fix the DummyTrait approach, so I'll explain how. (I actually tested it this time...)

With my original suggestion:

impl<'a, T> DerefMut for &'a T where T: DummyTrait { ... }

the compiler does not prevent downstream crates from adding impls for local types that don't impl DummyTrait. This is because the way std could create a conflicting impl would be to add a "blanket impl", impl<T> DummyTrait for T (possibly with where clauses), and adding a blanket impl of a trait is always a breaking change (see last two paragraphs).

However, an impl like impl<T> DummyTrait for DummyStruct<T> is not considered a blanket impl, and is not a breaking change. Therefore, if you have:

pub trait DummyTrait {}
pub struct DummyStruct<T>(T);
impl<'a, T> DerefMut for &'a T where DummyStruct<&'a T>: DummyTrait { … }

Trying to add an impl in a downstream crate now produces this error (here, cra is the parent crate playing the role of std):

error[E0119]: conflicting implementations of trait `cra::DerefMut` for type     `&Foo`:
 --> src/main.rs:3:1
  |
3 | impl DerefMut for &'static Foo {}
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: conflicting implementation in crate `cra`:
          - impl<'a, T> cra::DerefMut for &'a T
            where cra::DummyStruct<&'a T>: cra::DummyTrait;
  = note: upstream crates may add new impl of trait `cra::DummyTrait` for type  `cra::DummyStruct<&Foo>` in future versions

It's also not possible for a downstream crate to try to satisfy the dummy impl by implementing DummyTrait:

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
 --> src/main.rs:3:1
  |
3 | impl DummyTrait for DummyStruct<&'static Foo> {}
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ impl doesn't use types inside crate

The other option is to reserve the impl (basically a poor man's negative impl)

#[rustc_reservation_impl=
    "a custom implementation would be permitted since &_ is fundamental, but be very unexpected, \
    and Pin requires the lack of this impl for soundness"]
impl<T: ?Sized> DerefMut for &'_ T {
    fn deref_mut(&&T) -> Self::Target {
        panic!("not allowed")
    }
}

#[rustc_reservation_impl=
    "a custom implementation would be permitted since &_ is fundamental, but be very unexpected, \
    and Pin requires the lack of this impl for soundness"]
impl<T: ?Sized> Clone for &'_ mut T {
    fn clone(&&mut T) -> Self {
        panic!("not allowed")
    }
}
11 Likes

Ah, cool. Didn't know about that one.

I think this actually has nothing to do with Pin.as_ref and Pin.as_mut, and everything to do with the CoerceUnsized impl for Pin. Here's the POC with the as_ref and as_mut calls removed. Note that as_mut is still called so that we can poll the future, but this occurs after we've constructed the 'evil' future. Once we have Pin<&dyn Dubious>, it's game over.

It's entirely unecessary to call as_ref or as_mut when constructing the 'evil' pin, due to the CoerceUnized impl for Pin. You can simply write let evil: Pin<&dyn Dubious<'_>> = Pin::new(&Wrapper), which due to the implicitly inserted cast, will just work.

The actual calls to as_ref or as_mut, in and of themselves, do nothing. When we start with a Pin<&Wrapper>, calling as_ref will give us a Pin<&((&Wrapper) as Deref>::Target)> - a.k.a a Pin<&Wrapper>, since <&Wrapper as Deref>::Target> == Wrapper. However, the CoerceUnsized impl for Pin means that the return value is implicitly cast to the desired type (Pin<&dyn Dubious<'_>>).

I think the root issue here is that the CoerceUnsized impl for Pin allows you to violate the contract of Pin, without using unsafe When you construct a Pin normally, you must either:

  1. Use Pin::new, which asserts that the target type is Unpin.
  2. Use Pin::new_unchecked, which requires to assert via unsafe that your pointer type upholds the contract of Pin.

However, using the CoerceUnized impl for Pin, you can write Pin<T> as Pin<U> when U: CoerceUnsized<T> holds. At no point were you required to assert (via unsafe) that U actually upholds the contract of Pin. As @comex showed, this can be weaponized via dyn Trait - you can go from a Pin<&T> to a Pin<&dyn MyTrait>, despite the fact that &dyn MyTrait does not deref to an Unpin type (dyn MyTrait). You should have been required to prove that dyn Trait is Unpin (it isn't) or that &dyn Trait upholds the contract of Pin (it doesn't, via its 'malicious' DerefMut impl).

10 Likes

Basically, I think that @comex's Method 1and Method 2 are both special cases of Method 3. In particular:

I think this is the other way around - Method 1 does require coercion, but the coercion is obscured by the intermediate Pin.as_ref call.

3 Likes

I think there are two ways to solve this:

  1. Restrict the CoerceUnized impl for Pin to only allow coercing to types U such that <U as Deref>::Target: Unpin holds. Essentially, this is the same requirement as Pin::new. Note that this would by design rule out coercing to types which deref to !Unpin types - see approach two.

  2. Add a new unsafe trait PinSafePointer, similar to @RalfJung's proposed PinDeref and PinDerefMut. This is added as a bound on U in the CoerceUnsized impl for Pin. Implementing this trait for a type T requires that every possible value of T is safe to pass to Pin::new_unchecked. Effectively, this is expressing the contract of Pin::new_unchecked at the type level, rather than the value level.

Personally, I would lean towards option 1. I think this discussion has shown that CoerceUnized is very confusing and subtle, especially when it interacts with Pin. I think allowing Pin coercions is fine in the 'obviously correct' case of coercing to a type that derefs to an Unpin.

However, I think the kind of coercions involved in the POC (coercing to a type which derefs to a !Unpin) type will essentially never occur in real code. If someone has a desire to convert between two such Pin types, I think they should be required to explicitly acknowledge the implications of doing so by first calling, Pin.into_inner_unchecked, calling deref/deref_mut, and constructing a new Pin via Pin::new_uncheckd.

The one thing I'm not certain about is how this interacts with dynamically sized types. Does restricting the CoerceUnsized impl make certain use now impossible, or only achievable via transmute?

1 Like

They are, but we ended up with a hacky fix that appears to have minimal breakage in the ecosystem and a elegant/"real" fix that has catastrophic breakage, we may very well take the hacky one.

1 Like

In general, I think it's a giant red flag to have a type MyType such that

  1. MyType has at least one private field
  2. An impl CoerceUnsized<MyType> for ... exists, where ... is anything (a type variable, a concrete type, etc).

With this combination, you're saying that:

  1. You dont want consumers outside the module constructing arbitrary instances of MyType (i.e. with fields set to arbitrary values). It's likely that there's some additonal invariant you want to maintain.
  2. You do want with consumers outside the module to construct instances of MyType from arbitrary values of some other type.

While it's possible that both could be true, it seems really unlikely that anyone would actually want this. I wonder if it would make sense to have a lint for this.

EDIT: Cell, Rc, and Arc meet the above conditions, but are perfectly fine, since they can in fact be constructed for any T. Maybe it would be better to make CoerceUnsized an unsafe trait.

4 Likes

I'm not sure if this is the right thread, but what could have been done differently to avoid this problem? Should Pin have baked another year or were we doomed to do something wrong just due to the complexity of the API?

10 Likes

The implicated functions are Pin::as_mut (method 1) and Pin::clone (method 2), not Pin::as_ref.

The original PoC uses as_ref() (method 1) and as_mut() (method 2) to upcast to a trait object; you've shown that that's unnecessary, and you can rely on implicit coercion instead. But that's not the interesting part. dyn MyTrait does not impl Unpin, but you don't get any extra abilities from the type not being Unpin. It simply means that the type system no longer guarantees that is safe to unpin – but at that point the concrete type is still Wrapper, which actually is safe.

The dangerous part happens when it calls as_mut() for method 1 – a different call from the one you removed – or clone() for method 2. That's when the Wrapper is swapped out for the future.

Don't believe me? Well...

Here is a version that does the coercion before pinning. It goes from Box<Wrapper> to Box<dyn Dubious> to Pin<Box<dyn Dubious>> to Pin<&mut dyn Dubious>, so there's no coercion happening within the pin.

I'd like to make a version that doesn't use trait objects at all, but the lifetimes don't work out; having a type contain a mutable reference to itself makes the borrow checker act rather weird. I may have been wrong when I said trait objects weren't the only possibility.

5 Likes

Interesting observation! However, I think this is also consistent with what I said: Pin<&dyn Dubious> would not be a problem if we couldn't actually employ its malicious DerefMut impl.

The issue is having a Pin<Ptr<T>> where Ptr (with its Deref/DerefMut) do not uphold the contract, and then actually using that Deref/DerefMut. If we guarded all operations that use the Deref/DerefMut by some unsafe trait bound, then creating Pin<&dyn Dubious> would not be a problem any more.

Basically, we have to either guard the construction of Pin<Ptr<T>> or the use, making sure that Ptr is a well-behaved pointer type. We currently try doing that during construction, but maybe we should try guarding the uses instead. That's an easier argument to make as we do not have to make "Ptr is well-behaved" part of the invariant and maintain it everywhere; we can just check it locally when we need it.

(And as @comex' latest post shows, even removing CoerceUnsized we still would not properly "guard the construction.")

That I agree with.

Good question. We even had a sketch of a formal model for pinning, but that model was always about the three separate pointer types (PinBox<T>/PinMut<T>/Pin<T>). The switch to the generic Pin was done without formal modelling.

Actually trying to prove things about Pin could have helped, but one of our team actually did that over the summer and he did not find this issue. The problem is that our formalization does not actually model traits. So while we do have a formal version of the contract that Deref/DerefMut need to satisfy, we cannot even state things like "for any Pin<T> where T: Deref, this impl satisfies the contract".

Basically, our formal model actually models the variant I proposed, where we do use an unsafe marker trait, and associate the contract to that. That was my mental model for the generic Pin pointer wrapper from the beginning, which is why I was so surprised when the actual API ended up just relying on the (safe!) Deref/DerefMut traits.

So maybe one lesson is that we don't actually understand our trait system enough to write APIs that rely on coherence, i.e., that make statements like "the (unique) Trait impl for T satisfies X", as opposed to locally demanding "give me some Trait impl for T satisfying X". When assuming things about a trait impl, we should always do that through some unsafe trait, and never through an unsafe function that makes assumptions about "whatever happens to be" the trait impl for its type parameter T. (This is not to say that the latter is impossible to get right, but it clearly much more tricky as now the whole set of coherence rules becomes safety-critical for this API.)

16 Likes

I filed an issue so that this has a tracking number:

But I think we should keep discussion here now.

6 Likes