Unsoundness in `Pin`

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

Just to check my understanding, what makes these types special is precisely the safe wrappers that give you Pin<P> for Box, &mut, and &. In other words, if we add a new safe wrapper, we have to check for similar exploits with those types, but in general we don't have to worry beyond that?

I'm not sure that works -- reservation impls were designed to allow overlap from downstream crates. They act in somewhat surprising ways, IIRC. See the tracking issue for more details.

3 Likes

This seems much more defensible than the "fake clone impls", which are quite hard for me to understand. We could certainly consider phasing it in with a warning period, given the soundness justification. There is (additionally) a need for a "pure deref" concept, though, and I hesitate to have too many marker traits -- but I suppose we could add this one as and then layer in a pure one that (likely, but not certainly) subsumes it.

1 Like

Interesting indeed. I'm curious whether anyone can estimate the likely breakage from the various proposed fixes?

1 Like

Nice catch! Note that your example can also work with coercion, but it's definitely true that coercion isn't necessary to construct a Pin with a 'malicious' DerefMut impl.

That sounds right to me. Would that imply that Pin::new_unchecked could (conceptually) have its requirements relaxed? That is, it is sound to call Pin::new_unchecked with a type with a malicious DerefMut impl, so long as you do not implement the unsafe trait? (Note that I don't think you would ever actually do this in practice).

Based on the points brought up by @comex and @RalfJung, I now think the best solution is to guard the usage of the Deref and DerefMut impls by an unsafe trait.

2 Likes

Well if there's going to be a 2021 edition, could this be a candidate for one of the breaking changes?

1 Like

Not really. If we're going to fix this, I think we would fix it universally. Editions are good for "surface level" changes where we wish to keep both old/new working (and interoperating). They don't really apply to things like "which traits you have to implement".

3 Likes

This matches my understanding of the issue.

Basically, the function that returns a Pin<&T> has an obligation to show that the Deref/DerefMut impls are "good", but it turns out they are failing to actually satisfy that obligation. They are currently implicitly reasoning based on "there is no DerefMut", but alas, turns out that's not universally true.

Good question. I think yes.

5 Likes

That's correct as to the exploits that don't involve coercion.

For coercion (method 3), Pin::new is a safe function that gives you a Pin<P> for arbitrary P where <P as Deref>::Target: Unpin. If you could then coerce it to Pin<Q> where Q implements Deref with a different Target, it would be unsound. But that requires P: CoerceUnsized. For now, out of the builtin/libstd types that implement CoerceUnsized, none of them allow a downstream crate to add its own Deref implementation; and implementing CoerceUnsized oneself is unstable. But that could easily change. As such, we almost certainly want some kind of restriction on Pin's CoerceUnsized impl.

I don't think this works. A coercion like Pin<Box<MyFuture>> to Pin<Box<dyn Future>> seems like a pretty normal use case for real code – where MyFuture may or may not implement Unpin, and dyn Future definitely does not.

Would we still need that with the unsafe-marker-trait proposal? The entire assumption about well-behaved Deref would not be part of the Pin invariant any more... and isn't that the only reason why Pin::new needs side-conditions? I might be missing something here but I feel we could have a safe Pin constructor for any type; it just wouldn't be very useful unless the type also implements the (unsafe) marker trait.

Hey, was PTO the last two days. Good catch.

So yea, the issue identified here is pretty plainly that we didn't consider manual implementations of DerefMut and Clone for reference types when we generalized from PinMut/PinBox to Pin. Such impls are obviously absolutely absurd in practice and the most charitable thing you could say about them is that they would result in extremely confusing code.

My immediate inclination is to think of this as a problem with how we integrate Deref/DerefMut/Clone with the reference types. We operate on the assumption that these traits are effectively blanket implemented (or blanket not implemented) by these types, but the reality is more complex and creates a weird semantic loophole.

I've only read a bit of the discussion so far, so I'm going to do that and think more before deciding what I think we should do about this.

3 Likes