From 353112db20f282dca24ab08010b054e0a2849ffb Mon Sep 17 00:00:00 2001 From: WhySoBad <49595640+WhySoBad@users.noreply.github.com> Date: Wed, 6 May 2026 16:16:55 +0200 Subject: [PATCH] Remove `TcpStream::peer_addr` check for TCP sockets Also add tests about socket readiness after calling `shutdown` --- src/tools/miri/src/shims/unix/socket.rs | 80 ++++++++++-------- .../libc/libc-socket-no-blocking-epoll.rs | 81 +++++++++++++++++++ .../miri/tests/pass-dep/libc/libc-socket.rs | 5 -- 3 files changed, 129 insertions(+), 37 deletions(-) diff --git a/src/tools/miri/src/shims/unix/socket.rs b/src/tools/miri/src/shims/unix/socket.rs index ca0ddfd4726a..c835ba79fbe7 100644 --- a/src/tools/miri/src/shims/unix/socket.rs +++ b/src/tools/miri/src/shims/unix/socket.rs @@ -602,11 +602,8 @@ fn connect( ), } - // Mio returns a potentially unconnected stream. - // We can be ensured that the connection is established when - // [`TcpStream::take_err`] and [`TcpStream::peer_addr`] both - // don't return an error after receiving an [`Interest::WRITEABLE`] - // event on the stream. + // This begins establishing the connection, but does not block until the stream is fully connected. + // We deal with that below. match TcpStream::connect(address) { Ok(stream) => { *socket.state.borrow_mut() = SocketState::Connecting(stream); @@ -1112,22 +1109,45 @@ fn shutdown(&mut self, socket: &OpTy<'tcx>, how: &OpTy<'tcx>) -> InterpResult<'t return this.set_last_error_and_return_i32(LibcError("ENOTCONN")); }; - let shut_rd = this.eval_libc_i32("SHUT_RD"); - let shut_wr = this.eval_libc_i32("SHUT_WR"); - let shut_rdwr = this.eval_libc_i32("SHUT_RDWR"); + let is_read_shutdown = how == this.eval_libc_i32("SHUT_RD"); + let is_write_shutdown = how == this.eval_libc_i32("SHUT_WR"); + let is_read_write_shutdown = how == this.eval_libc_i32("SHUT_RDWR"); let how = match () { - _ if how == shut_rd => Shutdown::Read, - _ if how == shut_wr => Shutdown::Write, - _ if how == shut_rdwr => Shutdown::Both, + _ if is_read_shutdown => Shutdown::Read, + _ if is_write_shutdown => Shutdown::Write, + _ if is_read_write_shutdown => Shutdown::Both, // An invalid value was passed to `how`. _ => return this.set_last_error_and_return_i32(LibcError("EINVAL")), }; - match stream.shutdown(how) { - Ok(_) => interp_ok(Scalar::from_i32(0)), - Err(e) => this.set_last_error_and_return_i32(e), - } + if let Err(e) = stream.shutdown(how) { + return this.set_last_error_and_return_i32(e); + }; + + drop(state); + + // Because we map cross platform mio readiness to epoll readiness and + // the different platforms don't treat `shutdown` the same way, we set + // the readiness after a `shutdown` manually to achieve more consistent + // epoll readiness. Otherwise we do not generate enough epoll events + // on partial shutdowns on Windows hosts. + let mut readiness = socket.io_readiness.borrow_mut(); + // Closing the read end of a socket causes an EPOLLRDHUP event. + readiness.read_closed |= is_read_shutdown || is_read_write_shutdown; + // Only shutting down the write end doesn't cause an EPOLLHUP event + // and thus we won't set the `write_closed` readiness for it here. + readiness.write_closed |= is_read_write_shutdown; + // The Linux kernel also sets EPOLLIN when both ends of a socket are closed: + // + readiness.readable |= is_read_write_shutdown; + + drop(readiness); + + // Update the epoll readiness for the socket. + this.update_epoll_active_events(socket, /* force_edge */ false)?; + + interp_ok(Scalar::from_i32(0)) } } @@ -1506,24 +1526,20 @@ fn ensure_connected( return action.call(this, Err(())) } - // There was no error during connecting. We still need to ensure that - // the wakeup wasn't spurious. We do this by attempting to read the - // peer address of the socket (following the advice given by mio): + // There was no error during connecting. Mio advises also reading the peer address + // to ensure that socket is actually connected and that it wasn't a spurious wake-up: // - - match stream.peer_addr() { - Ok(_) => { /* fall-through to below */}, - Err(e) if matches!(e.kind(), io::ErrorKind::NotConnected | io::ErrorKind::InProgress) => { - // We received a spurious wakeup from the OS. This should be considered an OS bug: - // - panic!("{foreign_name}: received writable event from OS but socket is not yet connected") - }, - Err(_) => { - // For all other errors the socket is connected. Since we're not interested in the - // peer address and only want to know whether the socket is connected, we can ignore - // the error and continue. - } - } + // + // Attempting to read the peer address would introduce an edge-case where the + // write end of the socket could already be shutdown before it received a + // writable event. When we then call [`TcpStream::peer_addr`] we receive an + // error. This would need extra state for storing whether the write end was + // manually closed using `shutdown`. + // Also, tokio doesn't read the peer address and everything seems to be fine, + // so we don't do that either: + // + // In other words, we are assuming that there will be no spurious + // wakeups while establishing the connection. // The connection is established. diff --git a/src/tools/miri/tests/pass-dep/libc/libc-socket-no-blocking-epoll.rs b/src/tools/miri/tests/pass-dep/libc/libc-socket-no-blocking-epoll.rs index b162319d7557..6e3dac25956b 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-socket-no-blocking-epoll.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-socket-no-blocking-epoll.rs @@ -24,6 +24,9 @@ fn main() { test_recv_nonblock(); #[cfg(not(windows_hosts))] test_send_nonblock(); + test_shutdown_read_write(); + test_shutdown_read(); + test_shutdown_write(); } /// Test that connecting to a server socket works when the client @@ -295,3 +298,81 @@ fn test_send_nonblock() { reader_thread.join().unwrap(); } + +/// Test that the EPOLLHUP and EPOLLRDHUP readiness are set when both +/// the read and write ends of a socket are closed. +fn test_shutdown_read_write() { + let (server_sockfd, addr) = net::make_listener_ipv4().unwrap(); + let client_sockfd = + unsafe { errno_result(libc::socket(libc::AF_INET, libc::SOCK_STREAM, 0)).unwrap() }; + let epfd = unsafe { libc::epoll_create1(0) }; + + // Spawn the server thread. + let server_thread = thread::spawn(move || net::accept_ipv4(server_sockfd).unwrap()); + + net::connect_ipv4(client_sockfd, addr).unwrap(); + + epoll_ctl_add(epfd, client_sockfd, EPOLLET | EPOLLHUP | EPOLLRDHUP | EPOLLIN).unwrap(); + + // Close the read and write end of the socket. + unsafe { libc::shutdown(client_sockfd, libc::SHUT_RDWR) }; + + // Ensure that the "read end closed", "write end closed", and "readable" readiness are set. + check_epoll_wait::<8>( + epfd, + &[Ev { events: EPOLLRDHUP | EPOLLHUP | EPOLLIN, data: client_sockfd }], + -1, + ); + + server_thread.join().unwrap(); +} + +/// Test that the EPOLLRDHUP readiness is set when the read +/// end of a socket is closed. +fn test_shutdown_read() { + let (server_sockfd, addr) = net::make_listener_ipv4().unwrap(); + let client_sockfd = + unsafe { errno_result(libc::socket(libc::AF_INET, libc::SOCK_STREAM, 0)).unwrap() }; + let epfd = unsafe { libc::epoll_create1(0) }; + + // Spawn the server thread. + let server_thread = thread::spawn(move || net::accept_ipv4(server_sockfd).unwrap()); + + net::connect_ipv4(client_sockfd, addr).unwrap(); + + epoll_ctl_add(epfd, client_sockfd, EPOLLET | EPOLLHUP | EPOLLRDHUP).unwrap(); + + // Close the read end of the socket. + unsafe { libc::shutdown(client_sockfd, libc::SHUT_RD) }; + + // Ensure that the "read end closed" readiness is set. + check_epoll_wait::<8>(epfd, &[Ev { events: EPOLLRDHUP, data: client_sockfd }], -1); + + server_thread.join().unwrap(); +} + +/// Test that the EPOLLRDHUP readiness is set when the write +/// end of the peer socket is closed. +fn test_shutdown_write() { + let (server_sockfd, addr) = net::make_listener_ipv4().unwrap(); + let client_sockfd = + unsafe { errno_result(libc::socket(libc::AF_INET, libc::SOCK_STREAM, 0)).unwrap() }; + let epfd = unsafe { libc::epoll_create1(0) }; + + // Spawn the server thread. + let server_thread = thread::spawn(move || { + let (peerfd, _) = net::accept_ipv4(server_sockfd).unwrap(); + // Close the write end of the peer socket. + unsafe { libc::shutdown(peerfd, libc::SHUT_WR) }; + }); + + net::connect_ipv4(client_sockfd, addr).unwrap(); + + epoll_ctl_add(epfd, client_sockfd, EPOLLET | EPOLLHUP | EPOLLRDHUP).unwrap(); + + // Ensure that the "read end closed" readiness is set when + // the write end of the peer is closed. + check_epoll_wait::<8>(epfd, &[Ev { events: EPOLLRDHUP, data: client_sockfd }], -1); + + server_thread.join().unwrap(); +} diff --git a/src/tools/miri/tests/pass-dep/libc/libc-socket.rs b/src/tools/miri/tests/pass-dep/libc/libc-socket.rs index 33f99371eee6..6e6704b0b279 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-socket.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-socket.rs @@ -562,9 +562,6 @@ fn test_shutdown() { assert_eq!(bytes_read, 0); } - // TODO: Once epoll is available for TCP sockets, ensure that the rdhup and hup readiness - // are set. - // Closing should affect previously duplicated handles. unsafe { let err = @@ -608,7 +605,6 @@ fn test_shutdown_readable_after_write_close() { unsafe { // Close the write end. libc::shutdown(client_sockfd, libc::SHUT_WR); - // Ensure that we're still readable. let mut byte = [0u8]; errno_result(libc::read(client_sockfd, byte.as_mut_ptr().cast(), 1)).unwrap(); @@ -633,7 +629,6 @@ fn test_shutdown_writable_after_read_close() { unsafe { // Close the read end. libc::shutdown(client_sockfd, libc::SHUT_RD); - // Ensure that we're still writable. errno_result(libc::write(client_sockfd, [1u8].as_ptr().cast(), 1)).unwrap(); }