Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pin docs should mention pitfalls of generic code #62391

Open
Aaron1011 opened this issue Jul 4, 2019 · 3 comments
Open

Pin docs should mention pitfalls of generic code #62391

Aaron1011 opened this issue Jul 4, 2019 · 3 comments
Labels
A-async-await A-docs AsyncAwait-Triaged C-enhancement P-medium T-lang T-libs-api

Comments

@Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Jul 4, 2019

The pin module docs currently have a note about implementing Future combinators:

When implementing a Future combinator, you will usually need structural pinning for the nested futures, as you need to get pinned references to them to call poll.

However, this can be tricky in the presence of generics. Consider this code.

use std::future::Future;
use std::task::Context;
use std::pin::Pin;
use std::task::Poll;
use std::marker::{Unpin, PhantomPinned};

mod other_mod {
    use super::*;

    pub enum DubiousDrop {
        First(PhantomPinned),
        Second(PhantomPinned)
    }

    impl Drop for DubiousDrop {
        fn drop(&mut self) {
            std::mem::forget(std::mem::replace(self, match self {
                &mut DubiousDrop::First(_) => DubiousDrop::Second(PhantomPinned),
                &mut DubiousDrop::Second(_) => DubiousDrop::First(PhantomPinned)
            }))
        }
    }

    impl Future for DubiousDrop {
        type Output = ();
        fn poll(self: Pin<&mut Self>, _cx: &mut Context) -> Poll<()> {
            Poll::Ready(())
        }
    }
}


struct MyWrapper<F: Future<Output = ()>> {
    pub fut: F
}

impl<F: Future<Output = ()>> Future for MyWrapper<F> {
    type Output = ();
    
    fn poll(self: Pin<&mut Self>, cx: &mut Context) -> Poll<()> {
        // Unsound - we don't know that 'fut' upholds the
        // guarantees of 'Pin'
        unsafe { self.map_unchecked_mut(|s| &mut s.fut) }.poll(cx)
    }
}

fn assert_unpin<T: Unpin>() {}

fn boom(cx: &mut Context) {
    // Uncomment to trigger a compilation error
    //assert_unpin::<MyWrapper<other_mod::DubiousDrop>>();
    let mut wrapper = Box::pin(MyWrapper { fut: other_mod::DubiousDrop::First(PhantomPinned) });
    Pin::new(&mut wrapper).poll(cx);
}

fn main() {}

Here, we have an enum DubiousDrop, whose destructor changes the enum variant. This type is slightly contrived but is perfectly safe (in fact, the entire containing module is safe).

We then implement a simple pass-through future combinator called MyWrapper, which simply delegates to a wrapped future using a pin projection. Unfortunately, MyWrapper is unsound - it constructs a Pin<&mut F> without knowing if F upholds the Pin guarantees. Our DubiousDrop enum explicitly does not uphold these guarantees - its destructor invalidates its memory by changing the enum variant, and it is not Unpin. Therefore, calling boom triggers UB in safe code.

AFAICT, there are two ways to prevent this kind of issue:

  1. Require that the wrapped Future be Unpin (in this case, adding an F: Unpin bound to MyWrapper. This is the approach taken by types like futures::future::Select.
  2. Make it unsafe to construct the wrapper/combinator type, and document that the caller must uphold the Pin requirements on their type.

However, none of this appears to explicitly documented, and it's fairly subtle. I think it's important for the std::pin and std::pin::Pin docs to make these requirements very clear, as it seems to be somewhat of a footgun.

@Centril Centril added the A-docs label Jul 5, 2019
@Centril
Copy link
Contributor

@Centril Centril commented Jul 5, 2019

cc @RalfJung @withoutboats

@Centril Centril added the C-enhancement label Jul 5, 2019
@withoutboats
Copy link
Contributor

@withoutboats withoutboats commented Jul 5, 2019

I don't see any undefined behavior here. Mutating a pinned value is perfectly legal. Consider that this code (switching between enum variants) is essentially what every async function does every time you call poll.

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 7, 2019

Here, we have an enum DubiousDrop, whose destructor changes the enum variant. This type is slightly contrived but is perfectly safe (in fact, the entire containing module is safe).

Our DubiousDrop enum explicitly does not uphold these guarantees - its destructor invalidates its memory by changing the enum variant, and it is not Unpin.

There is no known way to break the pinning guarantees in safe code, and that would indeed be a soundness bug.

As @withoutboats said, you code is fine. However, I can see how the wording in the pinning docs might be a bit confusing here. I also don't have a good idea for how to improve them though.

What is not allowed is to invalidate the storage of a pinned type. That is not the same as invalidating the storage of the contents of a pinned type! In your example, the only storage DubiousDrop invalidates is that of a PhantomPinned. There is nothing wrong with that. If you replace PhantomPinned by some real pinned data, like a future, you will notice that you won't be able to construct a violation in safe code.

It might seem strange that the same storage considered from the "inside" vs the "outside" of the type looks different, but this is actually not that uncommon: in a MaybeUninit<T>, it also makes a huge difference whether we consider that storage from the "outside" (at type MaybeUninit<T>) or from the "inside" (at type T). Even for pinning itself, the docs describe that a type like MyStruct<T> { t: T } could decide that the field t is not pinned when MyStruct is pinned -- even though they cover the same storage.

@jonas-schievink jonas-schievink added T-lang T-libs-api A-async-await labels Mar 6, 2020
@tmandry tmandry added AsyncAwait-Triaged P-medium labels Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await A-docs AsyncAwait-Triaged C-enhancement P-medium T-lang T-libs-api
Projects
None yet
Development

No branches or pull requests

6 participants