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

Fix soundness hole in join macros #2649

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 7 additions & 6 deletions futures-macro/src/join.rs
Expand Up @@ -38,6 +38,7 @@ fn bind_futures(fut_exprs: Vec<Expr>, span: Span) -> (Vec<TokenStream2>, Vec<Ide
// Move future into a local so that it is pinned in one place and
// is no longer accessible by the end user.
let mut #name = __futures_crate::future::maybe_done(#expr);
let mut #name = unsafe { __futures_crate::Pin::new_unchecked(&mut #name) };
});
name
})
Expand All @@ -58,12 +59,12 @@ pub(crate) fn join(input: TokenStream) -> TokenStream {
let poll_futures = future_names.iter().map(|fut| {
quote! {
__all_done &= __futures_crate::future::Future::poll(
unsafe { __futures_crate::Pin::new_unchecked(&mut #fut) }, __cx).is_ready();
#fut.as_mut(), __cx).is_ready();
}
});
let take_outputs = future_names.iter().map(|fut| {
quote! {
unsafe { __futures_crate::Pin::new_unchecked(&mut #fut) }.take_output().unwrap(),
#fut.as_mut().take_output().unwrap(),
}
});

Expand Down Expand Up @@ -96,17 +97,17 @@ pub(crate) fn try_join(input: TokenStream) -> TokenStream {
let poll_futures = future_names.iter().map(|fut| {
quote! {
if __futures_crate::future::Future::poll(
unsafe { __futures_crate::Pin::new_unchecked(&mut #fut) }, __cx).is_pending()
#fut.as_mut(), __cx).is_pending()
{
__all_done = false;
} else if unsafe { __futures_crate::Pin::new_unchecked(&mut #fut) }.output_mut().unwrap().is_err() {
} else if #fut.as_mut().output_mut().unwrap().is_err() {
// `.err().unwrap()` rather than `.unwrap_err()` so that we don't introduce
// a `T: Debug` bound.
// Also, for an error type of ! any code after `err().unwrap()` is unreachable.
#[allow(unreachable_code)]
return __futures_crate::task::Poll::Ready(
__futures_crate::Err(
unsafe { __futures_crate::Pin::new_unchecked(&mut #fut) }.take_output().unwrap().err().unwrap()
#fut.as_mut().take_output().unwrap().err().unwrap()
)
);
}
Expand All @@ -118,7 +119,7 @@ pub(crate) fn try_join(input: TokenStream) -> TokenStream {
// an `E: Debug` bound.
// Also, for an ok type of ! any code after `ok().unwrap()` is unreachable.
#[allow(unreachable_code)]
unsafe { __futures_crate::Pin::new_unchecked(&mut #fut) }.take_output().unwrap().ok().unwrap(),
#fut.as_mut().take_output().unwrap().ok().unwrap(),
}
});

Expand Down
10 changes: 6 additions & 4 deletions futures/tests/async_await_macros.rs
Expand Up @@ -346,36 +346,38 @@ fn stream_select() {
});
}

#[cfg_attr(not(target_pointer_width = "64"), ignore)]
#[test]
fn join_size() {
let fut = async {
let ready = future::ready(0i32);
join!(ready)
};
assert_eq!(mem::size_of_val(&fut), 12);
assert_eq!(mem::size_of_val(&fut), 24);

let fut = async {
let ready1 = future::ready(0i32);
let ready2 = future::ready(0i32);
join!(ready1, ready2)
};
assert_eq!(mem::size_of_val(&fut), 20);
assert_eq!(mem::size_of_val(&fut), 40);
}

#[cfg_attr(not(target_pointer_width = "64"), ignore)]
#[test]
fn try_join_size() {
let fut = async {
let ready = future::ready(Ok::<i32, i32>(0));
try_join!(ready)
};
assert_eq!(mem::size_of_val(&fut), 16);
assert_eq!(mem::size_of_val(&fut), 24);

let fut = async {
let ready1 = future::ready(Ok::<i32, i32>(0));
let ready2 = future::ready(Ok::<i32, i32>(0));
try_join!(ready1, ready2)
};
assert_eq!(mem::size_of_val(&fut), 28);
assert_eq!(mem::size_of_val(&fut), 48);
}

#[test]
Expand Down
32 changes: 32 additions & 0 deletions futures/tests/future_join.rs
@@ -0,0 +1,32 @@
use futures::executor::block_on;
use futures::future::Future;
use std::task::Poll;

/// This tests verifies (through miri) that self-referencing
/// futures are not invalidated when joining them.
#[test]
fn futures_join_macro_self_referential() {
block_on(async { futures::join!(yield_now(), trouble()) });
}

async fn trouble() {
let lucky_number = 42;
let problematic_variable = &lucky_number;

yield_now().await;

// problematic dereference
let _ = { *problematic_variable };
}

fn yield_now() -> impl Future<Output = ()> {
let mut yielded = false;
std::future::poll_fn(move |cx| {
if core::mem::replace(&mut yielded, true) {
Poll::Ready(())
} else {
cx.waker().wake_by_ref();
Poll::Pending
}
})
}