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):
CoerceUnsizedto type with differentDerefimpl - 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:
-
We could require
CoerceUnsizedto be "coherent" with respect toDerefandDerefMut, in the sense that callingderef()andderef_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
derefandderef_mutto be pure. We could makeCoerceUnsizedunsafeand offload enforcement to the programmer, but that seems unfortunate if it doesn't fundamentally need to be unsafe. -
Or we could simply condition
Pin'sCoerceUnsizedimpl on an unsafe marker trait. So if you define a structFoothat coerces toBar, you still couldn't coercePin<Foo>toPin<Bar>, without implementing the unsafe trait forFoo.
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.
