Open steffahn opened 3 years ago
This is related to #68015 even though I didn’t really know much about that issue while finding this one. #68015 was summarized as
It is possible to exploit Pin on nightly Rust (but not stable) by creating smart pointers that implement CoerceUnsized but have strange behavior.
In relation to that summary, this issue (#85099) demonstrates that it is in fact possible to abuse existing CoerceUnsized
implementations on stable
.
I’ve also made an explanation of this issue (#85099) (which also relates it to the summary of #68015 quoted above). This explanation is reproduced below:
The type Pin<&LocalType>
implements Deref<Target = LocalType>
but it doesn’t implement DerefMut
. The types Pin
and &
are #[fundamental]
so that an impl DerefMut for Pin<&LocalType>>
is possible. You can use LocalType == SomeLocalStruct
or LocalType == dyn LocalTrait
and you can coerce Pin<Pin<&SomeLocalStruct>>
into Pin<Pin<&dyn LocalTrait>>
. (Indeed, two layers of Pin
!!) This allows creating a pair of “smart pointers that implement CoerceUnsized
but have strange behavior” on stable (Pin<&SomeLocalStruct>
and Pin<&dyn LocalTrait>
become the smart pointers with “strange behavior” and they already implement CoerceUnsized
).
More concretely: Since Pin<&dyn LocalTrait>: Deref<dyn LocalTrait>
, a “strange behavior” DerefMut
implementation of Pin<&dyn LocalTrait>
can be used to dereference an underlying Pin<&SomeLocalStruct>
into, effectively, a target type (wrapped in the trait object) that’s different from SomeLocalStruct
. The struct SomeLocalStruct
might always be Unpin
while the different type behind the &mut dyn LocalTrait
returned by DerefMut
can be !Unpin
. Having SomeLocalStruct: Unpin
allows for easy creation of the Pin<Pin<&SomeLocalStruct>>
which coerces into Pin<Pin<&dyn LocalTrait>>
even though Pin<&dyn LocalTrait>::Target: !Unpin
(and even the actual Target
type inside of the trait object being returned by the DerefMut
can be !Unpin
).
Methods on LocalTrait
can be used both to make the DerefMut
implementation possible and to convert the Pin<&mut dyn LocalTrait>
_(from a Pin::as_mut
call on &mut Pin<Pin<&dyn LocalTrait>>
) back into a pinned mutable referene to the concrete “type behind the &mut dyn LocalTrait
returned by DerefMut
”_.
And just to be clear (I was confused for a moment), this works on stable even though the example in the other linked issue doesn't.
@khoek Thanks, I’ve put in some clarification.
Error: Label P-high can only be set by Rust team members
Please let @rust-lang/release
know if you're having trouble with this bot.
Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.
@rustbot label -I-prioritize +P-high
Hi, I am new to rust development and not really sure where to post this, so i figured here would be fine. I found this issue and took interest in fixing it. I looked through related issues and the Zulip thread, but did not find any newer contributions. But since this bug is still present in stable I guess it has not been fixed. I was not able to find out, if anyone is working on a fix, or if a fix will be introduced by other changes, so I will just give my novice thoughts on the matter.
If I understand the crux of this problem correctly, then it lies within the conversion from Pin<T> to Pin<U> where T: Deref<Target: Unpin> and U: Deref<Target: !Unpin> while still having access to Pin<T> . This should not be allowed, since one can unpin T::Target (since it is Unpin ) and then gain unpinned access to the previously pinned U::Target (which is !Unpin ), thus being able to move it. Several steps are necessary to create this behaviour: |
Abstract step | Step taken in this example |
---|---|---|
1. have some data T: Unpin with a mechanism to get a pointer to U: !Unpin |
T = SomeLocalStruct(&cell) and U = impl Future |
|
2. pin a pointer P to T |
Pin::new(Pin::new(s)) |
|
3. convert Pin<P<T>> to Pin<Q<U>> (Q may be a different or the same pointer type as P ) |
first coerce from Pin<Pin<&SomeLocalStruct<'_, Fut>>> to Pin<Pin<&dyn SomeTrait<'_, Fut>>> , this is possible, due to #[fundamental] on Pin and &_ . then use the custom DerefMut implementation for Pin<&dyn SomeTrait<'_, Fut>> to effectively convert from Pin<Pin<&SomeLocalStruct<'_, Fut>>> to Pin<&mut Fut> . lastly downcast the trait. |
|
4. gain access to unpinned U through T and move it |
cell.into_inner() reclaims Fut and then is moved due to the Box::pin call. |
The issue lies with step 3 and in this case there are several possibilites to fixing the behaviour: | Possible Fix | Reasoning | Consequences |
---|---|---|---|
remove #[fundamental] from Pin |
#[fundamental] is needed for two parts of the third step, coercion from Pin<Pin<&SomeLocalStruct<'_, Fut>>> to Pin<Pin<&dyn SomeTrait<'_, Fut>>> and implementing DerefMut on Pin<&dyn SomeTrait> removing it would prevent the unsoundness |
This breaks the stdlib (due to the blanket impl of Generator for Pin<Box<, >> in library/alloc/src/boxed.rs:1810:32) and is not a viable option, because this would prohibit creating a general function taking Pin<&dyn SomeTrait> being used, because one cannot coerce to that type from Pin<&T> where T: SomeTrait |
|
add a negative implementation of DerefMut for Pin<P> where P: !DerefMut |
This seems very intuitive and natural to me, if a pointer type does not implement DerefMut , then why should Pin<P> ? The fix for #66544 introduced impl !DerefMut for &_ and adding impl<P: !DerefMut> !DerefMut for Pin<P> {} would follow the same spirit |
The stdlib does not depend on any DerefMut impls for Pin<P: !DerefMut> , I have not looked at any crates this could break, but as this impl seems very unnatural to me, I think that very few crates actually depend on it. However it can only be implemented if negative bounds would be implemented. |
|
prohibit DerefMut impls for &_ impl<T: ?Sized> !DerefMut for Pin<&T> {} |
this is the "light" version of the previous suggestion, since it does not require negative bounds, it might be easier to implement. It also seems natural to prohibit such implementations, because &_: !DerefMut . It also should be enough, because the other part needed for this instance of the problem is the coercion to a dynamic trait object, as this is only possible with #[fundamental] types, only they are hazards for this problem. But every #[fundamental] type other than &_ implements DerefMut , so one cannot implement DerefMut for that pinned pointer, leaving &_ as the only problematic candidate. |
There are two problems with this solution, first it sadly does not compile [^1]. I did not expect that error, because &_ does not implement DerefMut (and never will, due to implementing !DerefMut ) and as such it should not come into consideration when computing possibilites for P: DerefMut<Target: Unpin> . So I am a bit confused, if this is another error concerning negative impls and if I should file/find an issue for that. Second this does not pervent a potential implementation of DerefMut for Pin<Pin<&dyn SomeTrait>> , adding another Pin where needed, producing the same issue. This could be fixed, by adding as many negative impls for Pin<...<&_>...> as necessary, but the negative bounds solution is a lot prettier. |
[^1]: compile error, when adding negative DerefMut
impl for Pin<&_>
error[E0751]: found both positive and negative implementation of trait `ops::deref::DerefMut` for type `pin::Pin<&_>`:
--> library/core/src/pin.rs:874:1
|
871 | impl<T: ?Sized> !DerefMut for Pin<&T> {}
| ------------------------------------- negative implementation here
...
874 | impl<P: DerefMut<Target: Unpin>> DerefMut for Pin<P> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ positive implementation here
For more information about this error, try `rustc --explain E0751`.
@y86-dev Thanks for the excellent write-up!
I believe that #[fundamental]
only comes into play when writing a local impl for Pin<SomeType>
. In my understanding, CoerceUnsized
does not interact with #[fundamental]
at all (except maybe if you were writing a CoerceUnsized
impl for a foreign type, but the example doesn't include any new CoerceUnsized
impls).
@steffahn: If I understand correctly, this issue is only possible because of the blanket impl<P, U> CoerceUnsized<Pin<U>> for Pin<P> where P: CoerceUnsized<U> {}
. Without that impl, you could still write a DerefMut
impl for Pin<&LocalType>
, allowing you to obtain a Pin<&mut LocalType
. However, you can only (safely) obtain a Pin<Pin<&LocalType>>
if LocalType: Unpin
, so you could already use the safe constructor Pin::new(&mut LocalType)
.
If LocalType: !Unpin
, then you cannot create a Pin<Pin<&LocalType>>
in safe code:
use std::marker::PhantomPinned;
use std::pin::Pin;
fn main() {
let val: Pin<Pin<&PhantomPinned>> = Pin::new(Pin::new(&PhantomPinned));
}
produces:
error[E0277]: `PhantomPinned` cannot be unpinned
--> src/main.rs:5:59
|
5 | let val: Pin<Pin<&PhantomPinned>> = Pin::new(Pin::new(&PhantomPinned));
| ^^^^^^^^^^^^^^ the trait `Unpin` is not implemented for `PhantomPinned`
|
= note: consider using `Box::pin`
note: required by `Pin::<P>::new`
error[E0277]: `PhantomPinned` cannot be unpinned
--> src/main.rs:5:50
|
5 | let val: Pin<Pin<&PhantomPinned>> = Pin::new(Pin::new(&PhantomPinned));
| ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Unpin` is not implemented for `PhantomPinned`
|
= note: consider using `Box::pin`
note: required by `Pin::<P>::new`
To obtain a Pin<Pin<&LocalType>>
with LocalType: !Unpin
, you would either need to use the CoerceUnsized
impl with dyn Trait
, or use Pin::new_unchecked
. Since LocalType
is (by definition) local, you are arguably violating the unsafe
contract being using Pin::new_unchecked
in this way - if you later move LocalType
, you are moving a type that you promised not to move.
With that in mind, I think we can eliminate the problem by replacing the blanket CoerceUnsized
impl with several concrete impls. The blanket impl<P, U> CoerceUnsized<Pin<U>> for Pin<P> where P: CoerceUnsized<U> {}
applies recursively to Pin<Pin<T>>
, allowing us to coerce Pin<Pin<&T>>
to Pin<Pin<&dyn Trait>>
. However, I think this impl is essentially useless in practice. Not only is a Pin<Pin<&T>>
a pretty useless type, but it can be manually constructed by unwrapping the the Pin
s, coercing &T
to &dyn T
, and then wrapping the &dyn T
in two layers of Pin
.
Since CoerceUnsized
is an unstable trait, stable code can only be relying on the impls provided by the standard library for certain pointer types (Box
, Cell
, Pin
, etc). We could replace the blanket impl<P, U> CoerceUnsized<Pin<U>> for Pin<P> where P: CoerceUnsized<U> {}
with an impl for each standard library 'pointer' type, excluding Pin:
impl<P, U> CoerceUnsized<Pin<Box<U>>> for Pin<Box<P>> where P: CoerceUnsized<U> {}
impl<P, U> CoerceUnsized<Pin<Box<U>>> for Pin<Box<P>> where P: CoerceUnsized<U> {}
impl<P, U> CoerceUnsized<Pin<Cell<U>>> for Pin<Cell<P>> where P: CoerceUnsized<U> {}
...
This would allow users to continue relying on 'useful' coercions (e.g. Pin<&MyFuture>
to Pin<&dyn Future>
), while banning coercions involving Pin<Pin<&T>>
. Even though users will still be able to write DerefMut
impls for Pin<&LocalType>
, the lack of the necessary CoerceUnsized
impl will prevent this DerefMut
impl from being exploited in safe code.
@Aaron1011 I tried to implement your approach, but @steffahn 's example still compiles and segfaults.
I removed the CoerceUnsized
impl on Pin
:~~
impl<P, U> CoerceUnsized<Pin<U>> for Pin<P> where P: CoerceUnsized<U> {}
from pin.rs
. Because some coercion with Box
is used in the stdlib, i added the following impl to boxed.rs
:
impl<P, U, A> CoerceUnsized<Pin<Box<U, A>>> for Pin<Box<P, A>>
where
P: ?Sized + Unsize<U>,
U: ?Sized,
A: Allocator
{}
~~This is would normally be implemented due to the CoerceUnsized
impl on Pin
I might have made a mistake, but I was still able to compile the example.
It seems, that CoerceUnsized
does not play a role in coercing from Pin<Pin<&SomeLocalStruct<'_, Fut>>>
to Pin<Pin<&dyn SomeTrait<'_, Fut>>>
.
I think we either need negative bounds or some modification of #[fundamental]
(I would prefer the former, but negative bounds seem to have their own bunch of issues), but I do not know where to start there.
I also do not know how prevalent this problem might be, maybe a lint may be enough for the moment (until it does not compile anymore), like "DerefMut implementations on Pin<P>
where P does not implement DerefMut are generally unsound".~~
Edit: I made a mistake compiling the stdlib and ended up not using the freshly compiled version, so scrap all the above.
While @Aaron1011's approach works, I would prefer not to modify the behaviour of Pin<Pin<P>>
because
Pin<Pin<P>>
should behave equivalently to Pin<P>
, if it does not, this may create confusion for developers who do not know about this issuePin<Pin<P>>
to Pin<Pin<U>>
conversion, but rather from an unorthodox DerefMut
implemenation for Pin<&_>
so instead of chaning CoerceUnsized
I would suggest to add a lint warning when impl DerefMut for Pin<&_>
is found.
For the moment this lint could be warn
and in the future, when we can make this a hard error, we can change it to deny (or let negative impls give the compiler error).
I cannot think of a good reason to manually implement DerefMut for Pin<P>
when P does not already have a DerefMut
implementation.
I have not really worked on the compiler, so I do not know how complicated and how exactly one would create such a lint, but I am confident that this is possible.We're assuming that T-types is taking ownership of finding a path to a solution here.
(My instinct is that a negative impl is the right long term solution, and we just need to decide whether to slowly ease that in via a future-incompatibility lint of the form suggested by @y86-dev )
(NOT A CONTRIBUTION)
I've only just now seen this issue, but I want to say that I think adding a negative impl of DerefMut is the correct approach. The soundness of the Pin APIs rests on std types not implementing DerefMut or Clone, which is broken by fundamental tricks. As I argued when we fixed the previous issue in 2020, I think library writers should be able to rely on basic, seemingly obvious assumptions like "shared references (including pinned shared references) do not implement DerefMut" without having to think about the nuances of the fundamental attribute. I don't believe the impls you'd be excluding have any valid use cases, and I think its bad for Rust that impl DerefMut for Pin<&dyn LocalTrait>
is permitted even if there weren't a known way to break the soundness of the Pin APIs with it.
I even think it might be a good idea to exempt DerefMut and Clone from #[fundamental]
entirely at this point, though it makes defining #[fundamental]
even messier.
add a negative implementation of
DerefMut for Pin<P> where P: !DerefMut
I assume the recursive impl is necessary because if just the impl for Pin<&T>
were added people would just be able to implement DerefMut for Pin<Pin<&dyn Trait>>
? Didn't see this spelled out. Again, just excluding DerefMut from fundamental might be an option that's easier to implement.
(As another aside, I would guess there's might be a way to get the unsound behaviour through an impl of Clone for Pin<&mut dyn LocalTrait>
; unless someone can explain why that isn't the case whatever solution should also handle that.)
Visiting for P-high 2023 Q1 review .
I'd like to hear what T-types has to say about @withoutboats 's suggestion in the previous comment that we should, at minimum, add a negative impl of DerefMut
for Pin<P>
, or maybe even going so far as to exclude DerefMut
from #[fundamental]
entirely.
(It sounds like something we might need to tie to the 2024 edition? Or maybe we justify it as a soundness fix.)
@rustbot label: +I-types-nominated
This issue didn't get any types input over the last few months :sweat_smile:
I haven't thought about this too deeply, but my feelings here are:
impl<T: ?Sized + !DerefMut> !DerefMut for Pin<T>
can be supported. Though negative bounds do mean "has an explicit negative impl" and not "is not implemented". This is enough to disable it for Pin<&T>
because &T
has an explicit negative impl but would still allow an explicit impl for Pin<MyLocalTy>
,. Afaict that shouldn't be unsound though.#[fundamental]
" for impls of DerefMut
(and Deref
, and Copy
and so on) also works. I think I prefer this one as being DerefMut
is a core property of a type so implementing it for a fundamental one doesn't feel meaningful. Is there a clear theoretical reason why these traits are special? no. It does feel like a sensible restriction thoughwill try to get to this in the next t-types planning meeting. Would also mentor experimentation with either of these 2 options. Please open a thread in t-types if you're interested.
talked about this in the t-types triage meeting: https://rust-lang.zulipchat.com/#narrow/stream/326132-t-types.2Fmeetings/topic/2023-05-08.20triage.20meeting/near/356721636
the consensus was to go ahead with removing the special behavior for fundamental types for DerefMut
, preventing you from implementing DerefMut for Fundamental<MyType>
. While this is a special case of another special case and not perfectly clean design, we decided this to be the best course of action for now.
We also quickly touched on extending the "disable fundamental types behavior" to more traits and would like to experiment with that. We did not reach a conclusion for this however.
I am available to mentor an implementation of disabling the fundamental type behavior for DerefMut
. The steps are roughly:
#[rustc_no_fundamental_type_behavior]
(name bikesheddable, given that it is private it doesn't matter too much)TraitDef
DerefMut
if you get stuck, please open a thread on zulip in the t-types stream.
Is the idea that this is a stopgap solution until the negative impl can be used? I agree with all the voices above who said that that is clearly the right solution: Pin<P>
comes with invariants of the form "If P
implements DerefMut
then...", and negative impls are the most direct and explicit way to be able to say "This invariant is satisfied because my type definitely does not impl DerefMut
".
What is not clear to me yet is where exactly this assumption is made for P = Pin<_>
. My first guess is that it is somehow entangled with the trait object part? That's just a hunch though, assuming that this issue needs trait objects to be exploited.
(NOT A CONTRIBUTION)
Just want to reiterate that this is probably also breakable with Clone
on Pin<&mut dyn LocalTrait>
so whatever solution should probably be applied to Clone as well.
Is the idea that this is a stopgap solution until the negative impl can be used? I agree with all the voices above who said that that is clearly the right solution:
Pin<P>
comes with invariants of the form "IfP
implementsDerefMut
then...", and negative impls are the most direct and explicit way to be able to say "This invariant is satisfied because my type definitely does notimpl DerefMut
".
At least for me "disabling the fundamental ty behavior for some traits" is the final solution. The negative impl for Pin<ImmutPtr>
would require a negative trait bound as well because Pin<impl DerefMut<Target: Unpin>>: DerefMut
holds.
We would therefore need the impl impl<T: !DerefMut> !DerefMut for Pin<T>
. Even if this were to supported it would only mean that we guarantee that DerefMut
is not implemented for Pin<T>
or it guaranteed to not be implemented for T
. While this works alright for immutably references.
People would still be allowed to implement Pin<SomeOtherImmutablePtr>: DerefMut
. It feels fairly likely that a similar unsoundness can then be triggered using that other pointer type if the crate author forgets to add a negative impl for SomeOtherImmutablePtr
. I think making the reasoning about Deref(Mut)
- and Clone
- local to the crate defining SomeOtherImmutablePtr
feels more likely to prevent such issues in the future.
What is not clear to me yet is where exactly this assumption is made for
P = Pin<_>
. My first guess is that it is somehow entangled with the trait object part? That's just a hunch though, assuming that this issue needs trait objects to be exploited.
This is complex enough that I tend to forget the details again after a while, but in general the issue is that Pin
assumes that certain operations maintain Unpin
-ability. Without thinking too much about this concrete issue, iirc we have the following sequence
Pin<&concrete_a>
-> Pin<&dyn Trait>
[coercion] -> Pin<&mut dyn Trait>
[deref_mut] -> Pin<&mut concrete_b>
[vtable dispatch in downcast
]
we have to assume that each of these transitions does not change whether the pointee is Unpin
.
the first step is trivially correct, going from Pin<&dyn Trait>
-> <Pin<&mut dyn Trait>>
is where the assumption is broken by replacing the underlying type in the trait object and the last step is a bit weird but is also okay as long as you cannot replace the pointee for trait objects. (which we did in the deref_mut
step)
We would therefore need the impl impl<T: !DerefMut> !DerefMut for Pin
. Even if this were to supported it would only mean that we guarantee that DerefMut is not implemented for Pin or it guaranteed to not be implemented for T. While this works alright for immutably references.
I think this is exactly the argument we need to make a formal proof of Pin
go through. In contrast, I have no idea how types being fundamental or not can be useful in a formal proof. It certainly makes the argument a lot more complicated because it has to rely on coherence rather than the idea of "this type will certainly not implement that trait".
I think making the reasoning about Deref(Mut) - and Clone - local to the crate defining SomeOtherImmutablePtr feels more likely to prevent such issues in the future.
I think writing negative impls is the best way to make this argument local. Then one can directly point at another piece of code in the local crate which ensures that the invariant is maintained. In contrast, anything based on fundamental/coherence relies on the global invariant of coherence -- worse, it relies on how exactly the compiler ensures coherence (i.e., just knowing "the entire crate graph is coherent wrt trait impls" is insufficient, we need to argue based on how the compiler reasons about coherence in a compositional way -- things that I consider details of the orphan rules, not something I would like to see unsafe code based on).
It certainly makes the argument a lot more complicated because it has to rely on coherence rather than the idea of "this type will certainly not implement that trait".
coherence allows you to say that "this type will certainly not implement that trait" :grin:
things that I consider details of the orphan rules, not something I would like to see unsafe code based on
to me this feels similar to reasoning about Rc: !Sync
:thinking: this also relies on impl details of coherence. I am not opposed to adding both a negative impl and the restriction to fundamental, but I think the removing the fundamental specialcase for "core traits" or whatever is a safer way to prevent similar bugs going forward.
(NOT A CONTRIBUTION)
I would tend to agree with both @lcnr and @RalfJung. Ideally, we would move to explicit negative impls as the basis for soundness proofs (so yes, there should be an explicit impl<T> !Sync for Rc<T>
), but also adding an exception to fundamental for these traits that clearly should never be used with fundamental enables users even without the negative impl feature.
coherence allows you to say that "this type will certainly not implement that trait" grin
I don't see how. Coherence is first and foremost a consistency property: for any given type and trait, there will be at most one impl of the trait for the type. This does not help in the slightest to make statements of the form "this type will certainly not implement that trait".
The way rustc ensures coherence is through some form of "orphan rules" (using Haskell terminology since I don't know if we have a Rust term for this). Those rules are subtle and subject to change. Only with those rules is it possible to make statements of the form "no other crate can impl this trait for this type". For people that have internalized these rules, that might seem like a natural way of expressing constraints to the compiler, but really it is not -- I would not be able to do it, since despite having worked on Rust for a long time I don't know the orphan rules well enough. Furthermore, if we make this an acceptable soundness argument then it becomes a lot harder to evolve the orphan rules over time.
to me this feels similar to reasoning about Rc: !Sync thinking this also relies on impl details of coherence.
Well IMO we really should have impl !Sync for Rc
anyway... ;)
I'm not objecting to adjusting how fundamental
works. But adjusting fundamental
does not resolve my concern about soundness bugs like this -- I would prefer to live in a world where we never use the orphan rules in a soundness argument, and instead reason solely based on actual impls we can point to.
@lcnr Hey! Is it possible to get assigned to the issue? I would be interested in working on implementing this in the upcoming weeks to get more insight into the compiler.
(@lambinoo I've assigned you; tip: you can use @rustbot claim
to get assigned to an issue in this repo)
Being able to implement Clone for Box<dyn MyTrait>
is certainly useful and something many people do. I suspect it's more tenable to fix Pin
specifically (ala PR 116706) than to break all those.
Haven’t got the time to do any more explaining right now, but here’s the code (Edit: I added a few inline comments anyways):
(playground)
segfaults in debug build due to an
async
-block future being moved after the first.poll()
call@rustbot label C-bug, A-pin, T-compiler, T-libs, T-lang and someone please add I-unsound 💥