select/join allows for moving pinned futures

[udoprog]

Sorry to be the bearer of bad news. Pinning is a tricky subject and can be quite subtle.

Describe the bug

The current implementation of select allows for moving a future which is assumed to be pinned. Among other potential issues, this enables reading freed memory in safe code.

To Reproduce

The following should showcase the issue:

Edit: More compact example, and a bit more comments.

use std::future::Future;
use std::pin::Pin;
use std::task::{Context, Poll};
use std::mem;
use futures::future;

use pasts::prelude::*;

async fn doit(leak: u128) {
    if leak == 42 {
        // Take a reference to leak which is a local variable to this function. And have it live across an await.
        let leak = &leak;
        println!("{} {:?}", leak, leak as *const _);

        let mut pending = true;

        // future that only returns pending the first time it's polled.
        future::poll_fn(move |_| if mem::take(&mut pending) {
            Poll::Pending
        } else {
            Poll::Ready(())
        }).await;

        println!("{} {:?}", leak, leak as *const _);
    } else {
        // do nothing to drive the select
    }
}

#[tokio::main]
async fn main() {
    let mut v = [doit(42), doit(0)];
    println!("old location of future: {:?}", v.as_ptr());

    v.select().await;

    // move the future.
    let a = std::mem::replace(&mut v[0], doit(0));
    println!("new location of future: {:?}", &a as *const _);
    a.await;
}

Running it in debug mode for me gives:

old location of future: 0x7ffe11d595b0
42 0x7ffe11d595c0
new location of future: 0x7ffe11d59618
2595995492391351414651693012353024 0x7ffe11d595c0

In effect: The second read of the reference to &foo uses an outdated memory location, since the future has been moved the second time it was polled.

Expected behavior

The select implementation should require the futures to be Unpin, or maintain Pin invariants in some other way to prevent this from compiling.

[AldaronLau]

Well, thanks for finding this bug anyway! I'll have to fix this.

[AldaronLau]

Select now requires the futures to be Unpin. This doesn't change the API or break any of my code using pasts, but it does break the above example. I'll release pasts 0.2.0 later today. Thanks, again.

[udoprog]

Much obliged. Thanks for the fast response!

[udoprog]

So I looked at the fix. It looks mostly good, what I'd be concerned of would be if SelectFuture can somehow be constructed manually. I don't think that's the case (since the module is private), but there has been cases in the past where you can access hidden types through traits. All though not as a return value position to my knowledge.

A "safer" fix in my view would be to constrain the Future implementation for SelectFuture with Unpin, which wouldn't change how it's used (since it's already constrained in the trait), and would even allow for getting rid of the unsafe code in it.

Another note is that I believe the join implementation has the same issue as here. All though I haven't produced code that showcases it yet. You can at least do:

let mut v = (fut,);
v.join().await;
// future can now be freely moved out of `v`.
let (fut,) = v;

So you'd probably want to add the same Unpin constraint there as well.

[AldaronLau]

Oh wow, thanks! For some reason I thought join was implemented differently, but it looks like it's the same. I'll have to try that with Select, getting rid of the unsafe code in it would be nice! Re openning as it's the same problem that needs to get fixed, even though a different type.

[AldaronLau]

I made some changes that'll hopefully fix those problems. I'll wait for you to let me know if it all "looks good" before I close the issue again and release a new version with the changes. Thanks a ton!

[udoprog]

Yeah, from a cursory glance it looks correct to me. Thanks!