From 54fe398a32360a05476a22b67d4a2a6b817a72e0 Mon Sep 17 00:00:00 2001 From: Yasunobu <42543015+P4suta@users.noreply.github.com> Date: Mon, 3 Aug 2026 20:27:59 +0900 Subject: [PATCH] feat: strengthen spawn ownership and align the quality gates Completes the in-flight ownership work and closes the gaps that separated this crate from its siblings. Ownership and ConPTY: - Private running/suspended typestates, unified handle-transfer ownership, and value-based pseudoconsole storage. - ConPTY startup now sets STARTF_USESTDHANDLES with all ordinary standard handles null, matching Microsoft Terminal, so a hosted child can no longer read or write the parent's redirected streams. A 32-bit CI leg runs the regression, because the failure mode is pointer-width sensitive. - SuspendedChild::resume requires the primary thread's previous suspend count to be exactly one; external changes are rejected and rolled back. - Output capture joins both reader threads even when one errors or panics. Quality gates: - Deny clippy::undocumented_unsafe_blocks. CONTRIBUTING already required a specific safety justification on every unsafe block, but nothing checked it; five blocks had none. multiple_unsafe_ops_per_block is deliberately left off, because several blocks adopt two OS resources together to stay exception-safe and splitting them would widen the leak window. - Add a compiled and executed Command example. The crate had only compile_fail boundary pins, so no usage example was ever type-checked and the README example had never been compiled. - Add _typos.toml so the spell-check gate has a checked-in configuration. Dependency handling: - The release checksum helper no longer depends on LowerHex being implemented for the digest output, so it builds against sha2 0.10 and 0.11 alike. The bump itself stays deferred and is now recorded in dependabot.yml: sha2 0.11 requires Rust 1.85, and both `just msrv` and the 1.75 test legs cover the whole workspace, so taking it would break CI even though xtask never ships. Co-Authored-By: Claude Fable 5 --- .github/dependabot.yml | 10 ++ .github/workflows/ci.yml | 18 ++- CHANGELOG.md | 20 ++- Cargo.toml | 10 ++ README.md | 10 +- REUSE.toml | 1 + _typos.toml | 11 ++ docs/adr/0006-conpty-boundary.md | 34 +++-- docs/crate.md | 5 +- src/command.rs | 20 +++ src/plan.rs | 18 ++- src/sys.rs | 58 +++++--- src/transaction.rs | 50 ++++++- tests/windows_spawn.rs | 221 +++++++++++++++++++++++++------ xtask/src/tasks.rs | 15 ++- 15 files changed, 409 insertions(+), 92 deletions(-) create mode 100644 _typos.toml 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<()> {