From d36c57a0e54127dc80f749b47a67a2f5c724064d Mon Sep 17 00:00:00 2001 From: Pedro Henrique Penna Date: Wed, 11 Nov 2026 21:40:17 -0800 Subject: [PATCH] cli: keep a cold run's console when OpenVMM exits before the peer check A cold `nvx-rs run` on Linux could exit with OpenVMM's status, 0, and print none of OpenVMM's console output, with no error. The CLI test `one_shot_runs_exit_with_the_workload_or_openvmm_status` failed that way in CI run 43156460151 (job 130977435086, on #1501), and once in a local run of SL6.2b's on nvx-mshv-test. The run connects to the boot console on which OpenVMM listens, then checks the socket's original peer: its `SO_PEERCRED` PID must be OpenVMM's, and its `SO_PEERPIDFD` pidfd must not report an exit. The fake OpenVMM accepts the connection, writes its line and exits at once. When it exits between the run's `connect` and that check, the check refuses the peer as exited, and `connect_before_exit` yields to OpenVMM's exit status, dropping the connection with the output already queued in it. A real OpenVMM, too, writes its output and exits without waiting for the run to read it, so a run that stalls at that point on a loaded host loses its output silently. The console now connects with `connect_child_endpoint`, which checks the peer as `connect_endpoint` does, but accepts an original peer that has exited and has not been reaped, and still refuses a reaped one. That is safe because the run's own unreaped child keeps its PID, which no other process can reuse until the run reaps it, so a peer that holds the child's PID is the child. `PidFd::holds_its_id` tells an unreaped process from a reaped one with signal 0 through the pidfd, which fails with `ESRCH` only once the process is reaped. Every other connection, the control sessions and the restore-ready endpoint, still refuses an exited peer. Windows is unchanged: its `connect_endpoint` checks only the pipe's server process ID. Tests: `a_child_endpoint_accepts_its_exited_child_until_the_child_is_reaped` connects to a listener whose creator, the test's child, has exited and is not reaped: `connect_endpoint` refuses it, `connect_child_endpoint` accepts it and reads through it, and both refuse it once the test has reaped the creator. It shares the Python launcher of `a_transferred_listener_cannot_authenticate_its_exited_creator`, which now runs through the same helper and still reaps the creator first. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- nvx-rs/src/host/cli/run.rs | 11 ++-- nvx-rs/src/host/platform/linux/endpoint.rs | 35 +++++++++- nvx-rs/src/host/platform/linux/mod.rs | 2 +- nvx-rs/src/host/platform/linux/tests.rs | 75 ++++++++++++++++++++-- nvx-rs/src/host/sys/process.rs | 31 ++++++++- 5 files changed, 137 insertions(+), 17 deletions(-) diff --git a/nvx-rs/src/host/cli/run.rs b/nvx-rs/src/host/cli/run.rs index 2d82f1252..98015c134 100644 --- a/nvx-rs/src/host/cli/run.rs +++ b/nvx-rs/src/host/cli/run.rs @@ -697,11 +697,12 @@ fn connect_running( endpoint: &str, ) -> Result>> { let pid = launched.process.reference().pid; - connect_before_exit( - endpoint, - || platform::connect_endpoint(endpoint, pid, Duration::from_millis(250)), - |timeout| launched.wait_status(timeout), - ) + // On Linux, OpenVMM may write its console output and exit before the console's peer check. + #[cfg(target_os = "linux")] + let connect = || platform::connect_child_endpoint(endpoint, pid); + #[cfg(not(target_os = "linux"))] + let connect = || platform::connect_endpoint(endpoint, pid, Duration::from_millis(250)); + connect_before_exit(endpoint, connect, |timeout| launched.wait_status(timeout)) } /// Connects to the verified endpoint, or yields to the owned child's confirmed exit. diff --git a/nvx-rs/src/host/platform/linux/endpoint.rs b/nvx-rs/src/host/platform/linux/endpoint.rs index 427dc7139..0d8e8b386 100644 --- a/nvx-rs/src/host/platform/linux/endpoint.rs +++ b/nvx-rs/src/host/platform/linux/endpoint.rs @@ -139,7 +139,7 @@ impl ReadyListener { )); } if verify_original_peer { - check_original_peer(&connection)?; + check_original_peer(&connection, false)?; } connection.set_nonblocking(true)?; self.connection = Some(connection); @@ -178,7 +178,10 @@ impl ReadyListener { } /// The kernel returns a descriptor of the socket's original peer, not a new lookup of its PID. -fn check_original_peer(connection: &UnixStream) -> io::Result<()> { +/// +/// A peer that has exited is refused, unless `accept_unreaped` accepts one that has not been +/// reaped, which still holds its ID, as `connect_child_endpoint` requires. +fn check_original_peer(connection: &UnixStream, accept_unreaped: bool) -> io::Result<()> { let peer = match getsockopt(connection, PeerPidfd) { Err( sys::Errno::ENOPROTOOPT @@ -207,6 +210,9 @@ fn check_original_peer(connection: &UnixStream) -> io::Result<()> { } let events = descriptors[0].revents().unwrap_or(PollFlags::empty()); if events.intersects(PollFlags::POLLIN | PollFlags::POLLHUP) { + if accept_unreaped && sys::PidFd::from(peer).holds_its_id()? { + return Ok(()); + } return Err(io::Error::new( io::ErrorKind::PermissionDenied, "the original socket peer exited", @@ -261,6 +267,29 @@ pub(crate) fn connect_endpoint( endpoint: &str, expected_pid: u32, _timeout: Duration, +) -> io::Result> { + connect_peer(endpoint, expected_pid, false) +} + +/// Connects to the endpoint that the process `child` serves, as [`connect_endpoint`] does, for a +/// caller that launched `child` and has not reaped it: a cold run, whose OpenVMM may write its +/// boot console's output and exit before the run checks the console's peer. +/// +/// It accepts an original socket peer that has exited but has not been reaped, and still refuses +/// a reaped one. That is safe because the caller's own unreaped child keeps its PID, which no +/// other process can reuse until the caller reaps it, so a peer that holds the child's PID is the +/// child. Every other connection, as [`connect_endpoint`]'s, still refuses an exited peer. +#[cfg(feature = "cli")] +pub(crate) fn connect_child_endpoint(endpoint: &str, child: u32) -> io::Result> { + connect_peer(endpoint, child, true) +} + +/// Connects to the endpoint that the process `expected_pid` serves as the socket's original peer, +/// which [`check_original_peer`] checks, with `accept_unreaped`. +fn connect_peer( + endpoint: &str, + expected_pid: u32, + accept_unreaped: bool, ) -> io::Result> { let stream = sys::connect_nonblocking(endpoint)?; let peer = sys::peer_pid(&stream)?; @@ -272,7 +301,7 @@ pub(crate) fn connect_endpoint( ), )); } - check_original_peer(&stream)?; + check_original_peer(&stream, accept_unreaped)?; transport_from_handle(stream.into()) } diff --git a/nvx-rs/src/host/platform/linux/mod.rs b/nvx-rs/src/host/platform/linux/mod.rs index 8dffecfa0..ef8789520 100644 --- a/nvx-rs/src/host/platform/linux/mod.rs +++ b/nvx-rs/src/host/platform/linux/mod.rs @@ -19,7 +19,7 @@ pub(crate) use self::endpoint::{ }; #[cfg(feature = "cli")] pub(crate) use self::endpoint::{ - endpoint_server_pid_until, interrupt_transfers, transport_from_handle, + connect_child_endpoint, endpoint_server_pid_until, interrupt_transfers, transport_from_handle, }; pub use self::identity::file_identity; pub(crate) use self::identity::{ diff --git a/nvx-rs/src/host/platform/linux/tests.rs b/nvx-rs/src/host/platform/linux/tests.rs index 04ab1e6b6..c271d6ebd 100644 --- a/nvx-rs/src/host/platform/linux/tests.rs +++ b/nvx-rs/src/host/platform/linux/tests.rs @@ -138,7 +138,72 @@ fn a_transferred_listener_cannot_authenticate_its_exited_creator() { println!("{verified} pid={pid}"); return; } + run_with_transferred_listener( + "host::platform::linux::tests::a_transferred_listener_cannot_authenticate_its_exited_creator", + true, + verified, + ); +} + +#[cfg(feature = "cli")] +#[test] +fn a_child_endpoint_accepts_its_exited_child_until_the_child_is_reaped() { + use std::io::Write; + use std::os::fd::AsFd; + use std::os::unix::net::UnixListener; + + use nix::sys::wait::{Id, WaitPidFlag, waitid, waitpid}; + use nix::unistd::Pid; + + let verified = "NVX_UNREAPED_CHILD_PEER_ACCEPTED"; + if let Some(pid) = std::env::var_os("NVX_TEST_TRANSFERRED_CONTROL_LISTENER") { + let pid: u32 = pid.to_str().unwrap().parse().unwrap(); + let endpoint = std::env::var("NVX_TEST_TRANSFERRED_CONTROL_ENDPOINT").unwrap(); + let listener = UnixListener::from(std::io::stdin().as_fd().try_clone_to_owned().unwrap()); + // The listener's creator is this process's child: it exits, and only this process reaps it. + let creator = Pid::from_raw(i32::try_from(pid).unwrap()); + waitid( + Id::Pid(creator), + WaitPidFlag::WEXITED | WaitPidFlag::WNOWAIT, + ) + .unwrap(); + let error = connect_endpoint(&endpoint, pid, Duration::from_secs(2)) + .err() + .expect("other connections must still refuse an exited peer"); + assert_eq!(error.kind(), std::io::ErrorKind::PermissionDenied); + assert!(error.to_string().contains("original socket peer exited")); + drop(listener.accept().unwrap()); + let mut transport = connect_child_endpoint(&endpoint, pid).unwrap(); + let (mut peer, _) = listener.accept().unwrap(); + peer.write_all(b"output").unwrap(); + let mut output = [0; 6]; + let count = transport + .read(&mut output, Some(Duration::from_secs(2))) + .unwrap(); + assert_eq!(&output[..count], b"output"); + waitpid(creator, None).unwrap(); + let error = connect_child_endpoint(&endpoint, pid) + .err() + .expect("a reaped peer must not authenticate"); + assert_eq!(error.kind(), std::io::ErrorKind::PermissionDenied); + assert!(error.to_string().contains("original socket peer exited")); + println!("{verified} pid={pid}"); + return; + } + run_with_transferred_listener( + "host::platform::linux::tests::a_child_endpoint_accepts_its_exited_child_until_the_child_is_reaped", + false, + verified, + ); +} +/// Runs this binary's test `test` in a Python process that a child of its own, the creator, gave +/// a listener at a new socket before it exited, and checks that the test passed and printed +/// `verified`. The test finds the listener on its standard input, the creator's PID in +/// `NVX_TEST_TRANSFERRED_CONTROL_LISTENER`, and the socket's path in +/// `NVX_TEST_TRANSFERRED_CONTROL_ENDPOINT`. The process reaps the creator first if `reap`, and +/// leaves it to the test otherwise. +fn run_with_transferred_listener(test: &str, reap: bool, verified: &str) { let directory = tempfile::tempdir().unwrap(); let path = directory.path().join("control.sock"); let mut command = Command::new("python3"); @@ -163,7 +228,8 @@ assert messages[0][:2] == (socket.SOL_SOCKET, socket.SCM_RIGHTS) descriptors = array.array("i") descriptors.frombytes(messages[0][2]) assert len(descriptors) == 1 -assert os.waitpid(creator, 0) == (creator, 0) +if sys.argv[4] == "reap": + assert os.waitpid(creator, 0) == (creator, 0) os.dup2(descriptors[0], 0) if descriptors[0] != 0: os.close(descriptors[0]) @@ -174,10 +240,9 @@ os.execv(sys.argv[2], [sys.argv[2], "--exact", sys.argv[3], "--nocapture", "--te ]) .arg(&path) .arg(std::env::current_exe().unwrap()) - .arg( - "host::platform::linux::tests::a_transferred_listener_cannot_authenticate_its_exited_creator", - ) - .env(endpoint, &path) + .arg(test) + .arg(if reap { "reap" } else { "keep" }) + .env("NVX_TEST_TRANSFERRED_CONTROL_ENDPOINT", &path) .stdin(Stdio::null()) .stdout(Stdio::piped()) .stderr(Stdio::piped()); diff --git a/nvx-rs/src/host/sys/process.rs b/nvx-rs/src/host/sys/process.rs index 15b3f058e..05cf15e2f 100644 --- a/nvx-rs/src/host/sys/process.rs +++ b/nvx-rs/src/host/sys/process.rs @@ -102,17 +102,42 @@ impl PidFd { /// /// Fails with `ESRCH` if the process has exited. pub(crate) fn kill(&self) -> io::Result<()> { + self.send_signal(Signal::SIGKILL as libc::c_int)?; + Ok(()) + } + + /// Returns whether the process still holds its ID: it runs, or it has exited and has not + /// been reaped, as signal 0 shows without signaling it. No other process can take the ID + /// before the process is reaped. + pub(crate) fn holds_its_id(&self) -> io::Result { + match self.send_signal(0) { + // Another user's process refuses the signal, but it holds its ID. + Ok(()) | Err(Errno::EPERM) => Ok(true), + Err(Errno::ESRCH) => Ok(false), + Err(error) => Err(error.into()), + } + } + + /// Sends `signal` to the process, or with 0, only checks that it could. + fn send_signal(&self, signal: libc::c_int) -> Result<(), Errno> { // SAFETY: the descriptor is a valid pidfd, and a null siginfo with no flags is allowed. Errno::result(unsafe { libc::syscall( libc::SYS_pidfd_send_signal, self.0.as_raw_fd(), - Signal::SIGKILL as libc::c_int, + signal, ptr::null::(), 0, ) - })?; - Ok(()) + }) + .map(drop) + } +} + +impl From for PidFd { + /// Takes `descriptor`, which must be a pidfd, such as `SO_PEERPIDFD` returns. + fn from(descriptor: OwnedFd) -> Self { + Self(descriptor) } }