diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 72b614f..fef3aab 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -13,6 +13,16 @@ updates: - rust commit-message: prefix: deps + # `just msrv` and the Rust 1.75 test legs check the whole workspace, so an + # xtask dependency that raises its own MSRV above ours breaks CI even though + # it never ships. sha2 0.11 requires Rust 1.85; the checksum code already + # handles both generations, so this only needs lifting when the crate MSRV + # moves past 1.85. + ignore: + - dependency-name: sha2 + update-types: + - version-update:semver-minor + - version-update:semver-major groups: cargo-minor-and-patch: applies-to: version-updates diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f22472c..f931315 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -65,6 +65,22 @@ jobs: tool: just@1.57.0 - run: just test + i686-smoke: + name: Windows i686 execution smoke + runs-on: windows-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: dtolnay/rust-toolchain@e97e2d8cc328f1b50210efc529dca0028893a2d9 # v1 + with: + toolchain: stable + targets: i686-pc-windows-msvc + - uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 + - name: Run the ConPTY stdio isolation regression as a 32-bit process + run: >- + cargo test --target i686-pc-windows-msvc --test windows_spawn + pseudoconsole_regular_stdio_stays_off_parent_pipes -- --exact + linux: name: Linux empty API runs-on: ubuntu-latest @@ -107,7 +123,7 @@ jobs: ci-required: name: CI required if: always() - needs: [hygiene, test, linux, coverage, package] + needs: [hygiene, test, i686-smoke, linux, coverage, package] runs-on: ubuntu-latest steps: - if: contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') || contains(needs.*.result, 'skipped') diff --git a/CHANGELOG.md b/CHANGELOG.md index 70070a7..7e03ff5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,19 +5,35 @@ Versioning with Cargo's pre-1.0 compatibility rules. ## [Unreleased] +### Added + +- A compiled, executed `Command` documentation example. The crate previously had + only `compile_fail` boundary pins, so no usage example was ever type-checked. +- `clippy::undocumented_unsafe_blocks` is denied, making CONTRIBUTING's + "every `unsafe` block carries a specific safety justification" rule + machine-checked instead of a convention. +- A `_typos.toml` so the spell-check gate has a checked-in configuration + matching the sibling repositories. + ### Changed - Strengthened process creation with private running/suspended typestates, unified handle-transfer ownership, and value-based pseudoconsole storage. -- Aligned ConPTY startup with the Windows reference sequence by leaving - ordinary standard handles unused and omitting `STARTF_USESTDHANDLES`. +- Aligned ConPTY startup with Microsoft Terminal by setting + `STARTF_USESTDHANDLES` while keeping all ordinary standard handles null. - Require `SuspendedChild::resume` to observe the expected suspend count of exactly one; externally changed counts now fail and roll back the process. ### Fixed +- Prevented ConPTY children from reading or writing the parent's redirected + standard streams instead of the pseudoconsole channels. - Always join both output reader threads when output capture encounters a reader error or panic. +- The release-artifact checksum helper no longer relies on `LowerHex` being + implemented for the digest output, so it builds against both `sha2` 0.10 and + 0.11. The bump itself stays deferred because `sha2` 0.11 requires Rust 1.85, + above this crate's 1.75 minimum. ## [0.1.0] - 2026-08-02 diff --git a/Cargo.toml b/Cargo.toml index a4e3dc0..d2a1639 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -66,5 +66,15 @@ unused_qualifications = "deny" all = { level = "deny", priority = -1 } pedantic = { level = "deny", priority = -1 } +# CONTRIBUTING requires a specific safety justification on every `unsafe` block. +# This restriction lint is what makes that rule machine-checked rather than a +# convention, so a missing justification fails the build. +# +# `multiple_unsafe_ops_per_block` is deliberately not enabled: several blocks +# here acquire two OS resources that must be adopted together to stay +# exception-safe (see `pipe`), and splitting them would widen the window in +# which one handle can leak. +undocumented_unsafe_blocks = "deny" + [lints] workspace = true diff --git a/README.md b/README.md index ea8f684..9e98731 100644 --- a/README.md +++ b/README.md @@ -15,10 +15,12 @@ Non-Windows targets expose no public API. ## Installation -```toml -[dependencies] -windows-spawn = "0.1" -``` +`windows-spawn` 0.1.0 is not published yet, so registry installation is not +available. This repository is self-contained: it neither requires nor checks +out a downstream terminal crate. A downstream that validates the unpublished +version may temporarily supply its own local Cargo path override; that +bootstrap belongs to the downstream repository and is removed after 0.1.0 is +published. ## Minimal example diff --git a/REUSE.toml b/REUSE.toml index 87dd133..45f3482 100644 --- a/REUSE.toml +++ b/REUSE.toml @@ -6,6 +6,7 @@ path = [ ".cargo/**", ".gitattributes", ".gitignore", + "_typos.toml", "Cargo.toml", "CHANGELOG.md", "CONTRIBUTING.md", diff --git a/_typos.toml b/_typos.toml new file mode 100644 index 0000000..c513c77 --- /dev/null +++ b/_typos.toml @@ -0,0 +1,11 @@ +# SPDX-FileCopyrightText: 2026 Yasunobu Sakashita +# +# SPDX-License-Identifier: MIT OR Apache-2.0 + +[files] +ignore-hidden = false +extend-exclude = [ + ".git/", + "target/", + "mutants.out/", +] diff --git a/docs/adr/0006-conpty-boundary.md b/docs/adr/0006-conpty-boundary.md index 6bd8b94..377206f 100644 --- a/docs/adr/0006-conpty-boundary.md +++ b/docs/adr/0006-conpty-boundary.md @@ -14,19 +14,37 @@ Use the unsafe `AsPseudoConsole` trait. Implementors guarantee a stable, nonzero, live `HPCON` for the full borrow and retain ownership. The raw method is public so terminal libraries can implement the bridge. -`conpty-oxide` depends on windows-spawn and implements the trait. It owns -ConPTY creation, pipes, waits, Tokio integration, and lifecycle. windows-spawn -owns command lowering, attributes, Jobs, and `CreateProcessW`. +A terminal library may depend on windows-spawn and implement the trait. That +downstream owns ConPTY creation, pipes, waits, runtime integration, and +lifecycle. windows-spawn owns only command lowering, attributes, Jobs, and +`CreateProcessW`; it has no dependency on, checkout of, or CI pin to a +particular terminal library. The builder snapshots the `HPCON` numeric value and keeps only a lifetime marker, so the stored options do not require dynamic dispatch. During process -creation ConPTY has no ordinary standard-handle set: the zero-initialized -startup fields remain zero and `STARTF_USESTDHANDLES` is not set, matching the -Microsoft ConPTY creation sequence. +creation ConPTY uses an explicit startup-I/O mode: `STARTF_USESTDHANDLES` is +set while `hStdInput`, `hStdOutput`, and `hStdError` remain null. No standard +handle is added to `PROC_THREAD_ATTRIBUTE_HANDLE_LIST`, and a spawn with no +other handle transfer passes `bInheritHandles = FALSE`. + +This deliberately follows the production `ConptyConnection` implementation in +Microsoft Terminal. The shorter Microsoft Learn walkthrough zero-initializes +`STARTUPINFOEXW` and leaves `STARTF_USESTDHANDLES` clear. That sample explains +the pseudoconsole attribute but does not isolate a hosted process from the +calling process's normal standard-handle slots. In a parent with redirected +standard I/O, following it literally can let the child use the parent's pipes +instead of ConPTY. The explicit null slots make that ownership boundary +testable and deterministic. ## Consequences - Ordinary users pass a safe borrow and do not construct raw `HPCON` values. -- windows-spawn does not depend on a terminal library. +- Dependency and validation flow only from a terminal library to + windows-spawn; windows-spawn does not name a downstream integration target. - Pseudoconsole use conflicts with explicit standard streams and leaves the - ordinary startup handles unused. + ordinary startup handles explicitly null. + +## References + +- [Microsoft Terminal `ConptyConnection.cpp`](https://github.com/microsoft/terminal/blob/fbda436dc654cf551dd196b2667ef95d3e0a7262/src/cascadia/TerminalConnection/ConptyConnection.cpp) +- [Creating a Pseudoconsole session](https://learn.microsoft.com/en-us/windows/console/creating-a-pseudoconsole-session) diff --git a/docs/crate.md b/docs/crate.md index 6a1f834..74c6371 100644 --- a/docs/crate.md +++ b/docs/crate.md @@ -68,7 +68,10 @@ See [`SpawnOptions`] borrows one-spawn capabilities such as Jobs, an alternate parent, or a pseudoconsole. A borrowed `ConPTY` remains owned by the terminal library implementing [`AsPseudoConsole`]. That library defines when terminal -pipes close and when terminal EOF occurs. +pipes close and when terminal EOF occurs. Pseudoconsole process creation sets +`STARTF_USESTDHANDLES` with all three standard-handle slots null and does not +put standard handles in the inheritance list. This prevents a hosted child +from falling back to redirected standard handles owned by the parent. # Drop, wait, and EOF contract diff --git a/src/command.rs b/src/command.rs index eba50f5..9305893 100644 --- a/src/command.rs +++ b/src/command.rs @@ -38,6 +38,26 @@ pub(crate) enum EnvValue { /// privately duplicated when configured. Each spawn duplicates those handles /// again into the actual parent process and only then lowers their numeric /// values to decimal text. +/// +/// # Examples +/// +/// Run a command to completion and capture what it wrote, terminating any +/// descendants it leaves behind: +/// +/// ``` +/// use windows_spawn::{Command, DropPolicy, SpawnOptions}; +/// +/// // `.bat` and `.cmd` are rejected, so a shell boundary is always explicit. +/// let shell = std::env::var_os("COMSPEC").expect("COMSPEC is set on Windows"); +/// let mut command = Command::new(shell); +/// command.args(["/D", "/S", "/C"]).raw_arg("echo hello"); +/// +/// let output = command.output_with(SpawnOptions::new().drop_policy(DropPolicy::KillTree))?; +/// +/// assert!(output.status.success()); +/// assert!(String::from_utf8_lossy(&output.stdout).contains("hello")); +/// # Ok::<(), std::io::Error>(()) +/// ``` #[derive(Debug)] pub struct Command { pub(crate) program: OsString, diff --git a/src/plan.rs b/src/plan.rs index e8ceada..0adc913 100644 --- a/src/plan.rs +++ b/src/plan.rs @@ -49,11 +49,17 @@ pub(crate) struct StandardHandles { pub(crate) stderr: T, } +#[derive(Debug)] +pub(crate) enum StandardIo<'a> { + Ordinary(StandardHandles>), + PseudoConsole, +} + #[derive(Debug)] pub(crate) struct SpawnPlan<'command, 'options, M> { pub(crate) command: &'command Command, pub(crate) options: SpawnOptions<'options>, - pub(crate) stdio: Option>>, + pub(crate) stdio: StandardIo<'command>, state: PhantomData, } @@ -112,7 +118,7 @@ impl<'command, 'options, M> SpawnPlan<'command, 'options, M> { )); } let stdio = if options.pseudoconsole_raw().is_some() { - None + StandardIo::PseudoConsole } else { let handles = match io_mode { IoMode::Spawn => StandardHandles { @@ -126,7 +132,7 @@ impl<'command, 'options, M> SpawnPlan<'command, 'options, M> { stderr: configured_or(command.stderr.as_ref(), StdioSpec::Piped), }, }; - Some(handles) + StandardIo::Ordinary(handles) }; Ok(Self { @@ -398,7 +404,9 @@ mod tests { fn successful_plans_choose_the_expected_stdio_modes() { let command = Command::new("cmd.exe"); let output = SpawnPlan::new_running(&command, SpawnOptions::new(), IoMode::Output).unwrap(); - let output_stdio = output.stdio.unwrap(); + let StandardIo::Ordinary(output_stdio) = output.stdio else { + panic!("output capture must use ordinary standard I/O"); + }; assert!(matches!(output_stdio.stdin, StdioSpec::Null)); assert!(matches!(output_stdio.stdout, StdioSpec::Piped)); assert!(matches!(output_stdio.stderr, StdioSpec::Piped)); @@ -410,6 +418,6 @@ mod tests { IoMode::Spawn, ) .unwrap(); - assert!(pcon.stdio.is_none()); + assert!(matches!(pcon.stdio, StandardIo::PseudoConsole)); } } diff --git a/src/sys.rs b/src/sys.rs index 9038ac4..d71c303 100644 --- a/src/sys.rs +++ b/src/sys.rs @@ -85,10 +85,14 @@ pub(crate) fn duplicate_local( source: BorrowedHandle<'_>, inheritable: bool, ) -> io::Result { + // SAFETY: `GetCurrentProcess` takes no arguments, cannot fail, and returns + // the current-process pseudo-handle. The value is a constant that stays + // valid for the lifetime of the process and must never be closed. + let current = unsafe { GetCurrentProcess() }; duplicate_between( - unsafe { GetCurrentProcess() }, + current, raw(source), - unsafe { GetCurrentProcess() }, + current, inheritable, DUPLICATE_SAME_ACCESS, ) @@ -135,13 +139,17 @@ impl RemoteHandle<'_> { impl Drop for RemoteHandle<'_> { fn drop(&mut self) { + // SAFETY: `GetCurrentProcess` takes no arguments, cannot fail, and + // returns the current-process pseudo-handle. The value is a constant + // that stays valid for the lifetime of the process and is never closed. + let current = unsafe { GetCurrentProcess() }; // `duplicate_between` turns the temporary local copy into an // `OwnedHandle`; discarding the result closes it immediately. The // close-source option atomically removes the remote value. let _ = duplicate_between( raw(self.process), self.value, - unsafe { GetCurrentProcess() }, + current, false, DUPLICATE_SAME_ACCESS | DUPLICATE_CLOSE_SOURCE, ); @@ -462,12 +470,18 @@ pub(crate) struct StandardHandles { pub(crate) stderr: isize, } +#[derive(Clone, Copy)] +pub(crate) enum StartupStdio { + Ordinary(StandardHandles), + PseudoConsole, +} + pub(crate) struct ProcessRequest<'a> { pub(crate) application: &'a [u16], pub(crate) command_line: &'a mut [u16], pub(crate) environment: Option<&'a [u16]>, pub(crate) current_dir: Option<&'a [u16]>, - pub(crate) stdio: Option, + pub(crate) stdio: StartupStdio, pub(crate) inherit_handles: bool, pub(crate) creation_flags: u32, pub(crate) suspended: bool, @@ -542,9 +556,9 @@ pub(crate) fn create_process(request: &mut ProcessRequest<'_>) -> io::Result) { - if let Some(handles) = handles { - startup.StartupInfo.dwFlags |= STARTF_USESTDHANDLES; +fn set_standard_handles(startup: &mut STARTUPINFOEXW, stdio: StartupStdio) { + startup.StartupInfo.dwFlags |= STARTF_USESTDHANDLES; + if let StartupStdio::Ordinary(handles) = stdio { startup.StartupInfo.hStdInput = handles.stdin as HANDLE; startup.StartupInfo.hStdOutput = handles.stdout as HANDLE; startup.StartupInfo.hStdError = handles.stderr as HANDLE; @@ -710,10 +724,11 @@ pub(crate) fn environment_strings() -> io::Result> { OsString::from_wide(&entry[separator + 1..]), )); } - // SAFETY: move to the first unit after this entry's terminator. let advance = length .checked_add(1) .ok_or_else(|| io::Error::other("environment block is too large"))?; + // SAFETY: `advance` moves to the first unit after this entry's + // terminator, which is still inside the double-NUL-terminated block. cursor = unsafe { cursor.add(advance) }; } Ok(entries) @@ -821,6 +836,14 @@ mod tests { } } + fn current_process() -> BorrowedHandle<'static> { + // SAFETY: `GetCurrentProcess` cannot fail and returns the + // current-process pseudo-handle, a constant that stays valid for the + // whole process lifetime. `BorrowedHandle` never closes what it borrows, + // so a `'static` borrow of it can never dangle or double-close. + unsafe { BorrowedHandle::borrow_raw(GetCurrentProcess() as RawHandle) } + } + #[test] fn pipe_null_and_duplicate_primitives_preserve_ownership() -> io::Result<()> { assert_eq!( @@ -854,20 +877,13 @@ mod tests { let target = open_parent_process(host.id())?; drop(target); let before = process_handle_count(host.as_handle())?; - let local_before = process_handle_count(unsafe { - BorrowedHandle::borrow_raw(GetCurrentProcess() as RawHandle) - })?; + let local_before = process_handle_count(current_process())?; let remote = duplicate_remote(writable_null.as_handle(), host.as_handle(), true)?; assert_ne!(remote.value(), 0); assert!(process_handle_count(host.as_handle())? > before); drop(remote); assert_eq!(process_handle_count(host.as_handle())?, before); - assert_eq!( - process_handle_count(unsafe { - BorrowedHandle::borrow_raw(GetCurrentProcess() as RawHandle) - })?, - local_before - ); + assert_eq!(process_handle_count(current_process())?, local_before); let _ = host.kill(); let _ = host.wait(); Ok(()) @@ -921,10 +937,10 @@ mod tests { } #[test] - fn startup_info_uses_standard_handles_only_when_supplied() { + fn startup_info_distinguishes_pseudoconsole_and_ordinary_stdio() { let mut conpty = STARTUPINFOEXW::default(); - set_standard_handles(&mut conpty, None); - assert_eq!(conpty.StartupInfo.dwFlags & STARTF_USESTDHANDLES, 0); + set_standard_handles(&mut conpty, StartupStdio::PseudoConsole); + assert_ne!(conpty.StartupInfo.dwFlags & STARTF_USESTDHANDLES, 0); assert!(conpty.StartupInfo.hStdInput.is_null()); assert!(conpty.StartupInfo.hStdOutput.is_null()); assert!(conpty.StartupInfo.hStdError.is_null()); @@ -932,7 +948,7 @@ mod tests { let mut ordinary = STARTUPINFOEXW::default(); set_standard_handles( &mut ordinary, - Some(StandardHandles { + StartupStdio::Ordinary(StandardHandles { stdin: 1, stdout: 2, stderr: 3, diff --git a/src/transaction.rs b/src/transaction.rs index 7b976fd..be7792b 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -15,7 +15,9 @@ use crate::child::{Child, SuspendedChild}; use crate::command::{Arg, Command, EnvOp, EnvValue}; use crate::handles::{Job, StdioInner}; use crate::options::DropPolicy; -use crate::plan::{Running, SpawnPlan, SpawnState, StandardHandles, StdioSpec, Suspended}; +use crate::plan::{ + Running, SpawnPlan, SpawnState, StandardHandles, StandardIo, StdioSpec, Suspended, +}; use crate::sys::{self, NullAccess, StandardStream}; const QUOTE: u16 = 0x22; @@ -39,22 +41,22 @@ impl SpawnTransaction { let parent: Option> = plan.options.parent.map(AsHandle::as_handle); let mut transfer = HandleTransfer::new(parent); let (stdio_values, stdio) = match &plan.stdio { - Some(specs) => { + StandardIo::Ordinary(specs) => { let prepared = prepare_standard_handles(specs, &mut transfer)?; - let values = sys::StandardHandles { + let values = sys::StartupStdio::Ordinary(sys::StandardHandles { stdin: prepared.stdin.child, stdout: prepared.stdout.child, stderr: prepared.stderr.child, - }; + }); let owners = StandardHandles { stdin: prepared.stdin.parent, stdout: prepared.stdout.parent, stderr: prepared.stderr.parent, }; - (Some(values), owners) + (values, owners) } - None => ( - None, + StandardIo::PseudoConsole => ( + sys::StartupStdio::PseudoConsole, StandardHandles { stdin: None, stdout: None, @@ -628,6 +630,40 @@ mod tests { assert_ne!(lower, EnvKey::new(OsString::from("beta"))); } + #[test] + fn environment_preserves_windows_ordinal_distinctions() { + let mut command = Command::new("cmd.exe"); + command + .env_clear() + .env("S", "latin-s") + .env("ſ", "long-s") + .env("Μ", "greek-mu") + .env("µ", "micro-sign"); + let mut transfer = HandleTransfer::new(None); + let block = build_environment(&command, &mut transfer) + .unwrap() + .block + .unwrap(); + let entries: Vec = block + .split(|unit| *unit == 0) + .filter(|entry| !entry.is_empty()) + .map(String::from_utf16_lossy) + .collect(); + + assert_eq!(entries.len(), 4, "Windows-distinct keys were overwritten"); + for expected in ["S=latin-s", "ſ=long-s", "Μ=greek-mu", "µ=micro-sign"] { + assert!(entries.iter().any(|entry| entry == expected)); + } + assert_ne!( + EnvKey::new(OsString::from("S")), + EnvKey::new(OsString::from("ſ")) + ); + assert_ne!( + EnvKey::new(OsString::from("Μ")), + EnvKey::new(OsString::from("µ")) + ); + } + #[test] fn quoting_covers_empty_and_trailing_backslashes() { let mut command = Command::new("program.exe"); diff --git a/tests/windows_spawn.rs b/tests/windows_spawn.rs index 1704287..c3639e8 100644 --- a/tests/windows_spawn.rs +++ b/tests/windows_spawn.rs @@ -34,6 +34,12 @@ use windows_sys::Win32::System::Threading::{ WaitForSingleObject, }; +const PCON_ISOLATION_HELPER: &str = "WINDOWS_SPAWN_PCON_ISOLATION_HELPER"; +const PCON_STDIO_PROBE: &str = "WINDOWS_SPAWN_PCON_STDIO_PROBE"; +const PCON_STDIN_MARKER: &[u8] = b"windows-spawn-pcon-stdin"; +const PCON_STDOUT_MARKER: &[u8] = b"windows-spawn-pcon-stdout"; +const PCON_STDERR_MARKER: &[u8] = b"windows-spawn-pcon-stderr"; + fn cmd(script: &str) -> Command { let mut command = Command::new("cmd.exe"); command.args(["/D", "/S", "/C"]).raw_arg(script); @@ -131,6 +137,19 @@ impl Drop for ProcessExitGuard { } } +fn wait_bounded(child: &mut windows_spawn::Child) -> io::Result> { + let deadline = Instant::now() + Duration::from_secs(5); + loop { + if let Some(status) = child.try_wait()? { + return Ok(Some(status)); + } + if Instant::now() >= deadline { + return Ok(None); + } + thread::sleep(Duration::from_millis(10)); + } +} + #[test] fn args_environment_cwd_and_wait_cache_work() -> io::Result<()> { let directory = temporary_path("cwd"); @@ -222,10 +241,9 @@ fn resume_rejects_an_externally_changed_suspend_count() -> io::Result<()> { let mut process = ProcessExitGuard::new(local_duplicate(&suspended, false)?); // SAFETY: the primary thread handle remains owned by SuspendedChild and // has THREAD_SUSPEND_RESUME access from CreateProcessW. - assert_eq!( - unsafe { SuspendThread(suspended.primary_thread_handle().as_raw_handle()) }, - 1 - ); + let previous_suspend_count = + unsafe { SuspendThread(suspended.primary_thread_handle().as_raw_handle()) }; + assert_eq!(previous_suspend_count, 1); assert_eq!( suspended.resume().unwrap_err().kind(), @@ -260,9 +278,11 @@ fn native_standard_handle_probe() { let Ok(mode) = std::env::var("WINDOWS_SPAWN_STDIO_PROBE") else { return; }; - // SAFETY: these calls only inspect the process-owned standard handle slots. + // SAFETY: this only inspects the process-owned standard input slot. let input = unsafe { GetStdHandle(STD_INPUT_HANDLE) }; + // SAFETY: this only inspects the process-owned standard output slot. let output = unsafe { GetStdHandle(STD_OUTPUT_HANDLE) }; + // SAFETY: this only inspects the process-owned standard error slot. let error = unsafe { GetStdHandle(STD_ERROR_HANDLE) }; let valid = |handle: HANDLE| { !handle.is_null() @@ -279,22 +299,20 @@ fn native_standard_handle_probe() { let mut byte = [0_u8; 1]; let mut read = 0_u32; // SAFETY: buffers and byte-count outputs are valid for synchronous I/O. - assert_ne!( - unsafe { ReadFile(input, byte.as_mut_ptr(), 1, &mut read, std::ptr::null_mut()) }, - 0 - ); + let read_succeeded = + unsafe { ReadFile(input, byte.as_mut_ptr(), 1, &mut read, std::ptr::null_mut()) }; + assert_ne!(read_succeeded, 0); assert_eq!(read, 0); let mut written = 0_u32; // SAFETY: the one-byte buffer and byte-count output remain valid. - assert_ne!( - unsafe { WriteFile(output, byte.as_ptr(), 1, &mut written, std::ptr::null_mut()) }, - 0 - ); + let wrote_stdout = + unsafe { WriteFile(output, byte.as_ptr(), 1, &mut written, std::ptr::null_mut()) }; + assert_ne!(wrote_stdout, 0); assert_eq!(written, 1); - assert_ne!( - unsafe { WriteFile(error, byte.as_ptr(), 1, &mut written, std::ptr::null_mut()) }, - 0 - ); + // SAFETY: the one-byte buffer and byte-count output remain valid. + let wrote_stderr = + unsafe { WriteFile(error, byte.as_ptr(), 1, &mut written, std::ptr::null_mut()) }; + assert_ne!(wrote_stderr, 0); assert_eq!(written, 1); } } @@ -329,12 +347,20 @@ fn explicit_job_attachment_and_kill_tree_output_complete() -> io::Result<()> { // The background grandchild inherits stdout. Without terminating the // private Job after root exit, wait_with_output would never observe EOF. - let mut tree = cmd("start \"\" /b cmd.exe /D /C \"ping -n 8 127.0.0.1 >nul\" & echo root"); + // Keep the natural grandchild lifetime well beyond the assertion budget. + // This preserves the EOF proof without making a three-second wall-clock + // deadline flaky when the full integration suite creates processes in + // parallel on a loaded CI host. + let mut tree = cmd("start \"\" /b cmd.exe /D /C \"ping -n 20 127.0.0.1 >nul\" & echo root"); let started = Instant::now(); let output = tree.output_with(SpawnOptions::new().drop_policy(DropPolicy::KillTree))?; + let elapsed = started.elapsed(); assert!(output.status.success()); assert!(String::from_utf8_lossy(&output.stdout).contains("root")); - assert!(started.elapsed() < Duration::from_secs(3)); + assert!( + elapsed < Duration::from_secs(10), + "root-bounded output waited {elapsed:?} for the background grandchild" + ); Ok(()) } @@ -661,7 +687,38 @@ impl TestPseudoConsole { }) } - fn wait_for_output(&self, expected: &[u8]) -> io::Result { + fn write_input(&self, input: &[u8]) -> io::Result<()> { + let writer = self + .input_writer + .as_ref() + .expect("a live pseudoconsole retains its input writer"); + let mut written = 0_u32; + let length = u32::try_from(input.len()) + .map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "input is too large"))?; + // SAFETY: the input buffer and byte-count output are valid for the + // synchronous write, and the writer remains owned by self. + if unsafe { + WriteFile( + writer.as_raw_handle(), + input.as_ptr(), + length, + &mut written, + std::ptr::null_mut(), + ) + } == 0 + { + return Err(io::Error::last_os_error()); + } + if written != length { + return Err(io::Error::new( + io::ErrorKind::WriteZero, + "the pseudoconsole input write was incomplete", + )); + } + Ok(()) + } + + fn wait_for_output_markers(&self, expected: &[&[u8]]) -> io::Result { let output = self .output_reader .as_ref() @@ -703,10 +760,11 @@ impl TestPseudoConsole { return Err(io::Error::last_os_error()); } received.extend_from_slice(&buffer[..read as usize]); - if received - .windows(expected.len()) - .any(|window| window == expected) - { + if expected.iter().all(|marker| { + received + .windows(marker.len()) + .any(|window| window == *marker) + }) { return Ok(true); } } @@ -746,21 +804,6 @@ unsafe impl AsPseudoConsole for TestPseudoConsole { #[test] fn pseudoconsole_attribute_connects_the_child_console() -> io::Result<()> { - fn wait_bounded( - child: &mut windows_spawn::Child, - ) -> io::Result> { - let deadline = Instant::now() + Duration::from_secs(5); - loop { - if let Some(status) = child.try_wait()? { - return Ok(Some(status)); - } - if Instant::now() >= deadline { - return Ok(None); - } - thread::sleep(Duration::from_millis(10)); - } - } - let pseudoconsole = TestPseudoConsole::create()?; let mut command = Command::new(std::env::current_exe()?); command @@ -774,7 +817,7 @@ fn pseudoconsole_attribute_connects_the_child_console() -> io::Result<()> { wait_bounded(&mut child)?.expect("ConPTY child must terminate after kill") }; assert!(status.success()); - assert!(pseudoconsole.wait_for_output(b"windows-spawn-pcon-attached")?); + assert!(pseudoconsole.wait_for_output_markers(&[b"windows-spawn-pcon-attached"])?); Ok(()) } @@ -784,13 +827,107 @@ fn pseudoconsole_child_probe() { return; } // SAFETY: GetConsoleCP has no pointer preconditions. A nonzero code page - // proves this process was attached to a console even though ConPTY startup - // intentionally leaves all ordinary standard-handle slots zero. + // proves this process was attached to a console. Opening CONOUT$ directly + // is only an auxiliary connection check; the isolated stdio regression + // below exercises the child's ordinary stdin/stdout/stderr slots. assert_ne!(unsafe { GetConsoleCP() }, 0); let mut output = File::options().write(true).open("CONOUT$").unwrap(); output.write_all(b"windows-spawn-pcon-attached").unwrap(); } +#[test] +fn pseudoconsole_regular_stdio_stays_off_parent_pipes() -> io::Result<()> { + let mut helper = Command::new(std::env::current_exe()?); + helper + .args([ + "--exact", + "pseudoconsole_stdio_isolation_helper", + "--nocapture", + ]) + .env(PCON_ISOLATION_HELPER, "1") + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + let output = helper.output()?; + assert!( + output.status.success(), + "isolated ConPTY stdio helper failed: stdout={:?}, stderr={:?}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + for marker in [PCON_STDOUT_MARKER, PCON_STDERR_MARKER] { + assert!( + !output + .stdout + .windows(marker.len()) + .any(|window| window == marker), + "ConPTY child output leaked to its parent's stdout" + ); + assert!( + !output + .stderr + .windows(marker.len()) + .any(|window| window == marker), + "ConPTY child output leaked to its parent's stderr" + ); + } + Ok(()) +} + +#[test] +fn pseudoconsole_stdio_isolation_helper() -> io::Result<()> { + if std::env::var_os(PCON_ISOLATION_HELPER).is_none() { + return Ok(()); + } + + let pseudoconsole = TestPseudoConsole::create()?; + let mut command = Command::new(std::env::current_exe()?); + command + .args([ + "--exact", + "pseudoconsole_regular_stdio_probe", + "--nocapture", + ]) + .env(PCON_STDIO_PROBE, "1"); + let mut child = command.spawn_with(SpawnOptions::new().pseudoconsole(&pseudoconsole))?; + let mut input = PCON_STDIN_MARKER.to_vec(); + input.extend_from_slice(b"\r\n"); + pseudoconsole.write_input(&input)?; + + let status = if let Some(status) = wait_bounded(&mut child)? { + status + } else { + let _ = child.kill(); + wait_bounded(&mut child)?.expect("ConPTY stdio probe must terminate after kill") + }; + assert!(status.success()); + assert!(pseudoconsole.wait_for_output_markers(&[PCON_STDOUT_MARKER, PCON_STDERR_MARKER])?); + Ok(()) +} + +#[test] +fn pseudoconsole_regular_stdio_probe() -> io::Result<()> { + if std::env::var_os(PCON_STDIO_PROBE).is_none() { + return Ok(()); + } + + let mut line = String::new(); + io::stdin().read_line(&mut line)?; + if !line + .as_bytes() + .windows(PCON_STDIN_MARKER.len()) + .any(|window| window == PCON_STDIN_MARKER) + { + return Err(io::Error::other( + "standard input did not arrive through ConPTY", + )); + } + io::stdout().write_all(PCON_STDOUT_MARKER)?; + io::stdout().flush()?; + io::stderr().write_all(PCON_STDERR_MARKER)?; + io::stderr().flush() +} + #[test] fn capability_wrappers_and_all_option_builders_are_exercised() -> io::Result<()> { let path = temporary_path("capability"); diff --git a/xtask/src/tasks.rs b/xtask/src/tasks.rs index c395065..fa2f686 100644 --- a/xtask/src/tasks.rs +++ b/xtask/src/tasks.rs @@ -648,7 +648,20 @@ fn sha256(path: &Path) -> Result { } digest.update(&buffer[..read]); } - Ok(format!("{:x}", digest.finalize())) + Ok(lower_hex(&digest.finalize())) +} + +// `sha2` 0.11 returns `hybrid_array::Array` instead of `GenericArray`, and that +// type no longer implements `LowerHex`. Formatting the bytes ourselves keeps the +// checksum output identical across both generations of the crate. +fn lower_hex(bytes: &[u8]) -> String { + use std::fmt::Write as _; + + let mut output = String::with_capacity(bytes.len() * 2); + for byte in bytes { + write!(&mut output, "{byte:02x}").expect("writing to a String cannot fail"); + } + output } fn verify_release_tag(root: &Path, tag: &str) -> Result<()> {