safe doc(hidden) APIs (async_stream::AsyncStream::new, async_stream::Sender::send, async_stream::pair) allow UB

[ewilden]

This playground is an example of invoking UB using only the pub, safe API of this crate (pair(), AsyncStream::new(), and Sender::send()): https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8cf61ab15c81d7a946cdbf60a1fd4c46

The gist of this is inside of the "generator" passed to AsyncStream::new(receiver, generator), we can construct a Sender, Receiver pair for a different type from the Receiver the AsyncStream is yielding results from. We can use this Sender to send a u8 while generating an AsyncStream, which results in the AsyncStream yielding a String value that causes a segmentation fault when printed.

#[tokio::main(flavor = "current_thread")]
async fn main() {
    let (mut sndstr, mut rcvstr) = pair::<String>();
    let stream = AsyncStream::new(rcvstr, async {
        let (mut sndint, mut rcvint) = pair::<u8>();
        let send_fut = sndint.send(5);
        
        // hack to get tokio to wake again after Send::send
        tokio::select! {
            _ = send_fut => {} 
            _ = async {
                for _ in 1..=10 {
                    tokio::time::sleep(tokio::time::Duration::from_millis(10)).await;
                }
            } => {}
        }
    });
    
    for _ in 1..=10 {
        tokio::time::sleep(tokio::time::Duration::from_millis(10)).await;
    }

    stream.for_each(|item| {
        println!("about to segfault:");
        println!("item: {item:?}");
        futures::future::ready(())
    }).await;
    
    println!("done");
}

I'm not familiar enough with the crate implementation to say which part should be marked unsafe, but I think this shows at least one of (AsyncStream::new, Sender::send, pair) needs to be marked unsafe.

[taiki-e]

Thanks for the report. Those types are marked doc(hidden) and are not considered public API.

async-stream/async-stream/src/lib.rs

Lines 164 to 166 in ac3fa6b

	// Used by the macro, but not intended to be accessed publicly.
	#[doc(hidden)]
	pub use crate::async_stream::AsyncStream;

Is it possible to reproduce this using only the public API?

[ewilden]

Thanks for the response! I agree that these being doc(hidden) communicates that they are not intended to be part of the public API, and that actually trying to write code invoking them myself would constitute "abuse" rather than "use" -- apologies that the original issue title came across more strongly than intended.

I don't know of a way to produce this UB using only the doc-visible public API. I'm not able to find any reference for how unsafe conventions interact with doc(hidden), but my perspective as someone who would potentially review code using this crate is that, in the context of a code review, I don't expect to have to check whether we're referencing a doc(hidden) symbol in order to audit for potential UB (even if, as a reviewer, I would be very reluctant to let someone use a module path that included __private...). To me, marking these functions as unsafe is more in the spirit of how unsafe serves to highlight contracts you have to avoid violating in order to avoid UB -- please correct me if I'm off-base here, though!

[taiki-e]

Addressed by #84.