From 53c57decc9693c93bb0f47e21b3a43fe72396a9b Mon Sep 17 00:00:00 2001 From: Yasunobu <42543015+P4suta@users.noreply.github.com> Date: Tue, 13 Oct 2026 21:54:27 +0900 Subject: [PATCH 1/6] refactor: make spawn state and handle types explicit A suspended child and an uncommitted transaction kept their process in an `Option` that `resume` and the commits took out with an `expect`. The process handle now terminates a process that never ran when it drops, so each of them owns its parts directly. A running spawn commits by resuming a `SuspendedChild`, so both commits share one resume. The creation splits into functions for the standard I/O, the private Job, the Job list, the working directory, and the attribute list. Each handle names its object type, and each `sys` wrapper takes the type its call needs. Handle values carry the table they belong to, so a value of the creator's table can't join the handles that an alternate parent's child inherits. `AttributeList` borrows the values its attributes point to, so the compiler keeps them alive until `DeleteProcThreadAttributeList` runs. The lint registry drops the `expect_used` and `too_many_lines` exceptions of `src/child.rs` and `src/transaction.rs`. The public API loses only the `Drop` impl of `SuspendedChild`, whose process handle now ends the process. Refs #35 --- .rust-mutants.toml | 12 +- public-api/windows-spawn.txt | 2 - src/child.rs | 199 +++++++++++------- src/command.rs | 11 +- src/failure_tests.rs | 16 +- src/handles.rs | 65 +++--- src/lib.rs | 2 + src/resource.rs | 220 ++++++++++++++++++++ src/sys.rs | 273 ++++++++++++++----------- src/transaction.rs | 378 +++++++++++++++++------------------ xtask/src/gates.rs | 19 -- 11 files changed, 754 insertions(+), 443 deletions(-) create mode 100644 src/resource.rs diff --git a/.rust-mutants.toml b/.rust-mutants.toml index 44a9aa9..6418c85 100644 --- a/.rust-mutants.toml +++ b/.rust-mutants.toml @@ -60,9 +60,9 @@ outcome = "survived" [[mutation.expect]] path = "src/transaction.rs" -item = "SpawnTransaction::create" +item = "attribute_list" rule = "condition-to-false" -original = "attribute_count == 0" +original = "count == 0" reason = "Equivalence: CreateProcessW accepts an empty attribute list." outcome = "survived" @@ -94,20 +94,20 @@ outcome = "survived" [[mutation.skip]] path = "src/sys.rs" -lines = "1210-1210" +lines = "1246-1246" reason = "Unreachable: GetEnvironmentStringsW returns null only when out of memory." [[mutation.skip]] path = "src/sys.rs" -lines = "1219-1219" +lines = "1255-1255" reason = "Hang-only: continuing past the block terminator loops forever." [[mutation.skip]] path = "src/transaction.rs" -lines = "572-572" +lines = "565-565" reason = "Unreachable: GetFullPathNameW cannot return a double quote." [[mutation.skip]] path = "src/transaction.rs" -lines = "127-131" +lines = "165-169" reason = "Unreachable: wide_nul fails only on an interior NUL, which validate_command rejects first; rejects_every_malformed_text_component holds it." diff --git a/public-api/windows-spawn.txt b/public-api/windows-spawn.txt index 7c86c2d..2b85032 100644 --- a/public-api/windows-spawn.txt +++ b/public-api/windows-spawn.txt @@ -878,8 +878,6 @@ pub fn windows_spawn::SuspendedChild::main_thread_handle(&self) -> std::os::wind pub fn windows_spawn::SuspendedChild::resume(self) -> windows_spawn::Result impl core::fmt::Debug for windows_spawn::SuspendedChild pub fn windows_spawn::SuspendedChild::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result -impl core::ops::drop::Drop for windows_spawn::SuspendedChild -pub fn windows_spawn::SuspendedChild::drop(&mut self) impl std::os::windows::io::handle::AsHandle for windows_spawn::SuspendedChild pub fn windows_spawn::SuspendedChild::as_handle(&self) -> std::os::windows::io::handle::BorrowedHandle<'_> impl std::os::windows::io::raw::AsRawHandle for windows_spawn::SuspendedChild diff --git a/src/child.rs b/src/child.rs index a99c260..f5cbe66 100644 --- a/src/child.rs +++ b/src/child.rs @@ -7,6 +7,7 @@ use std::thread; use crate::error::{Operation, Phase, Result, SpawnError}; use crate::handles::{windows, Job, Stdio}; +use crate::resource::{Borrowed, Owned, Pipe, Process, Thread}; use crate::sys; /// The writable parent end of a child's standard-input pipe. @@ -14,12 +15,12 @@ use crate::sys; /// Like the output ends, it converts into [`Stdio`] and into [`OwnedHandle`]. #[derive(Debug)] pub struct ChildStdin { - handle: OwnedHandle, + handle: Owned, } impl Write for ChildStdin { fn write(&mut self, buffer: &[u8]) -> io::Result { - sys::write_handle(self.handle.as_handle(), buffer) + sys::write_handle(self.handle.borrow(), buffer) } fn flush(&mut self) -> io::Result<()> { @@ -29,7 +30,7 @@ impl Write for ChildStdin { impl Write for &ChildStdin { fn write(&mut self, buffer: &[u8]) -> io::Result { - sys::write_handle(self.handle.as_handle(), buffer) + sys::write_handle(self.handle.borrow(), buffer) } fn flush(&mut self) -> io::Result<()> { @@ -69,12 +70,12 @@ impl AsRawHandle for ChildStdin { /// ``` #[derive(Debug)] pub struct ChildStdout { - handle: OwnedHandle, + handle: Owned, } impl Read for ChildStdout { fn read(&mut self, buffer: &mut [u8]) -> io::Result { - sys::read_handle(self.handle.as_handle(), buffer) + sys::read_handle(self.handle.borrow(), buffer) } } @@ -93,12 +94,12 @@ impl AsRawHandle for ChildStdout { /// The readable parent end of a child's standard-error pipe. #[derive(Debug)] pub struct ChildStderr { - handle: OwnedHandle, + handle: Owned, } impl Read for ChildStderr { fn read(&mut self, buffer: &mut [u8]) -> io::Result { - sys::read_handle(self.handle.as_handle(), buffer) + sys::read_handle(self.handle.borrow(), buffer) } } @@ -116,37 +117,79 @@ impl AsRawHandle for ChildStderr { impl From for OwnedHandle { fn from(stream: ChildStdin) -> Self { - stream.handle + stream.handle.into_inner() } } impl From for OwnedHandle { fn from(stream: ChildStdout) -> Self { - stream.handle + stream.handle.into_inner() } } impl From for OwnedHandle { fn from(stream: ChildStderr) -> Self { - stream.handle + stream.handle.into_inner() } } impl From for Stdio { fn from(stream: ChildStdin) -> Self { - Self::from(stream.handle) + Self::from(OwnedHandle::from(stream)) } } impl From for Stdio { fn from(stream: ChildStdout) -> Self { - Self::from(stream.handle) + Self::from(OwnedHandle::from(stream)) } } impl From for Stdio { fn from(stream: ChildStderr) -> Self { - Self::from(stream.handle) + Self::from(OwnedHandle::from(stream)) + } +} + +/// The crate's handle to a process it created. +/// +/// Until the main thread has run, dropping the handle terminates the process, so a spawn that fails, or a suspended child that is dropped, leaves nothing running. +#[derive(Debug)] +pub(crate) struct ProcessHandle { + handle: Owned, + unstarted: bool, +} + +impl ProcessHandle { + /// Owns a process whose main thread hasn't run. + pub(crate) const fn unstarted(handle: Owned) -> Self { + Self { + handle, + unstarted: true, + } + } + + /// Records that the main thread runs, so dropping the handle leaves the process running. + pub(crate) fn mark_started(&mut self) { + self.unstarted = false; + } + + pub(crate) fn borrow(&self) -> Borrowed<'_, Process> { + self.handle.borrow() + } +} + +impl Drop for ProcessHandle { + fn drop(&mut self) { + if self.unstarted { + drop(sys::terminate_process(self.handle.borrow(), 1)); + } + } +} + +impl AsHandle for ProcessHandle { + fn as_handle(&self) -> BorrowedHandle<'_> { + self.handle.as_handle() } } @@ -157,7 +200,9 @@ impl From for Stdio { /// Dropping the value leaves the process running, unless [`crate::Command::own_process_tree`] made it the owner of the tree. #[derive(Debug)] pub struct Child { - /// Declared first so kill-on-close runs before the pipe and process handles drop. + /// Declared first, so a process that never ran ends with the exit code of a termination before its Job closes. + process: ProcessHandle, + /// Declared before the pipes, so closing the Job ends the tree while the pipes still hold their ends. kill_job: Option, /// A pipe connected to the child's standard input, when requested. pub stdin: Option, @@ -165,26 +210,25 @@ pub struct Child { pub stdout: Option, /// A pipe connected to the child's standard error, when requested. pub stderr: Option, - process: OwnedHandle, pid: u32, exit: Option, } impl Child { pub(crate) fn new( - process: OwnedHandle, + process: ProcessHandle, pid: u32, kill_job: Option, - stdin: Option, - stdout: Option, - stderr: Option, + stdin: Option>, + stdout: Option>, + stderr: Option>, ) -> Self { Self { + process, kill_job, stdin: stdin.map(|handle| ChildStdin { handle }), stdout: stdout.map(|handle| ChildStdout { handle }), stderr: stderr.map(|handle| ChildStderr { handle }), - process, pid, exit: None, } @@ -212,10 +256,10 @@ impl Child { if self.exit.is_some() { return Ok(()); } - let Err(error) = sys::terminate_process(self.process.as_handle(), 1) else { + let Err(error) = sys::terminate_process(self.process.borrow(), 1) else { return Ok(()); }; - if matches!(sys::try_wait_process(self.process.as_handle()), Ok(true)) { + if matches!(sys::try_wait_process(self.process.borrow()), Ok(true)) { return Ok(()); } Err(SpawnError::windows( @@ -238,7 +282,7 @@ impl Child { windows( Phase::Runtime, Operation::WaitProcess, - sys::wait_process(self.process.as_handle()), + sys::wait_process(self.process.borrow()), )?; let status = self.exit_status()?; self.exit = Some(status); @@ -257,7 +301,7 @@ impl Child { let exited = windows( Phase::Runtime, Operation::WaitProcess, - sys::try_wait_process(self.process.as_handle()), + sys::try_wait_process(self.process.borrow()), )?; if !exited { return Ok(None); @@ -271,7 +315,7 @@ impl Child { windows( Phase::Runtime, Operation::QueryExitCode, - sys::exit_status(self.process.as_handle()), + sys::exit_status(self.process.borrow()), ) } @@ -291,14 +335,14 @@ impl Child { let ids = windows( Phase::Runtime, Operation::QueryJob, - sys::job_process_ids(job.as_handle()), + sys::job_process_ids(job.borrow()), )?; let mut members = Vec::with_capacity(ids.len()); for id in ids { members.extend(windows( Phase::Runtime, Operation::OpenProcess, - sys::open_job_process(id, job.as_handle()), + sys::open_job_process(id, job.borrow()), )?); } drop(self.stdin.take()); @@ -309,7 +353,7 @@ impl Child { windows( Phase::Runtime, Operation::WaitProcess, - sys::wait_process(member.as_handle()), + sys::wait_process(member.borrow()), )?; } } @@ -331,11 +375,11 @@ impl Child { let stdout_reader = self .stdout .take() - .map(|stream| thread::spawn(move || drain_output(stream.handle.as_handle()))); + .map(|stream| thread::spawn(move || drain_output(stream.handle.borrow()))); let stderr_reader = self .stderr .take() - .map(|stream| thread::spawn(move || drain_output(stream.handle.as_handle()))); + .map(|stream| thread::spawn(move || drain_output(stream.handle.borrow()))); let status = self.wait(); let termination = self @@ -361,11 +405,11 @@ impl AsHandle for Child { impl AsRawHandle for Child { fn as_raw_handle(&self) -> RawHandle { - self.process.as_raw_handle() + self.process.as_handle().as_raw_handle() } } -fn drain_output(handle: BorrowedHandle<'_>) -> Result> { +fn drain_output(handle: Borrowed<'_, Pipe>) -> Result> { let mut bytes = Vec::new(); let mut buffer = [0_u8; 8_192]; loop { @@ -420,24 +464,19 @@ fn join_readers(stdout: Option, stderr: Option) -> Result<(Vec, - main_thread: OwnedHandle, + child: Child, + main_thread: Owned, } impl SuspendedChild { - pub(crate) const fn new(child: Child, main_thread: OwnedHandle) -> Self { - Self { - pid: child.id(), - child: Some(child), - main_thread, - } + pub(crate) const fn new(child: Child, main_thread: Owned) -> Self { + Self { child, main_thread } } /// Returns the process identifier captured at creation. #[must_use] pub const fn id(&self) -> u32 { - self.pid + self.child.id() } /// Borrows the suspended process's main thread handle. @@ -455,46 +494,33 @@ impl SuspendedChild { /// Returns a Windows error if the main thread can't be resumed, /// and one of kind [`io::ErrorKind::InvalidData`] if another thread suspended it again, so its suspend count was not one. /// The process is terminated in both cases. - pub fn resume(mut self) -> Result { + pub fn resume(self) -> Result { + let Self { + mut child, + main_thread, + } = self; let previous = windows( Phase::Resume, Operation::ResumeThread, - sys::resume_thread(self.main_thread.as_handle()), + sys::resume_thread(main_thread.borrow()), )?; if previous != 1 { return Err(SpawnError::unexpected_suspend_count(previous)); } - self.child.take().ok_or_else(|| { - SpawnError::windows( - Phase::Resume, - Operation::ResumeThread, - io::Error::other("suspended child lost its process"), - ) - }) + child.process.mark_started(); + Ok(child) } } impl AsHandle for SuspendedChild { - #[allow(clippy::expect_used)] fn as_handle(&self) -> BorrowedHandle<'_> { - self.child - .as_ref() - .expect("a suspended child owns its process until resume") - .as_handle() + self.child.as_handle() } } impl AsRawHandle for SuspendedChild { fn as_raw_handle(&self) -> RawHandle { - self.as_handle().as_raw_handle() - } -} - -impl Drop for SuspendedChild { - fn drop(&mut self) { - if let Some(child) = &mut self.child { - drop(child.kill()); - } + self.child.as_raw_handle() } } @@ -566,25 +592,52 @@ mod tests { assert!(alone.wait_tree().unwrap().code().is_some()); let mut owner = command.own_process_tree().spawn().unwrap(); - let job = owner.kill_job.as_ref().unwrap().as_handle(); + let job = owner.kill_job.as_ref().unwrap().borrow(); let ids = sys::job_process_ids(job).unwrap(); assert_eq!(ids, [usize::try_from(owner.id()).unwrap()]); let member = sys::open_job_process(ids[0], job).unwrap().unwrap(); - assert!(!sys::try_wait_process(member.as_handle()).unwrap()); + assert!(!sys::try_wait_process(member.borrow()).unwrap()); let outsider = usize::try_from(std::process::id()).unwrap(); assert!(sys::open_job_process(outsider, job).unwrap().is_none()); assert!(sys::open_job_process(0, job).unwrap().is_none()); assert!(owner.wait_tree().unwrap().code().is_some()); assert_eq!( - sys::job_process_ids(owner.kill_job.as_ref().unwrap().as_handle()).unwrap(), + sys::job_process_ids(owner.kill_job.as_ref().unwrap().borrow()).unwrap(), Vec::::new() ); } + /// A process that survives its handle reads the end of its input and exits 5, and a terminated one exits 1. + #[test] + fn only_a_process_that_never_ran_ends_when_its_handle_drops() { + let mut command = crate::Command::new("cmd.exe"); + command + .args(["/D", "/S", "/C"]) + .raw_arg("set /p x=&exit /b 5") + .stdin(Stdio::piped()) + .stdout(Stdio::null()) + .stderr(Stdio::null()); + let suspended = command.spawn_suspended().unwrap(); + let mut unstarted = sys::test_support::ProcessExitGuard::watch(suspended.as_handle()); + let thread = sys::test_support::duplicate(suspended.main_thread_handle()); + drop(suspended); + sys::test_support::tempt_resume(thread.as_handle()); + assert_eq!(unstarted.exit_code(), 1); + + for child in [ + command.spawn().unwrap(), + command.spawn_suspended().unwrap().resume().unwrap(), + ] { + let mut started = sys::test_support::ProcessExitGuard::watch(child.as_handle()); + drop(child); + assert_eq!(started.exit_code(), 5); + } + } + #[test] fn reader_errors_reach_the_caller() { let (reader, writer) = sys::test_support::pipe(); - assert!(drain_output(writer.as_handle()).is_err()); + assert!(drain_output(Borrowed::adopt(writer.as_handle())).is_err()); drop(reader); let mut exited = crate::Command::new("cmd.exe"); @@ -595,12 +648,16 @@ mod tests { .stderr(Stdio::null()); let source = exited.spawn().unwrap(); let (unreadable_reader, unreadable) = sys::test_support::pipe(); + let mut process = ProcessHandle::unstarted(Owned::adopt(sys::test_support::duplicate( + source.as_handle(), + ))); + process.mark_started(); let child = Child::new( - sys::test_support::duplicate(source.as_handle()), + process, source.id(), None, None, - Some(unreadable), + Some(Owned::adopt(unreadable)), None, ); assert!(child.wait_with_output().is_err()); diff --git a/src/command.rs b/src/command.rs index d5a6743..49c72cf 100644 --- a/src/command.rs +++ b/src/command.rs @@ -2,7 +2,7 @@ use std::ffi::{OsStr, OsString}; use std::fmt; -use std::os::windows::io::{AsHandle, OwnedHandle}; +use std::os::windows::io::AsHandle; use std::path::{Path, PathBuf}; use std::process::{ExitStatus, Output}; @@ -13,6 +13,7 @@ use crate::handles::Stdio; use crate::mitigation::MitigationPolicy; use crate::options::{ConsoleMode, Creation, PriorityClass, SpawnOptions}; use crate::plan::{IoMode, SpawnPlan}; +use crate::resource::{Borrowed, Foreign, Owned}; use crate::sys; use crate::transaction::SpawnTransaction; @@ -20,7 +21,7 @@ use crate::transaction::SpawnTransaction; pub(crate) enum Arg { Text(OsString), Raw(OsString), - Handle(OwnedHandle), + Handle(Owned), } #[derive(Debug)] @@ -32,7 +33,7 @@ pub(crate) enum EnvOp { #[derive(Debug)] pub(crate) enum EnvValue { Text(OsString), - Handle(OwnedHandle), + Handle(Owned), } /// A reusable description of a Windows process launch, with the API of [`std::process::Command`]. @@ -434,8 +435,8 @@ impl Command { } /// A private, non-inheritable duplicate that the command owns until a spawn lowers it. -fn private_duplicate(handle: &T) -> Result { - sys::duplicate_local(handle.as_handle(), false).map_err(|error| { +fn private_duplicate(handle: &T) -> Result> { + sys::duplicate_local(Borrowed::foreign(handle), false).map_err(|error| { SpawnError::windows(Phase::Preparation, Operation::DuplicateLocalHandle, error) }) } diff --git a/src/failure_tests.rs b/src/failure_tests.rs index 2bfd9d5..f125b6a 100644 --- a/src/failure_tests.rs +++ b/src/failure_tests.rs @@ -60,13 +60,15 @@ fn pipe(operation: crate::Operation, outcome: io::Result) -> Result { /// A private duplicate, typed as the crate types its own duplicates. fn duplicate(handle: &impl AsHandle) -> Result { - crate::sys::duplicate_local(handle.as_handle(), false).map_err(|error| { - SpawnError::windows( - Phase::Preparation, - crate::Operation::DuplicateLocalHandle, - error, - ) - }) + crate::sys::duplicate_local(crate::resource::Borrowed::foreign(handle), false) + .map(crate::resource::Owned::into_inner) + .map_err(|error| { + SpawnError::windows( + Phase::Preparation, + crate::Operation::DuplicateLocalHandle, + error, + ) + }) } /// Returns true if `error` holds the injected failure, as the failure itself or as one that a release met. diff --git a/src/handles.rs b/src/handles.rs index a6de165..2180a34 100644 --- a/src/handles.rs +++ b/src/handles.rs @@ -7,6 +7,7 @@ use std::marker::PhantomData; use std::os::windows::io::{AsHandle, AsRawHandle, BorrowedHandle, OwnedHandle, RawHandle}; use crate::error::{Operation, Phase, Result, SpawnError}; +use crate::resource::{Borrowed, JobObject, Owned, Process, Stream}; use crate::sys; /// Runs one Windows operation and types its failure with the phase and operation. @@ -26,7 +27,7 @@ pub(crate) enum StdioInner { Inherit, Null, Piped, - Owned(OwnedHandle), + Owned(Owned), /// The caller's standard output, in whichever slot the child receives it. CallerStdout, /// The caller's standard error, in whichever slot the child receives it. @@ -83,16 +84,16 @@ impl Stdio { windows( Phase::Preparation, Operation::DuplicateLocalHandle, - sys::duplicate_local(source.as_handle(), false), + sys::duplicate_local(Borrowed::foreign(source), false), ) - .map(Self::from) + .map(|duplicate| Self::from(duplicate.into_inner())) } } impl From for Stdio { fn from(handle: OwnedHandle) -> Self { Self { - inner: StdioInner::Owned(handle), + inner: StdioInner::Owned(Owned::adopt(handle)), } } } @@ -124,7 +125,7 @@ impl From for Stdio { /// A process handle validated for use as `PROC_THREAD_ATTRIBUTE_PARENT_PROCESS`. #[derive(Debug)] pub struct ParentProcess { - handle: OwnedHandle, + handle: Owned, } impl ParentProcess { @@ -151,9 +152,15 @@ impl ParentProcess { windows( Phase::Preparation, Operation::OpenProcess, - sys::validate_process_handle(handle.as_handle()), + sys::validate_process_handle(Borrowed::foreign(&handle)), )?; - Ok(Self { handle }) + Ok(Self { + handle: Owned::adopt(handle), + }) + } + + pub(crate) fn borrow(&self) -> Borrowed<'_, Process> { + self.handle.borrow() } } @@ -171,14 +178,14 @@ impl AsRawHandle for ParentProcess { impl From for OwnedHandle { fn from(parent: ParentProcess) -> Self { - parent.handle + parent.handle.into_inner() } } /// An owned Windows Job object. #[derive(Debug)] pub struct Job { - handle: OwnedHandle, + handle: Owned, } impl Job { @@ -201,9 +208,11 @@ impl Job { windows( Phase::Preparation, Operation::QueryJob, - sys::validate_job_handle(handle.as_handle()), + sys::validate_job_handle(Borrowed::foreign(&handle)), )?; - Ok(Self { handle }) + Ok(Self { + handle: Owned::adopt(handle), + }) } /// Creates an independent duplicate of this Job handle. @@ -215,7 +224,7 @@ impl Job { windows( Phase::Preparation, Operation::DuplicateLocalHandle, - sys::duplicate_local(self.handle.as_handle(), false), + sys::duplicate_local(self.handle.borrow(), false), ) .map(|handle| Self { handle }) } @@ -231,7 +240,7 @@ impl Job { windows( Phase::Runtime, Operation::AssignJob, - sys::assign_job(self.handle.as_handle(), process.as_handle()), + sys::assign_job(self.handle.borrow(), Borrowed::foreign(process)), ) } @@ -244,7 +253,7 @@ impl Job { windows( Phase::Runtime, Operation::TerminateJob, - sys::terminate_job(self.handle.as_handle(), exit_code), + sys::terminate_job(self.handle.borrow(), exit_code), ) } @@ -259,9 +268,13 @@ impl Job { windows( Phase::Preparation, Operation::ConfigureJob, - sys::set_job_kill_on_close(self.handle.as_handle(), kill), + sys::set_job_kill_on_close(self.handle.borrow(), kill), ) } + + pub(crate) fn borrow(&self) -> Borrowed<'_, JobObject> { + self.handle.borrow() + } } impl AsHandle for Job { @@ -278,7 +291,7 @@ impl AsRawHandle for Job { impl From for OwnedHandle { fn from(job: Job) -> Self { - job.handle + job.handle.into_inner() } } @@ -330,6 +343,13 @@ mod tests { const ACCESS_DENIED: i32 = 5; + /// A private duplicate of a handle whose kind the crate doesn't know. + fn duplicate(source: &T) -> OwnedHandle { + sys::duplicate_local(Borrowed::foreign(source), false) + .unwrap() + .into_inner() + } + #[test] fn stored_duplicates_are_private() -> Result<()> { use crate::sys::test_support::is_inheritable; @@ -374,25 +394,20 @@ mod tests { let _host_guard = sys::test_support::ProcessExitGuard::watch(host.as_handle()); let parent = ParentProcess::open(host.id()).unwrap(); assert!(format!("{parent:?}").contains("ParentProcess")); - let adopted_parent = - ParentProcess::from_handle(sys::duplicate_local(host.as_handle(), false).unwrap()) - .unwrap(); + let adopted_parent = ParentProcess::from_handle(duplicate(&host)).unwrap(); assert_ne!( adopted_parent.as_handle().as_raw_handle(), std::ptr::null_mut() ); let job = Job::create().unwrap(); - let duplicate = job.duplicate().unwrap(); - let adopted_job = Job::from_handle(duplicate.handle).unwrap(); + let adopted_job = Job::from_handle(OwnedHandle::from(job.duplicate().unwrap())).unwrap(); adopted_job.set_kill_on_close(true).unwrap(); adopted_job.set_kill_on_close(false).unwrap(); let file = File::open("NUL").unwrap(); - let not_process = sys::duplicate_local(file.as_handle(), false).unwrap(); - assert!(ParentProcess::from_handle(not_process).is_err()); - let not_job = sys::duplicate_local(file.as_handle(), false).unwrap(); - assert!(Job::from_handle(not_job).is_err()); + assert!(ParentProcess::from_handle(duplicate(&file)).is_err()); + assert!(Job::from_handle(duplicate(&file)).is_err()); drop(host); } } diff --git a/src/lib.rs b/src/lib.rs index a91e6f1..01556a2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -26,6 +26,8 @@ mod options; #[cfg(windows)] mod plan; #[cfg(windows)] +mod resource; +#[cfg(windows)] #[allow(unsafe_code)] mod sys; #[cfg(windows)] diff --git a/src/resource.rs b/src/resource.rs new file mode 100644 index 0000000..6e0ce39 --- /dev/null +++ b/src/resource.rs @@ -0,0 +1,220 @@ +//! Handles typed by the object they name, and handle values typed by the table they belong to. +//! +//! A `BorrowedHandle` or an `isize` would let a thread handle reach a call for processes, +//! or a value of the creator's table join a list of the inheritor's values; these types make both a compile error. + +use std::fmt; +use std::marker::PhantomData; +use std::os::windows::io::{AsHandle, AsRawHandle, BorrowedHandle, OwnedHandle, RawHandle}; + +/// A process. +pub(crate) enum Process {} +/// A thread. +pub(crate) enum Thread {} +/// A Job object. +pub(crate) enum JobObject {} +/// One end of an anonymous pipe. +pub(crate) enum Pipe {} +/// A file, device, pipe, or console handle that a child receives as a standard stream. +pub(crate) enum Stream {} +/// A handle the caller supplied, whose kind the crate doesn't know. +pub(crate) enum Foreign {} + +/// An owned handle, in the current process's table, to an object of kind `K`. +pub(crate) struct Owned { + handle: OwnedHandle, + kind: PhantomData, +} + +impl Owned { + /// Adopts `handle` as a handle to an object of kind `K`. + /// + /// The caller has created the object with an API of that kind, or checked the kind with one. + pub(crate) const fn adopt(handle: OwnedHandle) -> Self { + Self { + handle, + kind: PhantomData, + } + } + + /// Borrows the handle with its kind. + pub(crate) fn borrow(&self) -> Borrowed<'_, K> { + Borrowed { + handle: self.handle.as_handle(), + kind: PhantomData, + } + } + + /// Gives up the kind, for a caller that receives a plain handle. + pub(crate) fn into_inner(self) -> OwnedHandle { + self.handle + } +} + +impl AsHandle for Owned { + fn as_handle(&self) -> BorrowedHandle<'_> { + self.handle.as_handle() + } +} + +impl AsRawHandle for Owned { + fn as_raw_handle(&self) -> RawHandle { + self.handle.as_raw_handle() + } +} + +impl fmt::Debug for Owned { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + self.handle.fmt(formatter) + } +} + +/// A borrowed handle, in the current process's table, to an object of kind `K`. +pub(crate) struct Borrowed<'a, K> { + handle: BorrowedHandle<'a>, + kind: PhantomData, +} + +impl<'a> Borrowed<'a, Foreign> { + /// Borrows a handle the caller supplied. + pub(crate) fn foreign(source: &'a T) -> Self { + Self::adopt(source.as_handle()) + } +} + +impl<'a, K> Borrowed<'a, K> { + /// Borrows `handle` as a handle to an object of kind `K`, which the caller has established. + pub(crate) const fn adopt(handle: BorrowedHandle<'a>) -> Self { + Self { + handle, + kind: PhantomData, + } + } + + /// Returns the plain borrowed handle. + pub(crate) const fn untyped(self) -> BorrowedHandle<'a> { + self.handle + } +} + +impl Clone for Borrowed<'_, K> { + fn clone(&self) -> Self { + *self + } +} + +impl Copy for Borrowed<'_, K> {} + +/// The table of the process that creates the child. +pub(crate) enum Creator {} +/// The table of the process the child inherits from: the creator, or the alternate parent. +pub(crate) enum Inheritor {} + +/// A handle value valid in the handle table `T`. +/// +/// Its layout is an `isize`'s, so a slice of values is the array an attribute list reads. +#[repr(transparent)] +pub(crate) struct Value { + raw: isize, + table: PhantomData, +} + +impl Value { + pub(crate) const fn as_raw(self) -> isize { + self.raw + } +} + +impl Value { + /// The value of a handle in the current process's table. + pub(crate) fn of(handle: BorrowedHandle<'_>) -> Self { + Self { + raw: crate::sys::handle_value(handle.as_raw_handle()), + table: PhantomData, + } + } + + /// The same value in the inheritor's table, which is the creator's when no alternate parent is named. + pub(crate) const fn inherited(self) -> Value { + Value { + raw: self.raw, + table: PhantomData, + } + } +} + +impl Value { + /// The value of a standard stream that a child doesn't receive. + pub(crate) const INVALID: Self = Self { + raw: crate::sys::INVALID_RAW_HANDLE, + table: PhantomData, + }; + + /// The value of a duplicate in the alternate parent's table. + pub(crate) const fn in_parent(raw: isize) -> Self { + Self { + raw, + table: PhantomData, + } + } +} + +impl Clone for Value { + fn clone(&self) -> Self { + *self + } +} + +impl Copy for Value {} + +impl PartialEq for Value { + fn eq(&self, other: &Self) -> bool { + self.raw == other.raw + } +} + +impl Eq for Value {} + +impl fmt::Display for Value { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + self.raw.fmt(formatter) + } +} + +impl fmt::Debug for Value { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + self.raw.fmt(formatter) + } +} + +#[cfg(test)] +mod tests { + use std::fs::File; + use std::mem::size_of; + + use super::*; + + #[test] + fn values_keep_the_layout_and_text_of_their_number() { + assert_eq!(size_of::>(), size_of::()); + let file = File::open("NUL").unwrap(); + let value = Value::::of(file.as_handle()); + assert_eq!(value.to_string(), value.as_raw().to_string()); + assert_eq!(format!("{value:?}"), value.as_raw().to_string()); + assert_eq!(value.inherited().as_raw(), value.as_raw()); + assert_eq!(Value::::in_parent(42).as_raw(), 42); + assert_eq!(Value::::INVALID.as_raw(), -1); + let copy = value; + assert_eq!(copy, value); + assert_ne!(Value::::in_parent(1), Value::in_parent(2)); + + let owned = Owned::::adopt(OwnedHandle::from(file)); + assert_eq!( + owned.borrow().untyped().as_raw_handle(), + owned.as_raw_handle() + ); + assert!(format!("{owned:?}").contains("OwnedHandle")); + let raw = owned.as_raw_handle(); + assert_eq!(owned.into_inner().as_raw_handle(), raw); + } +} diff --git a/src/sys.rs b/src/sys.rs index b8349b0..dac4e99 100644 --- a/src/sys.rs +++ b/src/sys.rs @@ -3,6 +3,7 @@ use std::cmp::Ordering; use std::ffi::{c_void, OsString}; use std::io; +use std::marker::PhantomData; use std::mem::{size_of, size_of_val, ManuallyDrop}; use std::os::windows::ffi::OsStringExt; use std::os::windows::io::{AsRawHandle, BorrowedHandle, FromRawHandle, OwnedHandle}; @@ -49,6 +50,9 @@ use crate::domain::attribute_rules::{self, SizeFault}; use crate::domain::creation_flag_rules::{creation_word, ConsoleFlag, PriorityFlag}; use crate::domain::environment_rules; use crate::handles::BorrowedPseudoConsole; +use crate::resource::{ + Borrowed, Creator, Foreign, Inheritor, JobObject, Owned, Pipe, Process, Stream, Thread, Value, +}; pub(crate) const INVALID_RAW_HANDLE: isize = -1; @@ -94,21 +98,23 @@ pub(crate) enum NullAccess { Write, } -pub(crate) fn duplicate_local( - source: BorrowedHandle<'_>, +/// Duplicates a handle in the current process, keeping its kind. +pub(crate) fn duplicate_local( + source: Borrowed<'_, K>, inheritable: bool, -) -> io::Result { +) -> io::Result> { #[cfg(test)] fault::check(fault::Call::DuplicateLocal)?; // SAFETY: `GetCurrentProcess` cannot fail and returns a pseudo-handle that stays valid and is never closed. let current = unsafe { GetCurrentProcess() }; duplicate_between( current, - raw(source), + raw(source.untyped()), current, inheritable, DUPLICATE_SAME_ACCESS, ) + .map(Owned::adopt) } fn duplicate_between( @@ -137,15 +143,15 @@ fn duplicate_between( owned(duplicate) } -#[derive(Debug)] +/// A duplicate in an alternate parent's table, which this process closes there. pub(crate) struct RemoteHandle<'a> { - process: BorrowedHandle<'a>, + process: Borrowed<'a, Process>, value: HANDLE, } impl RemoteHandle<'_> { - pub(crate) fn value(&self) -> isize { - handle_value(self.value) + pub(crate) fn value(&self) -> Value { + Value::in_parent(handle_value(self.value)) } /// Closes the remote value and reports a failure, which the drop would discard. @@ -160,7 +166,7 @@ impl RemoteHandle<'_> { // SAFETY: `GetCurrentProcess` cannot fail and returns a pseudo-handle that stays valid and is never closed. let current = unsafe { GetCurrentProcess() }; duplicate_between( - raw(self.process), + raw(self.process.untyped()), self.value, current, false, @@ -170,6 +176,15 @@ impl RemoteHandle<'_> { } } +impl std::fmt::Debug for RemoteHandle<'_> { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter + .debug_tuple("RemoteHandle") + .field(&handle_value(self.value)) + .finish() + } +} + /// Closes a remote value that no explicit close reached. impl Drop for RemoteHandle<'_> { fn drop(&mut self) { @@ -177,9 +192,9 @@ impl Drop for RemoteHandle<'_> { } } -pub(crate) fn duplicate_remote<'a>( - source: BorrowedHandle<'_>, - target_process: BorrowedHandle<'a>, +pub(crate) fn duplicate_remote<'a, K>( + source: Borrowed<'_, K>, + target_process: Borrowed<'a, Process>, inheritable: bool, ) -> io::Result> { #[cfg(test)] @@ -189,8 +204,8 @@ pub(crate) fn duplicate_remote<'a>( if unsafe { DuplicateHandle( GetCurrentProcess(), - raw(source), - raw(target_process), + raw(source.untyped()), + raw(target_process.untyped()), &mut value, 0, i32::from(inheritable), @@ -206,7 +221,7 @@ pub(crate) fn duplicate_remote<'a>( }) } -pub(crate) fn standard_handle(stream: StandardStream) -> io::Result> { +pub(crate) fn standard_handle(stream: StandardStream) -> io::Result>> { #[cfg(test)] fault::check(fault::Call::StandardHandle)?; let id = match stream { @@ -221,10 +236,10 @@ pub(crate) fn standard_handle(stream: StandardStream) -> io::Result::adopt(borrowed), false).map(Some) } -pub(crate) fn null_handle(access: NullAccess) -> io::Result { +pub(crate) fn null_handle(access: NullAccess) -> io::Result> { #[cfg(test)] fault::check(fault::Call::NullHandle)?; let name = [u16::from(b'N'), u16::from(b'U'), u16::from(b'L'), 0]; @@ -244,7 +259,7 @@ pub(crate) fn null_handle(access: NullAccess) -> io::Result { ptr::null_mut(), ) }; - owned(handle) + owned(handle).map(Owned::adopt) } fn null_share_mode() -> u32 { @@ -253,12 +268,13 @@ fn null_share_mode() -> u32 { .fold(0, std::ops::BitOr::bitor) } -pub(crate) struct Pipe { - pub(crate) parent: OwnedHandle, - pub(crate) child: OwnedHandle, +/// The two ends of an anonymous pipe: the caller's, and the one the child receives. +pub(crate) struct PipeEnds { + pub(crate) parent: Owned, + pub(crate) child: Owned, } -pub(crate) fn create_pipe(parent_reads: bool) -> io::Result { +pub(crate) fn create_pipe(parent_reads: bool) -> io::Result { #[cfg(test)] fault::check(fault::Call::CreatePipe)?; let mut read = ptr::null_mut(); @@ -270,56 +286,59 @@ pub(crate) fn create_pipe(parent_reads: bool) -> io::Result { // SAFETY: CreatePipe succeeded, so both handles are valid and distinct; they are adopted together. let (read, write) = unsafe { ( - OwnedHandle::from_raw_handle(read), - OwnedHandle::from_raw_handle(write), + Owned::adopt(OwnedHandle::from_raw_handle(read)), + Owned::adopt(OwnedHandle::from_raw_handle(write)), ) }; if parent_reads { - Ok(Pipe { + Ok(PipeEnds { parent: read, child: write, }) } else { - Ok(Pipe { + Ok(PipeEnds { parent: write, child: read, }) } } -pub(crate) fn open_parent_process(pid: u32) -> io::Result { +pub(crate) fn open_parent_process(pid: u32) -> io::Result> { #[cfg(test)] fault::check(fault::Call::OpenProcess)?; // SAFETY: OpenProcess has no pointer preconditions. let handle = unsafe { OpenProcess(PROCESS_CREATE_PROCESS | PROCESS_DUP_HANDLE, 0, pid) }; - owned(handle) + owned(handle).map(Owned::adopt) } -pub(crate) fn validate_process_handle(handle: BorrowedHandle<'_>) -> io::Result<()> { +/// Checks with a process API that a caller's handle names a process. +pub(crate) fn validate_process_handle(handle: Borrowed<'_, Foreign>) -> io::Result<()> { #[cfg(test)] fault::check(fault::Call::ValidateProcess)?; // SAFETY: the borrowed handle is valid for the query. - if unsafe { GetProcessId(raw(handle)) } == 0 { + if unsafe { GetProcessId(raw(handle.untyped())) } == 0 { Err(io::Error::last_os_error()) } else { Ok(()) } } -pub(crate) fn create_job() -> io::Result { +pub(crate) fn create_job() -> io::Result> { #[cfg(test)] fault::check(fault::Call::CreateJob)?; // SAFETY: null arguments request an unnamed Job with default security. - owned(unsafe { CreateJobObjectW(ptr::null(), ptr::null()) }) + owned(unsafe { CreateJobObjectW(ptr::null(), ptr::null()) }).map(Owned::adopt) } -pub(crate) fn validate_job_handle(handle: BorrowedHandle<'_>) -> io::Result<()> { - query_job_limits(handle).map(drop) +/// Checks with a Job API that a caller's handle names a Job. +pub(crate) fn validate_job_handle(handle: Borrowed<'_, Foreign>) -> io::Result<()> { + query_job_limits(handle.untyped()).map(drop) } -pub(crate) fn set_job_kill_on_close(handle: BorrowedHandle<'_>, enable: bool) -> io::Result<()> { +pub(crate) fn set_job_kill_on_close(job: Borrowed<'_, JobObject>, enable: bool) -> io::Result<()> { #[cfg(test)] fault::check(fault::Call::SetJob)?; + let handle = job.untyped(); let mut limits = query_job_limits(handle)?; if enable { limits.BasicLimitInformation.LimitFlags |= JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; @@ -365,18 +384,22 @@ fn query_job_limits( } } -pub(crate) fn assign_job(job: BorrowedHandle<'_>, process: BorrowedHandle<'_>) -> io::Result<()> { +/// Assigns a process the caller supplied, whose kind `AssignProcessToJobObject` checks. +pub(crate) fn assign_job( + job: Borrowed<'_, JobObject>, + process: Borrowed<'_, Foreign>, +) -> io::Result<()> { #[cfg(test)] fault::check(fault::Call::AssignJob)?; // SAFETY: both handles are valid for the call. - bool_result(unsafe { AssignProcessToJobObject(raw(job), raw(process)) }) + bool_result(unsafe { AssignProcessToJobObject(raw(job.untyped()), raw(process.untyped())) }) } -pub(crate) fn terminate_job(job: BorrowedHandle<'_>, exit_code: u32) -> io::Result<()> { +pub(crate) fn terminate_job(job: Borrowed<'_, JobObject>, exit_code: u32) -> io::Result<()> { #[cfg(test)] fault::check(fault::Call::TerminateJob)?; // SAFETY: the Job handle is valid for the call. - bool_result(unsafe { TerminateJobObject(raw(job), exit_code) }) + bool_result(unsafe { TerminateJobObject(raw(job.untyped()), exit_code) }) } /// The `usize` slots of the two-`DWORD` header that `JOBOBJECT_BASIC_PROCESS_ID_LIST` puts before its identifiers. @@ -395,7 +418,7 @@ const _: () = assert!( /// The identifiers of the Job's processes whose exit the Job hasn't recorded. /// /// A process leaves the list before its handle is signaled, so an empty list means that no process of the Job still runs. -pub(crate) fn job_process_ids(job: BorrowedHandle<'_>) -> io::Result> { +pub(crate) fn job_process_ids(job: Borrowed<'_, JobObject>) -> io::Result> { #[cfg(test)] fault::check(fault::Call::ListJob)?; let mut room = PROCESS_ID_LIST_ROOM; @@ -406,7 +429,7 @@ pub(crate) fn job_process_ids(job: BorrowedHandle<'_>) -> io::Result> // SAFETY: `buffer` is writable for `bytes` bytes and aligned for JOBOBJECT_BASIC_PROCESS_ID_LIST, and the returned-size pointer is null. let listed = unsafe { QueryInformationJobObject( - raw(job), + raw(job.untyped()), JobObjectBasicProcessIdList, buffer.as_mut_ptr().cast(), bytes, @@ -441,8 +464,8 @@ pub(crate) fn job_process_ids(job: BorrowedHandle<'_>) -> io::Result> /// Opens a process of the Job to wait for it, or returns `None` if it has exited or its identifier now names a process outside the Job. pub(crate) fn open_job_process( id: usize, - job: BorrowedHandle<'_>, -) -> io::Result> { + job: Borrowed<'_, JobObject>, +) -> io::Result>> { #[cfg(test)] fault::check(fault::Call::OpenJobProcess)?; let id = @@ -462,12 +485,18 @@ pub(crate) fn open_job_process( }; let mut member = 0; // SAFETY: both handles are valid for the query and `member` is writable. - bool_result(unsafe { IsProcessInJob(process.as_raw_handle(), raw(job), &mut member) })?; - Ok((member != 0).then_some(process)) + bool_result(unsafe { + IsProcessInJob(process.as_raw_handle(), raw(job.untyped()), &mut member) + })?; + Ok((member != 0).then_some(Owned::adopt(process))) } -pub(crate) struct AttributeList { +/// A `PROC_THREAD_ATTRIBUTE_LIST` whose attributes point into values that live for `'a`. +/// +/// Windows reads those values until `DeleteProcThreadAttributeList` runs, so the borrows keep them alive and in place for as long as the list exists. +pub(crate) struct AttributeList<'a> { storage: Box<[usize]>, + values: PhantomData<&'a ()>, } /// Zeroed storage of `words` words, or `OutOfMemory` when no allocation holds them. @@ -482,7 +511,7 @@ fn zeroed_words(words: usize) -> io::Result> { Ok(storage.into_boxed_slice()) } -impl AttributeList { +impl<'a> AttributeList<'a> { pub(crate) fn new(count: u32) -> io::Result { #[cfg(test)] fault::check(fault::Call::AttributeList)?; @@ -510,10 +539,14 @@ impl AttributeList { if unsafe { InitializeProcThreadAttributeList(pointer, count, 0, &mut actual) } == 0 { return Err(io::Error::last_os_error()); } - Ok(Self { storage }) + Ok(Self { + storage, + values: PhantomData, + }) } - pub(crate) fn set_handle_list(&mut self, handles: &[isize]) -> io::Result<()> { + /// Lists the handles the child inherits, by their values in the inheritor's table. + pub(crate) fn set_handle_list(&mut self, handles: &'a [Value]) -> io::Result<()> { self.update( PROC_THREAD_ATTRIBUTE_HANDLE_LIST, handles.as_ptr().cast(), @@ -521,15 +554,16 @@ impl AttributeList { ) } - pub(crate) fn set_parent(&mut self, parent: &isize) -> io::Result<()> { + /// Names the alternate parent by the value of its handle in the creator's table. + pub(crate) fn set_parent(&mut self, parent: &'a Value) -> io::Result<()> { self.update( PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, ptr::addr_of!(*parent).cast(), - size_of::(), + size_of::>(), ) } - pub(crate) fn set_mitigation(&mut self, words: &[u64; 2]) -> io::Result<()> { + pub(crate) fn set_mitigation(&mut self, words: &'a [u64; 2]) -> io::Result<()> { self.update( PROC_THREAD_ATTRIBUTE_MITIGATION_POLICY, words.as_ptr().cast(), @@ -537,7 +571,8 @@ impl AttributeList { ) } - pub(crate) fn set_jobs(&mut self, jobs: &[isize]) -> io::Result<()> { + /// Names the Jobs by the values of their handles in the creator's table. + pub(crate) fn set_jobs(&mut self, jobs: &'a [Value]) -> io::Result<()> { self.update( PROC_THREAD_ATTRIBUTE_JOB_LIST, jobs.as_ptr().cast(), @@ -548,7 +583,7 @@ impl AttributeList { /// Unlike the other attributes, `lpValue` is the `HPCON` value itself, not its address. pub(crate) fn set_pseudoconsole( &mut self, - pseudoconsole: BorrowedPseudoConsole<'_>, + pseudoconsole: BorrowedPseudoConsole<'a>, ) -> io::Result<()> { self.update( PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE, @@ -561,7 +596,7 @@ impl AttributeList { #[cfg(test)] fault::check(fault::Call::UpdateAttribute)?; let attribute = widen(attribute); - // SAFETY: the list is initialized, `value` points to `bytes` readable bytes or is the `HPCON` value, and the transaction keeps every backing allocation stable until CreateProcessW returns. + // SAFETY: the list is initialized, and `value` is the `HPCON` value or points to `bytes` readable bytes that the setter borrowed for `'a`, so they stay alive and in place until the drop deletes the list. if unsafe { UpdateProcThreadAttribute( self.pointer(), @@ -585,7 +620,7 @@ impl AttributeList { } } -impl Drop for AttributeList { +impl Drop for AttributeList<'_> { fn drop(&mut self) { #[cfg(test)] ATTRIBUTE_LIST_DROPS.fetch_add(1, std::sync::atomic::Ordering::Relaxed); @@ -606,16 +641,17 @@ impl BorrowedPseudoConsole<'_> { pub const unsafe fn borrow_raw(raw: isize) -> Self { Self { raw, - owner: std::marker::PhantomData, + owner: PhantomData, } } } +/// The standard handles of a child, by their values in the inheritor's table. #[derive(Clone, Copy)] pub(crate) struct StandardHandles { - pub(crate) stdin: isize, - pub(crate) stdout: isize, - pub(crate) stderr: isize, + pub(crate) stdin: Value, + pub(crate) stdout: Value, + pub(crate) stderr: Value, } #[derive(Clone, Copy)] @@ -635,12 +671,12 @@ pub(crate) struct ProcessRequest<'a> { pub(crate) policy: u32, pub(crate) console: ConsoleFlag, pub(crate) priority: PriorityFlag, - pub(crate) attributes: Option<&'a AttributeList>, + pub(crate) attributes: Option<&'a AttributeList<'a>>, } pub(crate) struct CreatedProcess { - pub(crate) process: OwnedHandle, - pub(crate) thread: OwnedHandle, + pub(crate) process: Owned, + pub(crate) thread: Owned, pub(crate) pid: u32, } @@ -690,8 +726,8 @@ pub(crate) fn create_process(request: &mut ProcessRequest<'_>) -> io::Result) -> io::Result) -> io::Result<()> { +pub(crate) fn wait_process(process: Borrowed<'_, Process>) -> io::Result<()> { #[cfg(test)] fault::check(fault::Call::WaitProcess)?; // SAFETY: the process handle is valid for the wait. - match unsafe { WaitForSingleObject(raw(process), INFINITE) } { + match unsafe { WaitForSingleObject(raw(process.untyped()), INFINITE) } { WAIT_OBJECT_0 => Ok(()), _ => Err(io::Error::last_os_error()), } } -pub(crate) fn try_wait_process(process: BorrowedHandle<'_>) -> io::Result { +pub(crate) fn try_wait_process(process: Borrowed<'_, Process>) -> io::Result { #[cfg(test)] fault::check(fault::Call::TryWaitProcess)?; // SAFETY: the process handle is valid for the query. - match unsafe { WaitForSingleObject(raw(process), 0) } { + match unsafe { WaitForSingleObject(raw(process.untyped()), 0) } { WAIT_OBJECT_0 => Ok(true), WAIT_TIMEOUT => Ok(false), _ => Err(io::Error::last_os_error()), @@ -1112,7 +1148,7 @@ pub(crate) mod test_support { } } -pub(crate) fn exit_status(process: BorrowedHandle<'_>) -> io::Result { +pub(crate) fn exit_status(process: Borrowed<'_, Process>) -> io::Result { use std::os::windows::process::ExitStatusExt; #[cfg(test)] @@ -1120,25 +1156,25 @@ pub(crate) fn exit_status(process: BorrowedHandle<'_>) -> io::Result let mut code = 0_u32; // SAFETY: `code` is writable and the process handle is valid. - if unsafe { GetExitCodeProcess(raw(process), &mut code) } == 0 { + if unsafe { GetExitCodeProcess(raw(process.untyped()), &mut code) } == 0 { Err(io::Error::last_os_error()) } else { Ok(ExitStatus::from_raw(code)) } } -pub(crate) fn terminate_process(process: BorrowedHandle<'_>, exit_code: u32) -> io::Result<()> { +pub(crate) fn terminate_process(process: Borrowed<'_, Process>, exit_code: u32) -> io::Result<()> { #[cfg(test)] fault::check(fault::Call::TerminateProcess)?; // SAFETY: the process handle is valid for the call. - bool_result(unsafe { TerminateProcess(raw(process), exit_code) }) + bool_result(unsafe { TerminateProcess(raw(process.untyped()), exit_code) }) } -pub(crate) fn resume_thread(thread: BorrowedHandle<'_>) -> io::Result { +pub(crate) fn resume_thread(thread: Borrowed<'_, Thread>) -> io::Result { #[cfg(test)] fault::check(fault::Call::ResumeThread)?; // SAFETY: the thread handle is valid for the call. - let previous = unsafe { ResumeThread(raw(thread)) }; + let previous = unsafe { ResumeThread(raw(thread.untyped())) }; if previous == u32::MAX { Err(io::Error::last_os_error()) } else { @@ -1146,7 +1182,7 @@ pub(crate) fn resume_thread(thread: BorrowedHandle<'_>) -> io::Result { } } -pub(crate) fn read_handle(handle: BorrowedHandle<'_>, buffer: &mut [u8]) -> io::Result { +pub(crate) fn read_handle(handle: Borrowed<'_, Pipe>, buffer: &mut [u8]) -> io::Result { #[cfg(test)] fault::check(fault::Call::ReadHandle)?; if buffer.is_empty() { @@ -1157,7 +1193,7 @@ pub(crate) fn read_handle(handle: BorrowedHandle<'_>, buffer: &mut [u8]) -> io:: // SAFETY: `buffer` is writable for `length` bytes, the handle is valid, and a null OVERLAPPED requests synchronous I/O. if unsafe { ReadFile( - raw(handle), + raw(handle.untyped()), buffer.as_mut_ptr(), length, &mut read, @@ -1176,7 +1212,7 @@ pub(crate) fn read_handle(handle: BorrowedHandle<'_>, buffer: &mut [u8]) -> io:: } } -pub(crate) fn write_handle(handle: BorrowedHandle<'_>, buffer: &[u8]) -> io::Result { +pub(crate) fn write_handle(handle: Borrowed<'_, Pipe>, buffer: &[u8]) -> io::Result { #[cfg(test)] fault::check(fault::Call::WriteHandle)?; if buffer.is_empty() { @@ -1187,7 +1223,7 @@ pub(crate) fn write_handle(handle: BorrowedHandle<'_>, buffer: &[u8]) -> io::Res // SAFETY: `buffer` is readable for `length` bytes, the handle is valid, and a null OVERLAPPED requests synchronous I/O. if unsafe { WriteFile( - raw(handle), + raw(handle.untyped()), buffer.as_ptr(), length, &mut written, @@ -1362,6 +1398,7 @@ fn bool_result(result: i32) -> io::Result<()> { #[cfg(test)] mod tests { use std::fs::File; + use std::io::{Read, Write}; use std::os::windows::ffi::OsStrExt; use std::os::windows::io::{AsHandle, AsRawHandle}; use std::path::PathBuf; @@ -1381,23 +1418,27 @@ mod tests { ); let readable_null = null_handle(NullAccess::Read)?; let writable_null = null_handle(NullAccess::Write)?; - assert_eq!(read_handle(readable_null.as_handle(), &mut [])?, 0); - assert_eq!(write_handle(writable_null.as_handle(), &[])?, 0); - assert_eq!(write_handle(writable_null.as_handle(), b"discard")?, 7); + let mut unread = [0_u8; 1]; + assert_eq!(File::from(readable_null.into_inner()).read(&mut unread)?, 0); + let mut discard = File::from(duplicate_local(writable_null.borrow(), false)?.into_inner()); + assert_eq!(discard.write(b"discard")?, 7); + drop(discard); let parent_reads = create_pipe(true)?; - assert_eq!(write_handle(parent_reads.child.as_handle(), b"a")?, 1); + assert_eq!(read_handle(parent_reads.parent.borrow(), &mut [])?, 0); + assert_eq!(write_handle(parent_reads.child.borrow(), &[])?, 0); + assert_eq!(write_handle(parent_reads.child.borrow(), b"a")?, 1); let mut byte = [0_u8; 1]; - assert_eq!(read_handle(parent_reads.parent.as_handle(), &mut byte)?, 1); + assert_eq!(read_handle(parent_reads.parent.borrow(), &mut byte)?, 1); assert_eq!(byte, [b'a']); let parent_writes = create_pipe(false)?; - assert_eq!(write_handle(parent_writes.parent.as_handle(), b"b")?, 1); - assert_eq!(read_handle(parent_writes.child.as_handle(), &mut byte)?, 1); + assert_eq!(write_handle(parent_writes.parent.borrow(), b"b")?, 1); + assert_eq!(read_handle(parent_writes.child.borrow(), &mut byte)?, 1); assert_eq!(byte, [b'b']); - let private = duplicate_local(writable_null.as_handle(), false)?; - let inheritable = duplicate_local(writable_null.as_handle(), true)?; + let private = duplicate_local(writable_null.borrow(), false)?; + let inheritable = duplicate_local(writable_null.borrow(), true)?; drop((private, inheritable)); let mut host_command = crate::Command::new("cmd.exe"); @@ -1412,8 +1453,12 @@ mod tests { drop(target); let before = process_handle_count(host.as_handle())?; 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); + let remote = duplicate_remote( + writable_null.borrow(), + Borrowed::adopt(host.as_handle()), + true, + )?; + assert_ne!(remote.value().as_raw(), 0); assert!(process_handle_count(host.as_handle())? > before); drop(remote); assert_eq!(process_handle_count(host.as_handle())?, before); @@ -1426,8 +1471,8 @@ mod tests { fn jobs_attributes_and_standard_handles_cover_all_ffi_shapes() -> io::Result<()> { let drops_before = ATTRIBUTE_LIST_DROPS.load(std::sync::atomic::Ordering::Relaxed); let job = create_job()?; - validate_job_handle(job.as_handle())?; - set_job_kill_on_close(job.as_handle(), true)?; + validate_job_handle(Borrowed::foreign(&job))?; + set_job_kill_on_close(job.borrow(), true)?; assert_ne!( query_job_limits(job.as_handle())? .BasicLimitInformation @@ -1435,7 +1480,7 @@ mod tests { & JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, 0 ); - set_job_kill_on_close(job.as_handle(), false)?; + set_job_kill_on_close(job.borrow(), false)?; assert_eq!( query_job_limits(job.as_handle())? .BasicLimitInformation @@ -1444,10 +1489,10 @@ mod tests { 0 ); let process = open_parent_process(std::process::id())?; - let inherited = duplicate_local(job.as_handle(), true)?; - let handles = [handle_value(inherited.as_raw_handle())]; - let jobs = [handle_value(job.as_raw_handle())]; - let parent = handle_value(process.as_raw_handle()); + let inherited = duplicate_local(job.borrow(), true)?; + let handles = [Value::::of(inherited.as_handle()).inherited()]; + let jobs = [Value::::of(job.as_handle())]; + let parent = Value::::of(process.as_handle()); let words = [1_u64, 0_u64]; let mut attributes = AttributeList::new(4)?; attributes.set_handle_list(&handles)?; @@ -1484,9 +1529,9 @@ mod tests { set_standard_handles( &mut ordinary, StartupStdio::Ordinary(StandardHandles { - stdin: 1, - stdout: 2, - stderr: 3, + stdin: Value::in_parent(1), + stdout: Value::in_parent(2), + stderr: Value::in_parent(3), }), ); assert_ne!(ordinary.StartupInfo.dwFlags & STARTF_USESTDHANDLES, 0); @@ -1544,8 +1589,8 @@ mod tests { assert!(is_valid_handle(file.as_raw_handle())); assert!(!is_valid_handle(ptr::null_mut())); assert!(!is_valid_handle(INVALID_HANDLE_VALUE)); - assert!(validate_process_handle(file.as_handle()).is_err()); - assert!(validate_job_handle(file.as_handle()).is_err()); + assert!(validate_process_handle(Borrowed::foreign(&file)).is_err()); + assert!(validate_job_handle(Borrowed::foreign(&file)).is_err()); Ok(()) } @@ -1562,30 +1607,30 @@ mod tests { current_process(), PROCESS_QUERY_LIMITED_INFORMATION, ); - assert!(wait_process(unwaitable.as_handle()).is_err()); - assert!(try_wait_process(unwaitable.as_handle()).is_err()); + assert!(wait_process(Borrowed::adopt(unwaitable.as_handle())).is_err()); + assert!(try_wait_process(Borrowed::adopt(unwaitable.as_handle())).is_err()); let unqueryable = test_support::duplicate_with_access(current_process(), PROCESS_SYNCHRONIZE); - assert!(exit_status(unqueryable.as_handle()).is_err()); + assert!(exit_status(Borrowed::adopt(unqueryable.as_handle())).is_err()); // SAFETY: `GetCurrentThread` cannot fail and returns a pseudo-handle valid for this call. let thread = unsafe { BorrowedHandle::borrow_raw(GetCurrentThread()) }; let unresumable = test_support::duplicate_with_access(thread, PROCESS_SYNCHRONIZE); - assert!(resume_thread(unresumable.as_handle()).is_err()); + assert!(resume_thread(Borrowed::adopt(unresumable.as_handle())).is_err()); let (reader, writer) = test_support::pipe(); - assert!(read_handle(writer.as_handle(), &mut [0_u8; 1]).is_err()); - assert!(write_handle(reader.as_handle(), b"x").is_err()); + assert!(read_handle(Borrowed::adopt(writer.as_handle()), &mut [0_u8; 1]).is_err()); + assert!(write_handle(Borrowed::adopt(reader.as_handle()), b"x").is_err()); let job = create_job()?; let query_only = test_support::duplicate_with_access(job.as_handle(), JOB_OBJECT_QUERY); - assert!(set_job_kill_on_close(query_only.as_handle(), true).is_err()); + assert!(set_job_kill_on_close(Borrowed::adopt(query_only.as_handle()), true).is_err()); assert!(AttributeList::new(u32::MAX).is_err()); + let jobs = [Value::::of(job.as_handle())]; + let words = [1_u64, 0_u64]; let mut full = AttributeList::new(1)?; - let jobs = [handle_value(job.as_raw_handle())]; full.set_jobs(&jobs)?; - let words = [1_u64, 0_u64]; assert!(full.set_mitigation(&words).is_err()); assert_eq!( diff --git a/src/transaction.rs b/src/transaction.rs index c0da846..e2c4c62 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -7,28 +7,32 @@ use std::ffi::{OsStr, OsString}; use std::io; use std::marker::PhantomData; use std::os::windows::ffi::{OsStrExt, OsStringExt}; -use std::os::windows::io::{AsHandle, AsRawHandle, BorrowedHandle, OwnedHandle}; +#[cfg(test)] +use std::os::windows::io::BorrowedHandle; +use std::os::windows::io::{AsHandle, OwnedHandle}; use std::path::{Path, PathBuf}; -use crate::child::{Child, SuspendedChild}; +use crate::child::{Child, ProcessHandle, SuspendedChild}; use crate::command::{Arg, Command, EnvOp, EnvValue}; use crate::domain::attribute_rules; use crate::domain::command_line_rules::{append_argument, append_program, NUL, QUOTE, SPACE}; use crate::domain::environment_rules; use crate::error::{InputField, Operation, Phase, Releases, Result, SpawnError, ValidationError}; -use crate::handles::{windows, Job, Stdio, StdioInner}; +use crate::handles::{windows, BorrowedPseudoConsole, Job, ParentProcess, Stdio, StdioInner}; use crate::plan::{validate_command, Running, Source, SpawnPlan, StandardIo, Streams, Suspended}; +use crate::resource::{Borrowed, Creator, Inheritor, Owned, Pipe, Process, Thread, Value}; use crate::sys::{self, NullAccess, StandardStream}; /// The UTF-16 units `CreateProcessW` accepts in `lpCommandLine`, its terminator included. const COMMAND_LINE_CAPACITY: usize = 32_767; /// Owns the suspended process between its creation and an explicit commit. -/// Dropping before commit terminates the process, which never ran. +/// +/// The child's process handle terminates the process when it drops before a commit resumes the main thread, +/// so dropping an uncommitted transaction ends a process that never ran. pub(crate) struct SpawnTransaction { - created: Option, - kill_job: Option, - stdio: Streams>, + child: Child, + main_thread: Owned, state: PhantomData, } @@ -36,11 +40,7 @@ impl SpawnTransaction { /// Borrows the created process's main thread. #[cfg(test)] pub(crate) fn main_thread(&self) -> BorrowedHandle<'_> { - self.created - .as_ref() - .expect("an uncommitted transaction owns its process") - .thread - .as_handle() + self.main_thread.as_handle() } } @@ -51,9 +51,8 @@ impl SpawnTransaction { /// /// Returns the first failure, together with the failures of the releases that followed it. /// A release that fails after the creation terminates the process, which never ran. - pub(crate) fn new<'options>(plan: &SpawnPlan<'_, 'options, M>) -> Result { - let parent: Option> = plan.options.parent.map(AsHandle::as_handle); - let mut transfer = HandleTransfer::new(parent); + pub(crate) fn new(plan: &SpawnPlan<'_, '_, M>) -> Result { + let mut transfer = HandleTransfer::new(plan.options.parent.map(ParentProcess::borrow)); let created = Self::create(plan, &mut transfer); let mut releases = Releases::default(); transfer.reclaim(&mut releases); @@ -63,156 +62,61 @@ impl SpawnTransaction { } } - #[allow(clippy::too_many_lines)] - fn create<'options>( - plan: &SpawnPlan<'_, 'options, M>, - transfer: &mut HandleTransfer<'options>, - ) -> Result { - let (stdio_values, stdio) = match &plan.stdio { - StandardIo::Ordinary(specs) => { - let prepared = prepare_standard_handles(specs, transfer)?; - let values = sys::StartupStdio::Ordinary(sys::StandardHandles { - stdin: prepared.stdin.child, - stdout: prepared.stdout.child, - stderr: prepared.stderr.child, - }); - let owners = Streams { - stdin: prepared.stdin.parent, - stdout: prepared.stdout.parent, - stderr: prepared.stderr.parent, - }; - (values, owners) - } - StandardIo::PseudoConsole => ( - sys::StartupStdio::PseudoConsole, - Streams { - stdin: None, - stdout: None, - stderr: None, - }, - ), - }; + fn create(plan: &SpawnPlan<'_, '_, M>, transfer: &mut HandleTransfer<'_>) -> Result { + let (stdio, pipes) = prepare_standard_io(&plan.stdio, transfer)?; let creation = &plan.command.creation; - let kill_job = if creation.owns_tree { - let job = Job::create()?; - job.set_kill_on_close(true)?; - Some(job) - } else { - None - }; - let mut job_values: Vec = plan - .options - .jobs - .iter() - .map(|job| sys::handle_value(job.as_handle().as_raw_handle())) - .collect(); - if let Some(job) = &kill_job { - job_values.push(sys::handle_value(job.as_handle().as_raw_handle())); - } - - let command_line = build_command_line(plan.command, transfer)?; + let kill_job = tree_job(creation.owns_tree)?; + let jobs = job_values(&plan.options.jobs, kill_job.as_ref()); + let mut command_line = build_command_line(plan.command, transfer)?; let environment = build_environment(plan.command, transfer)?; - let child_path = environment.path.as_deref(); let application = windows( Phase::Preparation, Operation::ResolveExecutable, - resolve_executable(&plan.command.program, child_path), + resolve_executable(&plan.command.program, environment.path.as_deref()), )?; - let current_dir = plan - .command - .cwd - .as_ref() - .map(|path| wide_nul(path.as_os_str())) - .transpose() - .map_err(|_nul| { - SpawnError::from(ValidationError::InteriorNul { - field: InputField::CurrentDirectory, - }) - })?; - - let inherited_values = transfer.inherited_values().to_vec().into_boxed_slice(); - let parent_value = transfer - .parent() - .map(|handle| Box::new(sys::handle_value(handle.as_raw_handle()))); - let mitigation_words = creation.mitigation.words(); - let mitigation_value = (mitigation_words != [0, 0]).then(|| Box::new(mitigation_words)); - let job_values = job_values.into_boxed_slice(); - let pseudoconsole = plan.options.pseudoconsole; - - let attribute_count = attribute_rules::count(&[ - !inherited_values.is_empty(), - parent_value.is_some(), - mitigation_value.is_some(), - !job_values.is_empty(), - pseudoconsole.is_some(), - ]); - let mut attributes = if attribute_count == 0 { - None - } else { - Some(configure(sys::AttributeList::new(attribute_count))?) - }; - if let Some(list) = &mut attributes { - if !inherited_values.is_empty() { - configure(list.set_handle_list(&inherited_values))?; - } - if let Some(value) = &parent_value { - configure(list.set_parent(value))?; - } - if let Some(value) = &mitigation_value { - configure(list.set_mitigation(value))?; - } - if !job_values.is_empty() { - configure(list.set_jobs(&job_values))?; - } - if let Some(value) = pseudoconsole { - configure(list.set_pseudoconsole(value))?; - } - } + let current_dir = current_directory(plan.command)?; - let mut command_line = command_line; - let mut request = sys::ProcessRequest { - application: &application, - command_line: &mut command_line, - environment: environment.block.as_deref(), - current_dir: current_dir.as_deref(), - stdio: stdio_values, - inherit_handles: !inherited_values.is_empty(), - policy: creation.policy(), - console: creation.console_flag(), - priority: creation.priority_flag(), - attributes: attributes.as_ref(), - }; + let parent = transfer.parent_value(); + let mitigation = creation.mitigation.words(); + let inherited = transfer.inherited_values(); + let attributes = attribute_list(&AttributeValues { + inherited, + parent: parent.as_ref(), + mitigation: (mitigation != [0, 0]).then_some(&mitigation), + jobs: &jobs, + pseudoconsole: plan.options.pseudoconsole, + })?; let created = windows( Phase::Creation, Operation::CreateProcess, - sys::create_process(&mut request), + sys::create_process(&mut sys::ProcessRequest { + application: &application, + command_line: &mut command_line, + environment: environment.block.as_deref(), + current_dir: current_dir.as_deref(), + stdio, + inherit_handles: !inherited.is_empty(), + policy: creation.policy(), + console: creation.console_flag(), + priority: creation.priority_flag(), + attributes: attributes.as_ref(), + }), )?; drop(attributes); Ok(Self { - created: Some(created), - kill_job, - stdio, + child: Child::new( + ProcessHandle::unstarted(created.process), + created.pid, + kill_job, + pipes.stdin, + pipes.stdout, + pipes.stderr, + ), + main_thread: created.thread, state: PhantomData, }) } - - #[allow(clippy::expect_used)] - fn commit_parts(mut self) -> (Child, OwnedHandle) { - let created = self - .created - .take() - .expect("an uncommitted transaction owns its process"); - let child = Child::new( - created.process, - created.pid, - self.kill_job.take(), - self.stdio.stdin.take(), - self.stdio.stdout.take(), - self.stdio.stderr.take(), - ); - (child, created.thread) - } } impl SpawnTransaction { @@ -222,41 +126,125 @@ impl SpawnTransaction { /// /// Returns a failed resume, or a suspend count other than one; the process is terminated then. pub(crate) fn commit_child(self) -> Result { - if let Some(created) = &self.created { - let previous = windows( - Phase::Resume, - Operation::ResumeThread, - sys::resume_thread(created.thread.as_handle()), - )?; - if previous != 1 { - return Err(SpawnError::unexpected_suspend_count(previous)); - } - } - let (child, thread) = self.commit_parts(); - drop(thread); - Ok(child) + SuspendedChild::new(self.child, self.main_thread).resume() } } impl SpawnTransaction { pub(crate) fn commit_suspended(self) -> SuspendedChild { - let (child, thread) = self.commit_parts(); - SuspendedChild::new(child, thread) + SuspendedChild::new(self.child, self.main_thread) + } +} + +/// The private Job of a child that owns its tree, which ends the tree when its last handle closes. +fn tree_job(owns_tree: bool) -> Result> { + if !owns_tree { + return Ok(None); + } + let job = Job::create()?; + job.set_kill_on_close(true)?; + Ok(Some(job)) +} + +/// The values, in the creator's table, of the Jobs the child joins: the caller's, then its private one. +fn job_values(jobs: &[&Job], kill_job: Option<&Job>) -> Vec> { + jobs.iter() + .copied() + .chain(kill_job) + .map(|job| Value::::of(job.as_handle())) + .collect() +} + +/// The child's working directory as a terminated wide string, when the command sets one. +fn current_directory(command: &Command) -> Result>> { + command + .cwd + .as_ref() + .map(|path| wide_nul(path.as_os_str())) + .transpose() + .map_err(|_nul| { + SpawnError::from(ValidationError::InteriorNul { + field: InputField::CurrentDirectory, + }) + }) +} + +/// What the attributes of a spawn point to. +/// +/// The attribute list borrows each value, so every one outlives the list. +struct AttributeValues<'a> { + inherited: &'a [Value], + parent: Option<&'a Value>, + mitigation: Option<&'a [u64; 2]>, + jobs: &'a [Value], + pseudoconsole: Option>, +} + +/// Builds the attribute list of `values`, or returns `None` when the spawn sets no attribute. +fn attribute_list<'a>(values: &AttributeValues<'a>) -> Result>> { + let count = attribute_rules::count(&[ + !values.inherited.is_empty(), + values.parent.is_some(), + values.mitigation.is_some(), + !values.jobs.is_empty(), + values.pseudoconsole.is_some(), + ]); + if count == 0 { + return Ok(None); + } + let mut list = configure(sys::AttributeList::new(count))?; + if !values.inherited.is_empty() { + configure(list.set_handle_list(values.inherited))?; } + if let Some(parent) = values.parent { + configure(list.set_parent(parent))?; + } + if let Some(words) = values.mitigation { + configure(list.set_mitigation(words))?; + } + if !values.jobs.is_empty() { + configure(list.set_jobs(values.jobs))?; + } + if let Some(pseudoconsole) = values.pseudoconsole { + configure(list.set_pseudoconsole(pseudoconsole))?; + } + Ok(Some(list)) } -impl Drop for SpawnTransaction { - fn drop(&mut self) { - if let Some(created) = &self.created { - drop(sys::terminate_process(created.process.as_handle(), 1)); +/// Lowers the child's standard handles, and returns them with the caller's ends of the child's pipes. +fn prepare_standard_io( + stdio: &StandardIo<&Stdio>, + transfer: &mut HandleTransfer<'_>, +) -> Result<(sys::StartupStdio, Streams>>)> { + match stdio { + StandardIo::Ordinary(specs) => { + let prepared = prepare_standard_handles(specs, transfer)?; + let values = sys::StartupStdio::Ordinary(sys::StandardHandles { + stdin: prepared.stdin.child, + stdout: prepared.stdout.child, + stderr: prepared.stderr.child, + }); + let pipes = Streams { + stdin: prepared.stdin.parent, + stdout: prepared.stdout.parent, + stderr: prepared.stderr.parent, + }; + Ok((values, pipes)) } - drop(self.kill_job.take()); + StandardIo::PseudoConsole => Ok(( + sys::StartupStdio::PseudoConsole, + Streams { + stdin: None, + stdout: None, + stderr: None, + }, + )), } } struct PreparedStdio { - child: isize, - parent: Option, + child: Value, + parent: Option>, } fn prepare_standard_handles( @@ -286,7 +274,7 @@ fn prepare_stdio( StdioInner::Null => prepare_null(stream, transfer), StdioInner::Piped => prepare_pipe(stream, transfer), StdioInner::Owned(handle) => Ok(PreparedStdio { - child: transfer.lower(handle.as_handle())?, + child: transfer.lower(handle.borrow())?, parent: None, }), }, @@ -304,11 +292,11 @@ fn prepare_inherit( )?; match standard { Some(handle) => Ok(PreparedStdio { - child: transfer.lower(handle.as_handle())?, + child: transfer.lower(handle.borrow())?, parent: None, }), None => Ok(PreparedStdio { - child: sys::INVALID_RAW_HANDLE, + child: Value::INVALID, parent: None, }), } @@ -328,7 +316,7 @@ fn prepare_null( sys::null_handle(access), )?; Ok(PreparedStdio { - child: transfer.lower(handle.as_handle())?, + child: transfer.lower(handle.borrow())?, parent: None, }) } @@ -342,22 +330,24 @@ fn prepare_pipe( Operation::CreatePipe, sys::create_pipe(!matches!(stream, StandardStream::Input)), )?; - let child = transfer.lower(pipe.child.as_handle())?; + let child = transfer.lower(pipe.child.borrow())?; Ok(PreparedStdio { child, parent: Some(pipe.parent), }) } +/// The inheritable duplicates of one spawn, in the table the child inherits from, and their values. struct HandleTransfer<'a> { - parent: Option>, + parent: Option>, + /// Duplicates in the creator's table, which closes them when dropped. local: Vec, remote: Vec>, - inherited: Vec, + inherited: Vec>, } impl<'a> HandleTransfer<'a> { - const fn new(parent: Option>) -> Self { + const fn new(parent: Option>) -> Self { Self { parent, local: Vec::new(), @@ -366,7 +356,8 @@ impl<'a> HandleTransfer<'a> { } } - fn lower(&mut self, source: BorrowedHandle<'_>) -> Result { + /// Duplicates `source` inheritably into the inheritor's table, and returns the duplicate's value there. + fn lower(&mut self, source: Borrowed<'_, K>) -> Result> { let value = if let Some(parent) = self.parent { let handle = windows( Phase::Preparation, @@ -382,8 +373,8 @@ impl<'a> HandleTransfer<'a> { Operation::DuplicateLocalHandle, sys::duplicate_local(source, true), )?; - let value = sys::handle_value(handle.as_raw_handle()); - self.local.push(handle); + let value = Value::::of(handle.as_handle()).inherited(); + self.local.push(handle.into_inner()); value }; self.inherited.push(value); @@ -402,11 +393,13 @@ impl<'a> HandleTransfer<'a> { drop(self.local); } - const fn parent(&self) -> Option> { + /// The alternate parent's value in the creator's table, when the spawn names one. + fn parent_value(&self) -> Option> { self.parent + .map(|parent| Value::::of(parent.untyped())) } - fn inherited_values(&self) -> &[isize] { + fn inherited_values(&self) -> &[Value] { &self.inherited } } @@ -476,7 +469,7 @@ fn build_environment(command: &Command, transfer: &mut HandleTransfer<'_>) -> Re let value = match value { EnvValue::Text(value) => value.clone(), EnvValue::Handle(handle) => { - OsString::from(transfer.lower(handle.as_handle())?.to_string()) + OsString::from(transfer.lower(handle.borrow())?.to_string()) } }; match map.entry(EnvKey::new(key)) { @@ -587,7 +580,7 @@ fn build_command_line(command: &Command, transfer: &mut HandleTransfer<'_>) -> R Arg::Text(text) => append_argument(&mut result, &wide(text)), Arg::Raw(text) => result.extend(text.encode_wide()), Arg::Handle(handle) => { - let value = transfer.lower(handle.as_handle())?.to_string(); + let value = transfer.lower(handle.borrow())?.to_string(); append_argument(&mut result, &wide(OsStr::new(&value))); } } @@ -1194,10 +1187,9 @@ mod tests { ) .unwrap(); let transaction = SpawnTransaction::new(&plan).unwrap(); - let created = transaction.created.as_ref().unwrap(); - let mut process = ProcessExitGuard::watch(created.process.as_handle()); + let mut process = ProcessExitGuard::watch(transaction.child.as_handle()); assert_eq!( - suspend_count(created.thread.as_handle()), + suspend_count(transaction.main_thread()), 1, "a running spawn stays suspended until its commit" ); @@ -1225,9 +1217,8 @@ mod tests { ) .unwrap(); let transaction = SpawnTransaction::new(&plan).unwrap(); - let created = transaction.created.as_ref().unwrap(); - let mut process = ProcessExitGuard::watch(created.process.as_handle()); - let thread = duplicate(created.thread.as_handle()); + let mut process = ProcessExitGuard::watch(transaction.child.as_handle()); + let thread = duplicate(transaction.main_thread()); drop(transaction); tempt_resume(thread.as_handle()); assert_eq!( @@ -1248,9 +1239,8 @@ mod tests { ) .unwrap(); let transaction = SpawnTransaction::new(&plan).unwrap(); - let created = transaction.created.as_ref().unwrap(); - let mut process = ProcessExitGuard::watch(created.process.as_handle()); - let thread = duplicate(created.thread.as_handle()); + let mut process = ProcessExitGuard::watch(transaction.child.as_handle()); + let thread = duplicate(transaction.main_thread()); drop(transaction.commit_suspended()); tempt_resume(thread.as_handle()); assert_eq!( diff --git a/xtask/src/gates.rs b/xtask/src/gates.rs index dd4a131..f974bcd 100644 --- a/xtask/src/gates.rs +++ b/xtask/src/gates.rs @@ -33,25 +33,6 @@ const EXCEPTIONS: &[Exception] = &[ count: 1, reason: "dword_const is evaluated in constants, where an oversized value fails the build", }, - Exception { - file: "src/child.rs", - lint: "clippy::expect_used", - count: 1, - reason: "a SuspendedChild owns its process until resume consumes it", - }, - Exception { - file: "src/transaction.rs", - lint: "clippy::expect_used", - count: 1, - reason: "an uncommitted transaction owns its process until commit consumes it", - }, - Exception { - file: "src/transaction.rs", - lint: "clippy::too_many_lines", - count: 1, - reason: - "one function acquires every creation resource, so rollback has one owner (ADR 0007)", - }, Exception { file: "tests/support/mod.rs", lint: "clippy::disallowed_methods", From 257b4c4ca6b723bd8b60dadefdeb464e28b4ec3c Mon Sep 17 00:00:00 2001 From: Yasunobu <42543015+P4suta@users.noreply.github.com> Date: Tue, 13 Oct 2026 21:54:40 +0900 Subject: [PATCH 2/6] build: refuse trait objects and shared state in library code `cargo xtask gates` refuses a trait object outside the signature of `Error::source`, in every file. In library code it also refuses shared ownership, cell types, locks, atomics, `static` items, one-time initialization, `thread_local`, runtime reflection, and hash-based maps and sets. Items under `cfg(test)` or `cfg(kani)`, and `src/failure_tests.rs`, keep their thread-local state, counters, and downcasts. Scanner tests cover each rule with refused and accepted code. `deny.toml` bans the crates that PR #14 banned, which erase errors, erase serialization or futures, register items at run time, or keep implicit global state. ADR 0016 records these rules and the explicit spawn state, and credits the ADR 0009 of #14. Refs #35 --- deny.toml | 18 ++ docs/adr/0007-spawn-transaction.md | 2 +- ...pository-laws-are-enforced-mechanically.md | 2 +- .../0016-spawn-state-is-explicit-and-typed.md | 40 +++ xtask/src/gates.rs | 295 ++++++++++++++++++ 5 files changed, 355 insertions(+), 2 deletions(-) create mode 100644 docs/adr/0016-spawn-state-is-explicit-and-typed.md diff --git a/deny.toml b/deny.toml index d651535..7ffbb89 100644 --- a/deny.toml +++ b/deny.toml @@ -33,6 +33,24 @@ multiple-versions-include-dev = true skip = [] skip-tree = [] wildcards = "deny" +# ADR 0016: no erased errors, erased dispatch, runtime registration, or implicit global state. +deny = [ + { crate = "anyhow", reason = "erases error types" }, + { crate = "arc-swap", reason = "shares mutable global state" }, + { crate = "async-trait", reason = "erases futures behind trait objects" }, + { crate = "color-eyre", reason = "erases error types" }, + { crate = "dashmap", reason = "shares mutable state and orders by hash" }, + { crate = "downcast-rs", reason = "dispatches on runtime types" }, + { crate = "erased-serde", reason = "erases serialization behind trait objects" }, + { crate = "eyre", reason = "erases error types" }, + { crate = "inventory", reason = "registers items at run time" }, + { crate = "lazy_static", reason = "initializes global state implicitly" }, + { crate = "linkme", reason = "registers items at link time" }, + { crate = "once_cell", reason = "initializes global state implicitly" }, + { crate = "parking_lot", reason = "shares mutable state behind locks" }, + { crate = "serde_traitobject", reason = "serializes trait objects" }, + { crate = "typetag", reason = "registers trait-object types at run time" }, +] [sources] allow-git = [] diff --git a/docs/adr/0007-spawn-transaction.md b/docs/adr/0007-spawn-transaction.md index 341d5f4..dd057f2 100644 --- a/docs/adr/0007-spawn-transaction.md +++ b/docs/adr/0007-spawn-transaction.md @@ -1,6 +1,6 @@ # 0007 — Process creation is transactional -Status: accepted (2026-08-02) +Status: accepted (2026-08-02); ADR 0016 moves the rollback of the created process into its handle ## Context diff --git a/docs/adr/0013-repository-laws-are-enforced-mechanically.md b/docs/adr/0013-repository-laws-are-enforced-mechanically.md index 3d569a8..a49cd3d 100644 --- a/docs/adr/0013-repository-laws-are-enforced-mechanically.md +++ b/docs/adr/0013-repository-laws-are-enforced-mechanically.md @@ -1,6 +1,6 @@ # 0013 — Repository laws are enforced mechanically -Status: accepted (2026-09-25) +Status: accepted (2026-09-25); ADR 0016 extends the trait-object rule to every `dyn` ## Context diff --git a/docs/adr/0016-spawn-state-is-explicit-and-typed.md b/docs/adr/0016-spawn-state-is-explicit-and-typed.md new file mode 100644 index 0000000..6908ab5 --- /dev/null +++ b/docs/adr/0016-spawn-state-is-explicit-and-typed.md @@ -0,0 +1,40 @@ +# 0016 — Spawn state is explicit and typed + +Status: accepted (2026-10-13); moves the rollback of ADR 0007 into the process handle, and extends the trait-object rule of ADR 0013 + +## Context + +A suspended child and an uncommitted transaction kept their process in an `Option`, so that a consuming method could move it out past the type's `Drop`. +Each read of it needed an `expect`, and the lint registry excused both. +One function acquired every creation resource, and the registry excused its length too. + +Handles travelled as `BorrowedHandle`, and handle values as `isize`. +Nothing stopped a thread handle from reaching a call for processes, or a value of the creator's table from joining the values an alternate parent's child inherits. +The attribute list held pointers to values that the transaction kept alive by convention. + +PR #14 recorded in its ADR 0009 that trait objects, runtime reflection, shared interior mutability, global state, and hash-ordered collections hide what the compiler needs to check a spawn. +Main dropped #14, and with it those rules. + +## Decision + +- The process handle of a spawn carries its rollback. + Until the main thread runs, dropping the handle terminates the process. + A transaction, a `SuspendedChild`, and a `Child` own their parts directly, and `resume` and the commits move them out without an `Option`. + A running spawn commits by resuming a `SuspendedChild`, so both paths share one resume. +- The creation splits into functions for the standard I/O, the private Job, the Job list, the working directory, and the attribute list. +- Each handle names its object type. + `Owned` and `Borrowed<'a, K>` name a process, a thread, a Job, a pipe end, a standard stream, or a handle the caller supplied, and each wrapper in `sys` takes the type its Win32 call needs. +- Handle values carry the table they belong to. + `Value` names the alternate parent and the Jobs, and `Value` names the inherited handles and the standard handles. +- `AttributeList<'a>` borrows the values its attributes point to, so the compiler keeps them alive and in place until `DeleteProcThreadAttributeList` runs. +- `cargo xtask gates` refuses a trait object outside the signature of `Error::source`, in every file. + In library code it also refuses `Rc`, `Arc`, `Weak`, cell types, locks, atomics, `static` items, one-time initialization, `thread_local`, `Any`, `TypeId`, type names, and hash-based maps and sets. + Items under `cfg(test)` or `cfg(kani)`, and `src/failure_tests.rs`, keep their thread-local state, counters, and downcasts. +- `deny.toml` bans the crates #14 banned, which erase errors, erase serialization or futures, register items at run time, or keep implicit global state. + +## Consequences + +- The registry holds no `expect_used` or `too_many_lines` exception. +- A handle of the wrong kind, a value of the wrong table, or an attribute value that dies before its list fails to compile. +- Concurrency in the library needs a typed boundary and a model before it can pass the gate. +- The output readers of `Child::wait_with_output` stay threads that own what they read, so they share no state. diff --git a/xtask/src/gates.rs b/xtask/src/gates.rs index f974bcd..496de59 100644 --- a/xtask/src/gates.rs +++ b/xtask/src/gates.rs @@ -110,6 +110,54 @@ const DELAY_COMMANDS: [&str; 3] = [ concat!("timeout", " /t"), ]; +/// Library files that only tests compile; the library rules skip them as they skip test-only items. +const TEST_ONLY_FILES: [&str; 1] = ["src/failure_tests.rs"]; + +/// Why library code holds no shared or interior-mutable state; atomics, whose names start with `Atomic`, share it. +const SHARED_STATE: &str = + "shares or mutates state outside the borrow checker; pass owned values and borrows"; + +/// Names library code doesn't use, each set with its reason (ADR 0016). +const LIBRARY_BANS: [(&[&str], &str); 4] = [ + ( + &[ + "Rc", + "Arc", + "Weak", + "Cell", + "RefCell", + "OnceCell", + "LazyCell", + "UnsafeCell", + "Mutex", + "RwLock", + "Condvar", + "Barrier", + "atomic", + ], + SHARED_STATE, + ), + ( + &[ + "static", + "Once", + "OnceLock", + "LazyLock", + "lazy_static", + "thread_local", + ], + "keeps global state; pass the value to what needs it", + ), + ( + &["Any", "TypeId", "type_name", "type_name_of_val"], + "inspects a type at run time; dispatch on an enum or a trait bound", + ), + ( + &["HashMap", "HashSet", "RandomState"], + "iterates in a random order; use BTreeMap or BTreeSet", + ), +]; + /// A rule violation. #[derive(Debug, PartialEq, Eq)] pub(crate) struct Finding { @@ -209,6 +257,8 @@ fn check_exceptions(allows: &[Allow], exceptions: &[Exception]) -> Vec /// The offset of the token after the next one: `dyn` after `Box <`, the arguments after `name (`, and the bracket after `# !`. const AFTER_NEXT: usize = 2; +/// Two adjacent tokens, such as `fn source` read backwards. +const PAIR: usize = 2; /// The length of each comment delimiter: `//`, `/*`, and `*/`. const DELIMITER: usize = 2; /// The length of an escape: a backslash and the character it escapes. @@ -256,6 +306,8 @@ fn scan(file: &str, source: &str) -> Scan { .iter() .filter(|(_, token)| !matches!(token, Token::Comment { .. })) .collect(); + let guarded = test_only(&code); + let library = file.starts_with("src/") && !TEST_ONLY_FILES.contains(&file); let mut allows = Vec::new(); for (index, (line, token)) in code.iter().enumerate() { let next = |offset: usize| { @@ -264,6 +316,11 @@ fn scan(file: &str, source: &str) -> Scan { }; match token { Token::Ident(name) => { + if !guarded.get(index).copied().unwrap_or_default() { + if let Some(rule) = explicit_state_rule(&code, index, library) { + finding(*line, rule); + } + } if is_time_name(name) { finding( *line, @@ -336,6 +393,144 @@ fn scan(file: &str, source: &str) -> Scan { Scan { findings, allows } } +/// Returns the ADR 0016 rule that the identifier at `index` breaks. +/// +/// A trait object is refused in any file, and the library bans only in library code. +fn explicit_state_rule(code: &[&(usize, Token)], index: usize, library: bool) -> Option { + let before = |offset: usize| { + index + .checked_sub(offset) + .and_then(|at| code.get(at)) + .map(|(_, token)| token) + }; + let Some(Token::Ident(name)) = before(0) else { + return None; + }; + if name == "dyn" { + let boxed = before(AFTER_NEXT) == Some(&Token::Ident("Box".to_owned())) + && before(1) == Some(&Token::Punct('<')); + return (!boxed && !in_error_source(code, index)).then(|| { + "a trait object hides which type runs; own a closed set with an enum or an open one with a generic (ADR 0016)" + .to_owned() + }); + } + let lifetime = before(1) == Some(&Token::Punct('\'')); + if !library || lifetime { + return None; + } + library_ban(name).map(|reason| format!("`{name}` {reason} (ADR 0016)")) +} + +/// Returns why library code doesn't use `name`, if it doesn't. +fn library_ban(name: &str) -> Option<&'static str> { + if name.starts_with("Atomic") { + return Some(SHARED_STATE); + } + LIBRARY_BANS + .iter() + .find(|(names, _)| names.contains(&name)) + .map(|(_, reason)| *reason) +} + +/// Returns true if the `dyn` at `index` names the `Error` trait in the signature of a `source` method, the `Error::source` contract. +fn in_error_source(code: &[&(usize, Token)], index: usize) -> bool { + let names_error = code + .iter() + .skip(index.saturating_add(1)) + .map(|(_, token)| token) + .take_while(|token| matches!(token, Token::Ident(_) | Token::Punct(':'))) + .filter_map(|token| match token { + Token::Ident(name) => Some(name.as_str()), + Token::Literal(_) | Token::Str(_) | Token::Punct(_) | Token::Comment { .. } => None, + }) + .last() + == Some("Error"); + let signature: Vec<&Token> = code + .iter() + .take(index) + .rev() + .map(|(_, token)| token) + .take_while(|token| !matches!(token, Token::Punct('{' | '}' | ';'))) + .collect(); + names_error + && signature.windows(PAIR).any(|pair| { + matches!(pair, [Token::Ident(name), Token::Ident(keyword)] if name == "source" && keyword == "fn") + }) +} + +/// Marks each token of an item, statement, or field that a test-only `cfg` attribute guards. +fn test_only(code: &[&(usize, Token)]) -> Vec { + let mut guarded = vec![false; code.len()]; + let mut index = 0_usize; + while index < code.len() { + let open = index.saturating_add(1); + let attribute = matches!(code.get(index), Some((_, Token::Punct('#')))) + && matches!(code.get(open), Some((_, Token::Punct('[')))); + if attribute && is_test_only_cfg(&bracketed(code, open)) { + let end = item_end(code, open); + for flag in guarded.iter_mut().take(end).skip(index) { + *flag = true; + } + index = end; + } else { + index = open; + } + } + guarded +} + +/// Returns true if an attribute's tokens are `cfg(test)`, `cfg(kani)`, or `cfg(any(…))` over those two. +fn is_test_only_cfg(content: &[Token]) -> bool { + let [Token::Ident(cfg), Token::Punct('('), predicate @ .., Token::Punct(')')] = content else { + return false; + }; + let members = match predicate { + [Token::Ident(any), Token::Punct('('), members @ .., Token::Punct(')')] if any == "any" => { + members + } + single => single, + }; + cfg == "cfg" + && members.split(|token| *token == Token::Punct(',')).all( + |member| matches!(member, [Token::Ident(name)] if name == "test" || name == "kani"), + ) +} + +/// Returns the index after what the attribute whose `[` is at `open` guards. +/// +/// That ends with a `;` or `,` outside brackets, with the `}` that closes its body, or before the bracket that closes the enclosing block. +fn item_end(code: &[&(usize, Token)], open: usize) -> usize { + let mut depth = 0_usize; + let mut in_attribute = true; + for (index, (_, token)) in code.iter().enumerate().skip(open) { + match token { + Token::Punct('(' | '[' | '{') => depth = depth.saturating_add(1), + Token::Punct(close @ (')' | ']' | '}')) => { + if depth == 0 { + return index; + } + depth = depth.saturating_sub(1); + if depth == 0 { + if in_attribute { + in_attribute = false; + } else if *close == '}' { + return index.saturating_add(1); + } + } + } + Token::Punct(';' | ',') if depth == 0 && !in_attribute => { + return index.saturating_add(1); + } + Token::Ident(_) + | Token::Literal(_) + | Token::Str(_) + | Token::Punct(_) + | Token::Comment { .. } => {} + } + } + code.len() +} + fn is_time_name(name: &str) -> bool { let lower = name.to_ascii_lowercase(); let named = TIME_NAMES.contains(&name) @@ -791,6 +986,106 @@ mod tests { assert_clean("struct S { f: Box }"); } + fn rules_in(file: &str, source: &str) -> Vec { + scan(file, source) + .findings + .into_iter() + .map(|finding| finding.rule) + .collect() + } + + #[test] + fn trait_objects_are_refused_outside_the_error_source_contract() { + for source in [ + "fn f(x: &dyn Fn()) {}", + "fn cause(&self) -> Option<&dyn Error> { None }", + "fn source(&self) -> Option<&(dyn Display + 'static)> { None }", + "fn source(&self) {} fn g(x: &dyn Error) {}", + "m!(dyn T);", + "struct S { f: Box }", + ] { + assert_eq!(rules_in("tests/example.rs", source).len(), 1, "{source}"); + } + for source in [ + "impl Error for E { fn source(&self) -> Option<&(dyn Error + 'static)> { None } }", + "fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None }", + "#[cfg(test)] fn f(x: &dyn Fn()) {}", + ] { + assert_eq!(rules_in("tests/example.rs", source), Vec::::new()); + } + } + + #[test] + fn library_code_keeps_no_shared_global_reflected_or_hashed_state() { + for source in [ + "use std::rc::Rc;", + "fn f() { let x = Arc::new(1); }", + "struct S { c: RefCell }", + "fn f() { let x = AtomicBool::new(false); }", + "fn f() { let m = Mutex::new(0); }", + "static X: u8 = 0;", + "thread_local! {}", + "fn f() { let l = OnceLock::new(); }", + "fn f() -> TypeId { g() }", + "fn f() { let m = HashMap::new(); }", + "#[cfg(not(test))] static X: u8 = 0;", + "#[cfg(test)] COUNT.fetch_add(1); fn f() { let s = HashSet::new(); }", + "#[cfg(test)] static X: u8 = 0; fn f() { let m = Mutex::new(0); }", + ] { + assert_eq!(rules_in("src/example.rs", source).len(), 1, "{source}"); + assert_eq!(rules_in("tests/example.rs", source), Vec::::new()); + assert_eq!( + rules_in("src/failure_tests.rs", source), + Vec::::new() + ); + } + for source in [ + "fn f(x: &'static str) -> Box { g() }", + "const X: &'static str = \"\";", + "#[cfg(test)] mod tests { use std::sync::Arc; }", + "#[cfg(kani)] fn f() { let c = Cell::new(1); }", + "#[cfg(any(test, kani))] static X: u8 = 0;", + "struct S { #[cfg(test)] c: Cell, v: u8 }", + "struct S { v: u8, #[cfg(test)] c: Cell }", + "fn f() { #[cfg(test)] COUNT.fetch_add(1, atomic::Ordering::Relaxed); }", + ] { + assert_eq!( + rules_in("src/example.rs", source), + Vec::::new(), + "{source}" + ); + } + } + + #[test] + fn only_cfg_test_and_kani_guard_test_code() { + let content = |source: &str| { + lex(source) + .into_iter() + .map(|(_, token)| token) + .collect::>() + }; + for guard in [ + "cfg(test)", + "cfg(kani)", + "cfg(any(test, kani))", + "cfg(any(test))", + ] { + assert!(is_test_only_cfg(&content(guard)), "{guard}"); + } + for guard in [ + "cfg(not(test))", + "cfg(any(test, windows))", + "cfg(all(test, kani))", + "cfg()", + "cfg(any(test,))", + "cfg_attr(test, allow(x))", + "test", + ] { + assert!(!is_test_only_cfg(&content(guard)), "{guard}"); + } + } + #[test] fn literals_do_not_confuse_the_lexer() { assert_clean("fn f<'a>(x: &'a str) -> char { let _ = r#\"// \"#; '\\'' }"); From e0f933e639acc1e283c6d54067590c6affdb1383 Mon Sep 17 00:00:00 2001 From: Yasunobu <42543015+P4suta@users.noreply.github.com> Date: Tue, 13 Oct 2026 22:27:43 +0900 Subject: [PATCH 3/6] test: skip the unreachable question mark where the creation moved it Splitting the creation moved the `?` that only an interior NUL in the working directory reaches, which validation rejects first. The mutation skip now names its new line, so the configuration skips four places again, as it did before the split. Refs #35 --- .rust-mutants.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.rust-mutants.toml b/.rust-mutants.toml index 6418c85..2e08f09 100644 --- a/.rust-mutants.toml +++ b/.rust-mutants.toml @@ -109,5 +109,5 @@ reason = "Unreachable: GetFullPathNameW cannot return a double quote." [[mutation.skip]] path = "src/transaction.rs" -lines = "165-169" -reason = "Unreachable: wide_nul fails only on an interior NUL, which validate_command rejects first; rejects_every_malformed_text_component holds it." +lines = "77-77" +reason = "Unreachable: current_directory fails only on an interior NUL, which validate_command rejects first; rejects_every_malformed_text_component holds it." From 004adb98f9e279e87628d53fdb8d44114e33357a Mon Sep 17 00:00:00 2001 From: Yasunobu <42543015+P4suta@users.noreply.github.com> Date: Tue, 13 Oct 2026 23:06:03 +0900 Subject: [PATCH 4/6] build: narrow the trait-object exception and follow guarded statements The trait-object rule exempted any `dyn Error` in the signature of a function named `source`, parameters included. It now exempts only what `source` returns in an implementation of `Error`. A guarded `let` statement whose initializer ends with a brace, such as a struct literal followed by a method call, now stays guarded through its `;`. A closing brace ends the guarded item only when no `else`, method call, `?`, `;`, or `,` continues it. A test-only `cfg` that ends its list with a comma, which Rust accepts, now counts as test-only, while an empty member inside the list still doesn't. ADR 0016 states the narrowed exception, and that the registry lost its exceptions for `src/child.rs` and `src/transaction.rs`, not every exception. Refs #35 --- .../0016-spawn-state-is-explicit-and-typed.md | 4 +- xtask/src/gates.rs | 122 +++++++++++++++--- 2 files changed, 109 insertions(+), 17 deletions(-) diff --git a/docs/adr/0016-spawn-state-is-explicit-and-typed.md b/docs/adr/0016-spawn-state-is-explicit-and-typed.md index 6908ab5..d11634e 100644 --- a/docs/adr/0016-spawn-state-is-explicit-and-typed.md +++ b/docs/adr/0016-spawn-state-is-explicit-and-typed.md @@ -27,14 +27,14 @@ Main dropped #14, and with it those rules. - Handle values carry the table they belong to. `Value` names the alternate parent and the Jobs, and `Value` names the inherited handles and the standard handles. - `AttributeList<'a>` borrows the values its attributes point to, so the compiler keeps them alive and in place until `DeleteProcThreadAttributeList` runs. -- `cargo xtask gates` refuses a trait object outside the signature of `Error::source`, in every file. +- `cargo xtask gates` refuses a trait object in every file, except as the return type of `source` in an implementation of `Error`. In library code it also refuses `Rc`, `Arc`, `Weak`, cell types, locks, atomics, `static` items, one-time initialization, `thread_local`, `Any`, `TypeId`, type names, and hash-based maps and sets. Items under `cfg(test)` or `cfg(kani)`, and `src/failure_tests.rs`, keep their thread-local state, counters, and downcasts. - `deny.toml` bans the crates #14 banned, which erase errors, erase serialization or futures, register items at run time, or keep implicit global state. ## Consequences -- The registry holds no `expect_used` or `too_many_lines` exception. +- The registry holds no `expect_used` or `too_many_lines` exception for `src/child.rs` or `src/transaction.rs`. - A handle of the wrong kind, a value of the wrong table, or an attribute value that dies before its list fails to compile. - Concurrency in the library needs a typed boundary and a model before it can pass the gate. - The output readers of `Child::wait_with_output` stay threads that own what they read, so they share no state. diff --git a/xtask/src/gates.rs b/xtask/src/gates.rs index 496de59..87e132b 100644 --- a/xtask/src/gates.rs +++ b/xtask/src/gates.rs @@ -432,8 +432,10 @@ fn library_ban(name: &str) -> Option<&'static str> { .map(|(_, reason)| *reason) } -/// Returns true if the `dyn` at `index` names the `Error` trait in the signature of a `source` method, the `Error::source` contract. +/// Returns true if the `dyn` at `index` names the `Error` trait in the return type of `source` in an `impl … Error for …` block: +/// the `Error::source` contract. fn in_error_source(code: &[&(usize, Token)], index: usize) -> bool { + let token_at = |at: usize| code.get(at).map(|(_, found)| found); let names_error = code .iter() .skip(index.saturating_add(1)) @@ -445,16 +447,72 @@ fn in_error_source(code: &[&(usize, Token)], index: usize) -> bool { }) .last() == Some("Error"); - let signature: Vec<&Token> = code + let Some(function) = (0..index) + .rev() + .take_while(|at| !matches!(token_at(*at), Some(Token::Punct('{' | '}' | ';')))) + .find(|at| matches!(token_at(*at), Some(Token::Ident(keyword)) if keyword == "fn")) + else { + return false; + }; + let name = function.saturating_add(1); + names_error + && matches!(token_at(name), Some(Token::Ident(source)) if source == "source") + && return_arrow(code, name).is_some_and(|arrow| arrow < index) + && in_error_impl(code, function) +} + +/// Returns the index of the `>` of the `->` after the parameters of the function whose name is at `name`. +fn return_arrow(code: &[&(usize, Token)], name: usize) -> Option { + let mut depth = 0_usize; + let mut close = None; + for (at, (_, token)) in code.iter().enumerate().skip(name) { + if *token == Token::Punct('(') { + depth = depth.saturating_add(1); + } else if *token == Token::Punct(')') { + depth = depth.saturating_sub(1); + if depth == 0 { + close = Some(at); + break; + } + } + } + let dash = close?.saturating_add(1); + let arrow = dash.saturating_add(1); + let punct = |at: usize, expected: char| { + code.get(at) + .is_some_and(|(_, token)| *token == Token::Punct(expected)) + }; + (punct(dash, '-') && punct(arrow, '>')).then_some(arrow) +} + +/// Returns true if the item at `item` sits in the block of an `impl … Error for …`. +fn in_error_impl(code: &[&(usize, Token)], item: usize) -> bool { + let mut depth = 0_usize; + let mut opener = None; + for (at, (_, token)) in code.iter().enumerate().take(item).rev() { + if *token == Token::Punct('}') { + depth = depth.saturating_add(1); + } else if *token == Token::Punct('{') { + if depth == 0 { + opener = Some(at); + break; + } + depth = depth.saturating_sub(1); + } + } + let Some(opener) = opener else { + return false; + }; + let header: Vec<&Token> = code .iter() - .take(index) + .take(opener) .rev() .map(|(_, token)| token) .take_while(|token| !matches!(token, Token::Punct('{' | '}' | ';'))) .collect(); - names_error - && signature.windows(PAIR).any(|pair| { - matches!(pair, [Token::Ident(name), Token::Ident(keyword)] if name == "source" && keyword == "fn") + header.contains(&&Token::Ident("impl".to_owned())) + && header.windows(PAIR).any(|pair| { + matches!(pair, [Token::Ident(keyword), Token::Ident(error)] if keyword == "for" && error == "Error") }) } @@ -484,9 +542,9 @@ fn is_test_only_cfg(content: &[Token]) -> bool { let [Token::Ident(cfg), Token::Punct('('), predicate @ .., Token::Punct(')')] = content else { return false; }; - let members = match predicate { + let members = match without_trailing_comma(predicate) { [Token::Ident(any), Token::Punct('('), members @ .., Token::Punct(')')] if any == "any" => { - members + without_trailing_comma(members) } single => single, }; @@ -496,9 +554,25 @@ fn is_test_only_cfg(content: &[Token]) -> bool { ) } +/// Drops the one trailing comma Rust allows after the last element of a list. +fn without_trailing_comma(tokens: &[Token]) -> &[Token] { + tokens.strip_suffix(&[Token::Punct(',')]).unwrap_or(tokens) +} + +/// Returns true if `next`, the token after a closing brace, continues the statement the brace closed: +/// `else`, a method call, `?`, or the `;` or `,` that ends it. +fn continues(next: Option<&&(usize, Token)>) -> bool { + match next.map(|(_, token)| token) { + Some(Token::Ident(word)) => word == "else", + Some(Token::Punct(punct)) => matches!(punct, '.' | '?' | ';' | ','), + Some(Token::Literal(_) | Token::Str(_) | Token::Comment { .. }) | None => false, + } +} + /// Returns the index after what the attribute whose `[` is at `open` guards. /// -/// That ends with a `;` or `,` outside brackets, with the `}` that closes its body, or before the bracket that closes the enclosing block. +/// That ends with a `;` or `,` outside brackets, with a `}` that closes its body when nothing continues the statement, +/// or before the bracket that closes the enclosing block. fn item_end(code: &[&(usize, Token)], open: usize) -> usize { let mut depth = 0_usize; let mut in_attribute = true; @@ -513,7 +587,7 @@ fn item_end(code: &[&(usize, Token)], open: usize) -> usize { if depth == 0 { if in_attribute { in_attribute = false; - } else if *close == '}' { + } else if *close == '}' && !continues(code.get(index.saturating_add(1))) { return index.saturating_add(1); } } @@ -999,8 +1073,11 @@ mod tests { for source in [ "fn f(x: &dyn Fn()) {}", "fn cause(&self) -> Option<&dyn Error> { None }", - "fn source(&self) -> Option<&(dyn Display + 'static)> { None }", - "fn source(&self) {} fn g(x: &dyn Error) {}", + "impl Error for E { fn source(&self) -> Option<&(dyn Display + 'static)> { None } }", + "impl Error for E { fn source(&self) {} fn g(x: &dyn Error) {} }", + "impl Error for E { fn source(&self, x: &dyn Error) {} }", + "fn source(&self) -> Option<&(dyn Error + 'static)> { None }", + "impl Display for E { fn source(&self) -> Option<&(dyn Error + 'static)> { None } }", "m!(dyn T);", "struct S { f: Box }", ] { @@ -1008,10 +1085,15 @@ mod tests { } for source in [ "impl Error for E { fn source(&self) -> Option<&(dyn Error + 'static)> { None } }", - "fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None }", + "impl std::error::Error for E { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None } }", + "impl Error for W { fn f() {} fn source(&self) -> Option<&(dyn Error + 'static)> { None } }", "#[cfg(test)] fn f(x: &dyn Fn()) {}", ] { - assert_eq!(rules_in("tests/example.rs", source), Vec::::new()); + assert_eq!( + rules_in("tests/example.rs", source), + Vec::::new(), + "{source}" + ); } } @@ -1031,6 +1113,8 @@ mod tests { "#[cfg(not(test))] static X: u8 = 0;", "#[cfg(test)] COUNT.fetch_add(1); fn f() { let s = HashSet::new(); }", "#[cfg(test)] static X: u8 = 0; fn f() { let m = Mutex::new(0); }", + "#[cfg(test)] let x = S { a: 1 }.with(Cell::new(1)); fn f() { let m = HashMap::new(); }", + "#[cfg(test)] fn g() {} fn f() { let m = HashMap::new(); }", ] { assert_eq!(rules_in("src/example.rs", source).len(), 1, "{source}"); assert_eq!(rules_in("tests/example.rs", source), Vec::::new()); @@ -1048,6 +1132,9 @@ mod tests { "struct S { #[cfg(test)] c: Cell, v: u8 }", "struct S { v: u8, #[cfg(test)] c: Cell }", "fn f() { #[cfg(test)] COUNT.fetch_add(1, atomic::Ordering::Relaxed); }", + "fn f() { #[cfg(test)] let c = if a { Cell::new(1) } else { Cell::new(2) }; }", + "fn f() { #[cfg(test)] let s = S { a: 1 }.with(RefCell::new(1))?; }", + "match x { #[cfg(test)] A => { Cell::new(1) }, B => 2 }", ] { assert_eq!( rules_in("src/example.rs", source), @@ -1070,6 +1157,9 @@ mod tests { "cfg(kani)", "cfg(any(test, kani))", "cfg(any(test))", + "cfg(any(test,))", + "cfg(any(test, kani,))", + "cfg(test,)", ] { assert!(is_test_only_cfg(&content(guard)), "{guard}"); } @@ -1078,7 +1168,9 @@ mod tests { "cfg(any(test, windows))", "cfg(all(test, kani))", "cfg()", - "cfg(any(test,))", + "cfg(any())", + "cfg(any(test,,kani))", + "cfg(any(,))", "cfg_attr(test, allow(x))", "test", ] { From 0b299848e6f11488655d25064c5f600df42e3d53 Mon Sep 17 00:00:00 2001 From: Yasunobu <42543015+P4suta@users.noreply.github.com> Date: Tue, 13 Oct 2026 23:30:27 +0900 Subject: [PATCH 5/6] docs: say that a Job can list a process its waiter saw exit A Windows Server 2022 run showed the Job still listing the child for a moment after the wait for it returned. The comment on `job_process_ids` claimed that a process leaves the list before its handle signals. It now states what holds: an empty list means that no process of the Job still runs. Refs #35 --- src/sys.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sys.rs b/src/sys.rs index dac4e99..c4a84eb 100644 --- a/src/sys.rs +++ b/src/sys.rs @@ -417,7 +417,7 @@ const _: () = assert!( /// The identifiers of the Job's processes whose exit the Job hasn't recorded. /// -/// A process leaves the list before its handle is signaled, so an empty list means that no process of the Job still runs. +/// The Job can still list a process for a moment after a wait for it returns, but an empty list means that no process of the Job still runs. pub(crate) fn job_process_ids(job: Borrowed<'_, JobObject>) -> io::Result> { #[cfg(test)] fault::check(fault::Call::ListJob)?; From 9ef82ed7e6196cfc0a57c53a3519d2b0b3672854 Mon Sep 17 00:00:00 2001 From: Yasunobu <42543015+P4suta@users.noreply.github.com> Date: Wed, 14 Oct 2026 00:36:09 +0900 Subject: [PATCH 6/6] build: keep a guarded let, static, or const marked through its semicolon A guarded `let` whose initializer closes a block before an operator, such as `{ 1 } + Arc::strong_count(&arc)`, still ended at the brace. A `let` statement and a `static`, `const`, `use`, or `type` item now end only at their `;`. The scan finds the item's keyword past any further attribute and the visibility. A `const fn` still ends with its body, and the scanner tests cover both kinds. Refs #35 --- xtask/src/gates.rs | 73 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 70 insertions(+), 3 deletions(-) diff --git a/xtask/src/gates.rs b/xtask/src/gates.rs index 87e132b..b18bf7d 100644 --- a/xtask/src/gates.rs +++ b/xtask/src/gates.rs @@ -569,13 +569,68 @@ fn continues(next: Option<&&(usize, Token)>) -> bool { } } +/// Returns true if what starts at `start`, past further attributes and a visibility, +/// is a `let` statement or a `static`, `const`, `use`, or `type` item, which only a `;` ends. +fn ends_at_semicolon(code: &[&(usize, Token)], start: usize) -> bool { + let token_at = |at: usize| code.get(at).map(|(_, found)| found); + let mut at = start; + loop { + match token_at(at) { + Some(Token::Punct('#')) => at = group_end(code, at.saturating_add(1)), + Some(Token::Ident(word)) if word == "pub" => { + at = at.saturating_add(1); + if token_at(at) == Some(&Token::Punct('(')) { + at = group_end(code, at); + } + } + Some(Token::Ident(word)) if word == "const" => { + return !matches!( + token_at(at.saturating_add(1)), + Some(Token::Ident(next)) if matches!(next.as_str(), "fn" | "unsafe" | "async" | "extern") + ); + } + Some(Token::Ident(word)) => { + return matches!(word.as_str(), "let" | "static" | "use" | "type"); + } + Some(Token::Literal(_) | Token::Str(_) | Token::Punct(_) | Token::Comment { .. }) + | None => { + return false; + } + } + } +} + +/// Returns the index after the bracket that closes the one at `open`. +fn group_end(code: &[&(usize, Token)], open: usize) -> usize { + let mut depth = 0_usize; + for (index, (_, token)) in code.iter().enumerate().skip(open) { + match token { + Token::Punct('(' | '[' | '{') => depth = depth.saturating_add(1), + Token::Punct(')' | ']' | '}') => { + depth = depth.saturating_sub(1); + if depth == 0 { + return index.saturating_add(1); + } + } + Token::Ident(_) + | Token::Literal(_) + | Token::Str(_) + | Token::Punct(_) + | Token::Comment { .. } => {} + } + } + code.len() +} + /// Returns the index after what the attribute whose `[` is at `open` guards. /// -/// That ends with a `;` or `,` outside brackets, with a `}` that closes its body when nothing continues the statement, -/// or before the bracket that closes the enclosing block. +/// That ends with a `;` or `,` outside brackets, which alone ends a `let` statement and a `static`, `const`, `use`, or `type` item. +/// Another item ends with a `}` that closes its body when nothing continues the statement. +/// Either ends before the bracket that closes the enclosing block. fn item_end(code: &[&(usize, Token)], open: usize) -> usize { let mut depth = 0_usize; let mut in_attribute = true; + let mut semicolon = false; for (index, (_, token)) in code.iter().enumerate().skip(open) { match token { Token::Punct('(' | '[' | '{') => depth = depth.saturating_add(1), @@ -587,7 +642,11 @@ fn item_end(code: &[&(usize, Token)], open: usize) -> usize { if depth == 0 { if in_attribute { in_attribute = false; - } else if *close == '}' && !continues(code.get(index.saturating_add(1))) { + semicolon = ends_at_semicolon(code, index.saturating_add(1)); + } else if *close == '}' + && !semicolon + && !continues(code.get(index.saturating_add(1))) + { return index.saturating_add(1); } } @@ -1115,6 +1174,9 @@ mod tests { "#[cfg(test)] static X: u8 = 0; fn f() { let m = Mutex::new(0); }", "#[cfg(test)] let x = S { a: 1 }.with(Cell::new(1)); fn f() { let m = HashMap::new(); }", "#[cfg(test)] fn g() {} fn f() { let m = HashMap::new(); }", + "#[cfg(test)] let x = { 1 }; fn f() { let m = HashMap::new(); }", + "#[cfg(test)] const fn g() -> u8 { 1 } fn f() { let m = HashMap::new(); }", + "#[cfg(test)] pub(crate) const unsafe fn g() {} fn f() { let m = HashMap::new(); }", ] { assert_eq!(rules_in("src/example.rs", source).len(), 1, "{source}"); assert_eq!(rules_in("tests/example.rs", source), Vec::::new()); @@ -1135,6 +1197,11 @@ mod tests { "fn f() { #[cfg(test)] let c = if a { Cell::new(1) } else { Cell::new(2) }; }", "fn f() { #[cfg(test)] let s = S { a: 1 }.with(RefCell::new(1))?; }", "match x { #[cfg(test)] A => { Cell::new(1) }, B => 2 }", + "fn f() { #[cfg(test)] let value = { 1 } + Arc::strong_count(&arc); }", + "#[cfg(test)] pub(crate) static S: u8 = { 1 } + Cell::new(1).get();", + "#[cfg(test)] #[doc(hidden)] const N: usize = { 1 } + HashMap::::new().len();", + "#[cfg(test)] use std::{cell::Cell, sync::Arc};", + "#[cfg(test)] type Shared = { Arc };", ] { assert_eq!( rules_in("src/example.rs", source),