Skip to content

Commit

Permalink
Fix soundness hole in join macros (#2649)
Browse files Browse the repository at this point in the history
* fix soundness hole in join macros
add a miri regression test
update failing tests (join sizes increased due to fix)

* fix `CI / cross test` by ignoring `join_size` and `try_join_size` tests on "non-64-bit pointer" targets (e.g. `i686-unknown-linux-gnu`)
(this is the same fix that was also applied in PR #2447)
  • Loading branch information
Pointerbender authored Oct 13, 2022
1 parent 930d3e0 commit f947828
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 10 deletions.
13 changes: 7 additions & 6 deletions futures-macro/src/join.rs
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
@@ -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
}
})
}

0 comments on commit f947828

Please sign in to comment.