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

Potential unsoundness in FuturesUnordered #2786

Closed
robbie01 opened this issue Oct 25, 2023 · 2 comments · Fixed by #2788
Closed

Potential unsoundness in FuturesUnordered #2786

robbie01 opened this issue Oct 25, 2023 · 2 comments · Fixed by #2788
Labels
A-stream Area: futures::stream bug

Comments

@robbie01
Copy link

robbie01 commented Oct 25, 2023

I was browsing through the docs for FuturesUnordered when I noticed that its Sync bound is dependent on Fut being Sync rather than Send + Sync. This theoretically allows you to push a value that is !Send + Sync and use poll_next to retrieve it from another thread. Is my understanding of this correct? Apologies for raising an issue if I’m wrong.


Aside: why do FuturesUnordered::<Fut>::iter and the corresponding impl<'a, Fut> IntoIterator for &'a FuturesUnordered<Fut> require Fut: Unpin? That requirement can be circumvented safely via e.g. Pin::new(&f).iter_pin_ref().map(Pin::get_ref).

Aside aside: why do iter_pin_ref and iter_pin_mut require it to be pinned? FuturesUnordered implements Unpin unconditionally.

@robbie01
Copy link
Author

robbie01 commented Oct 25, 2023

Demonstrative example of unsafe-free UB (MutexGuard is !Send + Sync):

use std::{thread, future, sync::Mutex};
use futures::{executor, stream::{FuturesUnordered, StreamExt as _}};

fn main() {
    let m = Mutex::new(());
    let mut f = FuturesUnordered::new();
    
    thread::scope(|scope| {
        scope.spawn(|| {
            let guard = m.lock();
            f.push(future::ready(guard));
        });
    });

    dbg!(executor::block_on(f.next()));
}

@taiki-e taiki-e added bug A-stream Area: futures::stream labels Oct 26, 2023
taiki-e added a commit that referenced this issue Oct 26, 2023
taiki-e added a commit that referenced this issue Oct 26, 2023
@taiki-e
Copy link
Member

taiki-e commented Oct 26, 2023

Thanks for finding this! I filed #2788 to fix this.

taiki-e added a commit that referenced this issue Oct 26, 2023
taiki-e added a commit that referenced this issue Oct 26, 2023
taiki-e added a commit that referenced this issue Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stream Area: futures::stream bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants