diff --git a/nvx-rs/src/host/cli/download/archive.rs b/nvx-rs/src/host/cli/download/archive.rs index 97a770409..6d5c01b76 100644 --- a/nvx-rs/src/host/cli/download/archive.rs +++ b/nvx-rs/src/host/cli/download/archive.rs @@ -19,7 +19,6 @@ use super::system_program; use crate::ErrorCode; use crate::host::artifacts::CHECKSUMS_NAME; use crate::host::cli::{Exit, Result, check_interrupt, io_error}; -use crate::host::supervisor::spawn_command; /// The directory beside the package into which `tar` extracts it. const EXTRACTION_DIR: &str = "package"; @@ -83,7 +82,8 @@ fn tar(directory: &Path, arguments: &[&str]) -> Result { .stdin(Stdio::null()) .stdout(Stdio::piped()) .stderr(Stdio::piped()); - let output = spawn_command(&mut command) + let output = command + .spawn() .and_then(Child::wait_with_output) .map_err(|error| { Exit::error( diff --git a/nvx-rs/src/host/cli/download/archive/tests.rs b/nvx-rs/src/host/cli/download/archive/tests.rs index 44a403d3f..838b98494 100644 --- a/nvx-rs/src/host/cli/download/archive/tests.rs +++ b/nvx-rs/src/host/cli/download/archive/tests.rs @@ -1,7 +1,13 @@ //! The tests of packages: the names of their members, their extraction with the host's `tar`, //! and their checksums. +//! +//! Each test that runs `tar` holds the tests' spawn lock while it does: the CLI downloads before it +//! launches a VM, but these tests run beside tests that launch stand-in VMs, and on Windows a `tar` +//! that starts during a VM's start inherits that VM's handles, such as its log, which it holds +//! until it exits. use super::*; +use crate::host::tests::hold_spawns; /// The root directory of the packages of the tests. const ROOT: &str = "nvx-0.1.0-linux-kvm"; @@ -43,10 +49,11 @@ fn archive(source: &Path, archive: &Path) { } else { command.arg("-czf"); } - let status = spawn_command(command.arg(archive).arg(ROOT).current_dir(source)) - .unwrap() - .wait() - .unwrap(); + let status = + crate::host::tests::spawn_locked(command.arg(archive).arg(ROOT).current_dir(source)) + .unwrap() + .wait() + .unwrap(); assert!(status.success()); } @@ -65,6 +72,7 @@ fn staged(directory: &Path, name: &str, change: impl FnOnce(&Path)) -> PathBuf { #[test] fn gzipped_packages_extract_once_their_names_and_checksums_pass() { + let _spawns = hold_spawns(); let directory = tempfile::tempdir().unwrap(); let archive = staged(directory.path(), "nvx-0.1.0-linux-kvm.tar.gz", |_| {}); let package = extract(&archive).unwrap(); @@ -80,6 +88,7 @@ fn gzipped_packages_extract_once_their_names_and_checksums_pass() { #[cfg(windows)] #[test] fn zip_packages_extract_with_the_tar_of_windows() { + let _spawns = hold_spawns(); let directory = tempfile::tempdir().unwrap(); let archive = staged(directory.path(), "nvx-0.1.0-windows-whp.zip", |_| {}); let package = extract(&archive).unwrap(); @@ -91,6 +100,7 @@ fn zip_packages_extract_with_the_tar_of_windows() { #[test] fn packages_whose_checksums_fail_are_refused() { + let _spawns = hold_spawns(); for (change, expected) in [ ( (|root: &Path| fs::write(root.join("guest").join("vmlinux"), "other\n").unwrap()) @@ -120,6 +130,7 @@ fn packages_whose_checksums_fail_are_refused() { #[cfg(unix)] #[test] fn packages_with_symlinks_are_refused() { + let _spawns = hold_spawns(); let directory = tempfile::tempdir().unwrap(); let archive = staged(directory.path(), "nvx-0.1.0-linux-kvm.tar.gz", |root| { std::os::unix::fs::symlink("/etc/passwd", root.join("guest").join("link")).unwrap(); @@ -133,6 +144,7 @@ fn packages_with_symlinks_are_refused() { #[test] fn archives_that_tar_cannot_read_are_refused() { + let _spawns = hold_spawns(); let directory = tempfile::tempdir().unwrap(); let archive = directory.path().join("nvx-0.1.0-linux-kvm.tar.gz"); fs::write(&archive, "not an archive").unwrap(); diff --git a/nvx-rs/src/host/cli/download/git.rs b/nvx-rs/src/host/cli/download/git.rs index bfaff1dd6..b401edd43 100644 --- a/nvx-rs/src/host/cli/download/git.rs +++ b/nvx-rs/src/host/cli/download/git.rs @@ -10,7 +10,6 @@ use std::process::{Command, Output, Stdio}; use super::History; use crate::ErrorCode; use crate::host::cli::{Exit, Result}; -use crate::host::supervisor::spawn_command; /// The `git` of a checkout. pub(super) struct Git { @@ -43,7 +42,7 @@ impl Git { { command.env("GIT_CEILING_DIRECTORIES", ceiling); } - spawn_command(&mut command)?.wait_with_output() + command.spawn()?.wait_with_output() } } diff --git a/nvx-rs/src/host/cli/download/github.rs b/nvx-rs/src/host/cli/download/github.rs index eaa0f27d4..2b713e983 100644 --- a/nvx-rs/src/host/cli/download/github.rs +++ b/nvx-rs/src/host/cli/download/github.rs @@ -18,7 +18,6 @@ use serde_json::Value; use super::{Asset, Releases, remove_stale, system_program}; use crate::ErrorCode; use crate::host::cli::{Exit, Result, check_interrupt, io_error}; -use crate::host::supervisor::spawn_command; /// The base URL of the GitHub REST API. const API: &str = "https://api.github.com"; @@ -323,7 +322,7 @@ impl Curl { } else { Stdio::piped() }); - let mut child = spawn_command(&mut command).map_err(|error| { + let mut child = command.spawn().map_err(|error| { Exit::error( ErrorCode::Unavailable, format!( diff --git a/nvx-rs/src/host/cli/download/github/tests.rs b/nvx-rs/src/host/cli/download/github/tests.rs index 955101aee..b17d31c56 100644 --- a/nvx-rs/src/host/cli/download/github/tests.rs +++ b/nvx-rs/src/host/cli/download/github/tests.rs @@ -1,5 +1,10 @@ //! The tests of the client of the GitHub REST API: its messages, its configuration of `curl`, //! and its requests, which a real `curl` sends to servers on the loopback interface. +//! +//! Each test that runs `curl` holds the tests' spawn lock while it does: the CLI downloads before +//! it launches a VM, but these tests run beside tests that launch stand-in VMs, and on Windows a +//! `curl` that starts during a VM's start inherits that VM's handles, such as its log, which it +//! holds until it exits. use std::io::{BufRead, BufReader}; use std::net::{TcpListener, TcpStream}; @@ -9,6 +14,7 @@ use std::thread; use serde_json::json; use super::*; +use crate::host::tests::hold_spawns; impl Curl { /// Returns a `curl` that also speaks plain HTTP, to the servers of tests. @@ -134,6 +140,7 @@ fn answer( #[test] fn queries_and_downloads_carry_the_token_to_the_origin_of_the_api_alone() { + let _spawns = hold_spawns(); let (api_listener, api) = Server::bind(); let (asset_listener, assets) = Server::bind(); let asset_url = format!("{api}/repos/octo/nvx/releases/assets/7"); @@ -253,6 +260,7 @@ fn queries_and_downloads_carry_the_token_to_the_origin_of_the_api_alone() { #[test] fn requests_without_a_token_carry_none() { + let _spawns = hold_spawns(); let (listener, api) = Server::bind(); let server = Server::serve(listener, Vec::new()); let scratch = tempfile::tempdir().unwrap(); @@ -275,6 +283,7 @@ fn requests_without_a_token_carry_none() { #[test] fn redirects_resolve_relative_locations_against_the_urls_of_their_requests() { + let _spawns = hold_spawns(); let (listener, api) = Server::bind(); let server = Server::serve( listener, @@ -329,6 +338,7 @@ fn redirects_resolve_relative_locations_against_the_urls_of_their_requests() { #[test] fn the_last_response_to_a_retried_request_counts() { + let _spawns = hold_spawns(); let (listener, api) = Server::bind(); let path = "/repos/octo/nvx/releases/tags/v0.1.0-dev.a1b2c3d4e5f6"; // The status, the header, and the message of the first response, which `curl` retries, would @@ -399,6 +409,32 @@ fn requests_to_github_speak_https_alone() { ); } +#[test] +fn a_missing_curl_reports_the_program_and_query_context() { + let scratch = tempfile::tempdir().unwrap(); + let program = scratch.path().join("not-installed-curl"); + let github = GitHub::at( + API, + "octo/nvx", + None, + scratch.path(), + Curl { + program: program.clone(), + ..Curl::new() + }, + ) + .unwrap(); + let error = Exit::from(github.release("v1").unwrap_err()); + assert_eq!(error.code(), Some(ErrorCode::Unavailable)); + assert!( + error.message().unwrap().starts_with(&format!( + "GitHub release query needs curl, which did not run: {}: ", + program.display() + )), + "{error:?}" + ); +} + #[test] fn configurations_quote_their_values_and_carry_the_token_when_asked() { let curl = Curl { diff --git a/nvx-rs/src/host/cli/run.rs b/nvx-rs/src/host/cli/run.rs index c426dcc54..f6d47e6e1 100644 --- a/nvx-rs/src/host/cli/run.rs +++ b/nvx-rs/src/host/cli/run.rs @@ -1190,12 +1190,12 @@ fn foreground( if managed && output.console.is_some() { let endpoint = launched.outputs.endpoint.clone(); let capability = *launched.capability.as_bytes(); - let child = launched.child_mut().map_err(Exit::from_os_error)?; + let process = &launched.process; match attach_with_output( |cancellation| { Session::connect_launched_cancellable( &endpoint, - child, + process, &capability, 60.0, cancellation, diff --git a/nvx-rs/src/host/cli/session.rs b/nvx-rs/src/host/cli/session.rs index 991f91e14..31cd61672 100644 --- a/nvx-rs/src/host/cli/session.rs +++ b/nvx-rs/src/host/cli/session.rs @@ -37,10 +37,10 @@ use std::time::Duration; use super::deadline::{Deadline, RETRY_INTERVAL, sleep}; use super::{Exit, Result, check_interrupt}; use crate::host::platform::{self, Transport}; +use crate::host::process::Process; use crate::host::runtime::Runtime; use crate::host::sandbox::options::validate_exec_environment; use crate::host::session::{ControlSession, SessionError}; -use crate::host::supervisor::Launched; use crate::host::transport; use crate::host::vm::{self, SavedVm}; use crate::protocol::{ @@ -335,7 +335,7 @@ impl Session { }) } - /// Connects to `endpoint`, which `launched`, the OpenVMM process that a cold run launched, + /// Connects to `endpoint`, which `process`, the OpenVMM process that a cold run launched, /// serves, as [`Self::connect_unless_exited`] does, but stops waiting for the endpoint once /// OpenVMM has exited, as `nvx::host::session::connect` does: an OpenVMM that exits before it /// serves its endpoint, as one that refuses its configuration does, then ends the wait at @@ -344,22 +344,18 @@ impl Session { /// `cancellation`. pub(super) fn connect_launched_cancellable( endpoint: &str, - launched: &mut Launched, + process: &Process, capability: &[u8; CAPABILITY_LEN], timeout: f64, cancellation: AttachmentCancellation, ) -> Result { - let pid = launched.id(); + let pid = process.reference().pid; Self::connect_unless_exited( endpoint, pid, capability, timeout, - || { - launched - .wait_timeout(Duration::ZERO) - .is_ok_and(|status| status.is_some()) - }, + || process.has_exited().is_ok_and(|exited| exited), Some(cancellation), ) } @@ -686,7 +682,7 @@ impl Session { match kind { APP_STDOUT | APP_STDERR => { // The guest agent forwards at most this much output, so more breaks the - // protocol, as `nvx::host::instance` also finds. + // protocol. if stdout.len() + stderr.len() + response.len() > MAX_OUTPUT_BYTES { return Err(violation(&format!( "managed guest exceeded the {MAX_OUTPUT_BYTES}-byte output limit" diff --git a/nvx-rs/src/host/cli/session/tests.rs b/nvx-rs/src/host/cli/session/tests.rs index 50f017f3d..34882c923 100644 --- a/nvx-rs/src/host/cli/session/tests.rs +++ b/nvx-rs/src/host/cli/session/tests.rs @@ -827,6 +827,89 @@ fn check_waits_for_openvmm_that_exits(endpoint: &str) { assert_ne!(checks, 0); } +/// A process that runs for a minute unless it is killed, which dropping it kills and reaps. +#[cfg(any(target_os = "linux", windows))] +struct Sleeper(std::process::Child); + +#[cfg(any(target_os = "linux", windows))] +impl Sleeper { + /// Spawns the sleeper under a `SpawnLock`, so that on Windows it holds no handle of a VMM that + /// another test spawns meanwhile. + fn spawn() -> Self { + let mut command = if cfg!(windows) { + let mut command = std::process::Command::new("ping"); + command.args(["-n", "60", "127.0.0.1"]); + command + } else { + let mut command = std::process::Command::new("sleep"); + command.arg("60"); + command + }; + command + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()); + Self(crate::host::tests::spawn_locked(&mut command).unwrap()) + } + + /// Returns the verified process of the sleeper. + fn process(&self) -> Process { + use crate::host::process::{BootId, Opened, ProcessRef}; + + let reference = ProcessRef { + pid: self.0.id(), + start_time: platform::process_start_time(self.0.id()).unwrap().unwrap(), + boot: BootId::current().unwrap(), + }; + let Opened::Running(process) = Process::open(&reference).unwrap() else { + panic!("the sleeper runs"); + }; + process + } +} + +#[cfg(any(target_os = "linux", windows))] +impl Drop for Sleeper { + fn drop(&mut self) { + let _ = self.0.kill(); + let _ = self.0.wait(); + } +} + +/// Checks the wait for `endpoint`, which never appears, of a cold run whose OpenVMM is the +/// verified process that [`Session::connect_launched_cancellable`] receives: the wait goes on +/// while the process runs, and ends at once, as a closed endpoint, once it exits, rather than at +/// the timeout. +#[cfg(any(target_os = "linux", windows))] +fn check_launched_waits_end_once_their_process_exits(endpoint: &str) { + let mut sleeper = Sleeper::spawn(); + let process = sleeper.process(); + let started = Instant::now(); + let failure = thread::scope(|scope| { + let waiting = scope.spawn(|| { + Session::connect_launched_cancellable( + endpoint, + &process, + &CAPABILITY, + 60.0, + AttachmentCancellation::default(), + ) + .err() + }); + thread::sleep(Duration::from_millis(500)); + let ended_early = waiting.is_finished(); + sleeper.0.kill().unwrap(); + sleeper.0.wait().unwrap(); + assert!(!ended_early, "the wait ended while OpenVMM ran"); + waiting.join().unwrap().unwrap() + }); + assert!( + started.elapsed() < Duration::from_secs(30), + "{:?}", + started.elapsed() + ); + assert!(matches!(failure, Exit::EndpointClosed), "{failure:?}"); +} + #[cfg(any(target_os = "linux", windows))] fn check_forward_failure_during_connect(endpoint: &str) { let (attempted, attempt) = mpsc::sync_channel(1); @@ -1211,6 +1294,14 @@ mod endpoints { check_waits_for_openvmm_that_exits(path.to_str().unwrap()); } + #[test] + #[cfg(target_os = "linux")] + fn launched_waits_end_once_their_process_exits() { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("control.sock"); + check_launched_waits_end_once_their_process_exits(path.to_str().unwrap()); + } + #[test] #[cfg(target_os = "linux")] fn forward_failure_cancels_the_connection_before_its_timeout() { @@ -1446,6 +1537,13 @@ mod endpoints { check_waits_for_openvmm_that_exits(r"\\.\pipe\nvx-cli-session-test-exited"); } + #[test] + fn launched_waits_end_once_their_process_exits() { + check_launched_waits_end_once_their_process_exits( + r"\\.\pipe\nvx-cli-session-test-launched-exited", + ); + } + #[test] fn forward_failure_cancels_the_connection_before_its_timeout() { check_forward_failure_during_connect(r"\\.\pipe\nvx-cli-session-test-cancelled"); diff --git a/nvx-rs/src/host/tests.rs b/nvx-rs/src/host/tests.rs index c35858129..c183ef74d 100644 --- a/nvx-rs/src/host/tests.rs +++ b/nvx-rs/src/host/tests.rs @@ -32,7 +32,7 @@ pub(crate) fn released(mut free: impl FnMut() -> bool) -> bool { /// Spawns `command` under a `SpawnLock`, which it releases once the process exists, so that on /// Windows the process inherits no handle of a VMM that a concurrent test spawns, such as the /// VMM's log, which it would hold until it exits. -#[cfg(any(target_os = "linux", windows))] +#[cfg(any(target_os = "linux", windows, feature = "cli"))] pub(crate) fn spawn_locked( command: &mut std::process::Command, ) -> std::io::Result { @@ -40,6 +40,16 @@ pub(crate) fn spawn_locked( command.spawn() } +/// Returns a `SpawnLock`, which a test holds while the code that it tests spawns processes with +/// `std::process`, as the download spawns `tar` and `curl`, so that on Windows none of them +/// inherits a handle of a VMM that a concurrent test spawns, such as the VMM's log, which it would +/// hold until it exits. A concurrent spawn of a VMM waits for the lock, so a test holds it only +/// while it runs those processes. +#[cfg(feature = "cli")] +pub(crate) fn hold_spawns() -> super::platform::inherit::SpawnLock { + super::platform::inherit::lock_spawns() +} + /// Makes `link` a junction to the directory `target`, which any user may create, with `cmd`'s /// `mklink /J`, quoting both paths, since `cmd` splits its arguments at commas and semicolons. /// diff --git a/nvx-rs/src/host/vm.rs b/nvx-rs/src/host/vm.rs index 27c66511f..013d250b4 100644 --- a/nvx-rs/src/host/vm.rs +++ b/nvx-rs/src/host/vm.rs @@ -863,7 +863,7 @@ pub struct Launched { pub capability: Capability, /// The preparation's same restore-ready listener, absent for a cold launch. pub ready: Option, - /// Retains only a foreground caller's child for status and control attachment. + /// Retains only a foreground caller's child, for its status. #[cfg(feature = "cli")] child: ForegroundChild, } @@ -892,8 +892,8 @@ impl Launched { .map(|status| status as i32)) } - /// Gives the CLI connector the same owned child whose exit it observes. - pub(crate) fn child_mut(&mut self) -> io::Result<&mut spawn::Launched> { + /// Returns the owned child, whose status [`Self::wait_status`] polls. + fn child_mut(&mut self) -> io::Result<&mut spawn::Launched> { self.child .0 .as_mut()