From 90b3a27acad8352d682bf64551c8c6c6eb137eb8 Mon Sep 17 00:00:00 2001 From: "Stainsby, Hayden" Date: Mon, 4 Jul 2022 12:54:39 +0200 Subject: [PATCH] net: add track_caller to public APIs Functions that may panic can be annotated with #[track_caller] so that in the event of a panic, the function where the user called the panicking function is shown instead of the file and line within Tokio source. This change adds #[track_caller] to all the non-unstable public net APIs in the main tokio crate where the documentation describes how the function may panic due to incorrect context or inputs. Not all panic cases can have #[track_caller] applied fully as the callstack passes through a closure which isn't yet supported by the annotation (e.g. net functions called from outside a tokio runtime). Additionally, the documentation was updated to indicate additional cases in which the public net functions may panic (the same as the io functions). Tests are included to cover each potentially panicking function. Refs: #4413 --- tokio/src/io/poll_evented.rs | 2 + tokio/src/net/tcp/listener.rs | 4 +- tokio/src/net/tcp/stream.rs | 4 +- tokio/src/net/udp.rs | 2 + tokio/src/net/unix/datagram/socket.rs | 4 +- tokio/src/net/unix/listener.rs | 8 +- tokio/src/net/unix/stream.rs | 4 +- tokio/src/runtime/context.rs | 1 + tokio/tests/net_panic.rs | 221 ++++++++++++++++++++++++++ 9 files changed, 244 insertions(+), 6 deletions(-) create mode 100644 tokio/tests/net_panic.rs diff --git a/tokio/src/io/poll_evented.rs b/tokio/src/io/poll_evented.rs index ce4c1426acc..df071897d91 100644 --- a/tokio/src/io/poll_evented.rs +++ b/tokio/src/io/poll_evented.rs @@ -77,6 +77,7 @@ impl PollEvented { /// The runtime is usually set implicitly when this function is called /// from a future driven by a tokio runtime, otherwise runtime can be set /// explicitly with [`Runtime::enter`](crate::runtime::Runtime::enter) function. + #[track_caller] #[cfg_attr(feature = "signal", allow(unused))] pub(crate) fn new(io: E) -> io::Result { PollEvented::new_with_interest(io, Interest::READABLE | Interest::WRITABLE) @@ -97,6 +98,7 @@ impl PollEvented { /// a future driven by a tokio runtime, otherwise runtime can be set /// explicitly with [`Runtime::enter`](crate::runtime::Runtime::enter) /// function. + #[track_caller] #[cfg_attr(feature = "signal", allow(unused))] pub(crate) fn new_with_interest(io: E, interest: Interest) -> io::Result { Self::new_with_interest_and_handle(io, interest, Handle::current()) diff --git a/tokio/src/net/tcp/listener.rs b/tokio/src/net/tcp/listener.rs index 8aecb21aaa9..c5d3491054c 100644 --- a/tokio/src/net/tcp/listener.rs +++ b/tokio/src/net/tcp/listener.rs @@ -216,11 +216,13 @@ impl TcpListener { /// /// # Panics /// - /// This function panics if thread-local runtime is not set. + /// This function panics if there is no current reactor set, if the `rt` + /// feature flag is not enabled, or if thread-local runtime is not set. /// /// The runtime is usually set implicitly when this function is called /// from a future driven by a tokio runtime, otherwise runtime can be set /// explicitly with [`Runtime::enter`](crate::runtime::Runtime::enter) function. + #[track_caller] pub fn from_std(listener: net::TcpListener) -> io::Result { let io = mio::net::TcpListener::from_std(listener); let io = PollEvented::new(io)?; diff --git a/tokio/src/net/tcp/stream.rs b/tokio/src/net/tcp/stream.rs index 204d9ca256c..d9191309e92 100644 --- a/tokio/src/net/tcp/stream.rs +++ b/tokio/src/net/tcp/stream.rs @@ -181,11 +181,13 @@ impl TcpStream { /// /// # Panics /// - /// This function panics if thread-local runtime is not set. + /// This function panics if there is no current reactor set, if the `rt` + /// feature flag is not enabled, or if thread-local runtime is not set. /// /// The runtime is usually set implicitly when this function is called /// from a future driven by a tokio runtime, otherwise runtime can be set /// explicitly with [`Runtime::enter`](crate::runtime::Runtime::enter) function. + #[track_caller] pub fn from_std(stream: std::net::TcpStream) -> io::Result { let io = mio::net::TcpStream::from_std(stream); let io = PollEvented::new(io)?; diff --git a/tokio/src/net/udp.rs b/tokio/src/net/udp.rs index bd905e91a5f..218029176e0 100644 --- a/tokio/src/net/udp.rs +++ b/tokio/src/net/udp.rs @@ -170,6 +170,7 @@ impl UdpSocket { UdpSocket::new(sys) } + #[track_caller] fn new(socket: mio::net::UdpSocket) -> io::Result { let io = PollEvented::new(socket)?; Ok(UdpSocket { io }) @@ -210,6 +211,7 @@ impl UdpSocket { /// # Ok(()) /// # } /// ``` + #[track_caller] pub fn from_std(socket: net::UdpSocket) -> io::Result { let io = mio::net::UdpSocket::from_std(socket); UdpSocket::new(io) diff --git a/tokio/src/net/unix/datagram/socket.rs b/tokio/src/net/unix/datagram/socket.rs index def006c4761..275eb14325c 100644 --- a/tokio/src/net/unix/datagram/socket.rs +++ b/tokio/src/net/unix/datagram/socket.rs @@ -430,7 +430,8 @@ impl UnixDatagram { /// /// # Panics /// - /// This function panics if thread-local runtime is not set. + /// This function panics if there is no current reactor set, if the `rt` + /// feature flag is not enabled, or if thread-local runtime is not set. /// /// The runtime is usually set implicitly when this function is called /// from a future driven by a Tokio runtime, otherwise runtime can be set @@ -457,6 +458,7 @@ impl UnixDatagram { /// # Ok(()) /// # } /// ``` + #[track_caller] pub fn from_std(datagram: net::UnixDatagram) -> io::Result { let socket = mio::net::UnixDatagram::from_std(datagram); let io = PollEvented::new(socket)?; diff --git a/tokio/src/net/unix/listener.rs b/tokio/src/net/unix/listener.rs index 1785f8b0f73..35da60457a2 100644 --- a/tokio/src/net/unix/listener.rs +++ b/tokio/src/net/unix/listener.rs @@ -54,11 +54,13 @@ impl UnixListener { /// /// # Panics /// - /// This function panics if thread-local runtime is not set. + /// This function panics if there is no current reactor set, if the `rt` + /// feature flag is not enabled, or if thread-local runtime is not set. /// /// The runtime is usually set implicitly when this function is called /// from a future driven by a tokio runtime, otherwise runtime can be set /// explicitly with [`Runtime::enter`](crate::runtime::Runtime::enter) function. + #[track_caller] pub fn bind

(path: P) -> io::Result where P: AsRef, @@ -77,11 +79,13 @@ impl UnixListener { /// /// # Panics /// - /// This function panics if thread-local runtime is not set. + /// This function panics if there is no current reactor set, if the `rt` + /// feature flag is not enabled, or if thread-local runtime is not set. /// /// The runtime is usually set implicitly when this function is called /// from a future driven by a tokio runtime, otherwise runtime can be set /// explicitly with [`Runtime::enter`](crate::runtime::Runtime::enter) function. + #[track_caller] pub fn from_std(listener: net::UnixListener) -> io::Result { let listener = mio::net::UnixListener::from_std(listener); let io = PollEvented::new(listener)?; diff --git a/tokio/src/net/unix/stream.rs b/tokio/src/net/unix/stream.rs index fe2d825bf98..a24836f7dee 100644 --- a/tokio/src/net/unix/stream.rs +++ b/tokio/src/net/unix/stream.rs @@ -699,11 +699,13 @@ impl UnixStream { /// /// # Panics /// - /// This function panics if thread-local runtime is not set. + /// This function panics if there is no current reactor set, if the `rt` + /// feature flag is not enabled, or if thread-local runtime is not set. /// /// The runtime is usually set implicitly when this function is called /// from a future driven by a tokio runtime, otherwise runtime can be set /// explicitly with [`Runtime::enter`](crate::runtime::Runtime::enter) function. + #[track_caller] pub fn from_std(stream: net::UnixStream) -> io::Result { let stream = mio::net::UnixStream::from_std(stream); let io = PollEvented::new(stream)?; diff --git a/tokio/src/runtime/context.rs b/tokio/src/runtime/context.rs index 36fa28db356..4215124fc83 100644 --- a/tokio/src/runtime/context.rs +++ b/tokio/src/runtime/context.rs @@ -24,6 +24,7 @@ pub(crate) fn current() -> Handle { } cfg_io_driver! { + #[track_caller] pub(crate) fn io_handle() -> crate::runtime::driver::IoHandle { match CONTEXT.try_with(|ctx| { let ctx = ctx.borrow(); diff --git a/tokio/tests/net_panic.rs b/tokio/tests/net_panic.rs new file mode 100644 index 00000000000..3165e914347 --- /dev/null +++ b/tokio/tests/net_panic.rs @@ -0,0 +1,221 @@ +#![warn(rust_2018_idioms)] +#![cfg(feature = "full")] + +use std::error::Error; +use tokio::net::{TcpListener, TcpStream}; +use tokio::runtime::{Builder, Runtime}; + +mod support { + pub mod panic; +} +use support::panic::test_panic; + +#[test] +fn udp_socket_from_std_panic_caller() -> Result<(), Box> { + use std::net::SocketAddr; + use tokio::net::UdpSocket; + + let addr = "127.0.0.1:8080".parse::().unwrap(); + let std_sock = std::net::UdpSocket::bind(addr).unwrap(); + std_sock.set_nonblocking(true).unwrap(); + + let panic_location_file = test_panic(|| { + let rt = runtime_without_io(); + rt.block_on(async { + let _sock = UdpSocket::from_std(std_sock); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn tcp_listener_from_std_panic_caller() -> Result<(), Box> { + let std_listener = std::net::TcpListener::bind("127.0.0.1:8080").unwrap(); + std_listener.set_nonblocking(true).unwrap(); + + let panic_location_file = test_panic(|| { + let rt = runtime_without_io(); + rt.block_on(async { + let _ = TcpListener::from_std(std_listener); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn tcp_stream_from_std_panic_caller() -> Result<(), Box> { + let std_listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + + let std_stream = std::net::TcpStream::connect(std_listener.local_addr().unwrap()).unwrap(); + std_stream.set_nonblocking(true).unwrap(); + + let panic_location_file = test_panic(|| { + let rt = runtime_without_io(); + rt.block_on(async { + let _ = TcpStream::from_std(std_stream); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +#[cfg(unix)] +fn unix_listener_bind_panic_caller() -> Result<(), Box> { + use tokio::net::UnixListener; + + let dir = tempfile::tempdir().unwrap(); + let sock_path = dir.path().join("socket"); + + let panic_location_file = test_panic(|| { + let rt = runtime_without_io(); + rt.block_on(async { + let _ = UnixListener::bind(&sock_path); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +#[cfg(unix)] +fn unix_listener_from_std_panic_caller() -> Result<(), Box> { + use tokio::net::UnixListener; + + let dir = tempfile::tempdir().unwrap(); + let sock_path = dir.path().join("socket"); + let std_listener = std::os::unix::net::UnixListener::bind(&sock_path).unwrap(); + + let panic_location_file = test_panic(|| { + let rt = runtime_without_io(); + rt.block_on(async { + let _ = UnixListener::from_std(std_listener); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +#[cfg(unix)] +fn unix_stream_from_std_panic_caller() -> Result<(), Box> { + use tokio::net::UnixStream; + + let dir = tempfile::tempdir().unwrap(); + let sock_path = dir.path().join("socket"); + let _std_listener = std::os::unix::net::UnixListener::bind(&sock_path).unwrap(); + let std_stream = std::os::unix::net::UnixStream::connect(&sock_path).unwrap(); + + let panic_location_file = test_panic(|| { + let rt = runtime_without_io(); + rt.block_on(async { + let _ = UnixStream::from_std(std_stream); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +#[cfg(unix)] +fn unix_datagram_from_std_panic_caller() -> Result<(), Box> { + use std::os::unix::net::UnixDatagram as StdUDS; + use tokio::net::UnixDatagram; + + let dir = tempfile::tempdir().unwrap(); + let sock_path = dir.path().join("socket"); + + // Bind the socket to a filesystem path + // /let socket_path = tmp.path().join("socket"); + let std_socket = StdUDS::bind(&sock_path).unwrap(); + std_socket.set_nonblocking(true).unwrap(); + + let panic_location_file = test_panic(move || { + let rt = runtime_without_io(); + rt.block_on(async { + let _ = UnixDatagram::from_std(std_socket); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +#[cfg(windows)] +fn named_pipe_count_panic_caller() -> Result<(), Box> { + use std::time::Duration; + use tokio::net::windows::named_pipe::ClientOptions; + use tokio::time; + use winapi::shared::winerror; + + const PIPE_NAME: &str = r"\\.\pipe\named-pipe-idiomatic-client"; + + let rt = Builder::new_current_thread().enable_all().build().unwrap(); + rt.block_on(async { + let server = tokio::spawn(async move { + loop { + // Wait for a client to connect. + let connected = server.connect().await?; + + // Construct the next server to be connected before sending the one + // we already have of onto a task. This ensures that the server + // isn't closed (after it's done in the task) before a new one is + // available. Otherwise the client might error with + // `io::ErrorKind::NotFound`. + server = ServerOptions::new().create(PIPE_NAME)?; + + let client = tokio::spawn(async move { + /* use the connected client */ + Ok::<_, std::io::Error>(()) + }); + if true { + break; + } // needed for type inference to work + } + + Ok::<_, io::Error>(()) + }); + + let client = loop { + match ClientOptions::new().open(PIPE_NAME) { + Ok(client) => break client, + Err(e) if e.raw_os_error() == Some(winerror::ERROR_PIPE_BUSY as i32) => (), + Err(e) => return Err(e), + } + + time::sleep(Duration::from_millis(50)).await; + }; + + client.max_instances(255); + }); + + Ok(()) +} + +// Runtime without `enable_io` so it has no IO driver set. +fn runtime_without_io() -> Runtime { + Builder::new_current_thread().build().unwrap() +}