Unsoundness in `Pin`

I found this line of reasoning convincing. I don't have time to write more right now.

11 Likes

I think this whole argument is wrong, since this statement is wrong.

Pin does not wrap any type, or any smart pointer, but only smart pointers that are well-behaved, and that being well-behaved can certainly include implementing PartialEq, etc. in the obviously correct way of delegating to the PartialEq on &T where T is the pointer target.

Also in general behavior equivalent to the PartialEq implementation should be implementable in safe code outside of the module, and thus the PartialEq implementation should not require access to private fields or unsafe code, which means that the "bypassing" implementation is the correct one (since safe non-private code cannot access the smart pointer itself) and the current unsound implementation is wrong.

3 Likes

I agree with this, and I think the use case we're excluding seems so exceedingly niche:

  • A user has defined a smart pointer type that can be validly pinned (already a rare case).
  • A user actually wants to use that with the std Pin API.
  • That smart pointer type has a custom PartialEq/etc impl that they need to get called.

However, we are talking about losing some optimizations. For example, Rc == Rc currently does an addr comparison before comparing the value for types that implement Eq and so does Pin<Rc> == Pin<Rc> today. But these can be recovered with secret specializations.

Also, users can, in the most extreme case, recover whatever behavior they want by creating their own type for the code where they need equality checks (using an unsafe partialeq marker or non-generic pointer types, etc), converting between that and Pin whenever they want.

1 Like

This is only true for !Unpin types. With an Unpin type, I can use Pin::new with any smart pointer, without a (safety) requirement that it be well-behaved in any way whataoever.

I don't think we should be determining what 'obviously correct' means for user code, without providing any way to override this assumption. The PartialEq trait only requires (as a logical contract, not a safety requirement) that the implementation uphold reflexivity, symmetry, and transitivity. I don't think we should be assuming anything else about what a 'correct' PartialEq implementation looks like.

I'm not quite sure what you mean here.

If you're talking about the PartialEq implementation on Pin itself, I don't think it should necessarily be implementable outside the module. While it would make the correctness of Pin easier to verify, I think we should prioritize making Pin flexible with respect to custom pointer types.

If you're referring to the user-supplied PartialEq impl on the pointer type, I strongly disagree. One of the core ideas behind writing unsafe code is that it should (whenever feasible) be used to build a safe abstraction, which does not require users to even be aware of the existence of the underlying unsafe code.

We should not making any kind of assumptions as to where unsafe code 'should' exist. While it might be unusual for a PartialEq implementation to require it, there's no reason why it couldn't.

1 Like

Could such specializations be written in user code after specialization gets stabilized, or would they be a special kind of perma-unstable specialization for libstd only.

If it's the latter, I'm somewhat uncomfortable with the idea of giving libstd types this kind of 'special power' that will never be available to user code (assuming that a specialization trick is the only way of recovering performance in this case).

This will not help users relying on other crates which expect a Pin. I don't think this will be a common (or even very useful) thing to do, but I think it's worth pointing out that this change will impose some loss of functionality, even if it's still a net improvement.

Users could use the specialization to write unsound impls unless we introduced an unsafe marker trait as a part of exposing the specialization. There's nothing "perma-unstable" about adding private specializations to add safe optimizations without exposing an unsound API.

Sure no one is disputing that its a loss of functionality, just that it outweighs the cost of maintaining that functionality. Making it impossible to impl DerefMut for &MyType is a loss of functionality also. Only the "separate unsafe marker trait for each trait involved in Pin" solution involves no loss of functionality.

1 Like

Rust being lifetime-parametric means that the choice of lifetimes cannot affect whether a trait is implemented for a type, but only whether relying on an impl is an error or not.
So any lifetime bounds on an impl, or using 'static, or a lifetime parameter more than once, in the type or trait of the impl, will result in those lifetime requirements being imposed in an unsilenceable way, instead of being able to prevent the trait from being considered implemented.

To give a more concrete example, impl Copy for Foo<'static> means any Foo<'a> is copyable, but doing so will cause an error if 'a isn't 'static.

1 Like

Tangential, but this impl makes me uncomfortable:

impl<'a> DerefMut for &'a (dyn Dubious<'a> + 'a)

It passes the orphan check because the compiler recognizes dyn Dubious<'a> + 'a as a formula for a locally-defined type, even though Dubious is a locally-defined trait. That might be a mistake.

Should the dyn transformation from trait bound to type even be part of the local-type grammar? In contrast applying PhantomData to a type is much less dangerous but it doesn't produce a local type.

However, withoutboats provided an example that doesn't depend on funny business with trait objects, so restricting impl for dyn can't resolve the issue at hand.

2 Likes

This is my thinking, too. The variant without loss of functionality is so verbose that it's not worth it.

Now that makes me wonder... how does specialization interact with the contract we are imposing on Deref+DerefMut+Clone (whether that happens via "promise on Pin construction" or via "unsafe marker trait)? The contract must be satisfied by all specializations, which means it is definitely always incorrect to construct a Pin where one of those traits is implemented with a default fn -- is that correct?

I don't think this breaks anything, it's just another pitfall to be aware of.

I agree, we shouldn't do that.

But I can see two other options:

  • We could add a pointer equality check to Pin's PartialEq implementation.
  • We could make Pin<Rc<T>> not do a pointer equality check, but expect users to put one in their PartialEq impl instead.

Yes I think so, at least without considering other language features. If you can make it impossible or unsafe to add a specialization again, you're alright.

This is related to why I am now so wary of Pin::new, and why I'd jump toward seeing it as the biggest long term threat. Our safety contract relies on safe constructors having a "closed world" viewpoint - they see all the upstream code that could impact them, because they are supposed to be concrete over the pointer type constructor, and the orphan rules are supposed to prevent new instantiations. This reasoning was faulty because of several specific holes in the orphan rules.

However, Pin::new is generic over the pointer type, relying instead of the target type implementing Unpin to assume that the pointer type constructor doesn't need to uphold any guarantees. But we've seen, as part of the leverage used in the original attack, you can achieve with this an erasure of the unpin-ness of the target. Once we've closed up the known holes, this is where I see the most potential for future dangerousness. I'd be interested if you have a reason you're not particularly worried about Pin::new.

(To be clear, I think this problem is resolved by making CoerceUnsized unsafe to implement and imposing a contract on CoerceUnsized. In other words, i think my concern is probably isomorphic to "method 3." But its the part I feel the least certainty about it!)

1 Like

Do we know how much code is there out there using Pin::new?


I don't think I've ever used it - always had to use Pin::new_unchecked instead.

There are 108 uses of Pin::new in futures alone, 45 in tests, the rest in real code. Mostly related to wrappers of {AsyncRead, AsyncWrite, Stream} + Unpin where for ergonomics the future is written to operate over &mut impl Stream + Unpin instead of Pin<&mut impl Stream>.

EDIT: That's actually more than the 77 direct calls to Pin::new_unchecked, although adding in unsafe_pinned! usage puts that up to 288.

3 Likes

I don't think we should deprecate Pin::new. I just think it has the highest long-term risk!

3 Likes

Maybe it's worth adding a bound specifically for Pin::new, like P: PinPtr (where PinPtr is an unsafe trait) so at least smart pointer types have to opt-in to being constructed safely. There just aren't that many smart pointer types, so it wouldn't be a huge burden.

That's what I proposed earlier, but Ralf made a convincing argument it wasn't a good solution. Even setting aside proofs, consider this: we can't backwards compatibly add new requirements to that trait's contract, which would mean we wouldn't be able to add new impls to Pin that rely on it.

I just spoke with Niko for a bit, and we agreed that the current implementation of PartialEq should be treated as a straight forward bug. It is unsafe in a generic context to go from &Pin<P> -> &P, any place we do this and then pass &P to user defined code, except for Clone and the Deref traits, is a bug. Our attitude was that we should fix the impl to instead forward directly to the target type.

My suggestion was meant to be in addition to the proposed changes to eg. PartialEq. You said you thought Pin::new had a higher long term risk due to the very broad generic implementation, adding the PinPtr bound would mitigate that risk, or at least leave some wiggle room in case further issues are discovered. I suppose it depends on how real you consider that risk to be.

But this returns to the problem with unknown unknowns: we need to be able to concretely express the contract on whatever that trait would be, or it doesn't mean anything. In effect, based on what we know and understand right now, its the same as just putting the contract on CoerceUnsized.

1 Like

True, but:

  1. We have the option to over-restrict that contract: we can always relax constraints later.
  2. If we get our definition of that contract wrong, fixing it will break less of the ecosystem. When we discover our "unknown unknown" we need only find crates that implement PinPtr (probably very few) and audit them as to whether they follow the new contract or not. In all likelihood, the fact that they're "trying" to be smart pointer types it probably enough to make them valid. I expect most danger to come from people "abusing" Deref in some way who don't consider themselves to be implementing a pointer type, and those people would be unlikely to implement the unsafe trait regardless of what contract it has.

Anyway I'm not really advocating either way on this, just thought it was worth mentioning.

1 Like

Well, I'd argue that you're seeing the world through compiler-tinted glasses. :slight_smile: From a user code perspective, "Copy is not implemented" and "trying to assert that Copy is implemented causes a compilation error" are indistinguishable. There's no reason that unsafe code shouldn't be able to rely on this to enforce its invariants. It's just that, due to lifetime parametricity, there's no real use case for doing so. But, for instance, I'd argue that this useless API is sound:

pub struct Wrap<'dummy, T> {
    pub t: T,
    pd: PhantomData<*mut &'dummy ()>,
}

unsafe impl<T> Sync for Wrap<'static, T> {}

pub fn with_wrap_nonsync<T>(
      t: T, f: impl for<'dummy> FnOnce(&Wrap<'dummy, T>)) {
    f(&Wrap { t, pd: PhantomData })
}

pub fn with_wrap_sync<T: Sync>(
      t: T, f: impl FnOnce(&Wrap<'static, T>)) {
    f(&Wrap { t, pd: PhantomData })
}

However, the unsafe code that implements such an API does have to consider how wrapper types deal with subtyping. In the above example, if Wrap were changed to make 'dummy contravariant, the API would be unsound because you could convert from &Wrap<'non_static> to &Wrap<'static>. But of course that's not Rust's fault: references being covariant over their type parameter is part of their contract, and it's the unsafe code's responsibility to take that into account.

I suppose the same applies to Pin. You could write unsafe code that is unsound iff a user takes advantage of Pin's covariance, but that's the code's fault, not Pin's.

1 Like