Thanks for pointing this out (I had missed this case) but it doesn't fundamentally shift my perspective: in this case, the problem is that the impl Into<Pin<Box<T>> for Box<T> assumes the "sensibility" of box and the native references; but its just another alternative path to get to the bad fundamental impls on native references.
Because this uses an API which is specific to Box, it doesn't provide another avenue for custom malicious smart pointers to trick Pin::new, and so its ultimately only relevant to how we fix the problem with native references.
You missed the importance of the word all in this comment. My point is that Pin is totally compatible with the fact that you can define pathological pointer types. What its not compatible with though is pathological impls of certain traits on native reference types or on types which implement CoerceUnsized. We only rely on those types properly implementing these traits.
EDIT: We also rely on box/rc/arc implementing them sensibly because we have safe constructors for pins of those types, but no one has shown a way to override the behavior of their impls of these traits.
Correctness of Pin::as_mut fundamentally relies on DerefMut satisfying a particular specification (namely, "to not unpin"). So yes, this does rely on particular behavior of particular impls.
The problem is that the "safety check" (in terms of then the API surface mandates the client to check this property) for this occurs in Pin::new_unchecked, not in Pin::as_mut. We thought that this is enough, Pin::as_mut can rely on properties established by Pin::new_unchecked -- but it turns out that reasoning has some gaps. We can fill the gaps we are aware of right now, but I see no good way to convince ourselves that we fixed all the gaps. Hence the proposal to avoid that reasoning entirely.
In general, we rely on this for all small pointer types that support pinning. That was my point exactly.
Ah, good point. There are still some local requirements to check in Pin::new about the concrete instance that is being pinned.
Agreed. This is basically what I tried to say above:
"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."
(+ the constructor for Pin<Box<T>>)
Understood. And that is indeed the spirit of the original API, as is consistent with what I call "guarding the constructor".
My conclusion of this thread is that that is too fragile; guarding the use is easier -- easier to do and easier to explain and document as well; the unsafe trait gives us a good place to write down the requirements.
I don't believe that guarding the use will measurably decrease the likelihood of future problems, and certainly not catastrophic ones.
In particular, the known routes to unsoundness all pass through one of these traits: CoerceUnsized, DerefMut, Clone. So we make it unsafe to satisfty these traits for pin by making coerceunsized unsafe and adding two unsafe traits:
Okay, but why shouldn't we believe the new unsoundness will come through a trick with different trait that doesn't use these APIs (Deref or Debug or Eq any other trait)! Do you propose to add an unsafe marker trait for every trait bound in the pin APIs? If so, I find that an absurd lack of confidence in our ability to audit our own code; if not, you are fighting the last war.
EDIT: And what's to say that the way we implement these traits for pointer types in std doesn't end up actually being unsound? It happens that we would have prevented this error with these two traits (though I am certain we would never have thought to add a unsafe trait for clone), but maybe the next unsoundness would pass through one of the impls of these traits we include in std. This is the problem with unknown unknowns.
The problem in the stable part of our API is square, simple and straightforward: we designed pin around certain beliefs about the behavior of native references that can be violated by downstream impls. It's completely reasonable that we overlooked this pathological use of the fundamental attribute, and does not in any way suggest that we were sloppy or cavelier in the design of these APIs. We should fix the problem with native references, redundantly complicating the pin API because of this problem does nothing for us.
Some years ago in https://github.com/rust-lang/rfcs/pull/1023 it was discussed about having [fundamental] on the Deref and DerefMut traits. To the extent to what I understand the present issue that would solve it; although I have not fully grasped the problem. What other effects would have putting this annotation?
Just for reference, I've built a far reduced example of the unsoundness that does not use trait objects. It uses the shared ref implementing DerefMut unsoundness; the same principle should apply to the mutable ref implementing Clone unsoundness.
// A !Unpin type to reference
struct MyType<'a>(Cell<Option<&'a mut MyType<'a>>>, PhantomPinned);
impl<'a> DerefMut for &'a MyType<'a> {
fn deref_mut(&mut self) -> &mut MyType<'a> {
self.0.replace(None).unwrap()
}
}
fn main() {
let mut unpinned = Box::new(MyType(Cell::new(None), PhantomPinned));
let pinned = Box::pin(MyType(Cell::new(Some(&mut unpinned)),
PhantomPinned);
let mut pinned_ref: Pin<&MyType<'_>> = pinned.as_ref();
// call the unsound DerefMut impl
let pinned_mut: Pin<&mut MyType<'_>> = pinned_ref.as_mut();
// pinned_mut points to the address at `unpinned`, which
// can be moved in the future. UNSOUND!
}
I don't think this has any persuasive bearing on the conversation, I just think it might be easier for people following along to understand what's happening with a short version.
The Deref/DerefMut, Clone, and CoerceUnsize tricks all have thr same root cause - they allow constructing a Pin in safe code. CoerceUnsize is extra special, in that it allows you to construct a new instance of a type with private fields fron outside the module. However, all of them require that we uphold the requirements of Pin - the pinned value can never be moved out of.
This requirement exists whether or not we block any 'weird' impls on fundamental types. However, having the unsafe marker traits means that we are explicitly asserting that we uphold this requirement for any of the potentially problematic impls. Without the marker trait, we need to argue from coherence that any 'bad' impls canmt exist. As @RalfJung pointed out, we can no longer determine that any method on Pin is sound by looking only at the pin module.
We are already relying on correct impls for std pointer types. However, we are relying on the lack of an impl (or the prescence of a seemingly-unrelated reservation impl). Having unsafe marker traits would explicitly indicate wherever we are relying on the behavior of impls for soundness.
However, I think it's important to note that the marker trait solution would have to include a marker trait for Clone, which I didnt realize at first. I think we could minimize the cost to users by automatically adding it when Clone is derived, and by including a blanket impl for types that deref to an Unpin type.
Also, note that these additional requirements only affect peopLe writing custom pointer types. There are no new requirements on the pointee type (e.g. dyn Future)
How do you subsequently move unpinned? &mut unpinned here borrows unpinned for its entire lifetime: I think it's the only valid lifetime that we can put for 'a in unpinned: MyType<'a> as MyType is invariant.
Only thing I was able to do is to wrap unpinned in a ManuallyDrop, and when the stack frame is popped, I believe this indeed violates the contracts of Pin as the destructor of unpinned is not being called.
An unsafe marker trait for asserting that a smart pointer type has "boring" behavior already exists in a crate: https://crates.io/crates/stable_deref_trait . Does such a marker trait fit our needs? Or do we need further constraints on the trait bound beyond what such a trait provides?
I find this logic persuasive in any case (i.e., that users should be able to rely on that). I don't think reservation impls are going to work for this purpose, as I've stated, but I think there are other mechanisms we could consider.
To elaborate a bit on what I was saying in the T-lang meeting today. I think we could extend the impl !Foo for T syntax of negative impls for this purpose. The current coherence rules for negative impls already guarantee (I believe) that negative/positive impls cannot overlap. Therefore, if we added a impl<T> !Clone for &mut T and a impl<T> !DerefMut for &T, I believe that would prevent positive impls from being added downstream, but it would have no other real effects. In particular, negative impls are not currently taken into account for enabling negative reasoning, and in general they don't permit more code to be accepted, I believe they would only prevent code from being accepted.
Of course I could be overly optimistic about that last bit!
My main concern with this route is that it is in some sense adding to the "technical debt" we've accumulated around fundamental and negative coherence reasoning. We've always had hopes to formalize that reasoning in a better way.
That said, the proposal I've been contemplating in my head -- or at least a small part of it -- would be pretty compatible. Basically I've long thought it would be useful to permit negative impls as a way to declare that some impl will never be provided (and that adding such an impl is a semver-incompatible change). This would enable negative reasoning downstream. The nice thing about a proposal like this is that it is basically limited to coherence and has no real effect on the other semantics of the language. (Note that this proposal is not really able to replace fundamental, though.)
Note that I am not proposing adding negative where clauses (e.g., where T: !Foo), just impls. Those get us into all kinds of trouble (permitting users to provide negative goals that have to be proven can lead to negative cycles, which are bad mojo and I would prefer to avoid that possibility).
UPDATE: I've read the thread through once now, but I think I have to read it through again. I don't really know what I think the best step forward is (but the above content seems accurate regardless as a way to prevent impls of DerefMut for &T, if we decide to do that). As boats pointed out in our lang team meeting, there is nothing stopping us from both trying to prevent such impls from existing and adding an PinSafeDerefMut trait.
In the case of making that impl <T> !Clone for &mut T would impl<'a> Clone for Box<&'a mut (dyn Dubious<'a> + 'a)> stop being a conflicting implementation? If that is true then I think we would had the problem again. Could we then add impl<T:DerefMut> !Clone for Box<T> to fix it? Then Box<Box<i32>> would have some problem with Clone. And if we add impl<T> !Clone for Box<&mut T> someone could try to implement a Clone for Box<Box<&mut T>>.
And similar considerations to make for any other/future fundamental type.
A point of information. I was reading the safety section of the Pin documentation.
It emphasizes that the primary invariant which must be maintained is that one cannot move out of their self arguments. It gives some examples where this would not be true, such as Pin on an &mut (because mem::swap) or an Rc (because aliases could outlive the pin's own ref count, and then use Rc::get_mut).
Are there add'l requirements that are implied but not stated here? For example, that Deref and DerefMut are giving references to the same data?
I'll be honest and say that I find these docs kind of confusing. I believe @withoutboats is saying that "when we enabled Pin<&T>, we were assuming DerefMut did not exist, and therefore we did not have to prove things". But the way the safety docs are stated does not clearly (to my mind) discriminate between what must be proven for Deref and what add'l things must be proven for DerefMut.
That depends. The specific change I was imagining doing in the very short term would not permit any add'l impls, so the impl of Clone for Box would still be considered to conflict. However, I think the logical next step would be to indeed extend coherence to understand negative impls as a semver promise, in which case yes, the impl would be permitted. I'm making a narrow point here: in particular i'm not sure about the implications of that for this particular bug.
The pin documents are mainly focused on people using Pin with existing pointers, not people who have defined a pointer and create a constructor for a pinned version of their pointer. So when they talk about not moving out of the &mut, they're talking about those users who want to mutate something inside of a Pin (for example, someone writing a future combinator by hand).
However, the requirements on constructing pointers are more complicated and not well documented. In general we have not stated them abstractly, but instead just reasoned about the soundness each pointer type we have added a constructor for.
One thing comex has revealed (mainly discussed as "method 3" in the original post) is that with the combination of Pin::new and CoerceUnsized, you can construct a pin pointer with an Unpin target and then coerce it into a Pin pointer to a target that is not known to implement Unpin, because your concrete target type gets erased. Therefore, Pin::new, as a generic safe constructor, does impose restrictions on types which implement CoerceUnsized (an unstable trait).
Stating abstractly the requirements on pointers to be pinned seems very difficult: the entire idea of using non-generic constructors was that we could reason about each pointer individually even though they are passing through the same Pin type. I believe that even looking at something like Clone or DerefMut alone, you get a tree of ANDs and ORs for the possibly ways you could meet the requirements, and I don't feel confident that would be exhaustive. This is one of the downsides, to me, of adding the unsafe marker traits: I really don't know how we could document them except to explain how each implementation we provide is valid given the specific semantics of that pointer.
I don't really understand this point. When you reason about a particular type, you must be trying to prove something about it -- what is it that you are trying to prove? That seems like a first draft of the "abstract requirements", no?
Well what you are trying to prove is that, given this constructor, there is no way to get an &mut ? in safe code to the same address you get an Pin<&mut ?> to, until after the destructor has run. But proving this is highly variegated based on the API exposed for that type.
Right. Cloning a Box<&mut T> seems problematic, since then there are two &mut T, but you have obtained them in a safe way. It could create a problem if someone would make some incorrect assumptions on DerefMut, but not with Pin.
Indeed, the fact that Box works fine even implementing both DerefMut and Clone suggests me that every derivative of a Pin should guarantee to be correctly pinned assuming the original pin is correct. The Pin::as_mut is fine when deref_mut returns always the same physical address and it coincides with deref, and Pin<Box<T>>::clone is fine because pinning a box is always fine. These are two very different ways to provide the guarantee, which I guess it is to what @withoutboats referred as highly variegated.
