A `Pin` unsoundness involving an `impl DerefMut for Pin<&dyn LocalTrait>` #85099

steffahn · 2021-05-09T01:10:36Z

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):

use std::{
    cell::{RefCell, RefMut},
    future::Future,
    mem,
    ops::DerefMut,
    pin::Pin,
    sync::Arc,
    task::{Context, Poll, Wake},
};

struct SomeLocalStruct<'a, Fut>(&'a RefCell<Fut>);

trait SomeTrait<'a, Fut> {
    #[allow(clippy::mut_from_ref)]
    fn deref_helper(&self) -> &mut (dyn SomeTrait<'a, Fut> + 'a) {
        unimplemented!()
    }
    fn downcast(self: Pin<&mut Self>) -> Pin<&mut Fut> {
        unimplemented!()
    }
}

impl<'a, Fut: Future<Output = ()>> SomeTrait<'a, Fut> for SomeLocalStruct<'a, Fut> {
    fn deref_helper(&self) -> &mut (dyn SomeTrait<'a, Fut> + 'a) {
        let x = Box::new(self.0.borrow_mut());
        let x: &'a mut RefMut<'a, Fut> = Box::leak(x);
        &mut **x
    }
}
impl<'a, Fut: Future<Output = ()>> SomeTrait<'a, Fut> for Fut {
    fn downcast(self: Pin<&mut Self>) -> Pin<&mut Fut> {
        self
    }
}


impl<'b, 'a, Fut> DerefMut for Pin<&'b dyn SomeTrait<'a, Fut>> {
    fn deref_mut<'c>(
        self: &'c mut Pin<&'b dyn SomeTrait<'a, Fut>>,
    ) -> &'c mut (dyn SomeTrait<'a, Fut> + 'b) {
        self.deref_helper()
    }
}

// obviously a “working” function with this signature is problematic
pub fn unsound_pin<Fut: Future<Output = ()>>(
    fut: Fut,
    callback: impl FnOnce(Pin<&mut Fut>),
) -> Fut {
    let cell = RefCell::new(fut);
    let s: &SomeLocalStruct<'_, Fut> = &SomeLocalStruct(&cell);
    let p: Pin<Pin<&SomeLocalStruct<'_, Fut>>> = Pin::new(Pin::new(s));
    let mut p: Pin<Pin<&dyn SomeTrait<'_, Fut>>> = p;
    let r: Pin<&mut dyn SomeTrait<'_, Fut>> = p.as_mut();
    let f: Pin<&mut Fut> = r.downcast();
    callback(f);
    cell.into_inner()
}

////////////////////////////////////////////////////////////////////////////////////////////////////
// everything below here just exploitation of `unsound_pin` to get a segfault

fn yield_now() -> impl Future<Output = ()> {
    struct Yield(bool);
    impl Future for Yield {
        type Output = ();

        fn poll(mut self: Pin<&mut Self>, _cx: &mut std::task::Context<'_>) -> Poll<Self::Output> {
            if matches!(mem::replace(&mut *self, Yield(true)), Yield(true)) {
                Poll::Ready(())
            } else {
                Poll::Pending
            }
        }
    }
    Yield(false)
}

fn main() {
    let fut = async {
        let x = &&0;
        let reference = &x;
        yield_now().await; // future will be moved here
        dbg!(reference);
    };

    struct Waker;
    impl Wake for Waker {
        fn wake(self: std::sync::Arc<Self>) {}
    }
    let waker = Arc::new(Waker).into();
    let mut cx = Context::from_waker(&waker);

    let fut = unsound_pin(fut, |fut| {
        let _ = fut.poll(&mut cx);
    });
    // moving `fut`  vvv  after the first poll above, then polling again
    let _ = Box::pin(fut).as_mut().poll(&mut cx);
}

   Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 1.10s
     Running `target/debug/playground`
[src/main.rs:84] reference = timeout: the monitored command dumped core
/playground/tools/entrypoint.sh: line 11:     8 Segmentation fault      timeout --signal=KILL ${timeout} "$@"

(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 💥

The text was updated successfully, but these errors were encountered:

steffahn · 2021-05-09T11:38:25Z

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:

Explanation

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”.

khoek · 2021-05-09T12:37:36Z

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.

steffahn · 2021-05-09T12:55:26Z

@khoek Thanks, I’ve put in some clarification.

rustbot · 2021-05-11T13:55:31Z

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.

apiraino · 2021-05-11T14:45:47Z

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-high

y86-dev · 2021-11-18T14:19:18Z

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 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 ¹. 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.

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`.

↩

Aaron1011 · 2021-11-27T19:00:34Z

@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> for Pin where P: CoerceUnsized {}. 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> for Pin where P: CoerceUnsized {} 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 Pins, 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> for Pin where P: CoerceUnsized {} 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.

y86-dev · 2022-02-16T16:39:28Z

@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 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.

y86-dev · 2022-03-25T09:18:25Z

While @Aaron1011's approach works, I would prefer not to modify the behaviour of Pin<Pin> because

Pin<Pin> should behave equivalently to Pin, if it does not, this may create confusion for developers who do not know about this issue
it requires modifying the CoerceUnsized implementation of Pin again, when we can solve this issue using an other way (e.g. negative impls), adding additional burden for that
the unsoundness issue does not arise from using the Pin<Pin> to Pin<Pin> 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 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.

pnkfelix · 2022-11-18T15:52:40Z

Visiting for P-high review

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 )

withoutboats · 2022-12-07T02:57:07Z

(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 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.)

pnkfelix · 2023-03-03T16:02:50Z

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, 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

lcnr · 2023-04-20T12:25:17Z

This issue didn't get any types input over the last few months 😅

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.
ignoring "being #[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 though

will 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.

…unsound-issues, r=jackh726 Add `known-bug` tests for 11 unsound issues r? `@jackh726` Should tests for other issues be in separate PRs? Thanks. Edit: Partially addresses rust-lang#105107. This PR adds `known-bug` tests for 11 unsound issues: - rust-lang#25860 - rust-lang#49206 - rust-lang#57893 - rust-lang#84366 - rust-lang#84533 - rust-lang#84591 - rust-lang#85099 - rust-lang#98117 - rust-lang#100041 - rust-lang#100051 - rust-lang#104005

…unsound-issues, r=jackh726 Add `known-bug` tests for 11 unsound issues r? ``@jackh726`` Should tests for other issues be in separate PRs? Thanks. Edit: Partially addresses rust-lang#105107. This PR adds `known-bug` tests for 11 unsound issues: - rust-lang#25860 - rust-lang#49206 - rust-lang#57893 - rust-lang#84366 - rust-lang#84533 - rust-lang#84591 - rust-lang#85099 - rust-lang#98117 - rust-lang#100041 - rust-lang#100051 - rust-lang#104005

…unsound-issues, r=jackh726 Add `known-bug` tests for 11 unsound issues r? `@jackh726` Should tests for other issues be in separate PRs? Thanks. Edit: Partially addresses rust-lang#105107. This PR adds `known-bug` tests for 11 unsound issues: - rust-lang#25860 - rust-lang#49206 - rust-lang#57893 - rust-lang#84366 - rust-lang#84533 - rust-lang#84591 - rust-lang#85099 - rust-lang#98117 - rust-lang#100041 - rust-lang#100051 - rust-lang#104005

…unsound-issues, r=jackh726 Add `known-bug` tests for 11 unsound issues r? ``@jackh726`` Should tests for other issues be in separate PRs? Thanks. Edit: Partially addresses rust-lang#105107. This PR adds `known-bug` tests for 11 unsound issues: - rust-lang#25860 - rust-lang#49206 - rust-lang#57893 - rust-lang#84366 - rust-lang#84533 - rust-lang#84591 - rust-lang#85099 - rust-lang#98117 - rust-lang#100041 - rust-lang#100051 - rust-lang#104005

…unsound-issues, r=jackh726 Add `known-bug` tests for 11 unsound issues r? `@jackh726` Should tests for other issues be in separate PRs? Thanks. Edit: Partially addresses rust-lang#105107. This PR adds `known-bug` tests for 11 unsound issues: - rust-lang#25860 - rust-lang#49206 - rust-lang#57893 - rust-lang#84366 - rust-lang#84533 - rust-lang#84591 - rust-lang#85099 - rust-lang#98117 - rust-lang#100041 - rust-lang#100051 - rust-lang#104005

…unsound-issues, r=jackh726 Add `known-bug` tests for 11 unsound issues r? ``@jackh726`` Should tests for other issues be in separate PRs? Thanks. Edit: Partially addresses rust-lang#105107. This PR adds `known-bug` tests for 11 unsound issues: - rust-lang#25860 - rust-lang#49206 - rust-lang#57893 - rust-lang#84366 - rust-lang#84533 - rust-lang#84591 - rust-lang#85099 - rust-lang#98117 - rust-lang#100041 - rust-lang#100051 - rust-lang#104005

lcnr · 2023-05-09T16:23:02Z

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.

lcnr · 2023-05-09T16:30:52Z

I am available to mentor an implementation of disabling the fundamental type behavior for DerefMut. The steps are roughly:

add a new attribute #[rustc_no_fundamental_type_behavior] (name bikesheddable, given that it is private it doesn't matter too much)
set a new bool based on this attribute in the TraitDef
in the orphan check whenever we recurse for fundamental types, first check whether the trait has this attribute https://github.com/rust-lang/rust/blob/master/compiler/rustc_trait_selection/src/traits/coherence.rs#L690-L699
add the attribute to DerefMut

if you get stuck, please open a thread on zulip in the t-types stream.

RalfJung · 2023-05-09T18:17:46Z

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 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.

withoutboats · 2023-05-10T11:34:23Z

(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.

lcnr · 2023-05-19T14:44:02Z

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 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".

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)

RalfJung · 2023-05-19T15:33:16Z

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).

lcnr · 2023-05-22T14:34:25Z

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" 😁

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 🤔 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.

withoutboats · 2023-05-22T19:04:16Z

(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.

RalfJung · 2023-05-27T16:52:15Z

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.

lambinoo · 2023-05-30T21:30:53Z

@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.

WaffleLapkin · 2023-05-30T21:46:42Z

(@lambinoo I've assigned you; tip: you can use @rustbot claim to get assigned to an issue in this repo)

WIP WIP fix for rust-lang#85099, will write up a description w/ `@lcnr` when I'm at my computer r? lcnr

QuineDot · 2024-03-03T23:43:50Z

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.

Mark-Simulacrum added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label May 9, 2021

rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 9, 2021

steffahn changed the title ~~A Pin unsoundness involving an impl DerefMut for Pin<&dyn LocalTrait>~~ A Pin unsoundness involving an impl DerefMut for Pin<&dyn LocalTrait> May 9, 2021

steffahn mentioned this issue May 9, 2021

Pin is unsound due to transitive effects of CoerceUnsized #68015

Open

rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels May 11, 2021

oli-obk added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Oct 21, 2022

ojeda mentioned this issue Jan 26, 2023

Rust unstable features needed for the kernel Rust-for-Linux/linux#2

Open

74 tasks

gburgessiv mentioned this issue Feb 25, 2023

Add known-bug tests for all(?) I-unsound issues #105107

Open

rustbot added the I-types-nominated The issue / PR has been nominated for discussion during a types team meeting. label Mar 3, 2023

lcnr added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Apr 20, 2023

whtahy mentioned this issue Apr 21, 2023

Add known-bug tests for 11 unsound issues #110480

Merged

lcnr mentioned this issue Apr 24, 2023

Consider polarity in new solver #110671

Merged

jackh726 added the S-bug-has-test Status: This bug is tracked inside the repo by a `known-bug` test. label Apr 25, 2023

lcnr added E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. and removed I-types-nominated The issue / PR has been nominated for discussion during a types team meeting. labels May 9, 2023

WaffleLapkin assigned lambinoo May 30, 2023

compiler-errors assigned compiler-errors and unassigned lambinoo Oct 13, 2023

compiler-errors removed E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Oct 13, 2023

compiler-errors mentioned this issue Oct 13, 2023

WIP fix unsoundness via adjusting overlap check for some Pin impls #116706

Closed

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 13, 2023

Auto merge of rust-lang#116706 - compiler-errors:pin-negate, r=<try>

05d512a

WIP WIP fix for rust-lang#85099, will write up a description w/ `@lcnr` when I'm at my computer r? lcnr

compiler-errors removed their assignment Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A `Pin` unsoundness involving an `impl DerefMut for Pin<&dyn LocalTrait>` #85099

A `Pin` unsoundness involving an `impl DerefMut for Pin<&dyn LocalTrait>` #85099

A Pin unsoundness involving an impl DerefMut for Pin<&dyn LocalTrait> #85099

A Pin unsoundness involving an impl DerefMut for Pin<&dyn LocalTrait> #85099

Comments

Explanation

Footnotes

A `Pin` unsoundness involving an `impl DerefMut for Pin<&dyn LocalTrait>` #85099

A `Pin` unsoundness involving an `impl DerefMut for Pin<&dyn LocalTrait>` #85099