From d0d44e998284ef7be74a1abdeb72825c8f98e023 Mon Sep 17 00:00:00 2001 From: Yasunobu <42543015+P4suta@users.noreply.github.com> Date: Fri, 25 Sep 2026 04:27:39 +0900 Subject: [PATCH 1/3] chore: enforce the repository laws mechanically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review alone did not keep the rules, so each law now has a check that CI runs (ADR 0013). - `[workspace.lints]` denies the Clippy `all`, `pedantic`, `nursery`, and `cargo` groups, a restriction set taken from domyjob, and the rustc lints Rust 1.75 recognizes. - `clippy.toml` also bans `mem::forget`, `process::exit`, `env::set_var`, and `env::remove_var`, and permits `unwrap`, `expect`, panics, and indexing in tests. - `cargo xtask gates` is a lexer with no dependencies, because `syn` needs `unicode-ident`, whose license `deny.toml` does not allow. It allows only doc and `// SAFETY:` comments, requires every `#[allow]` to be registered with a count and a reason, bans time-named identifiers, delay commands, and bounded `WaitForSingleObject`, and bans `#[default]` and `Box`. `just gates`, `xtask ci`, and the hygiene job run it. Code changes the lints required: - Handle values convert through `sys::handle_value` and `sys::handle_from_value`, the only registered `as` conversions in the library; pointer-integer `From` conversions need Rust 1.84. - Indexing became `get`, arithmetic became checked or saturating, and Win32 lengths convert through `try_from`. - The mitigation enums and `DropPolicy` implement `Default` by hand, so the default is stated instead of implied by declaration order. The public API snapshot shows `default() -> Self`, and `SpawnOptions::parent_process` is now `const`. - `SuspendedChild::id`, `SuspendedChild::as_handle`, and `SpawnTransaction::commit_parts` keep `expect` as registered exceptions: the value owns its process until it is consumed. - Integration tests register `as_conversions`, `expect_used`, and `unwrap_in_result` at the crate level; the probe exits through one registered `process::exit`. - `WAIT_TIMEOUT` is exempt from the time gate: it is what the 0 ms `try_wait` query returns for a running process. - `redundant_pub_crate` is allowed because it contradicts `unreachable_pub`, which the crate keeps. The mutation skips move to the new line numbers of the same code. --- .github/workflows/ci.yml | 1 + .rust-mutants.toml | 8 +- CONTRIBUTING.md | 2 + Cargo.toml | 45 +- clippy.toml | 9 + ...pository-laws-are-enforced-mechanically.md | 39 + justfile | 3 + public-api/windows-spawn.txt | 26 +- src/child.rs | 13 +- src/failure_tests.rs | 13 +- src/lib.rs | 3 +- src/mitigation.rs | 124 +-- src/options.rs | 12 +- src/plan.rs | 8 +- src/sys.rs | 162 ++-- src/transaction.rs | 50 +- tests/argv_roundtrip.rs | 1 + tests/support/mod.rs | 21 +- tests/windows_spawn.rs | 47 +- xtask/src/cli.rs | 9 +- xtask/src/gates.rs | 822 ++++++++++++++++++ xtask/src/main.rs | 9 +- xtask/src/tasks.rs | 52 +- 23 files changed, 1249 insertions(+), 230 deletions(-) create mode 100644 docs/adr/0013-repository-laws-are-enforced-mechanically.md create mode 100644 xtask/src/gates.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2b97f0d..81e4c0b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,6 +37,7 @@ jobs: - run: python -m pip install "reuse[charset-normalizer]==6.2.0" - run: just fmt - run: just clippy + - run: just gates - run: just doc - run: just public-api - run: just msrv diff --git a/.rust-mutants.toml b/.rust-mutants.toml index 67b63a3..f814ab8 100644 --- a/.rust-mutants.toml +++ b/.rust-mutants.toml @@ -120,20 +120,20 @@ outcome = "survived" [[mutation.skip]] path = "src/sys.rs" -lines = "1004-1004" +lines = "996-996" reason = "Unreachable: GetEnvironmentStringsW returns null only when out of memory." [[mutation.skip]] path = "src/sys.rs" -lines = "1012-1012" +lines = "1004-1004" reason = "Hang-only: continuing past the block terminator loops forever." [[mutation.skip]] path = "src/transaction.rs" -lines = "492-495" +lines = "498-501" reason = "Unreachable: GetFullPathNameW cannot return a double quote." [[mutation.skip]] path = "src/transaction.rs" -lines = "106-106" +lines = "104-104" 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/CONTRIBUTING.md b/CONTRIBUTING.md index f0d3ebb..2a5daca 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -36,6 +36,8 @@ gitleaks dir . --redact --no-banner - Keep the documented ownership and cleanup behavior, including on errors. - Give every `unsafe` block a specific safety justification. +- Write only doc comments and `// SAFETY:` comments; the reason for a change goes in its commit message (ADR 0013). +- Register every `#[allow]` in `xtask/src/gates.rs` with its reason; `just gates` checks the registry and the other repository laws. - Add deterministic tests for behavior changes; tests wait for events, never for time (ADR 0010). - Change the public API snapshot only with an API change, and state its compatibility impact. - Update the crate docs, ADRs, or security boundary when a contract changes. diff --git a/Cargo.toml b/Cargo.toml index da28656..6e227ef 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -56,15 +56,56 @@ default-members = ["."] resolver = "2" [workspace.lints.rust] -missing_docs = "deny" +future_incompatible = { level = "deny", priority = -1 } rust_2018_idioms = { level = "deny", priority = -1 } -unsafe_op_in_unsafe_fn = "deny" +unused = { level = "deny", priority = -1 } +let_underscore_drop = "deny" +meta_variable_misuse = "deny" +missing_copy_implementations = "deny" +missing_debug_implementations = "deny" +missing_docs = "deny" +non_ascii_idents = "deny" +single_use_lifetimes = "deny" +trivial_casts = "deny" +trivial_numeric_casts = "deny" unreachable_pub = "deny" +unsafe_op_in_unsafe_fn = "deny" +unused_lifetimes = "deny" unused_qualifications = "deny" +variant_size_differences = "deny" [workspace.lints.clippy] all = { level = "deny", priority = -1 } +cargo = { level = "deny", priority = -1 } +nursery = { level = "deny", priority = -1 } pedantic = { level = "deny", priority = -1 } +arithmetic_side_effects = "deny" +as_conversions = "deny" +clone_on_ref_ptr = "deny" +dbg_macro = "deny" +disallowed_methods = "deny" +disallowed_types = "deny" +error_impl_error = "deny" +expect_used = "deny" +fallible_impl_from = "deny" +indexing_slicing = "deny" +let_underscore_must_use = "deny" +map_err_ignore = "deny" +mem_forget = "deny" +panic = "deny" +shadow_unrelated = "deny" +str_to_string = "deny" +string_slice = "deny" +todo = "deny" +try_err = "deny" +unimplemented = "deny" +unreachable = "deny" +unused_result_ok = "deny" +unwrap_in_result = "deny" +unwrap_used = "deny" +wildcard_enum_match_arm = "deny" +# Contradicts rustc's `unreachable_pub`, which this crate keeps: items of private modules stay `pub(crate)`. +redundant_pub_crate = "allow" # Enforces CONTRIBUTING's rule that every `unsafe` block has a safety justification. # diff --git a/clippy.toml b/clippy.toml index 66f7454..4baffcd 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1,3 +1,8 @@ +allow-expect-in-tests = true +allow-indexing-slicing-in-tests = true +allow-panic-in-tests = true +allow-unwrap-in-tests = true + # Time decides nothing (docs/adr/0010-time-decides-nothing.md). disallowed-types = [ { path = "std::time::Duration", reason = "time decides nothing; wait for the event itself" }, @@ -13,4 +18,8 @@ disallowed-methods = [ { path = "std::fs::Metadata::modified", reason = "file times decide nothing; compare content" }, { path = "std::fs::Metadata::accessed", reason = "file times decide nothing; compare content" }, { path = "std::fs::Metadata::created", reason = "file times decide nothing; compare content" }, + { path = "std::mem::forget", reason = "leaks destructors; use ManuallyDrop" }, + { path = "std::process::exit", reason = "skips destructors; return an exit code" }, + { path = "std::env::set_var", reason = "races with other threads; pass the value explicitly" }, + { path = "std::env::remove_var", reason = "races with other threads; pass the value explicitly" }, ] diff --git a/docs/adr/0013-repository-laws-are-enforced-mechanically.md b/docs/adr/0013-repository-laws-are-enforced-mechanically.md new file mode 100644 index 0000000..4774d12 --- /dev/null +++ b/docs/adr/0013-repository-laws-are-enforced-mechanically.md @@ -0,0 +1,39 @@ +# 0013 — Repository laws are enforced mechanically + +Status: accepted (2026-09-25) + +## Context + +Rules kept by review alone erode: timed waits, explanatory comments, and silent conversions came back after they were removed. +Rust 1.75, the MSRV, has neither `#[expect]` nor `reason =`, so a lint exception can neither state its reason nor prove it is still needed. + +## Decision + +Every law has a mechanism, and CI runs it. + +- Code fails loudly and converts explicitly. + `[workspace.lints]` denies the Clippy `all`, `pedantic`, `nursery`, and `cargo` groups and a restriction set: no `unwrap`, `expect`, `panic`, indexing, unchecked arithmetic, `as`, or discarded `Result`. + `clippy.toml` permits `unwrap`, `expect`, panics, and indexing in tests only. +- Time decides nothing (ADR 0010). + `clippy.toml` bans the time types, timed waits, and file times. + `cargo xtask gates` bans time-named identifiers, delay commands in strings, and `WaitForSingleObject` with a bound other than `INFINITE` or `0`. +- Reasons belong in commit messages. + The gate allows doc comments and whole-line `// SAFETY:` runs, and nothing else. +- Every lint exception is registered. + The gate collects each `#[allow]` lint and requires an entry with the same file, lint, and count, with a reason, in `xtask/src/gates.rs`. + An entry no attribute uses also fails. +- Defaults are chosen explicitly. + The gate bans `#[default]`; a documented `impl Default` states the choice. +- Types are closed or generic. + The gate bans `Box`; a closed set is an enum, and an open one is a type parameter. +- Nothing escapes ownership or mutates process state. + `clippy.toml` bans `mem::forget`, `process::exit`, `env::set_var`, and `env::remove_var`. + +The gate is a lexer in xtask with no dependencies, because `syn` needs `unicode-ident`, whose license `deny.toml` does not allow. +Rustc lints are limited to those Rust 1.75 recognizes. + +## Consequences + +- Changing a law changes the lint table, `clippy.toml`, or the gate, with its tests. +- Pointer-integer conversions keep `as` behind registered exceptions; there is no alternative before Rust 1.84. +- When the MSRV reaches Rust 1.81, `#[expect(…, reason = …)]` replaces the registry. diff --git a/justfile b/justfile index bb4aaad..f0bc789 100644 --- a/justfile +++ b/justfile @@ -9,6 +9,9 @@ fmt: clippy: cargo xtask clippy +gates: + cargo xtask gates + test: cargo xtask test diff --git a/public-api/windows-spawn.txt b/public-api/windows-spawn.txt index a7819e9..c9edd64 100644 --- a/public-api/windows-spawn.txt +++ b/public-api/windows-spawn.txt @@ -10,7 +10,7 @@ impl core::cmp::Eq for windows_spawn::BlockNonCetBinaries impl core::cmp::PartialEq for windows_spawn::BlockNonCetBinaries pub fn windows_spawn::BlockNonCetBinaries::eq(&self, &windows_spawn::BlockNonCetBinaries) -> bool impl core::default::Default for windows_spawn::BlockNonCetBinaries -pub fn windows_spawn::BlockNonCetBinaries::default() -> windows_spawn::BlockNonCetBinaries +pub fn windows_spawn::BlockNonCetBinaries::default() -> Self impl core::fmt::Debug for windows_spawn::BlockNonCetBinaries pub fn windows_spawn::BlockNonCetBinaries::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::hash::Hash for windows_spawn::BlockNonCetBinaries @@ -35,7 +35,7 @@ impl core::cmp::Eq for windows_spawn::CetShadowStacks impl core::cmp::PartialEq for windows_spawn::CetShadowStacks pub fn windows_spawn::CetShadowStacks::eq(&self, &windows_spawn::CetShadowStacks) -> bool impl core::default::Default for windows_spawn::CetShadowStacks -pub fn windows_spawn::CetShadowStacks::default() -> windows_spawn::CetShadowStacks +pub fn windows_spawn::CetShadowStacks::default() -> Self impl core::fmt::Debug for windows_spawn::CetShadowStacks pub fn windows_spawn::CetShadowStacks::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::hash::Hash for windows_spawn::CetShadowStacks @@ -60,7 +60,7 @@ impl core::cmp::Eq for windows_spawn::ControlFlowGuard impl core::cmp::PartialEq for windows_spawn::ControlFlowGuard pub fn windows_spawn::ControlFlowGuard::eq(&self, &windows_spawn::ControlFlowGuard) -> bool impl core::default::Default for windows_spawn::ControlFlowGuard -pub fn windows_spawn::ControlFlowGuard::default() -> windows_spawn::ControlFlowGuard +pub fn windows_spawn::ControlFlowGuard::default() -> Self impl core::fmt::Debug for windows_spawn::ControlFlowGuard pub fn windows_spawn::ControlFlowGuard::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::hash::Hash for windows_spawn::ControlFlowGuard @@ -83,7 +83,7 @@ impl core::cmp::Eq for windows_spawn::DropPolicy impl core::cmp::PartialEq for windows_spawn::DropPolicy pub fn windows_spawn::DropPolicy::eq(&self, &windows_spawn::DropPolicy) -> bool impl core::default::Default for windows_spawn::DropPolicy -pub fn windows_spawn::DropPolicy::default() -> windows_spawn::DropPolicy +pub fn windows_spawn::DropPolicy::default() -> Self impl core::fmt::Debug for windows_spawn::DropPolicy pub fn windows_spawn::DropPolicy::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::hash::Hash for windows_spawn::DropPolicy @@ -108,7 +108,7 @@ impl core::cmp::Eq for windows_spawn::DynamicCode impl core::cmp::PartialEq for windows_spawn::DynamicCode pub fn windows_spawn::DynamicCode::eq(&self, &windows_spawn::DynamicCode) -> bool impl core::default::Default for windows_spawn::DynamicCode -pub fn windows_spawn::DynamicCode::default() -> windows_spawn::DynamicCode +pub fn windows_spawn::DynamicCode::default() -> Self impl core::fmt::Debug for windows_spawn::DynamicCode pub fn windows_spawn::DynamicCode::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::hash::Hash for windows_spawn::DynamicCode @@ -133,7 +133,7 @@ impl core::cmp::Eq for windows_spawn::FontDisable impl core::cmp::PartialEq for windows_spawn::FontDisable pub fn windows_spawn::FontDisable::eq(&self, &windows_spawn::FontDisable) -> bool impl core::default::Default for windows_spawn::FontDisable -pub fn windows_spawn::FontDisable::default() -> windows_spawn::FontDisable +pub fn windows_spawn::FontDisable::default() -> Self impl core::fmt::Debug for windows_spawn::FontDisable pub fn windows_spawn::FontDisable::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::hash::Hash for windows_spawn::FontDisable @@ -158,7 +158,7 @@ impl core::cmp::Eq for windows_spawn::LoaderIntegrity impl core::cmp::PartialEq for windows_spawn::LoaderIntegrity pub fn windows_spawn::LoaderIntegrity::eq(&self, &windows_spawn::LoaderIntegrity) -> bool impl core::default::Default for windows_spawn::LoaderIntegrity -pub fn windows_spawn::LoaderIntegrity::default() -> windows_spawn::LoaderIntegrity +pub fn windows_spawn::LoaderIntegrity::default() -> Self impl core::fmt::Debug for windows_spawn::LoaderIntegrity pub fn windows_spawn::LoaderIntegrity::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::hash::Hash for windows_spawn::LoaderIntegrity @@ -182,7 +182,7 @@ impl core::cmp::Eq for windows_spawn::Mitigation impl core::cmp::PartialEq for windows_spawn::Mitigation pub fn windows_spawn::Mitigation::eq(&self, &windows_spawn::Mitigation) -> bool impl core::default::Default for windows_spawn::Mitigation -pub fn windows_spawn::Mitigation::default() -> windows_spawn::Mitigation +pub fn windows_spawn::Mitigation::default() -> Self impl core::fmt::Debug for windows_spawn::Mitigation pub fn windows_spawn::Mitigation::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::hash::Hash for windows_spawn::Mitigation @@ -207,7 +207,7 @@ impl core::cmp::Eq for windows_spawn::ModuleTampering impl core::cmp::PartialEq for windows_spawn::ModuleTampering pub fn windows_spawn::ModuleTampering::eq(&self, &windows_spawn::ModuleTampering) -> bool impl core::default::Default for windows_spawn::ModuleTampering -pub fn windows_spawn::ModuleTampering::default() -> windows_spawn::ModuleTampering +pub fn windows_spawn::ModuleTampering::default() -> Self impl core::fmt::Debug for windows_spawn::ModuleTampering pub fn windows_spawn::ModuleTampering::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::hash::Hash for windows_spawn::ModuleTampering @@ -232,7 +232,7 @@ impl core::cmp::Eq for windows_spawn::RelocateImages impl core::cmp::PartialEq for windows_spawn::RelocateImages pub fn windows_spawn::RelocateImages::eq(&self, &windows_spawn::RelocateImages) -> bool impl core::default::Default for windows_spawn::RelocateImages -pub fn windows_spawn::RelocateImages::default() -> windows_spawn::RelocateImages +pub fn windows_spawn::RelocateImages::default() -> Self impl core::fmt::Debug for windows_spawn::RelocateImages pub fn windows_spawn::RelocateImages::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::hash::Hash for windows_spawn::RelocateImages @@ -257,7 +257,7 @@ impl core::cmp::Eq for windows_spawn::SignedBinaries impl core::cmp::PartialEq for windows_spawn::SignedBinaries pub fn windows_spawn::SignedBinaries::eq(&self, &windows_spawn::SignedBinaries) -> bool impl core::default::Default for windows_spawn::SignedBinaries -pub fn windows_spawn::SignedBinaries::default() -> windows_spawn::SignedBinaries +pub fn windows_spawn::SignedBinaries::default() -> Self impl core::fmt::Debug for windows_spawn::SignedBinaries pub fn windows_spawn::SignedBinaries::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::hash::Hash for windows_spawn::SignedBinaries @@ -282,7 +282,7 @@ impl core::cmp::Eq for windows_spawn::UserCetContextIpValidation impl core::cmp::PartialEq for windows_spawn::UserCetContextIpValidation pub fn windows_spawn::UserCetContextIpValidation::eq(&self, &windows_spawn::UserCetContextIpValidation) -> bool impl core::default::Default for windows_spawn::UserCetContextIpValidation -pub fn windows_spawn::UserCetContextIpValidation::default() -> windows_spawn::UserCetContextIpValidation +pub fn windows_spawn::UserCetContextIpValidation::default() -> Self impl core::fmt::Debug for windows_spawn::UserCetContextIpValidation pub fn windows_spawn::UserCetContextIpValidation::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::hash::Hash for windows_spawn::UserCetContextIpValidation @@ -528,7 +528,7 @@ pub const fn windows_spawn::SpawnOptions<'a>::drop_policy(self, windows_spawn::D pub fn windows_spawn::SpawnOptions<'a>::job(self, &'a windows_spawn::Job) -> Self pub const fn windows_spawn::SpawnOptions<'a>::mitigation(self, windows_spawn::MitigationPolicy) -> Self pub fn windows_spawn::SpawnOptions<'a>::new() -> Self -pub fn windows_spawn::SpawnOptions<'a>::parent_process(self, &'a windows_spawn::ParentProcess) -> Self +pub const fn windows_spawn::SpawnOptions<'a>::parent_process(self, &'a windows_spawn::ParentProcess) -> Self pub fn windows_spawn::SpawnOptions<'a>::pseudoconsole(self, &'a T) -> Self impl core::default::Default for windows_spawn::SpawnOptions<'_> pub fn windows_spawn::SpawnOptions<'_>::default() -> Self diff --git a/src/child.rs b/src/child.rs index 6ea4b5c..2676daa 100644 --- a/src/child.rs +++ b/src/child.rs @@ -207,7 +207,10 @@ fn drain_output(handle: BorrowedHandle<'_>) -> io::Result> { let Some(read) = std::num::NonZeroUsize::new(sys::read_handle(handle, &mut buffer)?) else { return Ok(bytes); }; - bytes.extend_from_slice(&buffer[..read.get()]); + let chunk = buffer.get(..read.get()).ok_or_else(|| { + io::Error::other("ReadFile reported more bytes than the buffer holds") + })?; + bytes.extend_from_slice(chunk); } } @@ -215,7 +218,7 @@ fn join_reader(reader: Option>>>) -> io::R match reader { Some(reader) => reader .join() - .map_err(|_| io::Error::other("output reader thread panicked"))?, + .map_err(|_payload| io::Error::other("output reader thread panicked"))?, None => Ok(Vec::new()), } } @@ -251,7 +254,7 @@ pub struct SuspendedChild { } impl SuspendedChild { - pub(crate) fn new(child: Child, main_thread: OwnedHandle) -> Self { + pub(crate) const fn new(child: Child, main_thread: OwnedHandle) -> Self { Self { child: Some(child), main_thread, @@ -264,6 +267,7 @@ impl SuspendedChild { /// /// Panics only if an internal ownership invariant is broken. #[must_use] + #[allow(clippy::expect_used)] pub fn id(&self) -> u32 { self.child .as_ref() @@ -300,6 +304,7 @@ impl SuspendedChild { } impl AsHandle for SuspendedChild { + #[allow(clippy::expect_used)] fn as_handle(&self) -> BorrowedHandle<'_> { self.child .as_ref() @@ -311,7 +316,7 @@ impl AsHandle for SuspendedChild { impl Drop for SuspendedChild { fn drop(&mut self) { if let Some(child) = &mut self.child { - let _ = child.kill(); + drop(child.kill()); } } } diff --git a/src/failure_tests.rs b/src/failure_tests.rs index 12c5815..7a0e3c5 100644 --- a/src/failure_tests.rs +++ b/src/failure_tests.rs @@ -5,7 +5,6 @@ use std::io::{self, Read, Write}; use std::os::windows::io::{AsHandle, AsRawHandle, OwnedHandle}; use std::thread; -use windows_sys::Win32::Foundation::HANDLE; use windows_sys::Win32::System::Console::{ClosePseudoConsole, CreatePseudoConsole, COORD, HPCON}; use crate::sys::fault::{self, Call}; @@ -351,8 +350,8 @@ impl TestConsole { let created = unsafe { CreatePseudoConsole( COORD { X: 80, Y: 25 }, - input_reader.as_raw_handle() as HANDLE, - output_writer.as_raw_handle() as HANDLE, + input_reader.as_raw_handle(), + output_writer.as_raw_handle(), 0, &mut value, ) @@ -360,10 +359,10 @@ impl TestConsole { assert!(created >= 0, "CreatePseudoConsole failed with {created:#x}"); drop((input_reader, output_writer)); let mut output = File::from(output_reader); - let _ = thread::spawn(move || { + drop(thread::spawn(move || { let mut buffer = [0_u8; 4096]; while matches!(output.read(&mut buffer), Ok(read) if read > 0) {} - }); + })); Self { value, input: Some(input_writer), @@ -376,10 +375,10 @@ impl Drop for TestConsole { fn drop(&mut self) { drop(self.input.take()); let value = self.value; - let _ = thread::spawn(move || { + drop(thread::spawn(move || { // SAFETY: this type uniquely owns the HPCON. unsafe { ClosePseudoConsole(value) }; - }); + })); } } diff --git a/src/lib.rs b/src/lib.rs index fee797c..5f2d9a7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -10,7 +10,8 @@ mod readme_examples {} mod child; #[cfg(windows)] mod command; -#[cfg(all(windows, test))] +#[cfg(windows)] +#[cfg(test)] #[allow(unsafe_code)] mod failure_tests; #[cfg(windows)] diff --git a/src/mitigation.rs b/src/mitigation.rs index 88d00d4..b0438e0 100644 --- a/src/mitigation.rs +++ b/src/mitigation.rs @@ -1,11 +1,10 @@ //! Typed process-creation mitigation policy encoding. /// The ordinary two-bit mitigation states used by the Windows SDK. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] #[repr(u64)] pub enum Mitigation { /// Let the child executable and operating system choose. - #[default] Defer = 0, /// Force the mitigation on. AlwaysOn = 1, @@ -14,11 +13,10 @@ pub enum Mitigation { } /// Mandatory-ASLR modes. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] #[repr(u64)] pub enum RelocateImages { /// Defer to the child. - #[default] Defer = 0, /// Relocate images even when they are not dynamic-base compatible. AlwaysOn = 1, @@ -29,11 +27,10 @@ pub enum RelocateImages { } /// Dynamic-code policy modes. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] #[repr(u64)] pub enum DynamicCode { /// Defer to the child. - #[default] Defer = 0, /// Prohibit dynamic code. Prohibit = 1, @@ -44,11 +41,10 @@ pub enum DynamicCode { } /// Control Flow Guard modes. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] #[repr(u64)] pub enum ControlFlowGuard { /// Defer to the child. - #[default] Defer = 0, /// Enable Control Flow Guard. AlwaysOn = 1, @@ -59,11 +55,10 @@ pub enum ControlFlowGuard { } /// Microsoft-signed binary policy modes. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] #[repr(u64)] pub enum SignedBinaries { /// Defer to the child. - #[default] Defer = 0, /// Permit only Microsoft-signed binaries. MicrosoftOnly = 1, @@ -74,11 +69,10 @@ pub enum SignedBinaries { } /// Non-system-font policy modes. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] #[repr(u64)] pub enum FontDisable { /// Defer to the child. - #[default] Defer = 0, /// Block non-system fonts. Block = 1, @@ -89,11 +83,10 @@ pub enum FontDisable { } /// Loader integrity continuity modes. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] #[repr(u64)] pub enum LoaderIntegrity { /// Defer to the child. - #[default] Defer = 0, /// Enforce loader integrity continuity. AlwaysOn = 1, @@ -104,11 +97,10 @@ pub enum LoaderIntegrity { } /// Module-tampering protection modes. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] #[repr(u64)] pub enum ModuleTampering { /// Defer to the child. - #[default] Defer = 0, /// Enable module-tampering protection. AlwaysOn = 1, @@ -122,11 +114,10 @@ pub enum ModuleTampering { /// /// Support depends on the Windows release, architecture, hardware, and child executable. /// A representable value may still be rejected by the host. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] #[repr(u64)] pub enum CetShadowStacks { /// Defer to the child. - #[default] Defer = 0, /// Enable user shadow stacks. AlwaysOn = 1, @@ -139,11 +130,10 @@ pub enum CetShadowStacks { /// CET set-context instruction-pointer validation modes. /// /// Support depends on the Windows release, architecture, hardware, and child executable. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] #[repr(u64)] pub enum UserCetContextIpValidation { /// Defer to the child. - #[default] Defer = 0, /// Enable validation. AlwaysOn = 1, @@ -156,11 +146,10 @@ pub enum UserCetContextIpValidation { /// Modes for blocking binaries without CET or EH continuation metadata. /// /// Support depends on the Windows release, architecture, and executable metadata. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] #[repr(u64)] pub enum BlockNonCetBinaries { /// Defer to the child. - #[default] Defer = 0, /// Block binaries without CET metadata. AlwaysOn = 1, @@ -170,6 +159,38 @@ pub enum BlockNonCetBinaries { NonEhContinuation = 3, } +/// Gives a two-bit policy field its encoding and its `Defer` default. +macro_rules! policy_field { + ($name:ident { $($variant:ident => $bits:literal),+ $(,)? }) => { + impl $name { + const fn bits(self) -> u64 { + match self { + $(Self::$variant => $bits,)+ + } + } + } + + impl Default for $name { + /// Defers to the child. + fn default() -> Self { + Self::Defer + } + } + }; +} + +policy_field!(Mitigation { Defer => 0, AlwaysOn => 1, AlwaysOff => 2 }); +policy_field!(RelocateImages { Defer => 0, AlwaysOn => 1, AlwaysOff => 2, RequireRelocations => 3 }); +policy_field!(DynamicCode { Defer => 0, Prohibit => 1, Allow => 2, ProhibitWithOptOut => 3 }); +policy_field!(ControlFlowGuard { Defer => 0, AlwaysOn => 1, AlwaysOff => 2, ExportSuppression => 3 }); +policy_field!(SignedBinaries { Defer => 0, MicrosoftOnly => 1, AlwaysOff => 2, MicrosoftAndStore => 3 }); +policy_field!(FontDisable { Defer => 0, Block => 1, Allow => 2, Audit => 3 }); +policy_field!(LoaderIntegrity { Defer => 0, AlwaysOn => 1, AlwaysOff => 2, Audit => 3 }); +policy_field!(ModuleTampering { Defer => 0, AlwaysOn => 1, AlwaysOff => 2, NoInherit => 3 }); +policy_field!(CetShadowStacks { Defer => 0, AlwaysOn => 1, AlwaysOff => 2, Strict => 3 }); +policy_field!(UserCetContextIpValidation { Defer => 0, AlwaysOn => 1, AlwaysOff => 2, Relaxed => 3 }); +policy_field!(BlockNonCetBinaries { Defer => 0, AlwaysOn => 1, AlwaysOff => 2, NonEhContinuation => 3 }); + /// A complete SDK 10.0.22621 process-creation mitigation policy. /// /// Each setter replaces one field. @@ -222,147 +243,147 @@ impl MitigationPolicy { /// Sets mandatory image relocation. #[must_use] pub const fn relocate_images(mut self, value: RelocateImages) -> Self { - self.words[0] = replace(self.words[0], 8, value as u64); + self.words[0] = replace(self.words[0], 8, value.bits()); self } /// Sets heap termination on corruption. #[must_use] pub const fn heap_terminate(mut self, value: Mitigation) -> Self { - self.words[0] = replace(self.words[0], 12, value as u64); + self.words[0] = replace(self.words[0], 12, value.bits()); self } /// Sets bottom-up ASLR. #[must_use] pub const fn bottom_up_aslr(mut self, value: Mitigation) -> Self { - self.words[0] = replace(self.words[0], 16, value as u64); + self.words[0] = replace(self.words[0], 16, value.bits()); self } /// Sets high-entropy ASLR. #[must_use] pub const fn high_entropy_aslr(mut self, value: Mitigation) -> Self { - self.words[0] = replace(self.words[0], 20, value as u64); + self.words[0] = replace(self.words[0], 20, value.bits()); self } /// Sets strict invalid-handle checking. #[must_use] pub const fn strict_handle_checks(mut self, value: Mitigation) -> Self { - self.words[0] = replace(self.words[0], 24, value as u64); + self.words[0] = replace(self.words[0], 24, value.bits()); self } /// Sets the Win32k system-call-disable mitigation. #[must_use] pub const fn disable_win32k_system_calls(mut self, value: Mitigation) -> Self { - self.words[0] = replace(self.words[0], 28, value as u64); + self.words[0] = replace(self.words[0], 28, value.bits()); self } /// Sets extension-point disabling. #[must_use] pub const fn disable_extension_points(mut self, value: Mitigation) -> Self { - self.words[0] = replace(self.words[0], 32, value as u64); + self.words[0] = replace(self.words[0], 32, value.bits()); self } /// Sets dynamic-code policy. #[must_use] pub const fn dynamic_code(mut self, value: DynamicCode) -> Self { - self.words[0] = replace(self.words[0], 36, value as u64); + self.words[0] = replace(self.words[0], 36, value.bits()); self } /// Sets Control Flow Guard policy. #[must_use] pub const fn control_flow_guard(mut self, value: ControlFlowGuard) -> Self { - self.words[0] = replace(self.words[0], 40, value as u64); + self.words[0] = replace(self.words[0], 40, value.bits()); self } /// Sets signed-binary loading policy. #[must_use] pub const fn signed_binaries(mut self, value: SignedBinaries) -> Self { - self.words[0] = replace(self.words[0], 44, value as u64); + self.words[0] = replace(self.words[0], 44, value.bits()); self } /// Sets non-system-font policy. #[must_use] pub const fn font_disable(mut self, value: FontDisable) -> Self { - self.words[0] = replace(self.words[0], 48, value as u64); + self.words[0] = replace(self.words[0], 48, value.bits()); self } /// Sets remote-image blocking. #[must_use] pub const fn block_remote_images(mut self, value: Mitigation) -> Self { - self.words[0] = replace(self.words[0], 52, value as u64); + self.words[0] = replace(self.words[0], 52, value.bits()); self } /// Sets low-integrity-label image blocking. #[must_use] pub const fn block_low_label_images(mut self, value: Mitigation) -> Self { - self.words[0] = replace(self.words[0], 56, value as u64); + self.words[0] = replace(self.words[0], 56, value.bits()); self } /// Sets System32 image preference. #[must_use] pub const fn prefer_system32_images(mut self, value: Mitigation) -> Self { - self.words[0] = replace(self.words[0], 60, value as u64); + self.words[0] = replace(self.words[0], 60, value.bits()); self } /// Sets loader integrity continuity. #[must_use] pub const fn loader_integrity(mut self, value: LoaderIntegrity) -> Self { - self.words[1] = replace(self.words[1], 4, value as u64); + self.words[1] = replace(self.words[1], 4, value.bits()); self } /// Sets strict Control Flow Guard. #[must_use] pub const fn strict_control_flow_guard(mut self, value: Mitigation) -> Self { - self.words[1] = replace(self.words[1], 8, value as u64); + self.words[1] = replace(self.words[1], 8, value.bits()); self } /// Sets module-tampering protection. #[must_use] pub const fn module_tampering(mut self, value: ModuleTampering) -> Self { - self.words[1] = replace(self.words[1], 12, value as u64); + self.words[1] = replace(self.words[1], 12, value.bits()); self } /// Sets restricted indirect branch prediction. #[must_use] pub const fn restrict_indirect_branch_prediction(mut self, value: Mitigation) -> Self { - self.words[1] = replace(self.words[1], 16, value as u64); + self.words[1] = replace(self.words[1], 16, value.bits()); self } /// Sets permission for a broker to downgrade dynamic-code policy. #[must_use] pub const fn allow_downgrade_dynamic_code(mut self, value: Mitigation) -> Self { - self.words[1] = replace(self.words[1], 20, value as u64); + self.words[1] = replace(self.words[1], 20, value.bits()); self } /// Sets speculative-store-bypass disabling. #[must_use] pub const fn disable_speculative_store_bypass(mut self, value: Mitigation) -> Self { - self.words[1] = replace(self.words[1], 24, value as u64); + self.words[1] = replace(self.words[1], 24, value.bits()); self } /// Sets CET user shadow stacks. #[must_use] pub const fn cet_user_shadow_stacks(mut self, value: CetShadowStacks) -> Self { - self.words[1] = replace(self.words[1], 28, value as u64); + self.words[1] = replace(self.words[1], 28, value.bits()); self } @@ -372,49 +393,49 @@ impl MitigationPolicy { mut self, value: UserCetContextIpValidation, ) -> Self { - self.words[1] = replace(self.words[1], 32, value as u64); + self.words[1] = replace(self.words[1], 32, value.bits()); self } /// Sets blocking of binaries without CET metadata. #[must_use] pub const fn block_non_cet_binaries(mut self, value: BlockNonCetBinaries) -> Self { - self.words[1] = replace(self.words[1], 36, value as u64); + self.words[1] = replace(self.words[1], 36, value.bits()); self } /// Sets extended Control Flow Guard. #[must_use] pub const fn extended_control_flow_guard(mut self, value: Mitigation) -> Self { - self.words[1] = replace(self.words[1], 40, value as u64); + self.words[1] = replace(self.words[1], 40, value.bits()); self } /// Sets ARM64 user-mode instruction-pointer authentication. #[must_use] pub const fn pointer_authentication(mut self, value: Mitigation) -> Self { - self.words[1] = replace(self.words[1], 44, value as u64); + self.words[1] = replace(self.words[1], 44, value.bits()); self } /// Sets CET dynamic APIs to out-of-process-only mode. #[must_use] pub const fn cet_dynamic_apis_out_of_process(mut self, value: Mitigation) -> Self { - self.words[1] = replace(self.words[1], 48, value as u64); + self.words[1] = replace(self.words[1], 48, value.bits()); self } /// Sets restricted CPU-core sharing. #[must_use] pub const fn restrict_core_sharing(mut self, value: Mitigation) -> Self { - self.words[1] = replace(self.words[1], 52, value as u64); + self.words[1] = replace(self.words[1], 52, value.bits()); self } /// Sets FSCTL system-call disabling. #[must_use] pub const fn disable_fsctl_system_calls(mut self, value: Mitigation) -> Self { - self.words[1] = replace(self.words[1], 56, value as u64); + self.words[1] = replace(self.words[1], 56, value.bits()); self } } @@ -468,8 +489,9 @@ mod tests { fn every_sdk_22621_field_has_the_expected_encoding() { macro_rules! field { ($policy:expr, $word:expr, $shift:expr, $value:expr) => {{ + let value: u64 = $value; let mut expected = [0_u64; 2]; - expected[$word] = ($value as u64) << $shift; + expected[$word] = value << $shift; assert_eq!($policy.words(), expected); }}; } diff --git a/src/options.rs b/src/options.rs index c049e95..e16c7e2 100644 --- a/src/options.rs +++ b/src/options.rs @@ -14,15 +14,21 @@ use crate::handles::{AsPseudoConsole, Job, ParentProcess}; use crate::mitigation::MitigationPolicy; /// What dropping a live [`crate::Child`] does to its process tree. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] pub enum DropPolicy { /// Close windows-spawn's process handle without terminating the process. - #[default] Detach, /// Terminate the child and all descendants in windows-spawn's private Job. KillTree, } +impl Default for DropPolicy { + /// Detaches, as dropping a [`std::process::Child`] does. + fn default() -> Self { + Self::Detach + } +} + /// Safe, named `CreateProcessW` creation flags. /// /// Unicode-environment, extended-startup-info, and suspended flags are set internally. @@ -164,7 +170,7 @@ impl<'a> SpawnOptions<'a> { /// Chooses another process as the logical parent. #[must_use] - pub fn parent_process(mut self, parent: &'a ParentProcess) -> Self { + pub const fn parent_process(mut self, parent: &'a ParentProcess) -> Self { self.parent = Some(parent); self } diff --git a/src/plan.rs b/src/plan.rs index 2e5272a..2ab6a25 100644 --- a/src/plan.rs +++ b/src/plan.rs @@ -276,16 +276,16 @@ mod tests { #[test] fn rejects_conflicting_console_flags() { let command = Command::new("cmd.exe"); - let options = SpawnOptions::new() + let detached_console = SpawnOptions::new() .creation_flags(CreationFlags::DETACHED_PROCESS | CreationFlags::NEW_CONSOLE); - assert!(SpawnPlan::new_running(&command, options, IoMode::Spawn).is_err()); + assert!(SpawnPlan::new_running(&command, detached_console, IoMode::Spawn).is_err()); for flags in [ CreationFlags::NO_WINDOW | CreationFlags::DETACHED_PROCESS, CreationFlags::NO_WINDOW | CreationFlags::NEW_CONSOLE, ] { - let options = SpawnOptions::new().creation_flags(flags); - assert!(SpawnPlan::new_running(&command, options, IoMode::Spawn).is_err()); + let conflicting = SpawnOptions::new().creation_flags(flags); + assert!(SpawnPlan::new_running(&command, conflicting, IoMode::Spawn).is_err()); } } diff --git a/src/sys.rs b/src/sys.rs index 4ea087c..febe37f 100644 --- a/src/sys.rs +++ b/src/sys.rs @@ -5,7 +5,7 @@ use std::ffi::{c_void, OsString}; use std::io; use std::mem::{size_of, size_of_val}; use std::os::windows::ffi::OsStringExt; -use std::os::windows::io::{AsRawHandle, BorrowedHandle, FromRawHandle, OwnedHandle, RawHandle}; +use std::os::windows::io::{AsRawHandle, BorrowedHandle, FromRawHandle, OwnedHandle}; use std::process::ExitStatus; use std::ptr; @@ -131,7 +131,7 @@ pub(crate) struct RemoteHandle<'a> { impl RemoteHandle<'_> { pub(crate) fn value(&self) -> isize { - self.value as isize + handle_value(self.value) } } @@ -140,13 +140,13 @@ impl Drop for RemoteHandle<'_> { fn drop(&mut self) { // SAFETY: `GetCurrentProcess` cannot fail and returns a pseudo-handle that stays valid and is never closed. let current = unsafe { GetCurrentProcess() }; - let _ = duplicate_between( + drop(duplicate_between( raw(self.process), self.value, current, false, DUPLICATE_SAME_ACCESS | DUPLICATE_CLOSE_SOURCE, - ); + )); } } @@ -193,7 +193,7 @@ pub(crate) fn standard_handle(stream: StandardStream) -> io::Result 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 as RawHandle), - OwnedHandle::from_raw_handle(write as RawHandle), + OwnedHandle::from_raw_handle(read), + OwnedHandle::from_raw_handle(write), ) }; if parent_reads { @@ -305,8 +305,7 @@ pub(crate) fn set_job_kill_on_close(handle: BorrowedHandle<'_>, enable: bool) -> raw(handle), JobObjectExtendedLimitInformation, ptr::addr_of!(limits).cast(), - u32::try_from(size_of::()) - .expect("Job limit structure size fits u32"), + dword(size_of::())?, ) } == 0 { @@ -328,8 +327,7 @@ fn query_job_limits( raw(handle), JobObjectExtendedLimitInformation, ptr::addr_of_mut!(limits).cast(), - u32::try_from(size_of::()) - .expect("Job limit structure size fits u32"), + dword(size_of::())?, ptr::null_mut(), ) } == 0 @@ -370,7 +368,9 @@ impl AttributeList { let words = storage_words(bytes); let mut storage = vec![0_usize; words].into_boxed_slice(); let pointer = storage.as_mut_ptr().cast(); - let mut actual = words * size_of::(); + let mut actual = words + .checked_mul(size_of::()) + .ok_or_else(|| io::Error::other("attribute list is too large"))?; // SAFETY: the `Box<[usize]>` is word-aligned, stable, at least `bytes` long, and owned by the returned `AttributeList`. if unsafe { InitializeProcThreadAttributeList(pointer, count, 0, &mut actual) } == 0 { return Err(io::Error::last_os_error()); @@ -380,7 +380,7 @@ impl AttributeList { pub(crate) fn set_handle_list(&mut self, handles: &[isize]) -> io::Result<()> { self.update( - PROC_THREAD_ATTRIBUTE_HANDLE_LIST as usize, + PROC_THREAD_ATTRIBUTE_HANDLE_LIST, handles.as_ptr().cast(), size_of_val(handles), ) @@ -388,15 +388,15 @@ impl AttributeList { pub(crate) fn set_parent(&mut self, parent: &isize) -> io::Result<()> { self.update( - PROC_THREAD_ATTRIBUTE_PARENT_PROCESS as usize, - (parent as *const isize).cast(), + PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, + ptr::addr_of!(*parent).cast(), size_of::(), ) } pub(crate) fn set_mitigation(&mut self, words: &[u64; 2]) -> io::Result<()> { self.update( - PROC_THREAD_ATTRIBUTE_MITIGATION_POLICY as usize, + PROC_THREAD_ATTRIBUTE_MITIGATION_POLICY, words.as_ptr().cast(), size_of::<[u64; 2]>(), ) @@ -404,7 +404,7 @@ impl AttributeList { pub(crate) fn set_jobs(&mut self, jobs: &[isize]) -> io::Result<()> { self.update( - PROC_THREAD_ATTRIBUTE_JOB_LIST as usize, + PROC_THREAD_ATTRIBUTE_JOB_LIST, jobs.as_ptr().cast(), size_of_val(jobs), ) @@ -413,15 +413,16 @@ impl AttributeList { /// Unlike the other attributes, `lpValue` is the `HPCON` value itself, not its address. pub(crate) fn set_pseudoconsole(&mut self, pseudoconsole: isize) -> io::Result<()> { self.update( - PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE as usize, - pseudoconsole as *const c_void, + PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE, + handle_from_value(pseudoconsole).cast_const(), size_of::(), ) } - fn update(&mut self, attribute: usize, value: *const c_void, bytes: usize) -> io::Result<()> { + fn update(&mut self, attribute: u32, value: *const c_void, bytes: usize) -> io::Result<()> { #[cfg(test)] fault::check(fault::Call::UpdateAttribute)?; + let attribute = usize::try_from(attribute).map_err(io::Error::other)?; // 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. if unsafe { UpdateProcThreadAttribute( @@ -453,15 +454,14 @@ fn probed_size(probe: i32, error: io::Error, bytes: usize) -> io::Result "attribute-list size probe unexpectedly succeeded", )); } - let insufficient = i32::try_from(ERROR_INSUFFICIENT_BUFFER).expect("Win32 error code fits i32"); - if error.raw_os_error() != Some(insufficient) || bytes == 0 { + if !is_win32_error(&error, ERROR_INSUFFICIENT_BUFFER) || bytes == 0 { return Err(error); } Ok(bytes) } /// Returns how many words hold `bytes`. -fn storage_words(bytes: usize) -> usize { +const fn storage_words(bytes: usize) -> usize { bytes.div_ceil(size_of::()) } @@ -510,10 +510,9 @@ pub(crate) fn create_process(request: &mut ProcessRequest<'_>) -> io::Result()).expect("startup structure size fits u32") + dword(size_of::())? } else { - u32::try_from(size_of::()) - .expect("startup structure size fits u32") + dword(size_of::())? }; set_standard_handles(&mut startup, request.stdio); startup.lpAttributeList = request @@ -555,8 +554,8 @@ pub(crate) fn create_process(request: &mut ProcessRequest<'_>) -> io::Result) -> io::Result BorrowedHandle<'static> { // SAFETY: `GetCurrentProcess` cannot fail and returns a pseudo-handle valid for the process lifetime. // `BorrowedHandle` never closes it, so a `'static` borrow cannot dangle or double-close. - unsafe { BorrowedHandle::borrow_raw(GetCurrentProcess() as RawHandle) } + unsafe { BorrowedHandle::borrow_raw(GetCurrentProcess()) } } pub(crate) fn process_handle_count(process: BorrowedHandle<'_>) -> std::io::Result { @@ -805,7 +802,7 @@ pub(crate) mod test_support { }; assert_ne!(duplicated, 0, "DuplicateHandle failed"); // SAFETY: DuplicateHandle returned a new, uniquely owned handle. - unsafe { OwnedHandle::from_raw_handle(duplicate as RawHandle) } + unsafe { OwnedHandle::from_raw_handle(duplicate) } } /// Returns a non-inheritable duplicate limited to `access`. @@ -825,7 +822,7 @@ pub(crate) mod test_support { }; assert_ne!(duplicated, 0, "DuplicateHandle failed"); // SAFETY: DuplicateHandle returned a new, uniquely owned handle. - unsafe { OwnedHandle::from_raw_handle(duplicate as RawHandle) } + unsafe { OwnedHandle::from_raw_handle(duplicate) } } /// Returns true if `handle` is inheritable. @@ -847,8 +844,8 @@ pub(crate) mod test_support { // SAFETY: CreatePipe succeeded, so both handles are new and distinct. unsafe { ( - OwnedHandle::from_raw_handle(read as RawHandle), - OwnedHandle::from_raw_handle(write as RawHandle), + OwnedHandle::from_raw_handle(read), + OwnedHandle::from_raw_handle(write), ) } } @@ -955,18 +952,13 @@ pub(crate) fn read_handle(handle: BorrowedHandle<'_>, buffer: &mut [u8]) -> io:: } == 0 { let error = io::Error::last_os_error(); - if matches!( - error.raw_os_error(), - Some(code) - if code == i32::try_from(ERROR_BROKEN_PIPE).expect("Win32 error code fits i32") - || code == i32::try_from(ERROR_HANDLE_EOF).expect("Win32 error code fits i32") - ) { + if is_win32_error(&error, ERROR_BROKEN_PIPE) || is_win32_error(&error, ERROR_HANDLE_EOF) { Ok(0) } else { Err(error) } } else { - Ok(read as usize) + usize::try_from(read).map_err(io::Error::other) } } @@ -991,7 +983,7 @@ pub(crate) fn write_handle(handle: BorrowedHandle<'_>, buffer: &[u8]) -> io::Res { Err(io::Error::last_os_error()) } else { - Ok(written as usize) + usize::try_from(written).map_err(io::Error::other) } } @@ -1020,15 +1012,8 @@ pub(crate) fn environment_strings() -> io::Result> { } // SAFETY: the range was just measured inside the current entry. let entry = unsafe { std::slice::from_raw_parts(cursor, length) }; - if let Some(separator) = entry[1..] - .iter() - .position(|unit| *unit == u16::from(b'=')) - .map(|index| index + 1) - { - entries.push(( - OsString::from_wide(&entry[..separator]), - OsString::from_wide(&entry[separator + 1..]), - )); + if let Some((key, value)) = split_entry(entry) { + entries.push((OsString::from_wide(key), OsString::from_wide(value))); } let advance = length .checked_add(1) @@ -1039,6 +1024,19 @@ pub(crate) fn environment_strings() -> io::Result> { Ok(entries) } +/// Splits `KEY=value` at the first `=` after the first unit, so hidden `=C:` entries keep their key. +fn split_entry(entry: &[u16]) -> Option<(&[u16], &[u16])> { + let equals = u16::from(b'='); + let separator = entry + .iter() + .skip(1) + .position(|unit| *unit == equals)? + .checked_add(1)?; + let key = entry.get(..separator)?; + let value = entry.get(separator.checked_add(1)?..)?; + Some((key, value)) +} + pub(crate) fn compare_ordinal(left: &[u16], right: &[u16]) -> Ordering { let left_len = i32::try_from(left.len()).unwrap_or(i32::MAX); let right_len = i32::try_from(right.len()).unwrap_or(i32::MAX); @@ -1099,8 +1097,8 @@ fn maximum_path(fill: impl FnOnce(*mut u16, u32) -> u32) -> io::Result> #[cfg(test)] fault::check(fault::Call::MaximumPath)?; let mut buffer = vec![0_u16; 32_768]; - let capacity = u32::try_from(buffer.len()).expect("maximum Windows path fits u32"); - let length = fill(buffer.as_mut_ptr(), capacity) as usize; + let capacity = dword(buffer.len())?; + let length = usize::try_from(fill(buffer.as_mut_ptr(), capacity)).map_err(io::Error::other)?; if length == 0 { return Err(io::Error::last_os_error()); } @@ -1114,13 +1112,13 @@ fn maximum_path(fill: impl FnOnce(*mut u16, u32) -> u32) -> io::Result> } fn raw(handle: BorrowedHandle<'_>) -> HANDLE { - handle.as_raw_handle() as HANDLE + handle.as_raw_handle() } fn owned(handle: HANDLE) -> io::Result { if is_valid_handle(handle) { // SAFETY: callers pass a new handle and transfer its only local ownership. - Ok(unsafe { OwnedHandle::from_raw_handle(handle as RawHandle) }) + Ok(unsafe { OwnedHandle::from_raw_handle(handle) }) } else { Err(io::Error::last_os_error()) } @@ -1130,6 +1128,30 @@ fn is_valid_handle(handle: HANDLE) -> bool { !handle.is_null() && handle != INVALID_HANDLE_VALUE } +/// Returns a handle's numeric value, as a child sees it in an argument or the environment. +#[allow(clippy::as_conversions)] +pub(crate) fn handle_value(handle: HANDLE) -> isize { + handle as isize +} + +/// Returns the handle a numeric value names. +#[allow(clippy::as_conversions)] +pub(crate) const fn handle_from_value(value: isize) -> HANDLE { + value as HANDLE +} + +/// Converts a size to a Win32 `DWORD`. +fn dword(value: usize) -> io::Result { + u32::try_from(value).map_err(io::Error::other) +} + +/// Returns true if `error` carries the Win32 error `code`. +fn is_win32_error(error: &io::Error, code: u32) -> bool { + error + .raw_os_error() + .is_some_and(|raw| u32::try_from(raw) == Ok(code)) +} + fn bool_result(result: i32) -> io::Result<()> { if result == 0 { Err(io::Error::last_os_error()) @@ -1224,9 +1246,9 @@ mod tests { ); let process = open_parent_process(std::process::id())?; let inherited = duplicate_local(job.as_handle(), true)?; - let handles = [inherited.as_raw_handle() as isize]; - let jobs = [job.as_raw_handle() as isize]; - let parent = process.as_raw_handle() as isize; + 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 words = [1_u64, 0_u64]; let mut attributes = AttributeList::new(4)?; attributes.set_handle_list(&handles)?; @@ -1236,7 +1258,7 @@ mod tests { drop(attributes); let mut pseudoconsole = AttributeList::new(1)?; - let _ = pseudoconsole.set_pseudoconsole(1); + drop(pseudoconsole.set_pseudoconsole(1)); drop(pseudoconsole); assert!(ATTRIBUTE_LIST_DROPS.load(std::sync::atomic::Ordering::Relaxed) > drops_before); @@ -1267,9 +1289,9 @@ mod tests { }), ); assert_ne!(ordinary.StartupInfo.dwFlags & STARTF_USESTDHANDLES, 0); - assert_eq!(ordinary.StartupInfo.hStdInput as isize, 1); - assert_eq!(ordinary.StartupInfo.hStdOutput as isize, 2); - assert_eq!(ordinary.StartupInfo.hStdError as isize, 3); + assert_eq!(handle_value(ordinary.StartupInfo.hStdInput), 1); + assert_eq!(handle_value(ordinary.StartupInfo.hStdOutput), 2); + assert_eq!(handle_value(ordinary.StartupInfo.hStdError), 3); } #[test] @@ -1318,7 +1340,7 @@ mod tests { assert!(open_parent_process(u32::MAX).is_err()); let file = File::open("NUL")?; - assert!(is_valid_handle(file.as_raw_handle() as HANDLE)); + 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()); @@ -1365,7 +1387,7 @@ mod tests { assert!(exit_status(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() as RawHandle) }; + 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()); @@ -1379,7 +1401,7 @@ mod tests { assert!(AttributeList::new(u32::MAX).is_err()); let mut full = AttributeList::new(1)?; - let jobs = [job.as_raw_handle() as isize]; + 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()); diff --git a/src/transaction.rs b/src/transaction.rs index 6314a63..2d56d05 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -48,9 +48,7 @@ impl SpawnTransaction { impl SpawnTransaction { #[allow(clippy::too_many_lines)] - pub(crate) fn new<'command, 'options>( - plan: &SpawnPlan<'command, 'options, M>, - ) -> io::Result { + pub(crate) fn new<'options>(plan: &SpawnPlan<'_, 'options, M>) -> io::Result { let parent: Option> = plan.options.parent.map(AsHandle::as_handle); let mut transfer = HandleTransfer::new(parent); let (stdio_values, stdio) = match &plan.stdio { @@ -88,10 +86,10 @@ impl SpawnTransaction { .options .jobs .iter() - .map(|job| job.as_handle().as_raw_handle() as isize) + .map(|job| sys::handle_value(job.as_handle().as_raw_handle())) .collect(); if let Some(job) = &kill_job { - job_values.push(job.as_handle().as_raw_handle() as isize); + job_values.push(sys::handle_value(job.as_handle().as_raw_handle())); } let command_line = build_command_line(plan.command, &mut transfer)?; @@ -108,21 +106,28 @@ impl SpawnTransaction { let inherited_values = transfer.inherited_values().to_vec().into_boxed_slice(); let parent_value = transfer .parent() - .map(|handle| Box::new(handle.as_raw_handle() as isize)); + .map(|handle| Box::new(sys::handle_value(handle.as_raw_handle()))); let mitigation_words = plan.options.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_raw(); - let attribute_count = u32::from(!inherited_values.is_empty()) - + u32::from(parent_value.is_some()) - + u32::from(mitigation_value.is_some()) - + u32::from(!job_values.is_empty()) - + u32::from(pseudoconsole.is_some()); + let attribute_count = [ + !inherited_values.is_empty(), + parent_value.is_some(), + mitigation_value.is_some(), + !job_values.is_empty(), + pseudoconsole.is_some(), + ] + .into_iter() + .filter(|present| *present) + .count(); let mut attributes = if attribute_count == 0 { None } else { - Some(sys::AttributeList::new(attribute_count)?) + Some(sys::AttributeList::new( + u32::try_from(attribute_count).map_err(io::Error::other)?, + )?) }; if let Some(list) = &mut attributes { if !inherited_values.is_empty() { @@ -167,6 +172,7 @@ impl SpawnTransaction { }) } + #[allow(clippy::expect_used)] fn commit_parts(mut self) -> (Child, OwnedHandle) { let created = self .created @@ -202,7 +208,7 @@ impl SpawnTransaction { impl Drop for SpawnTransaction { fn drop(&mut self) { if let Some(created) = &self.created { - let _ = sys::terminate_process(created.process.as_handle(), 1); + drop(sys::terminate_process(created.process.as_handle(), 1)); } drop(self.kill_job.take()); } @@ -296,7 +302,7 @@ struct HandleTransfer<'a> { } impl<'a> HandleTransfer<'a> { - fn new(parent: Option>) -> Self { + const fn new(parent: Option>) -> Self { Self { parent, local: Vec::new(), @@ -313,7 +319,7 @@ impl<'a> HandleTransfer<'a> { value } else { let handle = sys::duplicate_local(source, true)?; - let value = handle.as_raw_handle() as isize; + let value = sys::handle_value(handle.as_raw_handle()); self.local.push(handle); value }; @@ -323,7 +329,7 @@ impl<'a> HandleTransfer<'a> { Ok(value) } - fn parent(&self) -> Option> { + const fn parent(&self) -> Option> { self.parent } @@ -539,10 +545,10 @@ fn append_regular_arg(command: &mut Vec, argument: &OsStr) { let mut backslashes = 0_usize; for unit in argument.encode_wide() { if unit == BACKSLASH { - backslashes += 1; + backslashes = backslashes.saturating_add(1); } else { if unit == QUOTE { - command.extend(iter::repeat(BACKSLASH).take(backslashes + 1)); + command.extend(iter::repeat(BACKSLASH).take(backslashes.saturating_add(1))); } backslashes = 0; } @@ -555,12 +561,12 @@ fn append_regular_arg(command: &mut Vec, argument: &OsStr) { } fn resolve_executable(program: &OsStr, child_path: Option<&OsStr>) -> io::Result> { - let path = Path::new(program); + let program_path = Path::new(program); let has_exe_suffix = program .as_encoded_bytes() .get(program.len().saturating_sub(4)..) .is_some_and(|suffix| suffix.eq_ignore_ascii_case(b".exe")); - let is_file_name = path.file_name() == Some(program); + let is_file_name = program_path.file_name() == Some(program); if !is_file_name { if has_exe_suffix { @@ -586,7 +592,7 @@ fn resolve_executable(program: &OsStr, child_path: Option<&OsStr>) -> io::Result }; if let Some(paths) = child_path { - for directory in env::split_paths(paths).filter(|path| !path.as_os_str().is_empty()) { + for directory in env::split_paths(paths).filter(|entry| !entry.as_os_str().is_empty()) { if let Some(found) = search(directory) { return Ok(found); } @@ -605,7 +611,7 @@ fn resolve_executable(program: &OsStr, child_path: Option<&OsStr>) -> io::Result return Ok(found); } if let Some(paths) = env::var_os("PATH") { - for directory in env::split_paths(&paths).filter(|path| !path.as_os_str().is_empty()) { + for directory in env::split_paths(&paths).filter(|entry| !entry.as_os_str().is_empty()) { if let Some(found) = search(directory) { return Ok(found); } diff --git a/tests/argv_roundtrip.rs b/tests/argv_roundtrip.rs index 98c9d89..f5a9b9e 100644 --- a/tests/argv_roundtrip.rs +++ b/tests/argv_roundtrip.rs @@ -1,4 +1,5 @@ //! End-to-end UTF-16 argument and environment round-trip test. +#![allow(clippy::as_conversions, clippy::expect_used, clippy::unwrap_in_result)] #[cfg(windows)] mod windows { diff --git a/tests/support/mod.rs b/tests/support/mod.rs index a8b7abd..4478223 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -133,7 +133,7 @@ pub(crate) enum Role { } impl Role { - fn name(self) -> &'static str { + const fn name(self) -> &'static str { match self { Self::Gate => "gate", Self::Ran => "ran", @@ -198,18 +198,21 @@ pub(crate) fn run_probe_if_requested() { let Some(role) = std::env::var_os(ROLE) else { return; }; - let code = match run_role(&role) { - Ok(code) => code, - Err(_) => EXIT_SETUP_FAILED, - }; - std::process::exit(i32::from_ne_bytes(code.to_ne_bytes())); + let code = run_role(&role).unwrap_or(EXIT_SETUP_FAILED); + exit(code); +} + +/// Ends the probe with `code`; a probe reports by exit code, and no destructor may run after the gate. +#[allow(clippy::disallowed_methods)] +fn exit(code: u32) -> ! { + std::process::exit(i32::from_ne_bytes(code.to_ne_bytes())) } fn run_role(role: &OsString) -> io::Result { let role = role.to_str().unwrap_or_default(); if role == "ran" { let mut report = adopt_pipe(REPORT)?; - let _ = report.write_all(b"!"); + drop(report.write_all(b"!")); return Ok(EXIT_RAN); } let gate = adopt_pipe(GATE)?; @@ -251,7 +254,7 @@ fn wait_on_gate(mut gate: File, mut report: File) -> u32 { match gate.read(&mut byte) { Ok(1) => u32::from(byte[0]), Ok(_) => { - let _ = report.write_all(b"x"); + drop(report.write_all(b"x")); EXIT_RELEASED } Err(_) => loop { @@ -393,6 +396,6 @@ impl TempDir { impl Drop for TempDir { fn drop(&mut self) { - let _ = fs::remove_dir_all(&self.0); + drop(fs::remove_dir_all(&self.0)); } } diff --git a/tests/windows_spawn.rs b/tests/windows_spawn.rs index 4b81a0c..88c5919 100644 --- a/tests/windows_spawn.rs +++ b/tests/windows_spawn.rs @@ -1,5 +1,6 @@ #![cfg_attr(not(windows), allow(missing_docs))] #![cfg(windows)] +#![allow(clippy::as_conversions, clippy::expect_used, clippy::unwrap_in_result)] //! End-to-end Windows process creation tests. @@ -525,10 +526,10 @@ fn child_pipes_try_wait_and_cached_lifecycle_work() -> io::Result<()> { drop(stdin); let _ = child.stdout.as_ref().expect("piped stdout").as_handle(); let _ = child.stderr.as_ref().expect("piped stderr").as_handle(); - let output = child.wait_with_output()?; - assert!(output.status.success()); - assert_eq!(output.stdout, b"out:hello"); - assert_eq!(output.stderr, b"err"); + let piped = child.wait_with_output()?; + assert!(piped.status.success()); + assert_eq!(piped.stdout, b"out:hello"); + assert_eq!(piped.stderr, b"err"); let mut exited = cmd("exit /b 0").spawn()?; assert!(exited.wait()?.success()); @@ -549,8 +550,8 @@ fn child_pipes_try_wait_and_cached_lifecycle_work() -> io::Result<()> { let mut silent = cmd("exit /b 0"); silent.stdout(Stdio::null()).stderr(Stdio::null()); - let output = silent.spawn()?.wait_with_output()?; - assert!(output.stdout.is_empty() && output.stderr.is_empty()); + let silenced = silent.spawn()?.wait_with_output()?; + assert!(silenced.stdout.is_empty() && silenced.stderr.is_empty()); Ok(()) } @@ -609,14 +610,18 @@ impl TestPseudoConsole { drop((input_reader, output_writer)); let (sender, output) = mpsc::channel(); let mut output_reader = File::from(output_reader); - let _ = thread::spawn(move || { + drop(thread::spawn(move || { let mut buffer = [0_u8; 4096]; while let Ok(read) = output_reader.read(&mut buffer) { - if read == 0 || sender.send(buffer[..read].to_vec()).is_err() { + if read == 0 + || sender + .send(buffer.get(..read).unwrap_or_default().to_vec()) + .is_err() + { break; } } - }); + })); Ok(Self { value, input_writer: Some(input_writer), @@ -631,7 +636,7 @@ impl TestPseudoConsole { .expect("a live pseudoconsole retains its input writer"); let mut written = 0_u32; let length = u32::try_from(input.len()) - .map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "input is too large"))?; + .map_err(|error| io::Error::new(io::ErrorKind::InvalidInput, error))?; // SAFETY: the buffer and byte count are valid for the synchronous write, and `self` owns the writer. if unsafe { WriteFile( @@ -674,10 +679,10 @@ impl Drop for TestPseudoConsole { fn drop(&mut self) { drop(self.input_writer.take()); let value = self.value; - let _ = thread::spawn(move || { + drop(thread::spawn(move || { // SAFETY: this type uniquely owns the HPCON. unsafe { ClosePseudoConsole(value) }; - }); + })); } } @@ -763,14 +768,18 @@ fn forward( index: usize, sender: mpsc::Sender<(usize, Vec)>, ) { - let _ = thread::spawn(move || { + drop(thread::spawn(move || { let mut buffer = [0_u8; 4096]; while let Ok(read) = stream.read(&mut buffer) { - if read == 0 || sender.send((index, buffer[..read].to_vec())).is_err() { + if read == 0 + || sender + .send((index, buffer.get(..read).unwrap_or_default().to_vec())) + .is_err() + { break; } } - }); + })); } fn contains(haystack: &[u8], needle: &[u8]) -> bool { @@ -996,12 +1005,12 @@ fn reusable_command_environment_and_accessors_work() -> io::Result<()> { .current_dir(directory.path()); assert_eq!(command.get_program(), "cmd.exe"); assert_eq!(command.get_current_dir(), Some(directory.path())); - let output = command.output()?; - assert!(String::from_utf8_lossy(&output.stdout).contains("one-two")); + let modified = command.output()?; + assert!(String::from_utf8_lossy(&modified.stdout).contains("one-two")); command.env_clear().env("WINDOWS_SPAWN_ONE", "clear"); - let output = command.output()?; - assert!(String::from_utf8_lossy(&output.stdout).contains("clear-")); + let cleared = command.output()?; + assert!(String::from_utf8_lossy(&cleared.stdout).contains("clear-")); Ok(()) } diff --git a/xtask/src/cli.rs b/xtask/src/cli.rs index 985dd54..57b0c01 100644 --- a/xtask/src/cli.rs +++ b/xtask/src/cli.rs @@ -4,6 +4,7 @@ use std::path::PathBuf; pub(crate) enum SimpleTask { Fmt, Clippy, + Gates, Test, Doc, Msrv, @@ -58,6 +59,7 @@ pub(crate) fn parse(arguments: impl IntoIterator) -> Result no_arguments(&rest, Task::Simple(SimpleTask::Fmt)), "clippy" => no_arguments(&rest, Task::Simple(SimpleTask::Clippy)), + "gates" => no_arguments(&rest, Task::Simple(SimpleTask::Gates)), "test" => no_arguments(&rest, Task::Simple(SimpleTask::Test)), "doc" => no_arguments(&rest, Task::Simple(SimpleTask::Doc)), "msrv" => no_arguments(&rest, Task::Simple(SimpleTask::Msrv)), @@ -85,10 +87,9 @@ pub(crate) fn parse(arguments: impl IntoIterator) -> Result Result { - if rest.is_empty() { - Ok(task) - } else { - Err(format!("unexpected argument: {}", rest[0])) + match rest { + [] => Ok(task), + [first, ..] => Err(format!("unexpected argument: {first}")), } } diff --git a/xtask/src/gates.rs b/xtask/src/gates.rs new file mode 100644 index 0000000..4691e43 --- /dev/null +++ b/xtask/src/gates.rs @@ -0,0 +1,822 @@ +//! Repository laws Clippy cannot express, checked over the tokens of every Rust file (ADR 0013). + +use std::fmt; +use std::fs; +use std::io; +use std::path::{Path, PathBuf}; + +/// A lint exception the repository accepts, with the number of attributes that claim it. +struct Exception { + file: &'static str, + lint: &'static str, + count: usize, + reason: &'static str, +} + +/// Every `#[allow]` in the repository; an attribute outside this table, or a count that differs, fails the gate. +const EXCEPTIONS: &[Exception] = &[ + Exception { + file: "src/lib.rs", + lint: "unsafe_code", + count: 2, + reason: "sys is the only Win32 FFI boundary; failure_tests creates pseudoconsoles for tests", + }, + Exception { + file: "src/handles.rs", + lint: "unsafe_code", + count: 1, + reason: "AsPseudoConsole is an unsafe trait whose implementors vouch for a live HPCON", + }, + Exception { + file: "src/options.rs", + lint: "unsafe_code", + count: 1, + reason: "tests implement AsPseudoConsole", + }, + Exception { + file: "src/plan.rs", + lint: "unsafe_code", + count: 1, + reason: "tests implement AsPseudoConsole", + }, + Exception { + file: "src/sys.rs", + lint: "clippy::as_conversions", + count: 2, + reason: "handle values convert between pointers and integers, which has no From form before Rust 1.84", + }, + Exception { + file: "src/child.rs", + lint: "clippy::expect_used", + count: 2, + 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: "src/mitigation.rs", + lint: "clippy::too_many_lines", + count: 1, + reason: "the encoding test lists every SDK 22621 field", + }, + Exception { + file: "tests/support/mod.rs", + lint: "clippy::disallowed_methods", + count: 1, + reason: "a probe reports by exit code", + }, + Exception { + file: "tests/windows_spawn.rs", + lint: "missing_docs", + count: 1, + reason: "the test crate is empty off Windows", + }, + Exception { + file: "tests/windows_spawn.rs", + lint: "clippy::as_conversions", + count: 1, + reason: "tests call Win32 directly with handle values", + }, + Exception { + file: "tests/windows_spawn.rs", + lint: "clippy::expect_used", + count: 1, + reason: "tests assert by panicking", + }, + Exception { + file: "tests/windows_spawn.rs", + lint: "clippy::unwrap_in_result", + count: 1, + reason: "tests assert by panicking", + }, + Exception { + file: "tests/argv_roundtrip.rs", + lint: "clippy::as_conversions", + count: 1, + reason: "tests call Win32 directly with handle values", + }, + Exception { + file: "tests/argv_roundtrip.rs", + lint: "clippy::expect_used", + count: 1, + reason: "tests assert by panicking", + }, + Exception { + file: "tests/argv_roundtrip.rs", + lint: "clippy::unwrap_in_result", + count: 1, + reason: "tests assert by panicking", + }, +]; + +/// Identifiers that measure or wait on time (ADR 0010). +const TIME_NAMES: [&str; 6] = [ + "SystemTime", + "Instant", + "Duration", + "UNIX_EPOCH", + "sleep", + "elapsed", +]; + +/// Identifier fragments that name a time bound, matched case-insensitively. +const TIME_FRAGMENTS: [&str; 2] = ["timeout", "deadline"]; + +/// Time-named identifiers that decide nothing by time. +/// +/// `WAIT_TIMEOUT` is what a 0 ms `WaitForSingleObject` query returns for an unsignaled object. +const TIME_NAME_EXCEPTIONS: [&str; 1] = ["WAIT_TIMEOUT"]; + +/// Shell commands that wait for a duration, matched case-insensitively inside string literals. +/// +/// Each is split so this table does not match itself. +const DELAY_COMMANDS: [&str; 3] = [ + concat!("ping", " -n"), + concat!("start", "-sleep"), + concat!("timeout", " /t"), +]; + +/// A rule violation. +#[derive(Debug, PartialEq, Eq)] +pub(crate) struct Finding { + pub(crate) file: String, + pub(crate) line: usize, + pub(crate) rule: String, +} + +impl fmt::Display for Finding { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(formatter, "{}:{}: {}", self.file, self.line, self.rule) + } +} + +/// Checks every Rust file under the repository's source directories. +pub(crate) fn check(root: &Path) -> io::Result> { + let mut files = Vec::new(); + for directory in ["src", "tests", "examples", "xtask/src"] { + collect_rust_files(&root.join(directory), &mut files)?; + } + files.sort(); + let mut findings = Vec::new(); + let mut allows = Vec::new(); + for path in files { + let relative = path + .strip_prefix(root) + .unwrap_or(&path) + .to_string_lossy() + .replace('\\', "/"); + let source = fs::read_to_string(&path)?; + let scan = scan(&relative, &source); + findings.extend(scan.findings); + allows.extend(scan.allows); + } + findings.extend(check_exceptions(&allows, EXCEPTIONS)); + Ok(findings) +} + +fn collect_rust_files(directory: &Path, files: &mut Vec) -> io::Result<()> { + for entry in fs::read_dir(directory)? { + let path = entry?.path(); + if path.is_dir() { + collect_rust_files(&path, files)?; + } else if path.extension().is_some_and(|extension| extension == "rs") { + files.push(path); + } + } + Ok(()) +} + +/// An `#[allow]` lint and where it appears. +#[derive(Debug, PartialEq, Eq)] +struct Allow { + file: String, + line: usize, + lint: String, +} + +fn check_exceptions(allows: &[Allow], exceptions: &[Exception]) -> Vec { + let mut findings = Vec::new(); + for allow in allows { + let registered = exceptions + .iter() + .any(|exception| exception.file == allow.file && exception.lint == allow.lint); + if !registered { + findings.push(Finding { + file: allow.file.clone(), + line: allow.line, + rule: format!( + "#[allow({})] is not registered in xtask/src/gates.rs", + allow.lint + ), + }); + } + } + for exception in exceptions { + let count = allows + .iter() + .filter(|allow| allow.file == exception.file && allow.lint == exception.lint) + .count(); + if count != exception.count { + findings.push(Finding { + file: exception.file.to_owned(), + line: 0, + rule: format!( + "{} is registered {} time(s) ({}) but allowed {count} time(s)", + exception.lint, exception.count, exception.reason + ), + }); + } + } + findings +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum CommentKind { + Doc, + Plain, +} + +#[derive(Clone, Debug, PartialEq, Eq)] +enum Token { + Ident(String), + Literal(String), + Str(String), + Punct(char), + Comment { + kind: CommentKind, + text: String, + alone: bool, + block: bool, + }, +} + +struct Scan { + findings: Vec, + allows: Vec, +} + +fn scan(file: &str, source: &str) -> Scan { + let tokens = lex(source); + let mut findings = Vec::new(); + let mut finding = |line: usize, rule: String| { + findings.push(Finding { + file: file.to_owned(), + line, + rule, + }); + }; + check_comments(&tokens, &mut finding); + let code: Vec<&(usize, Token)> = tokens + .iter() + .filter(|(_, token)| !matches!(token, Token::Comment { .. })) + .collect(); + let mut allows = Vec::new(); + for (index, (line, token)) in code.iter().enumerate() { + let next = |offset: usize| { + code.get(index.saturating_add(offset)) + .map(|(_, following)| following) + }; + match token { + Token::Ident(name) => { + if is_time_name(name) { + finding( + *line, + format!("`{name}` uses time; wait for the event itself (ADR 0010)"), + ); + } + if name == "Box" + && next(1) == Some(&Token::Punct('<')) + && next(2) == Some(&Token::Ident("dyn".to_owned())) + { + finding( + *line, + "own a closed set with an enum or an open one with a generic, not Box" + .to_owned(), + ); + } + if name == "WaitForSingleObject" && next(1) == Some(&Token::Punct('(')) { + if let Some(argument) = second_argument(&code, index.saturating_add(2)) { + let bounded = !matches!( + argument.as_slice(), + [Token::Ident(bound)] if bound == "INFINITE" + ) && !matches!(argument.as_slice(), [Token::Literal(value)] if value == "0"); + if bounded { + finding( + *line, + "WaitForSingleObject waits only INFINITE or 0 (ADR 0010)" + .to_owned(), + ); + } + } + } + } + Token::Str(text) => { + let lower = text.to_ascii_lowercase(); + if DELAY_COMMANDS.iter().any(|command| lower.contains(command)) { + finding( + *line, + "a string scripts a delay; wait for the event itself (ADR 0010)".to_owned(), + ); + } + } + Token::Punct('#') => { + let start = if next(1) == Some(&Token::Punct('!')) { + 2 + } else { + 1 + }; + if next(start) == Some(&Token::Punct('[')) { + let content = bracketed(&code, index.saturating_add(start)); + if matches!(content.as_slice(), [Token::Ident(name)] if name == "default") { + finding( + *line, + "#[default] lets declaration order pick a default; write impl Default" + .to_owned(), + ); + } + for lint in allowed_lints(&content) { + allows.push(Allow { + file: file.to_owned(), + line: *line, + lint, + }); + } + } + } + Token::Literal(_) | Token::Punct(_) | Token::Comment { .. } => {} + } + } + Scan { findings, allows } +} + +fn is_time_name(name: &str) -> bool { + let lower = name.to_ascii_lowercase(); + let named = TIME_NAMES.contains(&name) + || TIME_FRAGMENTS + .iter() + .any(|fragment| lower.contains(fragment)); + named && !TIME_NAME_EXCEPTIONS.contains(&name) +} + +/// Allows doc comments and runs of whole-line `//` comments that start with `SAFETY:`. +fn check_comments(tokens: &[(usize, Token)], finding: &mut impl FnMut(usize, String)) { + let mut previous_line: Option = None; + let mut in_safety = false; + for (line, token) in tokens { + let Token::Comment { + kind, + text, + alone, + block, + } = token + else { + previous_line = None; + continue; + }; + if *kind == CommentKind::Doc { + previous_line = None; + continue; + } + let continues = *alone + && !*block + && previous_line.is_some_and(|previous| previous.saturating_add(1) == *line); + if !continues { + in_safety = *alone && !*block && text.trim_start().starts_with("SAFETY:"); + } + if !in_safety { + finding( + *line, + "only doc comments and `// SAFETY:` comments are written; put the reason in the commit message" + .to_owned(), + ); + } + previous_line = if *alone && !*block { Some(*line) } else { None }; + } +} + +/// Returns the tokens between the `[` at `open` and its matching `]`. +fn bracketed(code: &[&(usize, Token)], open: usize) -> Vec { + let mut depth = 0_usize; + let mut content = Vec::new(); + for (_, token) in code.iter().skip(open) { + if *token == Token::Punct('[') { + depth = depth.saturating_add(1); + if depth == 1 { + continue; + } + } else if *token == Token::Punct(']') { + depth = depth.saturating_sub(1); + if depth == 0 { + return content; + } + } + content.push(token.clone()); + } + content +} + +/// Returns the second top-level argument of the call whose tokens start at `after_open`. +fn second_argument(code: &[&(usize, Token)], after_open: usize) -> Option> { + let mut depth = 0_usize; + let mut argument = 0_usize; + let mut tokens = Vec::new(); + for (_, token) in code.iter().skip(after_open) { + match token { + Token::Punct('(' | '[' | '{') => depth = depth.saturating_add(1), + Token::Punct(')' | ']' | '}') if depth == 0 => { + return (argument == 1).then_some(tokens); + } + Token::Punct(')' | ']' | '}') => depth = depth.saturating_sub(1), + Token::Punct(',') if depth == 0 => { + if argument == 1 { + return Some(tokens); + } + argument = argument.saturating_add(1); + continue; + } + Token::Ident(_) + | Token::Literal(_) + | Token::Str(_) + | Token::Punct(_) + | Token::Comment { .. } => {} + } + if argument == 1 { + tokens.push(token.clone()); + } + } + None +} + +/// Returns the lint paths named by `allow(...)` inside an attribute. +fn allowed_lints(content: &[Token]) -> Vec { + let mut lints = Vec::new(); + let mut index = 0_usize; + while let Some(token) = content.get(index) { + let opens = matches!(token, Token::Ident(name) if name == "allow") + && content.get(index.saturating_add(1)) == Some(&Token::Punct('(')); + if opens { + let mut path = String::new(); + let mut depth = 0_usize; + for inner in content.iter().skip(index.saturating_add(1)) { + match inner { + Token::Punct('(') => depth = depth.saturating_add(1), + Token::Punct(')') => { + depth = depth.saturating_sub(1); + if depth == 0 { + break; + } + } + Token::Punct(',') if depth == 1 => lints.push(std::mem::take(&mut path)), + Token::Punct(':') => path.push(':'), + Token::Ident(name) => path.push_str(name), + Token::Literal(_) | Token::Str(_) | Token::Punct(_) | Token::Comment { .. } => { + } + } + } + if !path.is_empty() { + lints.push(path); + } + } + index = index.saturating_add(1); + } + lints +} + +/// Splits Rust source into the tokens the rules need, tracking lines. +fn lex(source: &str) -> Vec<(usize, Token)> { + let characters: Vec = source.chars().collect(); + let mut tokens = Vec::new(); + let mut index = 0_usize; + let mut line = 1_usize; + let mut line_has_code = false; + while let Some(&character) = characters.get(index) { + let next = characters.get(index.saturating_add(1)).copied(); + if character == '\n' { + line = line.saturating_add(1); + line_has_code = false; + index = index.saturating_add(1); + } else if character.is_whitespace() { + index = index.saturating_add(1); + } else if character == '/' && next == Some('/') { + let end = characters + .iter() + .skip(index) + .position(|candidate| *candidate == '\n') + .map_or(characters.len(), |offset| index.saturating_add(offset)); + let text: String = characters + .get(index..end) + .unwrap_or_default() + .iter() + .collect(); + let kind = if (text.starts_with("///") && !text.starts_with("////")) + || text.starts_with("//!") + { + CommentKind::Doc + } else { + CommentKind::Plain + }; + let body = text.get(2..).unwrap_or_default().to_owned(); + tokens.push(( + line, + Token::Comment { + kind, + text: body, + alone: !line_has_code, + block: false, + }, + )); + index = end; + } else if character == '/' && next == Some('*') { + let start_line = line; + let mut depth = 0_usize; + let mut text = String::new(); + while let Some(¤t) = characters.get(index) { + let following = characters.get(index.saturating_add(1)).copied(); + if current == '/' && following == Some('*') { + depth = depth.saturating_add(1); + text.push_str("/*"); + index = index.saturating_add(2); + } else if current == '*' && following == Some('/') { + depth = depth.saturating_sub(1); + text.push_str("*/"); + index = index.saturating_add(2); + if depth == 0 { + break; + } + } else { + if current == '\n' { + line = line.saturating_add(1); + } + text.push(current); + index = index.saturating_add(1); + } + } + let kind = if (text.starts_with("/**") && !text.starts_with("/***") && text != "/**/") + || text.starts_with("/*!") + { + CommentKind::Doc + } else { + CommentKind::Plain + }; + tokens.push(( + start_line, + Token::Comment { + kind, + text, + alone: !line_has_code, + block: true, + }, + )); + } else if character == '"' { + let (text, end, newlines) = quoted(&characters, index); + tokens.push((line, Token::Str(text))); + line = line.saturating_add(newlines); + line_has_code = true; + index = end; + } else if character == '\'' { + line_has_code = true; + let escaped = next == Some('\\'); + let closes_after_one = characters.get(index.saturating_add(2)) == Some(&'\''); + if escaped || closes_after_one { + let mut end = index.saturating_add(1); + while let Some(¤t) = characters.get(end) { + end = end.saturating_add(if current == '\\' { 2 } else { 1 }); + if current == '\'' { + break; + } + } + tokens.push((line, Token::Literal("'".to_owned()))); + index = end; + } else { + tokens.push((line, Token::Punct('\''))); + index = index.saturating_add(1); + } + } else if character.is_alphabetic() || character == '_' { + line_has_code = true; + let end = characters + .iter() + .skip(index) + .position(|candidate| !(candidate.is_alphanumeric() || *candidate == '_')) + .map_or(characters.len(), |offset| index.saturating_add(offset)); + let word: String = characters + .get(index..end) + .unwrap_or_default() + .iter() + .collect(); + let hashes = characters + .iter() + .skip(end) + .take_while(|candidate| **candidate == '#') + .count(); + let quote = characters.get(end.saturating_add(hashes)) == Some(&'"'); + if matches!(word.as_str(), "r" | "br" | "cr") && quote { + let (text, after, newlines) = raw_quoted(&characters, end, hashes); + tokens.push((line, Token::Str(text))); + line = line.saturating_add(newlines); + index = after; + } else if matches!(word.as_str(), "b" | "c") && characters.get(end) == Some(&'"') { + let (text, after, newlines) = quoted(&characters, end); + tokens.push((line, Token::Str(text))); + line = line.saturating_add(newlines); + index = after; + } else if word == "r" && hashes == 1 { + index = end.saturating_add(1); + } else { + tokens.push((line, Token::Ident(word))); + index = end; + } + } else if character.is_ascii_digit() { + line_has_code = true; + let end = characters + .iter() + .skip(index) + .position(|candidate| !(candidate.is_alphanumeric() || *candidate == '_')) + .map_or(characters.len(), |offset| index.saturating_add(offset)); + let literal: String = characters + .get(index..end) + .unwrap_or_default() + .iter() + .collect(); + tokens.push((line, Token::Literal(literal))); + index = end; + } else { + line_has_code = true; + tokens.push((line, Token::Punct(character))); + index = index.saturating_add(1); + } + } + tokens +} + +/// Reads a `"..."` literal starting at the quote; returns its text, the index after it, and its newlines. +fn quoted(characters: &[char], quote: usize) -> (String, usize, usize) { + let mut text = String::new(); + let mut index = quote.saturating_add(1); + let mut newlines = 0_usize; + while let Some(&character) = characters.get(index) { + if character == '\\' { + if let Some(&escaped) = characters.get(index.saturating_add(1)) { + text.push(escaped); + } + index = index.saturating_add(2); + continue; + } + index = index.saturating_add(1); + if character == '"' { + break; + } + if character == '\n' { + newlines = newlines.saturating_add(1); + } + text.push(character); + } + (text, index, newlines) +} + +/// Reads a raw literal whose `#`s start at `hashes_start`. +fn raw_quoted(characters: &[char], hashes_start: usize, hashes: usize) -> (String, usize, usize) { + let mut text = String::new(); + let mut index = hashes_start.saturating_add(hashes).saturating_add(1); + let mut newlines = 0_usize; + while let Some(&character) = characters.get(index) { + if character == '"' { + let closing = characters + .iter() + .skip(index.saturating_add(1)) + .take(hashes) + .filter(|candidate| **candidate == '#') + .count(); + if closing == hashes { + return ( + text, + index.saturating_add(1).saturating_add(hashes), + newlines, + ); + } + } + if character == '\n' { + newlines = newlines.saturating_add(1); + } + text.push(character); + index = index.saturating_add(1); + } + (text, index, newlines) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn rules(source: &str) -> Vec { + scan("src/example.rs", source) + .findings + .into_iter() + .map(|finding| finding.rule) + .collect() + } + + fn lints(source: &str) -> Vec { + scan("src/example.rs", source) + .allows + .into_iter() + .map(|allow| allow.lint) + .collect() + } + + #[test] + fn only_doc_and_safety_comments_are_allowed() { + assert!(rules(concat!( + "/// doc\n", + "//! inner\n", + "/** block doc */\n", + "fn f() {}\n" + )) + .is_empty()); + assert!(rules(concat!( + "// SAFETY: first\n", + "// second line\n", + "unsafe {}\n" + )) + .is_empty()); + assert_eq!(rules(concat!("/", "/ plain\n", "fn f() {}\n")).len(), 1); + assert_eq!(rules(concat!("fn f() {} /", "/ trailing\n")).len(), 1); + assert_eq!(rules(concat!("/", "* block */\n")).len(), 1); + assert_eq!( + rules(concat!("// SAFETY: a\n", "\n", "/", "/ detached\n")).len(), + 1 + ); + assert_eq!(rules(concat!("//", "// four slashes\n")).len(), 1); + assert!(rules("fn f() { let s = \"// not a comment\"; }\n").is_empty()); + } + + #[test] + fn allow_attributes_are_collected_and_checked_against_the_registry() { + assert_eq!( + lints("#[allow(clippy::expect_used, unsafe_code)]\n#![cfg_attr(test, allow(dead_code))]\n"), + ["clippy::expect_used", "unsafe_code", "dead_code"] + ); + let allows = vec![Allow { + file: "src/a.rs".to_owned(), + line: 3, + lint: "unsafe_code".to_owned(), + }]; + let registered = [Exception { + file: "src/a.rs", + lint: "unsafe_code", + count: 1, + reason: "test", + }]; + assert!(check_exceptions(&allows, ®istered).is_empty()); + assert_eq!(check_exceptions(&allows, &[]).len(), 1); + let twice = [Exception { + file: "src/a.rs", + lint: "unsafe_code", + count: 2, + reason: "test", + }]; + assert_eq!(check_exceptions(&allows, &twice).len(), 1); + assert_eq!(check_exceptions(&[], ®istered).len(), 1); + } + + #[test] + fn time_is_refused() { + for source in [ + concat!("fn f() { std::thread::sl", "eep(d); }"), + concat!("fn f() { let t = Inst", "ant::now(); }"), + concat!("fn f(r: R) { r.recv_time", "out(x); }"), + concat!("fn f(d: Dura", "tion) {}"), + concat!("fn f() { let x = \"ping", " -n 5 127.0.0.1\"; }"), + concat!("fn f() { WaitForSingleObject(h, 5", "000); }"), + ] { + assert_eq!(rules(source).len(), 1, "{source}"); + } + assert!( + rules("fn f() { WaitForSingleObject(h, INFINITE); WaitForSingleObject(h, 0); }") + .is_empty() + ); + assert!(rules("use x::{WaitForSingleObject, INFINITE, WAIT_TIMEOUT};").is_empty()); + } + + #[test] + fn defaults_and_boxed_trait_objects_are_refused() { + assert_eq!(rules(concat!("enum E { #[defa", "ult] A, B }")).len(), 1); + assert_eq!(rules(concat!("struct S { f: Box }")).len(), 1); + assert!(rules("struct S { f: Box }").is_empty()); + } + + #[test] + fn literals_do_not_confuse_the_lexer() { + assert!(rules("fn f<'a>(x: &'a str) -> char { let _ = r#\"// \"#; '\\'' }").is_empty()); + assert!(rules("fn f() { let c = '\"'; let s = b\"x\"; }").is_empty()); + } +} diff --git a/xtask/src/main.rs b/xtask/src/main.rs index ae08b1f..8d227cc 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -1,22 +1,23 @@ //! Repository automation for windows-spawn. mod cli; +mod gates; mod tasks; use std::env; -use std::process; +use std::process::ExitCode; -fn main() { +fn main() -> ExitCode { let arguments = env::args().skip(1); let result = match cli::parse(arguments) { Ok(task) => tasks::execute(task), Err(error) => Err(tasks::TaskError::from(error)), }; match result { - Ok(code) => process::exit(code), + Ok(code) => u8::try_from(code).map_or(ExitCode::FAILURE, ExitCode::from), Err(error) => { eprintln!("error: {error}"); - process::exit(1); + ExitCode::FAILURE } } } diff --git a/xtask/src/tasks.rs b/xtask/src/tasks.rs index a925388..90be162 100644 --- a/xtask/src/tasks.rs +++ b/xtask/src/tasks.rs @@ -1,4 +1,5 @@ use crate::cli::{SimpleTask, Task}; +use crate::gates; use semver::Version; use serde_json::Value; use sha2::{Digest, Sha256}; @@ -115,7 +116,7 @@ fn print_help() { println!( "\ Repository tasks: - cargo xtask fmt|clippy|test|doc|msrv|cross-targets|linux-empty + cargo xtask fmt|clippy|gates|test|doc|msrv|cross-targets|linux-empty cargo xtask supply-chain|reuse|typos|coverage|ci cargo xtask public-api [--update] cargo xtask package-check [--allow-dirty] @@ -207,14 +208,31 @@ fn run_simple(root: &Path, task: SimpleTask) -> Result<()> { SimpleTask::Reuse => run_program(root, "python", &["-m", "reuse", "lint"]), SimpleTask::Typos => run_program(root, "typos", &[]), SimpleTask::Coverage => coverage(root), + SimpleTask::Gates => run_gates(root), SimpleTask::Ci => run_ci(root), } } +fn run_gates(root: &Path) -> Result<()> { + let findings = gates::check(root)?; + for finding in &findings { + eprintln!("{finding}"); + } + if findings.is_empty() { + Ok(()) + } else { + Err(TaskError::Message(format!( + "{} repository law violation(s); see ADR 0013", + findings.len() + ))) + } +} + fn run_ci(root: &Path) -> Result<()> { for check in [ SimpleTask::Fmt, SimpleTask::Clippy, + SimpleTask::Gates, SimpleTask::Test, SimpleTask::Doc, SimpleTask::Msrv, @@ -288,7 +306,7 @@ fn compare_snapshot(expected: &str, actual: &str) -> std::result::Result<(), Str .unwrap_or_else(|| expected.len().min(actual.len())); Err(format!( "public API differs at line {}\nexpected: {}\n actual: {}\nrun `cargo xtask public-api --update` for an intentional change", - first + 1, + first.saturating_add(1), expected.get(first).copied().unwrap_or(""), actual.get(first).copied().unwrap_or("") )) @@ -636,20 +654,25 @@ fn sha256(path: &Path) -> Result { if read == 0 { break; } - digest.update(&buffer[..read]); + digest.update(buffer.get(..read).ok_or_else(|| { + TaskError::from("read reported more bytes than the buffer holds".to_owned()) + })?); } Ok(lower_hex(&digest.finalize())) } /// Formats a digest as lowercase hex; `sha2` 0.11 digests do not implement `LowerHex`. fn lower_hex(bytes: &[u8]) -> String { - use std::fmt::Write as _; - - let mut output = String::with_capacity(bytes.len() * 2); - for byte in bytes { - write!(&mut output, "{byte:02x}").expect("writing to a String cannot fail"); - } - output + bytes + .iter() + .flat_map(|byte| { + [ + char::from_digit(u32::from(*byte) / 16, 16), + char::from_digit(u32::from(*byte) % 16, 16), + ] + }) + .flatten() + .collect() } fn verify_release_tag(root: &Path, tag: &str) -> Result<()> { @@ -690,8 +713,9 @@ fn parse_release_tag(tag: &str) -> std::result::Result { let version = tag .strip_prefix('v') .ok_or_else(|| "release tag must be v-prefixed SemVer, for example v1.2.3".to_owned())?; - Version::parse(version) - .map_err(|_| "release tag must be v-prefixed SemVer, for example v1.2.3".to_owned()) + Version::parse(version).map_err(|error| { + format!("release tag must be v-prefixed SemVer, for example v1.2.3: {error}") + }) } fn draft_release(root: &Path, tag: &str, github_output: bool) -> Result<()> { @@ -843,7 +867,9 @@ fn normalize_path(path: &Path) -> PathBuf { Component::ParentDir => { normalized.pop(); } - _ => normalized.push(component.as_os_str()), + Component::Prefix(_) | Component::RootDir | Component::Normal(_) => { + normalized.push(component.as_os_str()); + } } } normalized From 34082d189eb80cdcb97a2a5f0088281d078a1bed Mon Sep 17 00:00:00 2001 From: Yasunobu <42543015+P4suta@users.noreply.github.com> Date: Fri, 25 Sep 2026 04:45:48 +0900 Subject: [PATCH 2/3] refactor: make unreachable conversion failures unrepresentable The first mutation run of this branch left survivors on `?` after conversions and range checks that cannot fail. Each is removed by construction instead of being annotated. - Structure sizes and the maximum path capacity are `DWORD` constants built by `dword_const`, so an oversized value fails the build. - `widen` converts `DWORD` results to `usize`, which has at least 32 bits on every Windows target. - `AttributeList::new` passes the allocation's own size, `drain_output` takes the bytes read, and `split_entry` splits at the separator it found. - `environment_strings` counts with `saturating_add`: an entry lives in memory, so its length cannot reach `usize::MAX`. - `SpawnTransaction::new` sums the attribute count as `u32`. - `create_process` decides the extended startup information once, so the equivalent size and flag mutants collapse into one decision. - `HandleTransfer::lower` no longer checks for a repeated value: every duplicate stays open for the transaction, so values never repeat. - A test holds `DropPolicy::default`, which no test reached. The gate reports registry mismatches without a line number. --- .rust-mutants.toml | 24 +----- ...pository-laws-are-enforced-mechanically.md | 2 +- src/child.rs | 5 +- src/options.rs | 12 ++- src/sys.rs | 83 ++++++++++--------- src/transaction.rs | 14 ++-- xtask/src/gates.rs | 29 +++++-- 7 files changed, 84 insertions(+), 85 deletions(-) diff --git a/.rust-mutants.toml b/.rust-mutants.toml index f814ab8..f674e38 100644 --- a/.rust-mutants.toml +++ b/.rust-mutants.toml @@ -34,15 +34,6 @@ original = "false" reason = "Equivalence: the local copy closes as the call returns; its inheritability matters only to a concurrent broad-inheritance spawn (ADR 0005)." outcome = "survived" -[[mutation.expect]] -path = "src/sys.rs" -item = "create_process" -rule = "condition-to-true" -original = "request.attributes.is_some()" -count = 2 -reason = "Equivalence: CreateProcessW accepts the STARTUPINFOEXW size and EXTENDED_STARTUPINFO_PRESENT with a null attribute list; every spawn test holds creation." -outcome = "survived" - [[mutation.expect]] path = "src/sys.rs" item = "read_handle" @@ -67,15 +58,6 @@ original = "base.is_null()" reason = "Fail-open: GetEnvironmentStringsW returns null only when out of memory." outcome = "survived" -[[mutation.expect]] -path = "src/sys.rs" -item = "environment_strings" -rule = "question-to-unwrap" -original = "?" -count = 2 -reason = "Fail-open: an environment entry or block cannot approach usize::MAX units." -outcome = "survived" - [[mutation.expect]] path = "src/transaction.rs" item = "SpawnTransaction::new" @@ -120,17 +102,17 @@ outcome = "survived" [[mutation.skip]] path = "src/sys.rs" -lines = "996-996" +lines = "997-997" reason = "Unreachable: GetEnvironmentStringsW returns null only when out of memory." [[mutation.skip]] path = "src/sys.rs" -lines = "1004-1004" +lines = "1005-1005" reason = "Hang-only: continuing past the block terminator loops forever." [[mutation.skip]] path = "src/transaction.rs" -lines = "498-501" +lines = "494-497" reason = "Unreachable: GetFullPathNameW cannot return a double quote." [[mutation.skip]] diff --git a/docs/adr/0013-repository-laws-are-enforced-mechanically.md b/docs/adr/0013-repository-laws-are-enforced-mechanically.md index 4774d12..3d569a8 100644 --- a/docs/adr/0013-repository-laws-are-enforced-mechanically.md +++ b/docs/adr/0013-repository-laws-are-enforced-mechanically.md @@ -35,5 +35,5 @@ Rustc lints are limited to those Rust 1.75 recognizes. ## Consequences - Changing a law changes the lint table, `clippy.toml`, or the gate, with its tests. -- Pointer-integer conversions keep `as` behind registered exceptions; there is no alternative before Rust 1.84. +- Pointer-integer and `DWORD` width conversions keep `as` behind registered exceptions in `sys`, because Rust 1.75 has no `From` form for them. - When the MSRV reaches Rust 1.81, `#[expect(…, reason = …)]` replaces the registry. diff --git a/src/child.rs b/src/child.rs index 2676daa..f608b8b 100644 --- a/src/child.rs +++ b/src/child.rs @@ -207,10 +207,7 @@ fn drain_output(handle: BorrowedHandle<'_>) -> io::Result> { let Some(read) = std::num::NonZeroUsize::new(sys::read_handle(handle, &mut buffer)?) else { return Ok(bytes); }; - let chunk = buffer.get(..read.get()).ok_or_else(|| { - io::Error::other("ReadFile reported more bytes than the buffer holds") - })?; - bytes.extend_from_slice(chunk); + bytes.extend(buffer.iter().take(read.get())); } } diff --git a/src/options.rs b/src/options.rs index e16c7e2..7e5fc87 100644 --- a/src/options.rs +++ b/src/options.rs @@ -147,7 +147,7 @@ impl Default for SpawnOptions<'_> { mitigation: MitigationPolicy::new(), pseudoconsole: None, creation_flags: CreationFlags::new(), - drop_policy: DropPolicy::Detach, + drop_policy: DropPolicy::default(), } } } @@ -246,6 +246,16 @@ mod tests { assert!(std::ptr::eq(options.jobs[1], &inner)); } + #[test] + fn defaults_detach_and_request_nothing() { + assert_eq!(DropPolicy::default(), DropPolicy::Detach); + let options = SpawnOptions::default(); + assert_eq!(options.drop_policy, DropPolicy::Detach); + assert_eq!(options.creation_flags, CreationFlags::new()); + assert!(options.jobs.is_empty() && options.parent.is_none()); + assert_eq!(options.pseudoconsole_raw(), None); + } + #[test] fn pseudoconsole_builder_snapshots_the_raw_value() { let pseudoconsole = TestPseudoConsole(Cell::new(42)); diff --git a/src/sys.rs b/src/sys.rs index febe37f..c8a7609 100644 --- a/src/sys.rs +++ b/src/sys.rs @@ -41,11 +41,19 @@ use windows_sys::Win32::System::Threading::{ PROCESS_DUP_HANDLE, PROCESS_INFORMATION, PROC_THREAD_ATTRIBUTE_HANDLE_LIST, PROC_THREAD_ATTRIBUTE_JOB_LIST, PROC_THREAD_ATTRIBUTE_MITIGATION_POLICY, PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE, - STARTF_USESTDHANDLES, STARTUPINFOEXW, + STARTF_USESTDHANDLES, STARTUPINFOEXW, STARTUPINFOW, }; pub(crate) const INVALID_RAW_HANDLE: isize = -1; +const JOB_LIMITS_SIZE: u32 = dword_const(size_of::()); +const STARTUPINFO_SIZE: u32 = dword_const(size_of::()); +const STARTUPINFOEX_SIZE: u32 = dword_const(size_of::()); + +/// The longest Windows path in UTF-16 units, including the terminator. +const MAXIMUM_PATH: usize = 32_768; +const MAXIMUM_PATH_CAPACITY: u32 = dword_const(MAXIMUM_PATH); + struct EnvironmentBlock(*mut u16); #[cfg(test)] @@ -305,7 +313,7 @@ pub(crate) fn set_job_kill_on_close(handle: BorrowedHandle<'_>, enable: bool) -> raw(handle), JobObjectExtendedLimitInformation, ptr::addr_of!(limits).cast(), - dword(size_of::())?, + JOB_LIMITS_SIZE, ) } == 0 { @@ -327,7 +335,7 @@ fn query_job_limits( raw(handle), JobObjectExtendedLimitInformation, ptr::addr_of_mut!(limits).cast(), - dword(size_of::())?, + JOB_LIMITS_SIZE, ptr::null_mut(), ) } == 0 @@ -368,9 +376,7 @@ impl AttributeList { let words = storage_words(bytes); let mut storage = vec![0_usize; words].into_boxed_slice(); let pointer = storage.as_mut_ptr().cast(); - let mut actual = words - .checked_mul(size_of::()) - .ok_or_else(|| io::Error::other("attribute list is too large"))?; + let mut actual = size_of_val(&*storage); // SAFETY: the `Box<[usize]>` is word-aligned, stable, at least `bytes` long, and owned by the returned `AttributeList`. if unsafe { InitializeProcThreadAttributeList(pointer, count, 0, &mut actual) } == 0 { return Err(io::Error::last_os_error()); @@ -422,7 +428,7 @@ impl AttributeList { fn update(&mut self, attribute: u32, value: *const c_void, bytes: usize) -> io::Result<()> { #[cfg(test)] fault::check(fault::Call::UpdateAttribute)?; - let attribute = usize::try_from(attribute).map_err(io::Error::other)?; + 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. if unsafe { UpdateProcThreadAttribute( @@ -509,23 +515,18 @@ pub(crate) fn create_process(request: &mut ProcessRequest<'_>) -> io::Result())? + let mut flags = request.creation_flags | CREATE_UNICODE_ENVIRONMENT; + if let Some(attributes) = request.attributes { + startup.StartupInfo.cb = STARTUPINFOEX_SIZE; + startup.lpAttributeList = attributes.pointer(); + flags |= EXTENDED_STARTUPINFO_PRESENT; } else { - dword(size_of::())? - }; + startup.StartupInfo.cb = STARTUPINFO_SIZE; + } set_standard_handles(&mut startup, request.stdio); - startup.lpAttributeList = request - .attributes - .map_or(ptr::null_mut(), AttributeList::pointer); - - let mut flags = request.creation_flags | CREATE_UNICODE_ENVIRONMENT; if request.suspended { flags |= CREATE_SUSPENDED; } - if request.attributes.is_some() { - flags |= EXTENDED_STARTUPINFO_PRESENT; - } let environment = request .environment .map_or(ptr::null(), |block| block.as_ptr().cast()); @@ -958,7 +959,7 @@ pub(crate) fn read_handle(handle: BorrowedHandle<'_>, buffer: &mut [u8]) -> io:: Err(error) } } else { - usize::try_from(read).map_err(io::Error::other) + Ok(widen(read)) } } @@ -983,7 +984,7 @@ pub(crate) fn write_handle(handle: BorrowedHandle<'_>, buffer: &[u8]) -> io::Res { Err(io::Error::last_os_error()) } else { - usize::try_from(written).map_err(io::Error::other) + Ok(widen(written)) } } @@ -1006,18 +1007,14 @@ pub(crate) fn environment_strings() -> io::Result> { let mut length = 0_usize; // SAFETY: the current entry is NUL-terminated. while unsafe { *cursor.add(length) } != 0 { - length = length - .checked_add(1) - .ok_or_else(|| io::Error::other("environment entry is too large"))?; + length = length.saturating_add(1); } // SAFETY: the range was just measured inside the current entry. let entry = unsafe { std::slice::from_raw_parts(cursor, length) }; if let Some((key, value)) = split_entry(entry) { entries.push((OsString::from_wide(key), OsString::from_wide(value))); } - let advance = length - .checked_add(1) - .ok_or_else(|| io::Error::other("environment block is too large"))?; + let advance = length.saturating_add(1); // SAFETY: `advance` moves just past this entry's terminator, still inside the block. cursor = unsafe { cursor.add(advance) }; } @@ -1026,15 +1023,13 @@ pub(crate) fn environment_strings() -> io::Result> { /// Splits `KEY=value` at the first `=` after the first unit, so hidden `=C:` entries keep their key. fn split_entry(entry: &[u16]) -> Option<(&[u16], &[u16])> { - let equals = u16::from(b'='); - let separator = entry + let (separator, _) = entry .iter() + .enumerate() .skip(1) - .position(|unit| *unit == equals)? - .checked_add(1)?; - let key = entry.get(..separator)?; - let value = entry.get(separator.checked_add(1)?..)?; - Some((key, value)) + .find(|(_, unit)| **unit == u16::from(b'='))?; + let (key, rest) = entry.split_at(separator); + rest.split_first().map(|(_, value)| (key, value)) } pub(crate) fn compare_ordinal(left: &[u16], right: &[u16]) -> Ordering { @@ -1096,9 +1091,8 @@ pub(crate) fn full_path(path: &[u16]) -> io::Result> { fn maximum_path(fill: impl FnOnce(*mut u16, u32) -> u32) -> io::Result> { #[cfg(test)] fault::check(fault::Call::MaximumPath)?; - let mut buffer = vec![0_u16; 32_768]; - let capacity = dword(buffer.len())?; - let length = usize::try_from(fill(buffer.as_mut_ptr(), capacity)).map_err(io::Error::other)?; + let mut buffer = vec![0_u16; MAXIMUM_PATH]; + let length = widen(fill(buffer.as_mut_ptr(), MAXIMUM_PATH_CAPACITY)); if length == 0 { return Err(io::Error::last_os_error()); } @@ -1140,9 +1134,18 @@ pub(crate) const fn handle_from_value(value: isize) -> HANDLE { value as HANDLE } -/// Converts a size to a Win32 `DWORD`. -fn dword(value: usize) -> io::Result { - u32::try_from(value).map_err(io::Error::other) +/// Converts a compile-time size to a Win32 `DWORD`; a size that does not fit fails constant evaluation. +#[allow(clippy::as_conversions, clippy::cast_possible_truncation)] +const fn dword_const(value: usize) -> u32 { + let wide = value as u64; + assert!(wide <= 0xFFFF_FFFF, "size does not fit a DWORD"); + wide as u32 +} + +/// Widens a `DWORD`; `usize` has at least 32 bits on every Windows target. +#[allow(clippy::as_conversions)] +const fn widen(value: u32) -> usize { + value as usize } /// Returns true if `error` carries the Win32 error `code`. diff --git a/src/transaction.rs b/src/transaction.rs index 2d56d05..5aa0b00 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -112,7 +112,7 @@ impl SpawnTransaction { let job_values = job_values.into_boxed_slice(); let pseudoconsole = plan.options.pseudoconsole_raw(); - let attribute_count = [ + let attribute_count: u32 = [ !inherited_values.is_empty(), parent_value.is_some(), mitigation_value.is_some(), @@ -120,14 +120,12 @@ impl SpawnTransaction { pseudoconsole.is_some(), ] .into_iter() - .filter(|present| *present) - .count(); + .map(u32::from) + .sum(); let mut attributes = if attribute_count == 0 { None } else { - Some(sys::AttributeList::new( - u32::try_from(attribute_count).map_err(io::Error::other)?, - )?) + Some(sys::AttributeList::new(attribute_count)?) }; if let Some(list) = &mut attributes { if !inherited_values.is_empty() { @@ -323,9 +321,7 @@ impl<'a> HandleTransfer<'a> { self.local.push(handle); value }; - if !self.inherited.contains(&value) { - self.inherited.push(value); - } + self.inherited.push(value); Ok(value) } diff --git a/xtask/src/gates.rs b/xtask/src/gates.rs index 4691e43..122eca4 100644 --- a/xtask/src/gates.rs +++ b/xtask/src/gates.rs @@ -19,7 +19,8 @@ const EXCEPTIONS: &[Exception] = &[ file: "src/lib.rs", lint: "unsafe_code", count: 2, - reason: "sys is the only Win32 FFI boundary; failure_tests creates pseudoconsoles for tests", + reason: + "sys is the only Win32 FFI boundary; failure_tests creates pseudoconsoles for tests", }, Exception { file: "src/handles.rs", @@ -42,8 +43,14 @@ const EXCEPTIONS: &[Exception] = &[ Exception { file: "src/sys.rs", lint: "clippy::as_conversions", - count: 2, - reason: "handle values convert between pointers and integers, which has no From form before Rust 1.84", + count: 4, + reason: "handle values and Win32 widths convert where Rust 1.75 has no From form", + }, + Exception { + file: "src/sys.rs", + lint: "clippy::cast_possible_truncation", + count: 1, + reason: "dword_const is evaluated in constants, where an oversized value fails the build", }, Exception { file: "src/child.rs", @@ -61,7 +68,8 @@ const EXCEPTIONS: &[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)", + reason: + "one function acquires every creation resource, so rollback has one owner (ADR 0007)", }, Exception { file: "src/mitigation.rs", @@ -150,13 +158,16 @@ const DELAY_COMMANDS: [&str; 3] = [ #[derive(Debug, PartialEq, Eq)] pub(crate) struct Finding { pub(crate) file: String, - pub(crate) line: usize, + pub(crate) line: Option, pub(crate) rule: String, } impl fmt::Display for Finding { fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(formatter, "{}:{}: {}", self.file, self.line, self.rule) + match self.line { + Some(line) => write!(formatter, "{}:{line}: {}", self.file, self.rule), + None => write!(formatter, "{}: {}", self.file, self.rule), + } } } @@ -213,7 +224,7 @@ fn check_exceptions(allows: &[Allow], exceptions: &[Exception]) -> Vec if !registered { findings.push(Finding { file: allow.file.clone(), - line: allow.line, + line: Some(allow.line), rule: format!( "#[allow({})] is not registered in xtask/src/gates.rs", allow.lint @@ -229,7 +240,7 @@ fn check_exceptions(allows: &[Allow], exceptions: &[Exception]) -> Vec if count != exception.count { findings.push(Finding { file: exception.file.to_owned(), - line: 0, + line: None, rule: format!( "{} is registered {} time(s) ({}) but allowed {count} time(s)", exception.lint, exception.count, exception.reason @@ -271,7 +282,7 @@ fn scan(file: &str, source: &str) -> Scan { let mut finding = |line: usize, rule: String| { findings.push(Finding { file: file.to_owned(), - line, + line: Some(line), rule, }); }; From 72256fe19bfada65a12b0d8ba4c819a5feb3272e Mon Sep 17 00:00:00 2001 From: Yasunobu <42543015+P4suta@users.noreply.github.com> Date: Fri, 25 Sep 2026 04:53:47 +0900 Subject: [PATCH 3/3] test: split environment entries directly No environment block holds an entry without a separator, so only a direct test reaches the None path of split_entry. --- src/sys.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/sys.rs b/src/sys.rs index c8a7609..74d5df3 100644 --- a/src/sys.rs +++ b/src/sys.rs @@ -1424,6 +1424,20 @@ mod tests { Ok(()) } + #[test] + fn environment_entries_split_at_the_first_separator_after_the_key() { + let wide = |text: &str| -> Vec { text.encode_utf16().collect() }; + let split = |text: &str| { + let entry = wide(text); + split_entry(&entry).map(|(key, value)| (key.to_vec(), value.to_vec())) + }; + assert_eq!(split("KEY=a=b"), Some((wide("KEY"), wide("a=b")))); + assert_eq!(split("=C:=C:\\work"), Some((wide("=C:"), wide("C:\\work")))); + assert_eq!(split("EMPTY="), Some((wide("EMPTY"), Vec::new()))); + assert_eq!(split("NOSEPARATOR"), None); + assert_eq!(split("="), None); + } + #[test] fn system_and_windows_directories_are_distinct() -> io::Result<()> { let system = system_directory()?.to_string_lossy().to_lowercase();