Introduce a PinSafePointer trait that generalizes PinCoerceUnsized#156935
Introduce a PinSafePointer trait that generalizes PinCoerceUnsized#156935Darksonn wants to merge 1 commit into
PinSafePointer trait that generalizes PinCoerceUnsized#156935Conversation
| // safely as `Box::pin((*p).clone())`. | ||
| #[unstable(feature = "pin_coerce_unsized_trait", issue = "150112")] | ||
| unsafe impl<T: ?Sized, A: Allocator> PinCoerceUnsized for Box<T, A> {} | ||
| unsafe impl<T: ?Sized, A: Allocator + 'static> PinSafePointer for Box<T, A> {} |
There was a problem hiding this comment.
I added A: 'static bounds to these impls since you generally cannot pin values in memory from a non-static allocator.
| /// [`clone`]: Clone::clone | ||
| /// [`Arc`]: ../../std/sync/struct.Arc.html "Arc" | ||
| /// [`Arc::get_mut`]: ../../std/sync/struct.Arc.html#method.get_mut "Arc::get_mut" | ||
| pub unsafe trait PinSafePointer: Deref + Sized {} |
There was a problem hiding this comment.
I added a Sized bound here due to #156920 (comment). This doesn't actually affect any use-cases since Pin<P> already requires P: Sized, but seemed like a good idea nonetheless.
83c7d24 to
23a34ef
Compare
| /// Calls to [`deref`]/[`deref_mut`] on the same `Pin<P>` instance must always | ||
| /// refer to the same object. That is, the address returned by these methods | ||
| /// must not change. This applies even if the pointer type is moved. |
There was a problem hiding this comment.
So this is still doing the thing where it talks about a DerefMut impl but DerefMut is not a supertrait of this trait. What are the valid ways in which an unsafe impl PinSafePointer can satisfy this requirement?
- There's a
DerefMutfor the same set of types in the same crate: that's easy, we can just check the condition. - There's a negative
DerefMutfor the same set of types in the same crate: that's easy, no impl can exist so the condition is trivially satisfied. - Neither of these hold true. Now we... uh, look at the orphan rule and hope for the best? I am not sure that's a good idea, this is exactly what has bitten us in the past...
The same concern applies to the requirements on Clone, the formatting traits, and any others that I missed. (IMO the docs should call out more explicitly: If this type also implements Foo, then blah. If it also implements Bar, then blah. etc)
There was a problem hiding this comment.
I agree that it's best to avoid this kind of safety requirement, but I think we have no choice. Luckily we do not need to worry about dyn PinSafePointer.
This PR is adding comments to every impl of PinSafePointer explaining why the various trait implementations are compatible or cannot exist for the particular type in question. Please see the comments for various ways we can make these arguments.
There was a problem hiding this comment.
Yeah in std we can do that because we have negative impls. But our users can't really do that... and the moment one wants to impl PinSafePointer for MyType<T> where MyType<T>: DerefMut for some but not all T, I don't see any way to make this work.
There was a problem hiding this comment.
Negative impls are only required for fundamental types, so I'm not too concerned about that.
There was a problem hiding this comment.
With regards to types where MyType<T>: DerefMut for some but not all T, we do in fact have one instance of exactly that. Please see #145608 for the trick to make that sound.
There was a problem hiding this comment.
I have seen that PR and ran away screaming.^^
I really hope the reasoning is sound, but I do not have a coherent proof in my head -- it relies too much on orphan rule details.
| // with `Rc::get_mut`), this means that this type treats `&Rc<T>` as evidence | ||
| // that the `T` is not pinned. The implementations of various traits are written | ||
| // accordingly. Since this type is not fundamental, downstream crates cannot | ||
| // provide malicious implementations of any of the traits relevant for `Pin`. |
There was a problem hiding this comment.
So this relies on something about the orphan rule, right? That makes me uneasy.^^
An explicit impl !DerefMut for Rc would be a lot better IMO.
There was a problem hiding this comment.
Yes, by the orphan rule downstream crates cannot implement DerefMut for Rc<_> under any scenario.
There was a problem hiding this comment.
Those are the current orphan rules. How do we know they will never be relaxed?
The only promise the orphan rules are making is "there will never be more than one impl for the same trait + type". I don't think we should rely on any property that goes beyond this, and I don't see how the argument here follows from that property.
There was a problem hiding this comment.
We can add a negative impl to simplify the reasoning for Rc, but I think there is no way out for the #145608 case unless someone comes up with an entirely different solution. It's simply unsound to add untrusted code to the coherence domain of core because it could implement DerefMut for Pin<LocalType> maliciously.
| /// If this pointer type uses `&P` references as evidence that this value is not | ||
| /// pinned, then it must not treat the `&self` argument passed to [`Clone`] or | ||
| /// the formatting traits ([`fmt::Debug`], [`fmt::Display`], [`fmt::Pointer`]) | ||
| /// as such evidence. | ||
| /// | ||
| /// As an example, given a `Pin<Arc<T>>` there is no way to obtain an `&Arc<T>` | ||
| /// (note that `Deref` just gives a `&T`). Because of this, the [`Arc`] type can | ||
| /// assume that an `&Arc<T>` value can only exist if the `T` is not pinned, | ||
| /// which justifies the soundness of the [`Arc::get_mut`] method. |
There was a problem hiding this comment.
I didn't realize get_mut becomes even more subtle when pinning gets involved. Impressive.
This PR renames
PinCoerceUnsizedtoPinSafePointerand adds several new safety requirements about the implementations of various safe traits.Closes: #152667
Closes: #147794
With this merged, the only remaining soundness issues with
Pinare:Pin::new’s check forTarget: Unpinbecomes insufficient #134407CoercePointeemay be implemented onPin<LocalType>in downstream crates. (unstable only)For your convenience here are the docs for the new trait:
PinSafePointerTrait that indicates that this is a pointer that does not misbehave when combined with
Pin.Note that for backwards compatibility reasons, it is possible to create a
Pin<P>for pointer typesPthat do not implement this trait. However, this can only be done safely if<P as Deref>::TargetimplementsUnpin, which means that pinning has no effect.Safety
Types that implement this trait must not provide "malicious" implementations of any safe traits used by
Pin.The pointer must always reference the same object
Calls to
deref/deref_muton the samePin<P>instance must always refer to the same object. That is, the address returned by these methods must not change. This applies even if the pointer type is moved.Furthermore, if the pointer type can participate in unsizing coercions or dynamic dispatch, then these coercions must also not change the underlying concrete type. Here, the concrete type of a trait object is the type that the vtable corresponds to. The concrete type of a slice is an array of the same element type and the length specified in the metadata. The concrete type of a sized type is the type itself.
As an example, after unsizing coercing a pinned pointer,
deref_mutmust not return a#[repr(transparent)]wrapper around the value it referenced before being unsized, even if the address is unchanged.The pointer must not move its pointee
The
deref_mutmethod and the pointer type's destructor are called with a&mut selfreceiver, but they must behave as-if it was aself: Pin<&mut Self>receiver. That is, they must not move out of the underlying value.As an example,
deref_mutmust not invokeswapon the inner value.Shared access to the pointer
If this pointer type uses
&Preferences as evidence that this value is not pinned, then it must not treat the&selfargument passed toCloneor the formatting traits (fmt::Debug,fmt::Display,fmt::Pointer) as such evidence.As an example, given a
Pin<Arc<T>>there is no way to obtain an&Arc<T>(note thatDerefjust gives a&T). Because of this, theArctype can assume that an&Arc<T>value can only exist if theTis not pinned, which justifies the soundness of theArc::get_mutmethod.Cloning pinned pointers
When a
Pin<P>is cloned, thePpointer value returned bycloneis passed toPin::new_unchecked. The implementation ofClonemust return a value such that this is sound.For example, when a
Pin<&T>is cloned, the resulting&Tpoints at the same value. The value is known to be pinned since aPin<&T>to it exists, so it is safe to wrap the&Treturned bycloneinPin.r? lcnr