diff --git a/Cargo.lock b/Cargo.lock index 8eaa6f31e..41dfeb5a8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -558,7 +558,7 @@ dependencies = [ [[package]] name = "cargo-gamma" -version = "0.1.0" +version = "0.2.0" dependencies = [ "cargo-gamma-lib", "clap", @@ -568,14 +568,14 @@ dependencies = [ [[package]] name = "cargo-gamma-attrs" -version = "0.1.0" +version = "0.2.0" dependencies = [ "cargo-gamma-attrs-impl", ] [[package]] name = "cargo-gamma-attrs-impl" -version = "0.1.0" +version = "0.2.0" dependencies = [ "proc-macro2", "syn 3.0.4", @@ -583,7 +583,7 @@ dependencies = [ [[package]] name = "cargo-gamma-engine" -version = "0.1.0" +version = "0.2.0" dependencies = [ "blake3", "bolero", @@ -601,7 +601,7 @@ dependencies = [ [[package]] name = "cargo-gamma-lib" -version = "0.1.0" +version = "0.2.0" dependencies = [ "blake3", "bolero", @@ -634,7 +634,7 @@ dependencies = [ [[package]] name = "cargo-gamma-process" -version = "0.1.0" +version = "0.2.0" dependencies = [ "camino", "cargo-gamma-unsafe", @@ -643,7 +643,7 @@ dependencies = [ [[package]] name = "cargo-gamma-rt" -version = "0.1.0" +version = "0.2.0" dependencies = [ "loom", "tempfile", @@ -651,7 +651,7 @@ dependencies = [ [[package]] name = "cargo-gamma-unsafe" -version = "0.1.0" +version = "0.2.0" dependencies = [ "libc", "loom", diff --git a/Cargo.toml b/Cargo.toml index 8ebb89321..404084d78 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,12 +34,12 @@ bytes = { version = "1.12.1", default-features = false } camino = { version = "1.2.5", default-features = false } # local dependencies cargo-aprz-lib = { path = "crates/cargo-aprz-lib", default-features = false, version = "1.1.2" } -cargo-gamma-attrs-impl = { path = "crates/cargo-gamma-attrs-impl", default-features = false, version = "0.1.0" } -cargo-gamma-engine = { path = "crates/cargo-gamma-engine", default-features = false, version = "0.1.0" } -cargo-gamma-lib = { path = "crates/cargo-gamma-lib", default-features = false, version = "0.1.0" } -cargo-gamma-process = { path = "crates/cargo-gamma-process", default-features = false, version = "0.1.0" } -cargo-gamma-rt = { path = "crates/cargo-gamma-rt", default-features = false, version = "0.1.0" } -cargo-gamma-unsafe = { path = "crates/cargo-gamma-unsafe", default-features = false, version = "0.1.0" } +cargo-gamma-attrs-impl = { path = "crates/cargo-gamma-attrs-impl", default-features = false, version = "0.2.0" } +cargo-gamma-engine = { path = "crates/cargo-gamma-engine", default-features = false, version = "0.2.0" } +cargo-gamma-lib = { path = "crates/cargo-gamma-lib", default-features = false, version = "0.2.0" } +cargo-gamma-process = { path = "crates/cargo-gamma-process", default-features = false, version = "0.2.0" } +cargo-gamma-rt = { path = "crates/cargo-gamma-rt", default-features = false, version = "0.2.0" } +cargo-gamma-unsafe = { path = "crates/cargo-gamma-unsafe", default-features = false, version = "0.2.0" } cargo-heather = { path = "crates/cargo-heather", default-features = false, version = "0.2.1" } # external dependencies cargo-platform = { version = "0.3.3", default-features = false } diff --git a/crates/cargo-gamma-attrs-impl/CHANGELOG.md b/crates/cargo-gamma-attrs-impl/CHANGELOG.md new file mode 100644 index 000000000..9bdb3282e --- /dev/null +++ b/crates/cargo-gamma-attrs-impl/CHANGELOG.md @@ -0,0 +1,14 @@ +# Changelog + +## [Unreleased] + +## [0.2.0] - 2026-09-03 + +- 🐛 Bug Fixes + + - harden discovery and process cleanup + - address hardening feedback ([#144](https://github.com/microsoft/ox-tools/pull/144)) + +## [0.1.0] - 2026-09-02 + +- Initial release. diff --git a/crates/cargo-gamma-attrs-impl/Cargo.toml b/crates/cargo-gamma-attrs-impl/Cargo.toml index fc3c99e75..20d82c9d4 100644 --- a/crates/cargo-gamma-attrs-impl/Cargo.toml +++ b/crates/cargo-gamma-attrs-impl/Cargo.toml @@ -4,7 +4,7 @@ [package] name = "cargo-gamma-attrs-impl" description = "Implementation of the inert attribute macros exposed by cargo-gamma-attrs" -version = "0.1.0" +version = "0.2.0" readme = "README.md" keywords = ["cargo", "mutation-testing", "testing"] categories = ["development-tools::testing"] diff --git a/crates/cargo-gamma-attrs-impl/README.md b/crates/cargo-gamma-attrs-impl/README.md index dd1d696d3..197f747cd 100644 --- a/crates/cargo-gamma-attrs-impl/README.md +++ b/crates/cargo-gamma-attrs-impl/README.md @@ -13,11 +13,49 @@ -This is an implementation detail of the cargo-gamma-attrs crate. Do not take a dependency on this -crate as it may change in incompatible ways without warning. +The implementation behind [`cargo-gamma-attrs`][__link0], +which is where the inert `#[gamma::skip]`, `#[gamma::expect_survived]` and +`#[gamma::expect_killed]` attributes are actually exposed. + +You almost certainly want that crate instead. This one is a normal library rather than a +proc-macro crate so its logic can be called by ordinary tests, covered, and mutation tested. +What remains in the proc-macro crate is a shim thin enough to read at a glance. + +## Why this crate exists + +`cargo-gamma-attrs` is a proc-macro crate, and a proc macro’s code runs only inside `rustc`, +while some *other* crate is being compiled. That puts it beyond the reach of both measurements +this project cares about: + +* A coverage harness collects counters from test binaries. A proc macro increments its counters + inside the compiler, which writes no profile the harness sees. +* A mutation run selects one mutant per test process at run time. A proc macro has already + finished by then, so none of its mutants can be active while a test is watching. + +Splitting the logic into an ordinary library makes it reachable by coverage and mutation tests. +The proc-macro crate remains a thin shim. + +## What the macros accept + +See the [`cargo-gamma-attrs`][__link1] documentation for the +user-facing description. In brief: a comma-separated selector list, optionally followed by +`reason = "..."` and `tag = "..."`, both of which must be string literals. + +`#[gamma::value()]` instead takes an expression. It is checked by [`value`][__link2], because its +argument is spliced into the user’s crate as a mutant and must be exactly one expression. + +## Stability + +This crate is an implementation detail of `cargo-gamma-attrs` and carries no stability +guarantee of its own. Depend on `cargo-gamma-attrs`.
This crate was developed as part of The Oxidizer Project. Browse this crate's source code. + + [__cargo_doc2readme_dependencies_info]: ggGmYW0CYXZlMC43LjNhdIQbFhzZ8rzWNNYbuRaDSGWynFgbH4PMdoT7GNcbVwNPtPjAhvFhYvRhcoQbH5RUmmY8e-sbYyqmHPyeK9obgdLJAJ7T65AbUAUW0Y4uz2thZIGDdmNhcmdvLWdhbW1hLWF0dHJzLWltcGxlMC4yLjB2Y2FyZ29fZ2FtbWFfYXR0cnNfaW1wbA + [__link0]: https://crates.io/crates/cargo-gamma-attrs + [__link1]: https://docs.rs/cargo-gamma-attrs + [__link2]: https://docs.rs/cargo-gamma-attrs-impl/0.2.0/cargo_gamma_attrs_impl/?search=value diff --git a/crates/cargo-gamma-attrs-impl/docs/DESIGN.md b/crates/cargo-gamma-attrs-impl/docs/DESIGN.md index 53c99ff4b..676f4556a 100644 --- a/crates/cargo-gamma-attrs-impl/docs/DESIGN.md +++ b/crates/cargo-gamma-attrs-impl/docs/DESIGN.md @@ -23,6 +23,10 @@ metadata. logic outside rustc makes it directly testable and mutation-testable. - It accepts exactly one Rust expression where an attribute promises an expression and rejects unsupported keys or malformed selectors. +- Delimiter depth and the combined chain of operators, casts, postfix links, + and `else` arms are bounded before input reaches `syn`. The chain categories + share one budget, matching the engine guard rather than allowing mixed syntax + to evade each independent limit. - A stated value is rejected on any function the tool would never mutate: a declaration with no body, a `const fn`, or a function whose body is empty. Accepting one there would leave a hint that reads as working and generates diff --git a/crates/cargo-gamma-attrs-impl/src/implementation.rs b/crates/cargo-gamma-attrs-impl/src/implementation.rs index 9bdc454a1..cecb5161d 100644 --- a/crates/cargo-gamma-attrs-impl/src/implementation.rs +++ b/crates/cargo-gamma-attrs-impl/src/implementation.rs @@ -95,11 +95,10 @@ pub fn value(attr: TokenStream, item: TokenStream) -> TokenStream { #[doc(hidden)] pub(super) const NESTING_LIMIT: usize = 64; -/// How many postfix links are allowed per delimiter nesting level. +/// How many expression-path links are allowed per delimiter nesting level. /// -/// This stays in step with `cargo_gamma_lib::parse::nesting`: a run of calls or indexes is a -/// recursive expression tree even though each delimiter closes before the next one opens; field, -/// method, and try links add the same recursive shape. +/// This stays in step with `cargo_gamma_lib::parse::nesting`: operators, casts, calls, indexes, +/// field and method access, try links, and `else` arms all add recursive expression shape. /// /// Exposed (hidden from docs) so `cargo-gamma-lib`'s agreement test can pin this copy against the /// library's own `CHAIN_FACTOR`. @@ -111,21 +110,32 @@ pub(super) const CHAIN_FACTOR: usize = 4; enum Previous { Other, Expression, + Operator, +} + +impl Previous { + const fn can_end_expression(self) -> bool { + matches!(self, Self::Expression | Self::Operator) + } } /// One token-stream level waiting to be walked. struct Frame { iter: proc_macro2::token_stream::IntoIter, depth: usize, - postfix: usize, - casts: usize, - operators: usize, + links: usize, ladders: usize, awaiting_else: bool, previous: Previous, } -/// Returns whether a token stream exceeds its delimiter or postfix-expression limits. +impl Frame { + fn exceeds_chain_limit(&self, limit: usize) -> bool { + self.links > limit || self.ladders > limit + } +} + +/// Returns whether a token stream exceeds its delimiter or expression-path limits. /// /// Nested groups are walked with an explicit stack rather than recursion, because this code runs /// inside `rustc` while compiling user code: a proc macro that exhausts the stack takes the @@ -136,7 +146,7 @@ struct Frame { /// `else`, and drops that chain the same way. Delimiter depth alone would let a long enough ladder /// through to overflow the stack instead of producing this guard's diagnostic. `ladders` counts /// every `else` that follows a completed group at the same level, mirroring -/// `cargo_gamma_engine::parse::nesting`'s `ladders` counter and bounded by the same `postfix_limit` +/// `cargo_gamma_engine::parse::nesting`'s `ladders` counter and bounded by the same chain limit /// as every other expression-path chain this walk already tracks. /// /// Exposed (hidden from docs) so `cargo-gamma-lib`'s agreement test can drive this scanner with @@ -150,13 +160,11 @@ struct Frame { reason = "delimiter and expression-path state must advance together through one token walk" )] pub(super) fn exceeds_nesting_limit(stream: &TokenStream, limit: usize) -> bool { - let postfix_limit = limit.saturating_mul(CHAIN_FACTOR); + let chain_limit = limit.saturating_mul(CHAIN_FACTOR); let mut frames = vec![Frame { iter: stream.clone().into_iter(), depth: 0, - postfix: 0, - casts: 0, - operators: 0, + links: 0, ladders: 0, awaiting_else: false, previous: Previous::Other, @@ -171,16 +179,14 @@ pub(super) fn exceeds_nesting_limit(stream: &TokenStream, limit: usize) -> bool } let postfix = - matches!(group.delimiter(), Delimiter::Parenthesis | Delimiter::Bracket) && frame.previous == Previous::Expression; + matches!(group.delimiter(), Delimiter::Parenthesis | Delimiter::Bracket) && frame.previous.can_end_expression(); if postfix { - frame.postfix += 1; + frame.links += 1; - if frame.postfix > postfix_limit { + if frame.exceeds_chain_limit(chain_limit) { return true; } - } else { - frame.postfix = 0; } let next_depth = frame.depth + 1; @@ -190,17 +196,15 @@ pub(super) fn exceeds_nesting_limit(stream: &TokenStream, limit: usize) -> bool } // A complete group can be the receiver of the next call or index, or the `{ }` - // block an `else` ladder continues from. The child gets a fresh postfix chain - // because only adjacent links share one expression. + // block an `else` ladder continues from. The child gets a fresh expression + // path because only links at the same token-stream level share one chain. frame.awaiting_else = group.delimiter() == Delimiter::Brace; frame.previous = Previous::Expression; frames.push(frame); frames.push(Frame { iter: group.stream().into_iter(), depth: next_depth, - postfix: 0, - casts: 0, - operators: 0, + links: 0, ladders: 0, awaiting_else: false, previous: Previous::Other, @@ -213,10 +217,9 @@ pub(super) fn exceeds_nesting_limit(stream: &TokenStream, limit: usize) -> bool frame.ladders = 0; frame.awaiting_else = false; } - frame.postfix = 0; - frame.casts += 1; + frame.links += 1; - if frame.casts > postfix_limit { + if frame.exceeds_chain_limit(chain_limit) { return true; } @@ -226,10 +229,10 @@ pub(super) fn exceeds_nesting_limit(stream: &TokenStream, limit: usize) -> bool TokenTree::Ident(ident) if ident == "else" && frame.awaiting_else => { // Reached only right after a completed group, which is what an `else` following // an `if`'s or a prior arm's `{ }` block looks like at the token-stream level. - frame.postfix = 0; frame.ladders += 1; + frame.links += 1; - if frame.ladders > postfix_limit { + if frame.exceeds_chain_limit(chain_limit) { return true; } @@ -242,6 +245,14 @@ pub(super) fn exceeds_nesting_limit(stream: &TokenStream, limit: usize) -> bool frame.ladders = 0; frame.awaiting_else = false; } + + if frame.previous == Previous::Expression { + // An operand after an expression begins a new chain across statement and + // keyword boundaries. Keep `ladders`: an `else if` condition resets + // `links`, while the separate ladder count must span every arm. + frame.links = 0; + } + frame.previous = Previous::Expression; } @@ -254,33 +265,30 @@ pub(super) fn exceeds_nesting_limit(stream: &TokenStream, limit: usize) -> bool punct.as_char(), '+' | '-' | '*' | '/' | '%' | '&' | '|' | '^' | '!' | '<' | '>' | '=' ) { - frame.operators += 1; + frame.links += 1; - if frame.operators > postfix_limit { + if frame.exceeds_chain_limit(chain_limit) { return true; } } - let postfix = matches!(punct.as_char(), '.' | '?') && frame.previous == Previous::Expression; + let postfix = matches!(punct.as_char(), '.' | '?') && frame.previous.can_end_expression(); if postfix { - frame.postfix += 1; + frame.links += 1; - if frame.postfix > postfix_limit { + if frame.exceeds_chain_limit(chain_limit) { return true; } - } else { - frame.postfix = 0; } if matches!(punct.as_char(), ',' | ';') { - frame.casts = 0; - frame.operators = 0; + frame.links = 0; frame.ladders = 0; } frame.previous = if matches!(punct.as_char(), '?' | '>') { - Previous::Expression + Previous::Operator } else { Previous::Other }; @@ -1624,6 +1632,16 @@ mod tests { assert_eq!(err, "expression nests too deeply to be safely parsed"); } + #[test] + fn mixed_operator_and_cast_chains_share_one_nesting_budget() { + let links = NESTING_LIMIT * CHAIN_FACTOR / 2 + 1; + let expression = format!("1{}{}", " + 1".repeat(links), " as u64".repeat(links)); + let error = validate_value(stream(&expression), &stream("fn f() -> u64 { 1 }")) + .expect_err("the combined expression chain must be rejected"); + + assert_eq!(error, "expression nests too deeply to be safely parsed"); + } + #[test] fn a_long_unary_chain_expands_to_a_guard_diagnostic() { let expression = format!("{}1", "-".repeat(NESTING_LIMIT * CHAIN_FACTOR + 1)); diff --git a/crates/cargo-gamma-attrs/CHANGELOG.md b/crates/cargo-gamma-attrs/CHANGELOG.md new file mode 100644 index 000000000..9bdb3282e --- /dev/null +++ b/crates/cargo-gamma-attrs/CHANGELOG.md @@ -0,0 +1,14 @@ +# Changelog + +## [Unreleased] + +## [0.2.0] - 2026-09-03 + +- 🐛 Bug Fixes + + - harden discovery and process cleanup + - address hardening feedback ([#144](https://github.com/microsoft/ox-tools/pull/144)) + +## [0.1.0] - 2026-09-02 + +- Initial release. diff --git a/crates/cargo-gamma-attrs/Cargo.toml b/crates/cargo-gamma-attrs/Cargo.toml index 01076733a..e041228dc 100644 --- a/crates/cargo-gamma-attrs/Cargo.toml +++ b/crates/cargo-gamma-attrs/Cargo.toml @@ -4,7 +4,7 @@ [package] name = "cargo-gamma-attrs" description = "Inert attribute macros for suppressing cargo-gamma mutations" -version = "0.1.0" +version = "0.2.0" readme = "README.md" keywords = ["cargo", "mutation-testing", "testing"] categories = ["development-tools::testing"] diff --git a/crates/cargo-gamma-engine/CHANGELOG.md b/crates/cargo-gamma-engine/CHANGELOG.md new file mode 100644 index 000000000..9bdb3282e --- /dev/null +++ b/crates/cargo-gamma-engine/CHANGELOG.md @@ -0,0 +1,14 @@ +# Changelog + +## [Unreleased] + +## [0.2.0] - 2026-09-03 + +- 🐛 Bug Fixes + + - harden discovery and process cleanup + - address hardening feedback ([#144](https://github.com/microsoft/ox-tools/pull/144)) + +## [0.1.0] - 2026-09-02 + +- Initial release. diff --git a/crates/cargo-gamma-engine/Cargo.toml b/crates/cargo-gamma-engine/Cargo.toml index 0d312fe2e..8af8851f3 100644 --- a/crates/cargo-gamma-engine/Cargo.toml +++ b/crates/cargo-gamma-engine/Cargo.toml @@ -4,7 +4,7 @@ [package] name = "cargo-gamma-engine" description = "Internal Rust source mutation engine for cargo-gamma" -version = "0.1.0" +version = "0.2.0" readme = "README.md" keywords = ["cargo", "mutation-testing", "testing"] categories = ["development-tools::testing"] diff --git a/crates/cargo-gamma-engine/README.md b/crates/cargo-gamma-engine/README.md index 9f7100dc4..48f0a32dc 100644 --- a/crates/cargo-gamma-engine/README.md +++ b/crates/cargo-gamma-engine/README.md @@ -13,11 +13,17 @@ -This is an implementation detail of the cargo-gamma tool. Do not take a dependency on this crate -as it may change in incompatible ways without warning. +This crate is an internal implementation detail of +[`cargo-gamma`][__link0]. It contains the Rust source parsing, +mutation collection, stable identity, and schema instrumentation pipeline. + +Do not depend on it directly. Its API may change incompatibly without notice; it is published +only so that `cargo-gamma` can be installed through crates.io.
This crate was developed as part of The Oxidizer Project. Browse this crate's source code. + + [__link0]: https://crates.io/crates/cargo-gamma diff --git a/crates/cargo-gamma-engine/src/parse/nesting.rs b/crates/cargo-gamma-engine/src/parse/nesting.rs index 7dfe00200..d439b6af4 100644 --- a/crates/cargo-gamma-engine/src/parse/nesting.rs +++ b/crates/cargo-gamma-engine/src/parse/nesting.rs @@ -49,6 +49,13 @@ pub const CHAIN_FACTOR: usize = 4; enum Previous { Other, Expression, + Operator, +} + +impl Previous { + const fn can_end_expression(self) -> bool { + matches!(self, Self::Expression | Self::Operator) + } } /// The offset of the token that first takes `text` past what `limit` allows. @@ -153,7 +160,7 @@ pub(super) fn beyond(text: &str, comments: &[Comment], limit: usize) -> Option') { - Previous::Expression + Previous::Operator } else { Previous::Other }; @@ -163,7 +170,7 @@ pub(super) fn beyond(text: &str, comments: &[Comment], limit: usize) -> Option { - let is_postfix = previous == Previous::Expression; + let is_postfix = previous.can_end_expression(); if is_postfix && link(&mut path, &mut peak, depth, path_limit) { return Some(at); diff --git a/crates/cargo-gamma-lib/CHANGELOG.md b/crates/cargo-gamma-lib/CHANGELOG.md new file mode 100644 index 000000000..9bdb3282e --- /dev/null +++ b/crates/cargo-gamma-lib/CHANGELOG.md @@ -0,0 +1,14 @@ +# Changelog + +## [Unreleased] + +## [0.2.0] - 2026-09-03 + +- 🐛 Bug Fixes + + - harden discovery and process cleanup + - address hardening feedback ([#144](https://github.com/microsoft/ox-tools/pull/144)) + +## [0.1.0] - 2026-09-02 + +- Initial release. diff --git a/crates/cargo-gamma-lib/Cargo.toml b/crates/cargo-gamma-lib/Cargo.toml index 4dfac61b7..592cda76b 100644 --- a/crates/cargo-gamma-lib/Cargo.toml +++ b/crates/cargo-gamma-lib/Cargo.toml @@ -4,7 +4,7 @@ [package] name = "cargo-gamma-lib" description = "Internal library for cargo-gamma" -version = "0.1.0" +version = "0.2.0" readme = "README.md" keywords = ["cargo", "mutation-testing", "testing"] categories = ["development-tools::testing"] @@ -41,7 +41,7 @@ blake3 = { workspace = true, features = ["std"] } camino = { workspace = true } cargo-gamma-engine = { workspace = true } cargo-gamma-process = { workspace = true } -cargo-gamma-rt = { workspace = true } +cargo-gamma-rt = { workspace = true, features = ["embedding"] } cargo-gamma-unsafe = { workspace = true } cargo_metadata = { workspace = true } clap = { workspace = true, features = ["std", "derive", "color", "help", "error-context", "usage", "suggestions", "env", "wrap_help"] } diff --git a/crates/cargo-gamma-lib/README.md b/crates/cargo-gamma-lib/README.md index ee3d80f12..ba093fbef 100644 --- a/crates/cargo-gamma-lib/README.md +++ b/crates/cargo-gamma-lib/README.md @@ -13,11 +13,15 @@ -This is an implementation detail of the cargo-gamma tool. Do not take a dependency on this crate -as it may change in incompatible ways without warning. +Internal implementation library for [`cargo-gamma`][__link0]. + +This crate is an implementation detail. Do not depend on it: it may change in incompatible +ways without warning, and it carries no semver commitment to anything it exposes.
This crate was developed as part of The Oxidizer Project. Browse this crate's source code. + + [__link0]: https://crates.io/crates/cargo-gamma diff --git a/crates/cargo-gamma-lib/docs/design/README.md b/crates/cargo-gamma-lib/docs/design/README.md index de891b2fa..cbe715464 100644 --- a/crates/cargo-gamma-lib/docs/design/README.md +++ b/crates/cargo-gamma-lib/docs/design/README.md @@ -20,8 +20,17 @@ verdicts, incremental reuse, reporting, and command dispatch. process-output lifecycle as later builds and tests. Their stdout and stderr are drained concurrently, and descendants are swept before inherited pipe handles are allowed to keep capture open. +- Build and verdict supervision surface a failure to terminate a timed-out or + otherwise abandoned subtree instead of continuing as though cleanup + succeeded. Verdict cleanup failure abandons the remaining mutation campaign + because surviving descendants can interfere with later mutants. Census + remains deliberately fail-open: it is an optimization, and its bounded + output drain converts cleanup failure into a missing census so ordinary + discovery can still proceed. - The injected guard protocol is provided by dependency-free - `cargo-gamma-rt`. + `cargo-gamma-rt`. Its package-local source bundle is exposed only through an + internal feature used by the coordinator, so published `cargo-gamma-lib` + packages never depend on repository-relative source paths. - The `internals` feature exists only for this crate's integration tests and is not a supported downstream API. - The agreement tests use `cargo-gamma-attrs-impl` through a versionless path @@ -52,6 +61,22 @@ includes both failure to acquire the startup environment and a guard reached before the runtime constructor installed its selection; either fixed marker disqualifies the process as mutation-score evidence. +Diff paths are resolved to the workspace-relative Rust files discovered by the +survey. Absolute or rooted paths inside the workspace are normalized to those +candidates; one from another checkout is normalized only when its suffix +uniquely identifies one candidate. Paths that traverse outside the workspace +are never accepted as source selections; this includes parent traversal and +Windows drive-relative prefixes. Non-source paths count as understood only +when they name regular workspace files. Diffs, checked-in hints, and +incremental records are read under a 256 MiB bound. An oversized diff is a +usage error, while oversized optimization artifacts are ignored under the same +fail-open contract as corrupt or foreign-version artifacts. + +Reports use the same source generation from which their mutant spans were +derived. If an analyzed source changes before report construction, the run +refuses to publish reports that would combine the completed verdicts with the +new source. + ### Redirected cache security On Unix, cargo-gamma creates a previously absent redirected cache with diff --git a/crates/cargo-gamma-lib/src/discover/diff.rs b/crates/cargo-gamma-lib/src/discover/diff.rs index 653067958..f52e6df9c 100644 --- a/crates/cargo-gamma-lib/src/discover/diff.rs +++ b/crates/cargo-gamma-lib/src/discover/diff.rs @@ -3,11 +3,14 @@ //! Restricting a run to the lines a unified diff touches. +#[cfg(test)] use std::fs; +use std::fs::File; use std::io::{Read, stdin}; -use camino::{Utf8Path, Utf8PathBuf}; +use camino::{Utf8Component, Utf8Path, Utf8PathBuf}; +use super::input; use crate::error::error; use crate::{HashMap, HashSet, Result}; @@ -42,18 +45,19 @@ impl Diff { /// # Errors /// /// Returns an error when `path` is `-` and reading `input` fails or its bytes are not UTF-8, or - /// when another path cannot be opened, read, or decoded as UTF-8. + /// when another path cannot be opened, read, or decoded as UTF-8. Inputs larger than + /// [`input::MAX_BYTES`] are refused before they can exhaust the process's memory. pub fn read_from(path: &Utf8Path, mut input: impl Read) -> Result { let text = if path == "-" { - let mut buffer = String::new(); + input::text(&mut input).map_err(|cause| error!("could not read a diff from standard input").caused_by(cause))? + } else { + let file = File::open(path).map_err(|cause| error!("could not read the diff `{path}`").caused_by(cause))?; - let _read = input - .read_to_string(&mut buffer) - .map_err(|cause| error!("could not read a diff from standard input").caused_by(cause))?; + input::text(file).map_err(|cause| error!("could not read the diff `{path}`").caused_by(cause))? + }; - buffer - } else { - fs::read_to_string(path).map_err(|cause| error!("could not read the diff `{path}`").caused_by(cause))? + let Some(text) = text else { + return Err(error!("the diff is larger than the {} bytes cargo-gamma will retain", input::MAX_BYTES).usage()); }; Ok(Self::parse(&text)) @@ -346,13 +350,26 @@ fn split_first_segment(path: &str) -> Option<(&str, &str)> { /// Finds the workspace file a diff path refers to, if any. fn locate(path: &Utf8Path, raw: &Utf8Path, root: &Utf8Path, known: &HashSet<&Utf8Path>, candidates: &[Utf8PathBuf]) -> Option { - // The set answers first, so the filesystem probe is paid only for a path the workspace file - // list does not already hold — which is what makes a miss cost one syscall rather than one - // syscall per candidate spelling per peeled segment. - let known = |candidate: &Utf8Path| known.contains(candidate) || root.join(candidate).exists(); - for candidate in [path, raw] { - if known(candidate) { + if candidate.is_absolute() { + if let Ok(relative) = candidate.strip_prefix(root) { + if known.contains(relative) { + return Some(relative.to_owned()); + } + + if safe_relative(relative) && relative.extension() != Some("rs") && root.join(relative).is_file() { + return Some(relative.to_owned()); + } + } + + continue; + } + + if !safe_relative(candidate) { + continue; + } + + if known.contains(candidate) { return Some(candidate.to_owned()); } @@ -368,7 +385,7 @@ fn locate(path: &Utf8Path, raw: &Utf8Path, root: &Utf8Path, known: &HashSet<&Utf break; } - if known(Utf8Path::new(rest)) { + if known.contains(Utf8Path::new(rest)) { return Some(Utf8PathBuf::from(rest)); } } @@ -379,12 +396,26 @@ fn locate(path: &Utf8Path, raw: &Utf8Path, root: &Utf8Path, known: &HashSet<&Utf // // This one stays a scan, and has to: a suffix match is not a lookup, and there is no key to // hash. It is reached only when every exact spelling has already failed. - let mut matched = candidates - .iter() - .filter(|file| ends_with_path(path, file) || ends_with_path(raw, file)); - let found = matched.next()?; + let mut matched = candidates.iter().filter(|file| { + [path, raw] + .into_iter() + .any(|candidate| (candidate.has_root() || safe_relative(candidate)) && ends_with_path(candidate, file)) + }); + + match (matched.next(), matched.next()) { + (Some(found), None) => Some(found.clone()), + (Some(_ambiguous), Some(_)) => None, + (None, _) => [path, raw] + .into_iter() + .find(|candidate| safe_relative(candidate) && candidate.extension() != Some("rs") && root.join(candidate).is_file()) + .map(ToOwned::to_owned), + } +} - matched.next().is_none().then(|| found.clone()) +/// Whether a path can be interpreted relative to the workspace without escaping it. +fn safe_relative(path: &Utf8Path) -> bool { + path.components() + .all(|component| matches!(component, Utf8Component::CurDir | Utf8Component::Normal(_))) } /// Whether `path` ends with `suffix` at a segment boundary. @@ -581,6 +612,61 @@ index 1234567..89abcde 100644 assert!(diff.touches(Utf8Path::new("src/lib.rs"), 12, 12)); } + #[test] + fn an_absolute_workspace_path_resolves_to_its_relative_candidate() { + let directory = tempfile::tempdir().expect("could not create a temporary directory"); + let root = Utf8PathBuf::from_path_buf(directory.path().to_path_buf()).expect("the temporary path is not UTF-8"); + let source = root.join("src/lib.rs"); + + fs::create_dir_all(source.parent().expect("source parent")).expect("source directory"); + fs::write(&source, "one\ntwo\n").expect("source"); + + let text = format!("--- {source}\n+++ {source}\n@@ -1 +1,2 @@\n one\n+two\n"); + let mut diff = Diff::parse(&text); + + diff.resolve(&root, &[Utf8PathBuf::from("src/lib.rs")]) + .expect("an absolute path inside the workspace resolves"); + + assert!(diff.touches(Utf8Path::new("src/lib.rs"), 2, 2)); + assert!(!diff.touches_file(&source), "the absolute spelling must not survive resolution"); + } + + #[test] + fn an_absolute_path_from_another_checkout_resolves_by_unique_suffix() { + let text = "--- /other/checkout/src/lib.rs\n+++ /other/checkout/src/lib.rs\n@@ -1 +1,2 @@\n one\n+two\n"; + let mut diff = Diff::parse(text); + + diff.resolve(Utf8Path::new("/this/checkout"), &[Utf8PathBuf::from("src/lib.rs")]) + .expect("the absolute path uniquely names a workspace candidate"); + + assert!(diff.touches(Utf8Path::new("src/lib.rs"), 2, 2)); + } + + #[test] + fn a_traversal_to_an_existing_external_source_is_refused() { + let directory = tempfile::tempdir().expect("could not create a temporary directory"); + let base = Utf8PathBuf::from_path_buf(directory.path().to_path_buf()).expect("the temporary path is not UTF-8"); + let root = base.join("workspace"); + + fs::create_dir(&root).expect("workspace"); + fs::write(base.join("outside.rs"), "one\ntwo\n").expect("external source"); + + let text = "--- ../outside.rs\n+++ ../outside.rs\n@@ -1 +1,2 @@\n one\n+two\n"; + let mut diff = Diff::parse(text); + let error = diff + .resolve(&root, &[Utf8PathBuf::from("src/lib.rs")]) + .expect_err("a path outside the workspace is not a workspace candidate"); + + assert!(error.is_usage(), "{error}"); + assert!(error.to_string().contains("../outside.rs"), "{error}"); + } + + #[cfg(windows)] + #[test] + fn a_drive_relative_path_is_not_safe_workspace_input() { + assert!(!safe_relative(Utf8Path::new("C:outside.rs"))); + } + // A diff produced from a subdirectory, or with a prefix nothing recognized, names the file with // leading directories the workspace does not have. The workspace file it uniquely ends with is // the file meant, and matching it is the difference between running the change and running @@ -656,6 +742,38 @@ index 1234567..89abcde 100644 assert!(diff.touches_file(Utf8Path::new("README.md"))); } + #[test] + fn a_directory_is_not_treated_as_an_understood_diff_path() { + let directory = tempfile::tempdir().expect("could not create a temporary directory"); + let root = Utf8PathBuf::from_path_buf(directory.path().to_path_buf()).expect("the temporary path is not UTF-8"); + + fs::create_dir(root.join("docs")).expect("docs directory"); + + let text = "--- docs\n+++ docs\n@@ -1 +1,2 @@\n one\n+two\n"; + let mut diff = Diff::parse(text); + let error = diff.resolve(&root, &[]).expect_err("a directory is not a diff file"); + + assert!(error.is_usage(), "{error}"); + assert!(error.to_string().contains("docs"), "{error}"); + } + + #[test] + fn an_absolute_non_source_path_inside_the_workspace_counts_as_understood() { + let directory = tempfile::tempdir().expect("could not create a temporary directory"); + let root = Utf8PathBuf::from_path_buf(directory.path().to_path_buf()).expect("the temporary path is not UTF-8"); + let readme = root.join("README.md"); + + fs::write(&readme, "hello").expect("could not write the file"); + + let text = format!("--- {readme}\n+++ {readme}\n@@ -1 +1,2 @@\n one\n+two\n"); + let mut diff = Diff::parse(&text); + + diff.resolve(&root, &[]) + .expect("an absolute non-source path inside the workspace is understood"); + + assert!(diff.touches_file(Utf8Path::new("README.md"))); + } + // The partial failure, which is the dangerous one. One resolvable path is enough to satisfy the // "resolved nothing" check above, so without this the unresolved source file is dropped // silently: `Survey::for_build` keeps only the files the diff is believed to touch, so its diff --git a/crates/cargo-gamma-lib/src/discover/hints.rs b/crates/cargo-gamma-lib/src/discover/hints.rs index cb950aaaf..715af4909 100644 --- a/crates/cargo-gamma-lib/src/discover/hints.rs +++ b/crates/cargo-gamma-lib/src/discover/hints.rs @@ -25,12 +25,13 @@ #[cfg(test)] use core::cell::RefCell; -use std::fs; +use std::fs::{self, File}; use std::io::ErrorKind; use camino::{Utf8Path, Utf8PathBuf}; use serde::{Deserialize, Serialize}; +use super::input; use super::record::{ContextDigest, Killer, RunRecord, Tier}; use crate::elements::Publication; use crate::error::error; @@ -159,7 +160,7 @@ impl Hints { /// Reads and validates the artifact at `path`, or nothing when it cannot be trusted. fn read(path: &Utf8Path) -> Option { - let text = fs::read_to_string(path.as_std_path()).ok()?; + let text = input::text(File::open(path.as_std_path()).ok()?).ok()??; let hints = serde_json::from_str::(&text).ok()?; (hints.version == VERSION).then_some(hints) @@ -267,12 +268,30 @@ impl Hints { let text = self.rendered()?; let workspace = path.parent().unwrap_or_else(|| Utf8Path::new(".")); + if u64::try_from(text.len()).unwrap_or(u64::MAX) > input::MAX_BYTES { + return Err(error!( + "the promoted hints are larger than the {} bytes cargo-gamma will retain", + input::MAX_BYTES + )); + } + // Absent and unreadable are different answers, and collapsing them into one `None` is what // turns the rollback below into a delete: undoing a creation means removing the file, and // a file that was there all along is not a creation. A file this cannot restore is a file // it must not replace, so an existing artifact it cannot read stops the promotion outright. - let before = match fs::read_to_string(path.as_std_path()) { - Ok(text) => Some(text), + let before = match File::open(path.as_std_path()) { + Ok(file) => match input::text(file) { + Ok(Some(text)) => Some(text), + Ok(None) => { + return Err(error!( + "`{path}` is larger than the {} bytes cargo-gamma will retain, so it cannot be safely replaced", + input::MAX_BYTES + )); + } + Err(cause) => { + return Err(error!("`{path}` is already there and could not be read, so it must not be replaced").caused_by(cause)); + } + }, Err(cause) if cause.kind() == ErrorKind::NotFound => None, Err(cause) => { return Err(error!("`{path}` is already there and could not be read, so it must not be replaced").caused_by(cause)); diff --git a/crates/cargo-gamma-lib/src/discover/input.rs b/crates/cargo-gamma-lib/src/discover/input.rs new file mode 100644 index 000000000..1f0fe2c4a --- /dev/null +++ b/crates/cargo-gamma-lib/src/discover/input.rs @@ -0,0 +1,57 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Bounded reads of discovery inputs and optimization artifacts. + +use std::io::{self, Read}; + +/// The largest discovery input or optimization artifact retained in memory. +/// +/// 256 MiB leaves ample room for legitimate compiler artifacts while bounding one malformed or +/// hostile input below the memory budget of ordinary development hosts. Raising this value raises +/// peak memory per concurrently read artifact; lowering it makes larger valid hints and records +/// fail with their callers' oversized-input errors. +pub(super) const MAX_BYTES: u64 = 256 * 1024 * 1024; + +/// Reads UTF-8 text without retaining more than [`MAX_BYTES`]. +/// +/// `None` means the input exceeded that bound. +/// +/// # Errors +/// +/// Returns the reader's I/O error, or [`io::ErrorKind::InvalidData`] when the retained bytes are +/// not valid UTF-8. +pub(super) fn text(input: impl Read) -> io::Result> { + text_with_limit(input, MAX_BYTES) +} + +fn text_with_limit(mut input: impl Read, limit: u64) -> io::Result> { + let mut bytes = Vec::new(); + let mut capped = input.by_ref().take(limit.saturating_add(1)); + + let _read = capped.read_to_end(&mut bytes)?; + let length = u64::try_from(bytes.len()).unwrap_or(u64::MAX); + + if length > limit { + return Ok(None); + } + + String::from_utf8(bytes) + .map(Some) + .map_err(|cause| io::Error::new(io::ErrorKind::InvalidData, cause)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn exact_limit_is_retained() { + assert_eq!(text_with_limit(&b"four"[..], 4).expect("read"), Some("four".to_owned())); + } + + #[test] + fn oversized_input_is_refused_after_one_extra_byte() { + assert_eq!(text_with_limit(&b"oversized"[..], 4).expect("read"), None); + } +} diff --git a/crates/cargo-gamma-lib/src/discover/mod.rs b/crates/cargo-gamma-lib/src/discover/mod.rs index 094eee597..8d3b36145 100644 --- a/crates/cargo-gamma-lib/src/discover/mod.rs +++ b/crates/cargo-gamma-lib/src/discover/mod.rs @@ -7,6 +7,7 @@ mod compile_fail; mod diff; mod glob; mod hints; +mod input; mod killers; mod modules; mod order; diff --git a/crates/cargo-gamma-lib/src/discover/record.rs b/crates/cargo-gamma-lib/src/discover/record.rs index 8b1966d75..202dd9990 100644 --- a/crates/cargo-gamma-lib/src/discover/record.rs +++ b/crates/cargo-gamma-lib/src/discover/record.rs @@ -3,17 +3,20 @@ //! Build facts and checked hints from the last run. +use std::env; +#[cfg(test)] +use std::fs; +use std::fs::File; use std::process::Command; use std::slice::Iter; -use std::{env, fs}; use blake3::Hasher; use camino::{Utf8Path, Utf8PathBuf}; use serde::{Deserialize, Serialize}; -use super::Plan; use super::killers::Killers; use super::workspace_snapshot::WorkspaceSnapshot; +use super::{Plan, input}; use crate::cfg::Build; use crate::model::{Mutant, MutantId, Outcome}; use crate::{HashMap, HashSet}; @@ -583,7 +586,7 @@ impl RunRecord { /// Reads the record from disk, or nothing when it is absent, unreadable or a foreign format. fn load_raw(base: &Utf8Path) -> Option { - let text = fs::read_to_string(base.join(FILE)).ok()?; + let text = input::text(File::open(base.join(FILE)).ok()?).ok()??; let record = serde_json::from_str::(&text).ok()?; (record.version == VERSION).then_some(record) diff --git a/crates/cargo-gamma-lib/src/elements/report.rs b/crates/cargo-gamma-lib/src/elements/report.rs index 0a343e030..0b48621d6 100644 --- a/crates/cargo-gamma-lib/src/elements/report.rs +++ b/crates/cargo-gamma-lib/src/elements/report.rs @@ -478,6 +478,16 @@ pub fn build(plan: &Plan, thresholds: Thresholds, run: Option) -> Resul .map_err(|cause| error!("could not read `{}`", file.absolute).caused_by(cause))?; let has_bom = original.starts_with('\u{feff}'); let source = SourceFile::parse(file.absolute.clone(), original.clone())?; + + if let Some(expected) = plan.digests.get(&file.path) + && crate::discover::digest(source.text().as_bytes()) != *expected + { + return Err(error!( + "`{}` changed after its mutants were discovered; rerun cargo-gamma so discovery, verdicts, and report source use the same generation", + file.path + )); + } + let rendered = mutants .iter() .map(|mutant| render_with_first_line_offset(mutant, &source, usize::from(has_bom))) @@ -1909,6 +1919,51 @@ mod tests { assert_eq!(emitted, expected); } + #[test] + fn a_report_refuses_source_changed_after_discovery() { + let directory = crate::testing::workdir("elements-source-generation"); + let root = Utf8PathBuf::from_path_buf(directory.path().to_path_buf()).expect("the scratch path is UTF-8"); + let path = Utf8PathBuf::from("lib.rs"); + let absolute = root.join(&path); + let discovered = "fn f() { a < b; }\n"; + + fs::write(&absolute, discovered).expect("discovered source"); + + let mut digests = HashMap::default(); + let _previous = digests.insert(path.clone(), crate::discover::digest(discovered.as_bytes())); + let plan = Plan { + skipped: Vec::new(), + digests, + root, + files: vec![TargetFile { + path: path.clone(), + absolute: absolute.clone(), + package: "subject".to_owned(), + }], + mutants: vec![Mutant { + file: path.into(), + ..mutant(Outcome::Killed, 9..14) + }], + suppressed: 0, + idle: Vec::new(), + sharded_out: 0, + settled_out: 0, + reach: HashMap::default(), + specs: HashMap::default(), + }; + + fs::write(&absolute, "fn f() { a > b; }\n").expect("edited source"); + + let error = build(&plan, Thresholds::default(), None).expect_err("mixed source generations must be refused"); + + assert!( + error + .to_string() + .contains("rerun cargo-gamma so discovery, verdicts, and report source use the same generation"), + "{error}" + ); + } + /// A mutant in a file the plan does not list would vanish from the denominator without a word. /// /// The skip is deliberate — a report cannot embed the source of a file the survey never diff --git a/crates/cargo-gamma-lib/src/exec/build/invoke.rs b/crates/cargo-gamma-lib/src/exec/build/invoke.rs index 564fe10c1..12304b078 100644 --- a/crates/cargo-gamma-lib/src/exec/build/invoke.rs +++ b/crates/cargo-gamma-lib/src/exec/build/invoke.rs @@ -149,9 +149,9 @@ pub(super) fn supervise_with_limits( Ok(None) => { if deadline.is_some_and(|deadline| Instant::now() >= deadline) { - collect(&mut subtree); - - break Ok(None); + break collect(&mut subtree).map(|()| None).map_err(|cause| { + error!("cargo in `{root}` could not be terminated after its build budget expired").caused_by(cause) + }); } thread::sleep(BUILD_POLL_INTERVAL); @@ -160,9 +160,12 @@ pub(super) fn supervise_with_limits( Err(cause) => { // Nothing more can be asked of this child, and dropping it would leave the whole // build tree running with no handle on it at all. - collect(&mut subtree); - - break Err(error!("could not wait for cargo in `{root}`").caused_by(cause)); + break match collect(&mut subtree) { + Ok(()) => Err(error!("could not wait for cargo in `{root}`").caused_by(cause)), + Err(cleanup) => { + Err(error!("cargo in `{root}` could not be terminated after it stopped being observable").caused_by(cleanup)) + } + }; } } }; @@ -227,14 +230,14 @@ pub(super) fn supervise_with_limits( /// /// The ordinary-exit path uses the same order through [`ProcessTree::observe`], which observes an exit /// without reaping it before sweeping and finally waiting. -fn collect(subtree: &mut ProcessTree) { +fn collect(subtree: &mut ProcessTree) -> io::Result<()> { #[cfg(unix)] debug_assert!( !subtree.released(), "the subtree is signalled while it still holds its leader and its watch slot" ); - let _reaped = subtree.terminate(); + subtree.terminate().map(|_status| ()) } /// How wide cargo is asked to draw its progress bar. diff --git a/crates/cargo-gamma-lib/src/exec/build/tests.rs b/crates/cargo-gamma-lib/src/exec/build/tests.rs index 5bf3f488e..59bf82845 100644 --- a/crates/cargo-gamma-lib/src/exec/build/tests.rs +++ b/crates/cargo-gamma-lib/src/exec/build/tests.rs @@ -2697,11 +2697,7 @@ fn guarded_workspace(prefix: &str) -> (tempfile::TempDir, Workspace) { // The tool's own runtime sources rather than stand-ins, so a change to the guard's signature // breaks this fixture rather than leaving it testing a shape nothing generates any more. - for (name, source) in [ - ("lib.rs", include_str!("../../../../cargo-gamma-rt/src/lib.rs")), - ("either.rs", include_str!("../../../../cargo-gamma-rt/src/either.rs")), - ("runtime.rs", include_str!("../../../../cargo-gamma-rt/src/runtime.rs")), - ] { + for (name, source) in gamma_rt::embedded::SOURCES { fs::write(runtime.join("src").join(name).as_std_path(), source).expect("runtime source"); } diff --git a/crates/cargo-gamma-lib/src/exec/census.rs b/crates/cargo-gamma-lib/src/exec/census.rs index 7910dd64e..aec5dd1fc 100644 --- a/crates/cargo-gamma-lib/src/exec/census.rs +++ b/crates/cargo-gamma-lib/src/exec/census.rs @@ -592,7 +592,9 @@ fn listed(mut command: Command, budget: Duration) -> Option>> { // Whatever the child spawned goes with it when no normal exit was observed: a listing that // started a server leaves it holding the pipe this is about to read, and the reader would then - // wait out the whole run for an end of file that never comes. + // wait out the whole run for an end of file that never comes. Cleanup failure stays fail-open: + // census is an optimization, and the bounded drain below turns an unclosed pipe into a missing + // census rather than failing mutation testing before the ordinary discovery path can run. if status.is_none() { let _reaped = subtree.terminate(); } else { diff --git a/crates/cargo-gamma-lib/src/exec/verdict.rs b/crates/cargo-gamma-lib/src/exec/verdict.rs index 765c5e747..3ac3a9fc9 100644 --- a/crates/cargo-gamma-lib/src/exec/verdict.rs +++ b/crates/cargo-gamma-lib/src/exec/verdict.rs @@ -196,13 +196,13 @@ pub(super) enum Verdict { /// that test, not to write a new one. Flaky(Option), - /// The run could not be measured as this run was configured, so nothing about the mutant was - /// learned. + /// The run-wide machinery could not continue producing trustworthy verdicts. /// - /// Two shapes reach this: the accounting the run asked for could not be installed, so nothing - /// was started at all, and a run that started but could not be followed to a conclusion — a - /// wait on the child that failed, which leaves the one question that would have settled the - /// mutant unanswerable. + /// Three shapes reach this: the accounting the run asked for could not be installed, so nothing + /// was started at all; a run that started but could not be followed to a conclusion; and a run + /// whose process-tree cleanup failed, leaving containment unproven. The last shape may occur + /// after this mutant's verdict was settled, but later mutants cannot safely run beside + /// descendants that may still hold scratch-tree locks or inherited pipes. /// /// Not a verdict about the mutant either way. It exists so that a failure of the machinery /// stops the run and says why, instead of quietly becoming an unprotected run or, worse, a @@ -749,6 +749,15 @@ fn configure( fn run_with(work: &Workspace, binary: &TestBinary, attempt: Attempt<'_>, progress: &Arc>) -> (Verdict, MemoryUsage) { let (active, timeout, stall, request) = (attempt.active, attempt.timeout, attempt.stall, attempt.request); + macro_rules! cut_short { + ($subtree:expr, $verdict_settled:expr) => { + match cut_short($subtree, request, binary, $verdict_settled) { + Ok(stopped) => stopped, + Err(unjudged) => return unjudged, + } + }; + } + let mut command = match launcher(work, binary, attempt.only) { Ok(command) => command, Err(reason) => return (Verdict::Unmetered(reason), MemoryUsage::default()), @@ -788,7 +797,7 @@ fn run_with(work: &Workspace, binary: &TestBinary, attempt: Attempt<'_>, progres let drained = match readers(&mut subtree, progress, &pulse, under_nextest) { Ok(drained) => drained, Err(cause) => { - let (usage, _ceiling) = cut_short(&mut subtree, request); + let (usage, _ceiling) = cut_short!(&mut subtree, false); return ( Verdict::Unjudged(format!("`{}` output could not be supervised: {cause}", binary.path)), @@ -842,7 +851,7 @@ fn run_with(work: &Workspace, binary: &TestBinary, attempt: Attempt<'_>, progres Ok(None) => { if let Some(verdict) = environment_verdict(progress) { - let (usage, _ceiling) = cut_short(&mut subtree, request); + let (usage, _ceiling) = cut_short!(&mut subtree, true); return (verdict, usage); } @@ -853,7 +862,7 @@ fn run_with(work: &Workspace, binary: &TestBinary, attempt: Attempt<'_>, progres // environment-error marker. Cutting there would convict a mutant before the // evidence that the test never started was available. if let Some(name) = failure_to_cut_short(under_nextest, progress) { - let (usage, ceiling) = cut_short(&mut subtree, request); + let (usage, ceiling) = cut_short!(&mut subtree, true); return (cut_by_named_failure(name, usage.peak, ceiling), usage); } @@ -861,7 +870,7 @@ fn run_with(work: &Workspace, binary: &TestBinary, attempt: Attempt<'_>, progres let stalled = stall.exceeded(progress); if stalled || deadline.is_some_and(|deadline| Instant::now() >= deadline) { - let (usage, ceiling) = cut_short(&mut subtree, request); + let (usage, ceiling) = cut_short!(&mut subtree, true); if let Some(verdict) = unfinished_nextest_failure(under_nextest, progress) { return (verdict, usage); @@ -900,7 +909,7 @@ fn run_with(work: &Workspace, binary: &TestBinary, attempt: Attempt<'_>, progres // ended and reaped first, since an orphan holds scratch-tree locks and the pipes the // readers below are waiting on, and both outlive this function into the next mutant. Err(cause) => { - let (usage, _ceiling) = cut_short(&mut subtree, request); + let stopped = cut_short(&mut subtree, request, binary, false); debug_assert!(subtree.released(), "the containment is released before the output is drained"); @@ -910,6 +919,11 @@ fn run_with(work: &Workspace, binary: &TestBinary, attempt: Attempt<'_>, progres // was going to be concluded from the text either way. let _discarded = collected(&drained, DRAIN_GRACE); + let (usage, _ceiling) = match stopped { + Ok(stopped) => stopped, + Err(unmetered) => return unmetered, + }; + return ( Verdict::Unjudged(format!("`{}` could not be asked whether it had finished: {cause}", binary.path)), usage, @@ -1034,7 +1048,7 @@ fn cut_by_named_failure(name: String, peak: Option, ceiling: Option) - prefer_named(Verdict::Failed(Some(name)), peak, ceiling) } -/// Ends a run whose verdict is already settled, and reports the ceiling when one fired. +/// Ends a run early, whether before or after its verdict is settled. /// /// Everything the workload spawned goes with it. An orphan holds locks in the scratch tree, which /// fails the next run, and an inherited pipe handle, which keeps whoever is reading this run's @@ -1045,8 +1059,14 @@ fn cut_by_named_failure(name: String, peak: Option, ceiling: Option) - /// test already named has two true facts and prefers the name; a run cut short by silence or by its /// budget has only the ceiling, and the memory is the cause of the overrun rather than a second /// symptom of it — reporting the stall instead would send the reader looking for a hang that is not -/// there. -fn cut_short(subtree: &mut ProcessTree, request: MemoryRequest) -> (MemoryUsage, Option) { +/// there. `verdict_settled` affects only the cleanup-failure diagnostic: pre-verdict reader and +/// observation failures must not claim that a verdict existed. +fn cut_short( + subtree: &mut ProcessTree, + request: MemoryRequest, + binary: &TestBinary, + verdict_settled: bool, +) -> Result<(MemoryUsage, Option), (Verdict, MemoryUsage)> { // Only Unix has a numeric watch slot that can be released too early. On Windows `released` // necessarily returns true because the job handle itself remains the authority over the child. #[cfg(unix)] @@ -1055,12 +1075,28 @@ fn cut_short(subtree: &mut ProcessTree, request: MemoryRequest) -> (MemoryUsage, "the subtree is signalled while it still holds its leader and its watch slot" ); - let _reaped = subtree.terminate(); - + let stopped = subtree.terminate(); let usage = subtree.usage(); + + if let Err(cause) = stopped { + let point = if verdict_settled { + "after its verdict was settled" + } else { + "after execution began" + }; + + return Err(( + Verdict::Unmetered(format!( + "`{}` and its descendants could not be terminated {point}; containment is unproven: {cause}", + binary.path, + )), + usage, + )); + } + let ceiling = request.limit.filter(|_limit| exhausted(&usage, false)); - (usage, ceiling) + Ok((usage, ceiling)) } /// Whether a finished run should be read as having been stopped by its memory ceiling. @@ -2347,6 +2383,32 @@ mod tests { ); } + #[test] + fn a_termination_failure_abandons_the_run() { + let (_directory, work) = scripted(&["sleep:30000"]); + let sleeper = crate::testing::helper(); + let _failed = process_faults::arm(ProcessFault::Terminate); + + let (verdict, _usage) = run_with( + &work, + &sleeper, + Attempt { + active: Some(1), + timeout: Some(Duration::from_millis(50)), + stall: Stall::NONE, + request: MemoryRequest { meter: false, limit: None }, + only: Only::All, + census: None, + }, + &Arc::new(Mutex::new(Progress::new(Watch::Off))), + ); + + assert!( + matches!(verdict, Verdict::Unmetered(ref reason) if reason.contains("could not be terminated")), + "{verdict:?}" + ); + } + /// A binary that goes quiet for longer than its stall budget is cut off early. #[test] fn a_binary_that_goes_quiet_is_stalled_at_the_last_test_it_named() { diff --git a/crates/cargo-gamma-lib/src/exec/workspace.rs b/crates/cargo-gamma-lib/src/exec/workspace.rs index 9370a4b27..58d83cc6d 100644 --- a/crates/cargo-gamma-lib/src/exec/workspace.rs +++ b/crates/cargo-gamma-lib/src/exec/workspace.rs @@ -51,16 +51,6 @@ pub(crate) const fn cache_lock_identity(locks: &CacheLocks) -> usize { locks.identity } -/// The guard runtime's sources, embedded so that the vendored copy cannot drift from the real one. -const RUNTIME_SOURCES: [(&str, &str); 3] = [ - ("lib.rs", include_str!("../../../cargo-gamma-rt/src/lib.rs")), - ("either.rs", include_str!("../../../cargo-gamma-rt/src/either.rs")), - ("runtime.rs", include_str!("../../../cargo-gamma-rt/src/runtime.rs")), -]; - -/// The workspace package contract inherited by the real runtime crate. -const WORKSPACE_MANIFEST: &str = include_str!("../../../../Cargo.toml"); - /// Identifies the workspace allowed to reuse a cache directory. const CACHE_OWNER: &str = ".cargo-gamma-owner"; @@ -1607,31 +1597,18 @@ fn vendor_runtime(at: &Utf8Path) -> Result<()> { fs::create_dir_all(source.as_std_path()).map_err(|cause| error!("could not create `{source}`").caused_by(cause))?; - let workspace: toml::Value = toml::from_str(WORKSPACE_MANIFEST) - .map_err(|cause| error!("could not read the embedded workspace package contract").caused_by(cause))?; - let package = workspace - .get("workspace") - .and_then(|workspace| workspace.get("package")) - .and_then(toml::Value::as_table) - .ok_or_else(|| error!("the embedded workspace manifest has no `[workspace.package]` table"))?; - let inherited = |name| { - package - .get(name) - .and_then(toml::Value::as_str) - .ok_or_else(|| error!("the embedded workspace package contract has no string `{name}`")) - }; - let edition = inherited("edition")?; - let rust_version = inherited("rust-version")?; let manifest = format!( - "[package]\nname = \"{RUNTIME_PACKAGE}\"\nversion = \"0.0.0\"\nedition = \"{edition}\"\nrust-version = \"{rust_version}\"\npublish = false\n\n\ - [features]\nloom = []\n\n[lints.rust]\nunexpected_cfgs = {{ level = \"warn\", check-cfg = ['cfg(coverage_nightly)', 'cfg(loom)'] }}\n\n\ - [lib]\nname = \"{RUNTIME_CRATE}\"\npath = \"src/lib.rs\"\n\n[workspace]\n" + "[package]\nname = \"{RUNTIME_PACKAGE}\"\nversion = \"0.0.0\"\nedition = \"{}\"\nrust-version = \"{}\"\npublish = false\n\n\ + [features]\nembedding = []\nloom = []\n\n[lints.rust]\nunexpected_cfgs = {{ level = \"warn\", check-cfg = ['cfg(coverage_nightly)', 'cfg(loom)'] }}\n\n\ + [lib]\nname = \"{RUNTIME_CRATE}\"\npath = \"src/lib.rs\"\n\n[workspace]\n", + gamma_rt::embedded::EDITION, + gamma_rt::embedded::RUST_VERSION ); fs::write(at.join("Cargo.toml").as_std_path(), manifest) .map_err(|cause| error!("could not write the runtime manifest in `{at}`").caused_by(cause))?; - for (name, contents) in RUNTIME_SOURCES { + for (name, contents) in gamma_rt::embedded::SOURCES { let path = source.join(name); fs::write(path.as_std_path(), contents).map_err(|cause| error!("could not write the runtime source `{path}`").caused_by(cause))?; @@ -2131,7 +2108,7 @@ mod tests { fn the_vendored_runtime_is_the_real_one() { // If these ever diverge, guards would be compiled against a runtime that is not the one // this build was tested with. - let runtime = RUNTIME_SOURCES + let runtime = gamma_rt::embedded::SOURCES .iter() .find_map(|(name, source)| (*name == "runtime.rs").then_some(*source)) .expect("runtime.rs is one of the embedded runtime sources"); @@ -2157,7 +2134,7 @@ mod tests { // The `[workspace]` table keeps it from being adopted by whatever workspace it lands near. assert!(manifest.contains("[workspace]")); - for (name, _contents) in RUNTIME_SOURCES { + for (name, _contents) in gamma_rt::embedded::SOURCES { assert!(at.join("src").join(name).as_std_path().is_file(), "{name} was not vendored"); } diff --git a/crates/cargo-gamma-lib/tests/agreement.rs b/crates/cargo-gamma-lib/tests/agreement.rs index 3b73672f4..4c7415e42 100644 --- a/crates/cargo-gamma-lib/tests/agreement.rs +++ b/crates/cargo-gamma-lib/tests/agreement.rs @@ -209,11 +209,35 @@ fn the_two_nesting_guards_agree_on_binary_chains() { nesting_guards("binary chains", CHAIN_CEILING, corpus::binary_chain); } +#[test] +fn the_two_nesting_guards_agree_on_greater_than_chains() { + nesting_guards("greater-than chains", CHAIN_CEILING, corpus::greater_than_chain); +} + +#[test] +fn the_two_nesting_guards_agree_on_shift_chains() { + nesting_guards("shift chains", CHAIN_CEILING, corpus::shift_chain); +} + #[test] fn the_two_nesting_guards_agree_on_cast_chains() { nesting_guards("cast chains", CHAIN_CEILING, corpus::cast_chain); } +#[test] +fn the_two_nesting_guards_agree_on_mixed_operator_and_cast_chains() { + nesting_guards( + "mixed operator and cast chains", + CHAIN_CEILING, + corpus::mixed_operator_and_cast_chain, + ); +} + +#[test] +fn the_engine_guards_mixed_postfix_and_cast_chains_no_later_than_the_proc_macro() { + nesting_guards_are_ordered("mixed postfix and cast chains", CHAIN_CEILING, corpus::mixed_postfix_and_cast_chain); +} + #[test] fn the_two_nesting_guards_agree_on_else_if_ladders() { nesting_guards("else if ladders", CHAIN_CEILING, corpus::else_if_ladder); @@ -395,6 +419,28 @@ mod corpus { text } + /// `n` greater-than comparisons: `a > a > a > ...`. + pub(super) fn greater_than_chain(n: usize) -> String { + let mut text = "a".to_owned(); + + for _ in 0..n { + text.push_str(" > a"); + } + + text + } + + /// `n` right shifts: `1 >> 1 >> 1 >> ...`. + pub(super) fn shift_chain(n: usize) -> String { + let mut text = "1".to_owned(); + + for _ in 0..n { + text.push_str(" >> 1"); + } + + text + } + /// `n` links of `as`-casts: `0 as i64 as i64 as i64...`. pub(super) fn cast_chain(n: usize) -> String { let mut text = "0".to_owned(); @@ -406,6 +452,16 @@ mod corpus { text } + /// `n` additions followed by `n` casts, so neither family owns the combined depth alone. + pub(super) fn mixed_operator_and_cast_chain(n: usize) -> String { + format!("{}{}", binary_chain(n), " as i64".repeat(n)) + } + + /// `n` method calls followed by `n` casts, crossing from postfix to cast nesting. + pub(super) fn mixed_postfix_and_cast_chain(n: usize) -> String { + format!("{}{}", postfix_methods(n), " as i64".repeat(n)) + } + /// An `else if` ladder with `n` middle arms. pub(super) fn else_if_ladder(n: usize) -> String { let mut text = "if true {}".to_owned(); diff --git a/crates/cargo-gamma-process/CHANGELOG.md b/crates/cargo-gamma-process/CHANGELOG.md new file mode 100644 index 000000000..9bdb3282e --- /dev/null +++ b/crates/cargo-gamma-process/CHANGELOG.md @@ -0,0 +1,14 @@ +# Changelog + +## [Unreleased] + +## [0.2.0] - 2026-09-03 + +- 🐛 Bug Fixes + + - harden discovery and process cleanup + - address hardening feedback ([#144](https://github.com/microsoft/ox-tools/pull/144)) + +## [0.1.0] - 2026-09-02 + +- Initial release. diff --git a/crates/cargo-gamma-process/Cargo.toml b/crates/cargo-gamma-process/Cargo.toml index 98f88b42e..1ae3b6a7a 100644 --- a/crates/cargo-gamma-process/Cargo.toml +++ b/crates/cargo-gamma-process/Cargo.toml @@ -4,7 +4,7 @@ [package] name = "cargo-gamma-process" description = "Internal bounded process-tree lifecycle for cargo-gamma" -version = "0.1.0" +version = "0.2.0" readme = "README.md" keywords = ["cargo", "mutation-testing", "testing"] categories = ["development-tools::testing"] diff --git a/crates/cargo-gamma-process/README.md b/crates/cargo-gamma-process/README.md index a539a1a8d..66fa7892e 100644 --- a/crates/cargo-gamma-process/README.md +++ b/crates/cargo-gamma-process/README.md @@ -13,11 +13,87 @@ -This is an implementation detail of the cargo-gamma tool. Do not take a dependency on this crate -as it may change in incompatible ways without warning. +This crate is an internal implementation detail of +[`cargo-gamma`][__link0]. It contains cargo-gamma’s bounded +process-tree containment, accounting, observation, and termination lifecycle. + +Do not depend on it directly. Its API may change incompatibly without notice; it is published +only so that `cargo-gamma` can be installed through crates.io. + +## Process-tree lifecycle + +Killing the process a run started is not enough. A test that shells out to a server, a database +or another build leaves those behind when the harness above them is cut off, and they take two +things with them: file locks inside the scratch tree, which turn the next run into a failure +that has nothing to do with any mutant, and inherited pipe handles, which keep whoever is +reading this tool’s output from ever seeing end of file. A run that ends with a hung consumer is +worse than one that ends with a wrong verdict, because nobody can even see the verdict. + +Both platforms have a way to say “this process and everything descended from it” — a process +group on Unix and a job object on Windows — but neither is reachable from `std`. The raw calls +live in `cargo-gamma-unsafe`, which exposes safe interfaces; this crate composes them into the +lifecycle used by the rest of the tool. + +The same boundary accounts for memory because it is the only place that knows the whole process +tree rather than only its leader. A [`MemoryRequest`][__link1] passed to [`prepare`][__link2] asks for measurement, +a ceiling, or neither; [`ProcessTree::usage`][__link3] answers once the process tree is gone. On Windows +requested accounting requires a dedicated job carrying the limit and accounting; on Linux a +cgroup leaf supplied by `cargo-gamma-unsafe` serves both purposes. + +Containment itself is not conditional on that request. A Unix process group is escapable — a +descendant that calls `setsid` leaves it, and every later signal to the group misses it — so +every Windows launch enters a job and every Linux launch attempts to enter a cgroup leaf whether +or not anything is being measured. An unmetered Linux launch may use best-effort process-group +containment only when the host has no supported cgroup facility. Sealed containment means a +boundary descendants cannot leave: [`containment`][__link4] reports when the host cannot provide one +before repository-controlled code executes, and [`ProcessTree::sealed`][__link5] reports the resulting +state for one launch. + +The boundary is in force from the child’s first instruction. On Linux the child moves itself +into the cgroup between fork and exec; on Windows it normally starts suspended, enters the +dedicated job, and only then runs. A child that cannot enter a job already created for it is +rejected because an inherited job cannot be opened later to terminate its descendants. A peak +that reached the limit is therefore enforced by the kernel rather than inferred after the fact. + +Because the Linux boundary is installed as a pre-exec step naming one particular leaf, and a +[`Command`][__link6] accumulates every such step it is given, a command can only +be prepared once. That is stated in the types rather than checked: [`prepare`][__link7] consumes the +command and returns a [`PreparedCommand`][__link8], whose consuming spawn advances to +[`SpawnedCommand`][__link9]. A failed spawn returns [`SpawnFailure`][__link10] with the preparation intact. The +caller classifies its underlying operating-system error, retries transient resource-related +spawn failures after [`PreparedCommand::backoff`][__link11], and propagates permanent failures. A +successful spawn can only be surrendered to [`ProcessTree::adopt`][__link12], so one preparation cannot +leave an earlier child outside containment and launch another; dropping the post-spawn state +before adoption terminates and reaps that child. + +[`output`][__link13] is the contained counterpart of +[`Command::output`][__link14]. It drains stdout and stderr concurrently, +then sweeps descendants before waiting for inherited pipe handles to close. + +A terminal delivers `Ctrl-C` to the whole foreground process group, so a child sharing this +process’s group dies with it automatically while a child leading its own group does not. Windows +normally preserves that guarantee through a dedicated job that dies with its last handle. Unix +installs explicit interruption handling through `cargo-gamma-unsafe`.
This crate was developed as part of The Oxidizer Project. Browse this crate's source code. + + [__cargo_doc2readme_dependencies_info]: ggGmYW0CYXZlMC43LjNhdIQbFhzZ8rzWNNYbuRaDSGWynFgbH4PMdoT7GNcbVwNPtPjAhvFhYvRhcoQbzJIscAd99o8bNjOMWNtQqzob7B-tkBhXsbcbeRzSwG3PJgJhZIGDc2NhcmdvLWdhbW1hLXByb2Nlc3NlMC4yLjBzY2FyZ29fZ2FtbWFfcHJvY2Vzcw + [__link0]: https://crates.io/crates/cargo-gamma + [__link1]: https://docs.rs/cargo-gamma-process/0.2.0/cargo_gamma_process/?search=MemoryRequest + [__link10]: https://docs.rs/cargo-gamma-process/0.2.0/cargo_gamma_process/?search=SpawnFailure + [__link11]: https://docs.rs/cargo-gamma-process/0.2.0/cargo_gamma_process/?search=PreparedCommand::backoff + [__link12]: https://docs.rs/cargo-gamma-process/0.2.0/cargo_gamma_process/?search=ProcessTree::adopt + [__link13]: https://docs.rs/cargo-gamma-process/0.2.0/cargo_gamma_process/?search=output + [__link14]: https://doc.rust-lang.org/stable/std/?search=process::Command::output + [__link2]: https://docs.rs/cargo-gamma-process/0.2.0/cargo_gamma_process/?search=prepare + [__link3]: https://docs.rs/cargo-gamma-process/0.2.0/cargo_gamma_process/?search=ProcessTree::usage + [__link4]: https://docs.rs/cargo-gamma-process/0.2.0/cargo_gamma_process/?search=containment + [__link5]: https://docs.rs/cargo-gamma-process/0.2.0/cargo_gamma_process/?search=ProcessTree::sealed + [__link6]: https://doc.rust-lang.org/stable/std/?search=process::Command + [__link7]: https://docs.rs/cargo-gamma-process/0.2.0/cargo_gamma_process/?search=prepare + [__link8]: https://docs.rs/cargo-gamma-process/0.2.0/cargo_gamma_process/?search=PreparedCommand + [__link9]: https://docs.rs/cargo-gamma-process/0.2.0/cargo_gamma_process/?search=SpawnedCommand diff --git a/crates/cargo-gamma-process/docs/DESIGN.md b/crates/cargo-gamma-process/docs/DESIGN.md index b9455ec3c..081cf8ef8 100644 --- a/crates/cargo-gamma-process/docs/DESIGN.md +++ b/crates/cargo-gamma-process/docs/DESIGN.md @@ -48,7 +48,22 @@ therefore covers the complete descendant tree. stdin and captures stdout and stderr. Both pipes are drained concurrently while the child runs, avoiding pipe-capacity deadlocks. When the leader exits, descendants are swept before their inherited pipe handles are drained to end - of file. + of file. Failures to terminate the process group, Linux cgroup, or Windows + job are propagated after the leader is reaped. A direct child-kill failure is + ignored only when an immediate status observation proves that the child has + already exited. Reap errors take precedence over leader-kill errors, which + take precedence over surrounding group or boundary sweep errors. Within a + Linux sweep, failure of the cgroup kill takes precedence over process-group + failure because the cgroup is the boundary that also reaches descendants + that called `setsid`. If subsequent cleanup cannot prove that descendants + released their pipe handles, its failure takes precedence over an earlier + output setup or observation failure. A failure from an output reader already + started likewise takes precedence over failure to start the other reader. + Reader threads are detached rather than joined indefinitely only when + explicit lifecycle state says cleanup could not prove inherited pipe + handles were closed. Before detachment, capture is disabled and retained + bytes are released; a surviving descendant may keep the bounded drain + blocked, but it cannot keep growing an ownerless output buffer. - On Windows, each child receives a dedicated job. If an enclosing job refuses nested assignment, the spawn is rejected: an inherited job does not provide a handle through which this process can later terminate the child's descendants. @@ -64,7 +79,8 @@ therefore covers the complete descendant tree. process-group id — after sweeping the non-numeric containment capability: the cgroup directory on Linux or job handle on Windows. That boundary can still be proven to reach this run's descendants after numeric identities may have been - reused. + reused. A failure of that final boundary sweep is included in the observation + error while the observation failure retains precedence. - The `fault-injection` feature is test-only infrastructure. - Tests that need a capability the host may not have are marked ignored and fail when asked for by name on a host that cannot supply it, rather than returning diff --git a/crates/cargo-gamma-process/src/faults.rs b/crates/cargo-gamma-process/src/faults.rs index 2fd88173a..e3df9a730 100644 --- a/crates/cargo-gamma-process/src/faults.rs +++ b/crates/cargo-gamma-process/src/faults.rs @@ -21,6 +21,9 @@ pub enum Fault { /// The spawn window refuses to open. Window, + + /// Terminating a contained subtree reports a cleanup failure. + Terminate, } /// Arms `fault` on this thread until the returned value is dropped. @@ -42,8 +45,10 @@ fn ripe_at(fault: Fault, ripe: Instant) -> Armed { } pub(crate) fn fired(fault: Fault) -> bool { - let now = Instant::now(); + fired_at(fault, Instant::now()) +} +fn fired_at(fault: Fault, now: Instant) -> bool { ARMED.with_borrow_mut(|armed| { armed .iter() @@ -95,6 +100,7 @@ mod tests { assert!(!fired(Fault::Prepare)); assert!(!fired(Fault::Boundary)); assert!(!fired(Fault::Window)); + assert!(!fired(Fault::Terminate)); } #[test] @@ -133,12 +139,13 @@ mod tests { #[test] fn a_delayed_fault_waits_and_then_fires_once() { - let _armed = arm_late(Fault::Window, Duration::from_millis(25)); + let now = Instant::now(); + let ripe = now + Duration::from_millis(25); + let _armed = ripe_at(Fault::Window, ripe); - assert!(!fired(Fault::Window)); - std::thread::sleep(Duration::from_millis(40)); - assert!(fired(Fault::Window)); - assert!(!fired(Fault::Window)); + assert!(!fired_at(Fault::Window, now)); + assert!(fired_at(Fault::Window, ripe)); + assert!(!fired_at(Fault::Window, ripe)); } #[test] diff --git a/crates/cargo-gamma-process/src/process_tree.rs b/crates/cargo-gamma-process/src/process_tree.rs index 692e2e026..c8f5d8c24 100644 --- a/crates/cargo-gamma-process/src/process_tree.rs +++ b/crates/cargo-gamma-process/src/process_tree.rs @@ -7,6 +7,8 @@ use core::fmt; use core::time::Duration; use std::io; use std::process::{Child, ChildStderr, ChildStdout, Command, ExitStatus, Output, Stdio}; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, Mutex}; use std::thread::{self, JoinHandle}; #[cfg(target_os = "linux")] @@ -359,7 +361,7 @@ impl SpawnedCommand { impl Drop for SpawnedCommand { fn drop(&mut self) { if let (Some(mut child), Some(guard)) = (self.child.take(), self.guard.take()) { - abandon(&mut child, &guard); + let _abandoned = abandon(&mut child, &guard); } } } @@ -738,6 +740,9 @@ pub struct ProcessTree { /// The leader retained until this subtree is observed or terminated. child: Option, + /// Whether cleanup failed or lost the leader before proving inherited pipes were closed. + output_cleanup_unproven: bool, + /// The process group the child leads, on the platform that has them. /// /// Absent when the child's id does not fit a signal's idea of one, which cannot happen on any @@ -804,11 +809,13 @@ impl ProcessTree { #[cfg(any(test, feature = "fault-injection"))] if faults::fired(faults::Fault::Adopt) { - abandon(&mut child, &guard); - - return Err(PlatformError::new_static( - Situation::Refused, - "the accounting boundary a test asked to fail would not take the child", + return Err(abandoning( + PlatformError::new_static( + Situation::Refused, + "the accounting boundary a test asked to fail would not take the child", + ), + &mut child, + &guard, )); } @@ -817,9 +824,7 @@ impl ProcessTree { let group = match group_id(child.id()) { Ok(group) => group, Err(reason) => { - abandon(&mut child, &guard); - - return Err(reason); + return Err(abandoning(reason, &mut child, &guard)); } }; @@ -842,11 +847,15 @@ impl ProcessTree { let watched = guard.spawning.watch(group); let Some(slot) = watched else { - abandon(&mut child, &guard); - - return Err(PlatformError::new( - Situation::Refused, - format!("process group {group} could not be watched for interrupts, so the child would have outlived a cancelled run"), + return Err(abandoning( + PlatformError::new( + Situation::Refused, + format!( + "process group {group} could not be watched for interrupts, so the child would have outlived a cancelled run" + ), + ), + &mut child, + &guard, )); }; @@ -854,6 +863,7 @@ impl ProcessTree { { Ok(Self { child: Some(child), + output_cleanup_unproven: false, group: Some(group), slot: Some(slot), cgroup: guard.cgroup, @@ -865,6 +875,7 @@ impl ProcessTree { { Ok(Self { child: Some(child), + output_cleanup_unproven: false, group: Some(group), slot: Some(slot), }) @@ -875,28 +886,33 @@ impl ProcessTree { { if let Some(job) = guard.job.as_ref() { if !job.assign(&child) { - abandon(&mut child, &guard); - - return Err(PlatformError::new_static( - Situation::Refused, - "a Windows job object could not be given the test binary it was created for, so its descendants could not be terminated safely", + return Err(abandoning( + PlatformError::new_static( + Situation::Refused, + "a Windows job object could not be given the test binary it was created for, so its descendants could not be terminated safely", + ), + &mut child, + &guard, )); } // The child has been waiting since it was created. It is now inside the new job, // where termination can reach every descendant it creates. if !job::release(child.id()) { - abandon(&mut child, &guard); - - return Err(PlatformError::new_static( - Situation::Refused, - "a Windows test binary could not be resumed inside the containment boundary holding it", + return Err(abandoning( + PlatformError::new_static( + Situation::Refused, + "a Windows test binary could not be resumed inside the containment boundary holding it", + ), + &mut child, + &guard, )); } } Ok(Self { child: Some(child), + output_cleanup_unproven: false, job: guard.job, metered: guard.metered, }) @@ -906,7 +922,10 @@ impl ProcessTree { { let SpawnGuard {} = guard; - Ok(Self { child: Some(child) }) + Ok(Self { + child: Some(child), + output_cleanup_unproven: false, + }) } } @@ -932,25 +951,43 @@ impl ProcessTree { /// # Errors /// /// Returns the operating system's reason when a reader thread could not be created, an output - /// stream could not be read, or the child could not be observed or reaped. A reader-thread - /// panic is reported as [`io::ErrorKind::Other`]. + /// stream could not be read, the platform's subtree boundary or Unix process group could not + /// be terminated, or the child could not be observed or reaped. A reader-thread panic is + /// reported as [`io::ErrorKind::Other`]. pub fn wait_with_output(mut self) -> io::Result { let stdout = output_reader(self.take_stdout(), "cargo-gamma-process-stdout")?; let stderr = match output_reader(self.take_stderr(), "cargo-gamma-process-stderr") { Ok(stderr) => stderr, Err(cause) => { - let _terminated = self.terminate(); - let _drained = join_output_reader(stdout, "stdout"); + if let Err(cleanup) = self.terminate() { + discard_output_reader(stdout.as_ref()); + return Err(cleanup); + } + let _stdout = join_output_reader(stdout, "stdout")?; return Err(cause); } }; - let status = wait_for_output(&mut self); + let status = match wait_for_output(&mut self) { + Ok(status) => Ok(status), + Err(cause) => { + if self.output_cleanup_unproven { + // Cleanup could not prove that descendants released their pipe handles. + discard_output_reader(stdout.as_ref()); + discard_output_reader(stderr.as_ref()); + return Err(cause); + } - if status.is_err() { - let _terminated = self.terminate(); - } + if let Err(cleanup) = self.terminate() { + discard_output_reader(stdout.as_ref()); + discard_output_reader(stderr.as_ref()); + return Err(cleanup); + } + + Err(cause) + } + }; // Both joins are attempted before either result is returned. If one reader failed, the // other must still be allowed to finish rather than being detached from this lifecycle. @@ -1066,14 +1103,18 @@ impl ProcessTree { /// only capability that can still reach a descendant of the subtree that really was this run's /// — and this is the last moment at which that remains true. #[cfg(unix)] - fn revoke_group(&mut self) { + fn revoke_group(&mut self) -> io::Result<()> { #[cfg(target_os = "linux")] - if let Some(cgroup) = self.cgroup.as_ref() { - cgroup.kill(); - } + let boundary_killed = self.cgroup.as_ref().map_or(Ok(()), Cgroup::kill); self.release(); self.group = None; + + #[cfg(target_os = "linux")] + return boundary_killed; + + #[cfg(not(target_os = "linux"))] + Ok(()) } /// Observes a completed child, kills survivors, and only then reaps its leader. @@ -1105,7 +1146,8 @@ impl ProcessTree { Ok(observed) => observed, Err(cause) => { if group::is_no_child_to_wait_for(&cause) { - self.revoke_group(); + self.output_cleanup_unproven = true; + let revoked = self.revoke_group(); // Dropped rather than restored. `Child` holds nothing but a pid the kernel // has already released, and every later lifecycle step — `terminate`, @@ -1113,7 +1155,7 @@ impl ProcessTree { // steps report an already-reaped leader instead of reaching a stranger. drop(child); - return Err(cause); + return Err(with_cleanup_failure(cause, revoked)); } self.child = Some(child); @@ -1122,20 +1164,37 @@ impl ProcessTree { } }; - let status = cleanup_after_observation( + match cleanup_after_observation( observed, || { - self.sweep(); + let swept = self.sweep(); self.release(); + + swept }, || child.wait(), - )?; - - if status.is_none() { - self.child = Some(child); + ) { + Observation::Pending => { + self.child = Some(child); + Ok(None) + } + Observation::Reaped(status) => Ok(Some(status)), + Observation::CleanupFailed(cause) => { + self.output_cleanup_unproven = true; + Err(cause) + } + Observation::ReapFailed(cause) => { + if group::is_no_child_to_wait_for(&cause) { + self.output_cleanup_unproven = true; + let revoked = self.revoke_group(); + drop(child); + Err(with_cleanup_failure(cause, revoked)) + } else { + self.child = Some(child); + Err(cause) + } + } } - - Ok(status) } #[cfg(not(unix))] @@ -1152,8 +1211,13 @@ impl ProcessTree { if status.is_some() { // Windows jobs retain object handles rather than numeric process identifiers, and // platforms without groups have no identifier that a sweep could reuse. - self.sweep(); + let swept = self.sweep(); self.release(); + + if let Err(cause) = swept { + self.output_cleanup_unproven = true; + return Err(cause); + } } else { self.child = Some(child); } @@ -1189,14 +1253,16 @@ impl ProcessTree { /// Kills the child and every process descended from it. /// - /// Falls back to killing the child alone whenever the subtree cannot be reached, because a run - /// that cut off one process is still better than one that cut off none. - fn kill(&self, child: &mut Child) { - self.sweep(); + /// The child is still killed when sweeping the surrounding subtree fails. A direct child-kill + /// failure takes precedence over the sweep failure after both cleanup attempts complete. + fn kill(&self, child: &mut Child) -> io::Result<()> { + let swept = self.sweep(); // The group or job may not have covered the child — the id could not be converted, the job // could not be created — and in any case this is what makes `wait` return. - let _killed = child.kill(); + let killed = kill_if_running(child); + + killed.and(swept) } /// Ends the subtree and reaps its leader without exposing its group id to reuse. @@ -1205,17 +1271,26 @@ impl ProcessTree { /// /// Returns [`io::ErrorKind::Other`] when this subtree's leader has already been reaped — /// through an earlier [`Self::terminate`], or because [`Self::observe`] found it consumed - /// elsewhere — and the operating system's reason when the reap itself fails. + /// elsewhere — and the operating system's reason when cleanup or reaping fails. A reap + /// failure takes precedence when both operations fail. pub fn terminate(&mut self) -> io::Result { let mut child = self .child .take() .ok_or_else(|| io::Error::other("the subtree leader was already reaped"))?; - self.kill(&mut child); + let killed = self.kill(&mut child); self.release(); - child.wait() + let reaped = child.wait()?; + killed?; + + #[cfg(any(test, feature = "fault-injection"))] + if faults::fired(faults::Fault::Terminate) { + return Err(io::Error::other("subtree termination failed as requested by a test")); + } + + Ok(reaped) } /// Ends descendants while their leader's process-group id is still reserved. @@ -1223,26 +1298,32 @@ impl ProcessTree { /// An exited leader can leave servers and inherited pipe handles behind. This private /// primitive is reachable only from [`Self::observe`] and [`Self::terminate`], which signal /// before reaping that leader, so `killpg` cannot name a replacement group. - fn sweep(&self) { + fn sweep(&self) -> io::Result<()> { // The cgroup reaches further than the process group does: a descendant that called // `setsid` has left the group but cannot leave the cgroup, so this goes first where it // exists. #[cfg(target_os = "linux")] - if let Some(cgroup) = self.cgroup.as_ref() { - cgroup.kill(); - } + let boundary_killed = self.cgroup.as_ref().map_or(Ok(()), Cgroup::kill); // Signalling the group has to come first: killing the leader on its own leaves the group // without one, and the descendants are then reparented and unreachable. #[cfg(unix)] - if let Some(group) = self.group { - let _killed = group::kill(group); - } + let killed = self.group.map_or(Ok(()), group::kill); #[cfg(windows)] - if let Some(job) = self.job.as_ref() { - job.terminate(); - } + let boundary_killed = self.job.as_ref().map_or(Ok(()), Job::terminate); + + #[cfg(target_os = "linux")] + return boundary_killed.and(killed); + + #[cfg(all(unix, not(target_os = "linux")))] + return killed; + + #[cfg(windows)] + return boundary_killed; + + #[cfg(not(any(unix, windows)))] + Ok(()) } #[cfg(all(test, unix))] @@ -1256,27 +1337,67 @@ impl ProcessTree { } } -fn output_reader(pipe: Option, name: &'static str) -> io::Result>>>> +struct OutputReader { + thread: JoinHandle>, + bytes: Arc>>, + retaining: Arc, +} + +fn output_reader(pipe: Option, name: &'static str) -> io::Result> where R: io::Read + Send + 'static, { pipe.map(|mut pipe| { - thread::Builder::new().name(name.to_owned()).spawn(move || { - let mut bytes = Vec::new(); + let bytes = Arc::new(Mutex::new(Vec::new())); + let retaining = Arc::new(AtomicBool::new(true)); + let captured = Arc::clone(&bytes); + let capture_enabled = Arc::clone(&retaining); + let thread = thread::Builder::new().name(name.to_owned()).spawn(move || { + let mut chunk = [0_u8; 8192]; + + loop { + let read = match pipe.read(&mut chunk) { + Ok(read) => read, + Err(cause) if cause.kind() == io::ErrorKind::Interrupted => continue, + Err(cause) => return Err(cause), + }; + if read == 0 { + return Ok(()); + } - pipe.read_to_end(&mut bytes)?; + let mut captured = captured.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + if capture_enabled.load(Ordering::Acquire) { + captured.extend_from_slice(&chunk[..read]); + } + } + })?; - Ok(bytes) - }) + Ok(OutputReader { thread, bytes, retaining }) }) .transpose() } -fn join_output_reader(reader: Option>>>, stream: &str) -> io::Result> { +fn discard_output_reader(reader: Option<&OutputReader>) { + if let Some(reader) = reader { + reader.retaining.store(false, Ordering::Release); + let mut bytes = reader.bytes.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + bytes.clear(); + bytes.shrink_to_fit(); + } +} + +fn join_output_reader(reader: Option, stream: &str) -> io::Result> { match reader { - Some(reader) => reader - .join() - .map_err(|_panic| io::Error::other(format!("the {stream} reader thread panicked")))?, + Some(reader) => { + reader + .thread + .join() + .map_err(|_panic| io::Error::other(format!("the {stream} reader thread panicked")))??; + Ok(Arc::try_unwrap(reader.bytes) + .expect("the reader thread is joined, so it released the only other capture reference") + .into_inner() + .unwrap_or_else(std::sync::PoisonError::into_inner)) + } None => Ok(Vec::new()), } } @@ -1316,6 +1437,18 @@ fn group_id(child_id: u32) -> Result { }) } +/// Preserves a containment refusal while reporting failure to clean up its child. +fn abandoning(refusal: PlatformError, child: &mut Child, guard: &SpawnGuard) -> PlatformError { + match abandon(child, guard) { + Ok(()) => refusal, + Err(cause) => PlatformError::because( + Situation::Refused, + format!("{refusal}; cleanup of the refused child also failed"), + cause, + ), + } +} + /// Ends a child that failed containment before it can escape its unregistered subtree. #[cfg_attr( not(any(target_os = "linux", windows)), @@ -1324,24 +1457,49 @@ fn group_id(child_id: u32) -> Result { reason = "only Linux cgroups and Windows jobs carry containment state needed during abandonment" ) )] -fn abandon(child: &mut Child, guard: &SpawnGuard) { +fn abandon(child: &mut Child, guard: &SpawnGuard) -> io::Result<()> { #[cfg(target_os = "linux")] - if let Some(cgroup) = guard.cgroup.as_ref() { - cgroup.kill(); - } + let boundary_killed = guard.cgroup.as_ref().map_or(Ok(()), Cgroup::kill); #[cfg(unix)] - if let Ok(group) = i32::try_from(child.id()) { - let _killed = group::kill(group); + let group_killed = i32::try_from(child.id()).map_or(Ok(()), group::kill); + + #[cfg(windows)] + let boundary_killed = guard.job.as_ref().map_or(Ok(()), Job::terminate); + + let child_killed = kill_if_running(child); + let reaped = child.wait().map(|_status| ()); + + #[cfg(target_os = "linux")] + { + reaped.and(child_killed).and(boundary_killed).and(group_killed) + } + + #[cfg(all(unix, not(target_os = "linux")))] + { + reaped.and(child_killed).and(group_killed) } #[cfg(windows)] - if let Some(job) = guard.job.as_ref() { - job.terminate(); + { + reaped.and(child_killed).and(boundary_killed) } - let _killed = child.kill(); - let _reaped = child.wait(); + #[cfg(not(any(unix, windows)))] + { + reaped.and(child_killed) + } +} + +/// Kills a child unless the failed kill proves to have raced with its exit. +fn kill_if_running(child: &mut Child) -> io::Result<()> { + match child.kill() { + Ok(()) => Ok(()), + Err(kill_error) => match child.try_wait() { + Ok(Some(_status)) => Ok(()), + Ok(None) | Err(_) => Err(kill_error), + }, + } } /// Performs the only safe order after an exit observation. @@ -1349,19 +1507,49 @@ fn abandon(child: &mut Child, guard: &SpawnGuard) { /// Kept separate so the regression can run the exact order against a fake process-group backend /// that reuses the group's numeric identifier as soon as its leader is reaped. #[cfg(any(unix, test))] -fn cleanup_after_observation(observed: bool, cleanup: impl FnOnce(), reap: impl FnOnce() -> io::Result) -> io::Result> { +enum Observation { + Pending, + Reaped(T), + CleanupFailed(io::Error), + ReapFailed(io::Error), +} + +#[cfg(unix)] +fn with_cleanup_failure(primary: io::Error, cleanup: io::Result<()>) -> io::Error { + match cleanup { + Ok(()) => primary, + Err(cleanup) => io::Error::new( + primary.kind(), + format!("{primary}; cleanup after the leader was reaped also failed: {cleanup}"), + ), + } +} + +#[cfg(any(unix, test))] +fn cleanup_after_observation( + observed: bool, + cleanup: impl FnOnce() -> io::Result<()>, + reap: impl FnOnce() -> io::Result, +) -> Observation { if !observed { - return Ok(None); + return Observation::Pending; } - cleanup(); - reap().map(Some) + let cleaned = cleanup(); + let reaped = reap(); + + match (cleaned, reaped) { + (Ok(()), Ok(status)) => Observation::Reaped(status), + (Err(cause), Ok(_status)) => Observation::CleanupFailed(cause), + (Ok(()), Err(cause)) => Observation::ReapFailed(cause), + (Err(_cleanup), Err(reap)) => Observation::ReapFailed(reap), + } } impl Drop for ProcessTree { fn drop(&mut self) { if let Some(mut child) = self.child.take() { - self.kill(&mut child); + let _killed = self.kill(&mut child); self.release(); let _reaped = child.wait(); } else { @@ -1388,6 +1576,117 @@ mod tests { use super::*; use crate::testing; + struct PausedReader { + reads: usize, + ready: std::sync::mpsc::SyncSender<()>, + resume: std::sync::mpsc::Receiver<()>, + } + + impl io::Read for PausedReader { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + self.reads += 1; + match self.reads { + 1 => { + buf.fill(b'x'); + self.ready.send(()).expect("the watchdog is waiting for captured output"); + Ok(buf.len()) + } + 2 => { + self.resume.recv().expect("the watchdog releases the inherited-pipe stand-in"); + buf.fill(b'y'); + Ok(buf.len()) + } + _ => Ok(0), + } + } + } + + #[test] + fn detached_output_capture_releases_bytes_and_stops_retaining() { + let (ready, started) = std::sync::mpsc::sync_channel(0); + let (resume, continue_reading) = std::sync::mpsc::sync_channel(0); + let reader = output_reader( + Some(PausedReader { + reads: 0, + ready, + resume: continue_reading, + }), + "bounded-output-capture", + ) + .expect("the output reader starts") + .expect("the stand-in pipe is present"); + + started + .recv_timeout(Duration::from_secs(1)) + .expect("the watchdog observes the first captured chunk"); + discard_output_reader(Some(&reader)); + assert!( + reader.bytes.lock().unwrap_or_else(std::sync::PoisonError::into_inner).is_empty(), + "detachment retained bytes already captured" + ); + + resume.send(()).expect("the blocked reader is released"); + assert!( + join_output_reader(Some(reader), "stand-in") + .expect("the reader finishes") + .is_empty(), + "capture resumed after detachment" + ); + } + + struct FailingReader; + + impl io::Read for FailingReader { + fn read(&mut self, _buf: &mut [u8]) -> io::Result { + Err(io::Error::new(io::ErrorKind::BrokenPipe, "injected read failure")) + } + } + + struct InterruptedReader(u8); + + impl io::Read for InterruptedReader { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + match self.0 { + 0 => { + self.0 = 1; + return Err(io::Error::new(io::ErrorKind::Interrupted, "injected interruption")); + } + 1 => self.0 = 2, + _ => return Ok(0), + } + + let text = b"complete"; + buf[..text.len()].copy_from_slice(text); + Ok(text.len()) + } + } + + #[test] + fn output_reader_preserves_absence_and_read_failures() { + let absent = output_reader::(None, "absent-output").expect("absence needs no thread"); + discard_output_reader(absent.as_ref()); + assert_eq!(join_output_reader(absent, "absent").expect("absence is empty"), Vec::::new()); + + let failed = output_reader(Some(FailingReader), "failing-output") + .expect("the failing reader thread starts") + .expect("the stand-in pipe is present"); + let error = join_output_reader(Some(failed), "failing").expect_err("the injected read failure is preserved"); + + assert_eq!(error.kind(), io::ErrorKind::BrokenPipe); + } + + #[test] + fn output_reader_retries_interrupted_reads() { + let reader = output_reader(Some(InterruptedReader(0)), "interrupted-output") + .expect("the interrupted reader thread starts") + .expect("the stand-in pipe is present"); + + assert_eq!( + join_output_reader(Some(reader), "interrupted").expect("the reader retries"), + b"complete" + ); + } + /// Why the containment tests below are ignored by default, and how to run them. /// /// A boundary a member cannot leave is not something every host has to offer. On Linux it needs @@ -1483,14 +1782,17 @@ mod tests { let group = RefCell::new(FakeGroup::default()); let result = cleanup_after_observation( true, - || group.borrow_mut().sweep(), + || { + group.borrow_mut().sweep(); + Ok(()) + }, || { group.borrow_mut().reap(); Ok(()) }, ); - assert!(matches!(result, Ok(Some(())))); + assert!(matches!(result, Observation::Reaped(()))); let group = group.into_inner(); @@ -1501,6 +1803,39 @@ mod tests { ); } + #[test] + fn a_cleanup_failure_is_reported_after_the_leader_is_reaped() { + let reaped = RefCell::new(false); + let result = cleanup_after_observation( + true, + || Err(io::Error::new(io::ErrorKind::PermissionDenied, "group kill failed")), + || { + *reaped.borrow_mut() = true; + Ok(()) + }, + ); + + assert!(matches!( + result, + Observation::CleanupFailed(ref cause) if cause.kind() == io::ErrorKind::PermissionDenied + )); + assert!(*reaped.borrow(), "cleanup failure must not leave the child unreaped"); + } + + #[test] + fn a_reap_failure_is_distinguished_from_a_cleanup_failure() { + let result = cleanup_after_observation( + true, + || Ok(()), + || Err::<(), _>(io::Error::new(io::ErrorKind::Interrupted, "reap failed")), + ); + + assert!(matches!( + result, + Observation::ReapFailed(ref cause) if cause.kind() == io::ErrorKind::Interrupted + )); + } + /// A request that asks for nothing gets containment without an accounting boundary. #[test] fn a_run_that_asks_for_no_accounting_reports_no_usage() { @@ -1621,6 +1956,7 @@ mod tests { let subtree = ProcessTree { child: None, + output_cleanup_unproven: false, group: None, slot: None, #[cfg(target_os = "linux")] @@ -1629,7 +1965,9 @@ mod tests { metered: false, }; - subtree.kill(&mut child); + subtree + .kill(&mut child) + .expect("the live shell fixture must be killable without a process group"); let status = child.wait().expect("wait"); @@ -1653,11 +1991,14 @@ mod tests { let mut child = command.spawn().expect("spawn"); let subtree = ProcessTree { child: None, + output_cleanup_unproven: false, job: None, metered: false, }; - subtree.kill(&mut child); + subtree + .kill(&mut child) + .expect("the live helper fixture must be killable without a job"); let status = child.wait().expect("wait"); @@ -1697,6 +2038,7 @@ mod tests { fn a_subtree_with_no_watched_slot_drops_without_touching_the_registry() { let subtree = ProcessTree { child: None, + output_cleanup_unproven: false, group: None, slot: None, #[cfg(target_os = "linux")] @@ -1722,6 +2064,7 @@ mod tests { let mut subtree = ProcessTree { child: None, + output_cleanup_unproven: false, group: Some(group), slot: Some(slot), #[cfg(target_os = "linux")] @@ -1762,6 +2105,7 @@ mod tests { let slot = spawning.watch(group).expect("a free slot"); let mut subtree = ProcessTree { child: None, + output_cleanup_unproven: false, group: Some(group), slot: Some(slot), #[cfg(target_os = "linux")] @@ -1770,12 +2114,27 @@ mod tests { metered: false, }; - subtree.revoke_group(); + subtree.revoke_group().expect("there is no sealed boundary to revoke"); assert!(subtree.released(), "the stale interrupt slot remains active"); assert_eq!(subtree.group, None, "drop could still signal a reused process-group id"); } + #[cfg(unix)] + #[test] + fn a_reaped_elsewhere_error_includes_a_boundary_cleanup_failure() { + let error = with_cleanup_failure( + io::Error::from(io::ErrorKind::NotFound), + Err(io::Error::new(io::ErrorKind::PermissionDenied, "cgroup kill failed")), + ); + + assert_eq!(error.kind(), io::ErrorKind::NotFound); + assert!( + error.to_string().contains("cgroup kill failed"), + "the boundary cleanup failure was hidden: {error}" + ); + } + /// Forgetting a slot past the end of the registry does nothing, rather than panicking. /// /// The slot a `ProcessTree` carries always came from `watch`, which never hands out an @@ -2829,6 +3188,15 @@ mod tests { let _reaped = child.wait(); } + /// A kill that races with natural exit has already achieved its cleanup goal. + #[test] + fn killing_an_exited_child_succeeds() { + let mut child = no_op_command().spawn().expect("spawn"); + let _status = child.wait().expect("the child exits"); + + kill_if_running(&mut child).expect("an exited child is no longer running"); + } + /// `observe` reports the reap race it cannot recover from, and revokes what it must. /// /// The leader is reaped through the standard library's own path before `observe` is asked @@ -2856,6 +3224,7 @@ mod tests { let slot = spawning.watch(group).expect("a free slot"); let mut subtree = ProcessTree { child: Some(child), + output_cleanup_unproven: false, group: Some(group), slot: Some(slot), #[cfg(target_os = "linux")] diff --git a/crates/cargo-gamma-rt/CHANGELOG.md b/crates/cargo-gamma-rt/CHANGELOG.md new file mode 100644 index 000000000..9bdb3282e --- /dev/null +++ b/crates/cargo-gamma-rt/CHANGELOG.md @@ -0,0 +1,14 @@ +# Changelog + +## [Unreleased] + +## [0.2.0] - 2026-09-03 + +- 🐛 Bug Fixes + + - harden discovery and process cleanup + - address hardening feedback ([#144](https://github.com/microsoft/ox-tools/pull/144)) + +## [0.1.0] - 2026-09-02 + +- Initial release. diff --git a/crates/cargo-gamma-rt/Cargo.toml b/crates/cargo-gamma-rt/Cargo.toml index a01ad37e7..42dfeb870 100644 --- a/crates/cargo-gamma-rt/Cargo.toml +++ b/crates/cargo-gamma-rt/Cargo.toml @@ -4,7 +4,7 @@ [package] name = "cargo-gamma-rt" description = "Runtime support library injected into crates under mutation test by cargo-gamma" -version = "0.1.0" +version = "0.2.0" readme = "README.md" keywords = ["cargo", "mutation-testing", "testing"] categories = ["development-tools::testing"] @@ -29,11 +29,14 @@ min-lines-percent = 89.0 # HARD CONSTRAINT: zero production dependencies and no build.rs. This crate is injected into the # dependency graph of the crate under test; anything else perturbs feature unification and breaks -# --offline. The empty `loom` feature selects only the repository's dedicated model target. +# --offline. Its empty features expose only coordinator or repository test plumbing. [lib] name = "gamma_rt" [features] +# Exposes the package-local runtime sources to cargo-gamma-lib. The vendored runtime never enables +# this feature, so it does not alter the crate injected into a user's dependency graph. +embedding = [] # The vendored runtime never enables this feature, so an ambient `--cfg loom` still keeps the # injected copy on its dependency-free atomic path. loom = [] diff --git a/crates/cargo-gamma-rt/README.md b/crates/cargo-gamma-rt/README.md index 31c6aa564..1ccea844f 100644 --- a/crates/cargo-gamma-rt/README.md +++ b/crates/cargo-gamma-rt/README.md @@ -13,11 +13,230 @@ -This is an implementation detail of the cargo-gamma tool. Do not take a dependency on this crate -as it may change in incompatible ways without warning. +Runtime support injected into crates under mutation test by `cargo-gamma`. + +This crate is injected into the dependency graph of the crate under test while a mutation run +is in progress. You should never need to depend on it directly. + +`cargo-gamma` rewrites the crate under test so that every mutation site carries a *guard*: a +cheap runtime check that activates exactly one mutant. That lets a whole population of mutants +live in a single compiled artifact — the *mutant schema*, after Untch, Offutt and Harrold, who +introduced the construction in 1993 — instead of requiring one build per mutant. Since a build +is by far the most expensive step in the loop, testing a mutant drops from minutes to the cost +of launching a process. + +## What a guard looks like + +[`a`][__link0] is the only function the instrumented source calls. Guard shape follows what Rust accepts +at the mutation site: + +```text +// an expression, whose value the mutant replaces +(if ::gamma_rt::a(7u32) { (a) <= (b) } else { a < b }) + +// a block, whose body the mutant replaces +{ if ::gamma_rt::a(12u32) { Default::default() } else { ..the real body.. } } + +// a statement, which the mutant deletes +if !::gamma_rt::a(19u32) { self.entries.push(value); } +``` + +Sites nest — in `a + b < c` the `<` site contains the `+` site — and only the `else` arm carries +instrumented children. Exactly one mutant is live in a process, so if the `<` mutant is active +then no `+` mutant can be, and the taken arm can hold plain original text. That is what keeps +the encoding linear in the size of the source rather than exponential in nesting depth. + +## You do not depend on this crate + +`cargo-gamma` copies the workspace to a scratch tree, writes this crate into it, and adds the +dependency there. Nothing is added to your manifest, nothing is fetched from the network, and +your own build is never instrumented. The package is `cargo-gamma-rt` but its library is named +`gamma_rt`, which is why instrumented source can say `::gamma_rt::a` without a rename. + +The copy embedded in the tool is this exact source, so the vendored runtime cannot drift from +the one the guards were generated against. + +## Why this crate has no dependencies + +It has zero dependencies, no build script and no `std`, by design. Its empty features expose +only coordinator or repository test plumbing; the vendored crate enables none of them. +Anything else would perturb feature unification in *the user’s* tree, which could change what +their code compiles to and therefore what their tests prove, or stop a `no_std` tree from +building once the shim is injected into it. Zero dependencies is a correctness requirement, +not a preference. + +For the same reason [`a`][__link1] must stay trivial. It is called at every mutation site of every +execution of the suite, so its cost is multiplied by the whole population: a cached atomic load +and a comparison, behind a branch the predictor learns immediately. + +## A worked example + +Given this function, and a mutant that turns `<` into `<=`: + +```rust +fn below(a: u32, b: u32) -> bool { + a < b +} +``` + +`cargo-gamma` rewrites it in the scratch tree as: + +```rust +fn below(a: u32, b: u32) -> bool { + if ::gamma_rt::a(7u32) { + (a) <= (b) + } else { + a < b + } +} +``` + +The whole population lives in one binary, and the run launches it once per mutant: + +```text +GAMMA_ACTIVE=7 ./target/debug/deps/my_crate-abc123 # mutant 7 is live +GAMMA_ACTIVE=8 ./target/debug/deps/my_crate-abc123 # mutant 8 is live +./target/debug/deps/my_crate-abc123 # nothing is live: the baseline +``` + +## Selection protocol + +The active mutant is named by the [`ACTIVE_VAR`][__link2] environment variable, captured exactly once +during process startup before user code can start threads. The value is a decimal mutant +ordinal. [`NONE`][__link3] means no mutant is active, which is how the baseline run and every ordinary +build behave — including builds of proc macros, where an active mutant could otherwise hang the +compiler. + +An unset, empty, or unparsable value all mean [`NONE`][__link4]. A build that links this crate but is not +being driven by a mutation run must behave exactly as it did before, and the ordinals are +1-based precisely so that “absent” and “explicitly unmutated” are the same answer. Failure to +acquire the startup environment is different: the runtime emits [`ENVIRONMENT_ERROR_MARKER`][__link5] +and exits, so the parent cannot mistake a mutant that never activated for a survivor. + +That distinction covers [`CENSUS_VAR`][__link6] as well as [`ACTIVE_VAR`][__link7]. An unset census variable is an +ordinary process, but a census variable this process could not *read* is a startup failure, not +an absent one: treating it as absence would run the mutant named by [`ACTIVE_VAR`][__link8], produce no +census file, and report a baseline failure the run would read as a verdict about that mutant. A +read interrupted by a signal is retried rather than counted as a failure, since an interruption +is not evidence of anything. + +Two further failure shapes exist because “captured, but wrong” is worse than either of the +above: + +* If some other native constructor — a loader or C-runtime startup hook that runs before + `main` — runs instrumented code before this crate’s own constructor installs the captured + selection, a guard reached in that window emits a fixed diagnostic and terminates immediately + without unwinding rather than silently reporting the baseline. This applies on a hosted target + outside a Miri execution, where that installation is expected. `NONE` would otherwise be + ambiguous between “genuinely unmutated” and “asked too early to know”, and only the first may + ever be reported as a passing mutant. +* On a Unix with no immutable startup environment image, [`ACTIVE_VAR`][__link9] is read through + `getenv` under the POSIX process-wide precondition that no native environment mutation runs + concurrently. This capture happens before Rust `main`, so safe Rust has not had an opportunity + to start a thread that violates the precondition; Rust environment mutation is unsafe for the + same reason. A foreign native constructor that starts concurrent `setenv`, `putenv`, + `unsetenv`, or equivalent mutation is outside this abstraction. The runtime still performs a + second independent read and rejects a disagreement through [`ENVIRONMENT_ERROR_MARKER`][__link10] and + immediate exit. That double-read is integrity detection for a visibly inconsistent result, + not a proof of memory safety or proof that forbidden foreign mutation did not occur. + +```rust +use gamma_rt::{ACTIVE_VAR, CENSUS_VAR, NONE, a, active, any}; + +// Stated for an ordinary process. A census is its own mode: it activates no mutant, so +// `active` reports `NONE`, and every guard answers `false` while recording the site it stands +// at — including the sites this example would walk past. +if std::env::var_os(CENSUS_VAR).is_none_or(|path| path.is_empty()) { + // In an ordinary process nothing is selected, so every guard takes its original arm. + if active() == NONE { + assert!(!any()); + assert!(!a(NONE)); + assert!(!a(1)); + assert!(!a(9_999)); + } + + // Only a positive ordinal can select a mutant. + if active() != NONE { + assert!(a(active())); + } +} + +assert_eq!(ACTIVE_VAR, "GAMMA_ACTIVE"); +``` + +Reading it once, rather than per call, is what makes the guard cheap; it also means a test that +sets the variable on itself changes nothing, which is the honest behavior. The run drives +selection by launching a fresh process per mutant. + +## Runtime entry points + +[`a`][__link11] is what the guards call, and the only runtime entry point instrumented source contains. +[`active`][__link12] and [`any`][__link13] are there for the tool’s own diagnostics and for anyone inspecting a +scratch tree by hand: + +```rust +use gamma_rt::{active, any}; + +// Useful in a scratch tree when you are trying to work out which mutant a failing +// reproduction actually ran. +if any() { + println!("mutant {} is live", active()); +} else { + println!("baseline"); +} +``` + +## Making two iterators one type + +[`Either`][__link14] is the one other thing instrumented source mentions, and it exists because the guard +is an `if`. A function returning `impl Iterator` returns a single concrete type that +its body picks, so `if a(n) { core::iter::empty() } else { ..the real body.. }` has arms of two +different types and will not compile. Wrapping each arm in a variant makes them one type: + +```text +{ if ::gamma_rt::a(4u32) { ::gamma_rt::Either::L(core::iter::empty()) } + else { ::gamma_rt::Either::R({ ..the real body.. }) } } +``` + +See [`Either`][__link15] for why this is not a `Box`. + +## `no_std`, and what it does not buy + +This crate is `#![no_std]`. It has to be: it is injected into the dependency graph of every +crate the tool instruments, so a shim that linked `std` could not be instrumented into a tree +whose target has no `std` in its sysroot — and that failure is not attributable to any mutant, +so the rollback loop cannot withdraw anything to rescue it. The whole tree simply stops +building. The tests link `std`, which costs nothing because they are never compiled into a +user’s build. + +What `no_std` does not buy is a mutation run on a target with no environment. A mutant is +selected by reading `GAMMA_ACTIVE`, which needs POSIX `getenv` or the Win32 equivalent; a +target with neither gets the arm that reports no mutant at all, so the instrumented code +compiles and runs the original everywhere. That is deliberately the safe direction — every +mutant is reported as surviving rather than a mutated program being reported as correct — but +it means the useful case is a `no_std` *crate* whose tests are run on a hosted target, which is +how nearly every `no_std` library is tested anyway.
This crate was developed as part of The Oxidizer Project. Browse this crate's source code. + + [__cargo_doc2readme_dependencies_info]: ggGmYW0CYXZlMC43LjNhdIQbFhzZ8rzWNNYbuRaDSGWynFgbH4PMdoT7GNcbVwNPtPjAhvFhYvRhcoQbhFwoyicofFob0JM_5SGotNcb-qKodur5pWUbrijzXi7ixeFhZIGDbmNhcmdvLWdhbW1hLXJ0ZTAuMi4wbmNhcmdvX2dhbW1hX3J0 + [__link0]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=a + [__link1]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=a + [__link10]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=ENVIRONMENT_ERROR_MARKER + [__link11]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=a + [__link12]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=active + [__link13]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=any + [__link14]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=Either + [__link15]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=Either + [__link2]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=ACTIVE_VAR + [__link3]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=NONE + [__link4]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=NONE + [__link5]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=ENVIRONMENT_ERROR_MARKER + [__link6]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=CENSUS_VAR + [__link7]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=ACTIVE_VAR + [__link8]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=ACTIVE_VAR + [__link9]: https://docs.rs/cargo-gamma-rt/0.2.0/cargo_gamma_rt/?search=ACTIVE_VAR diff --git a/crates/cargo-gamma-rt/docs/DESIGN.md b/crates/cargo-gamma-rt/docs/DESIGN.md index eac104a6a..39dfd8489 100644 --- a/crates/cargo-gamma-rt/docs/DESIGN.md +++ b/crates/cargo-gamma-rt/docs/DESIGN.md @@ -12,7 +12,9 @@ equivalent to the original program. ## Hard constraints - Zero ordinary dependencies. -- No features and no build script. +- No build script and no feature that changes injected runtime behavior. The + internal `embedding` feature exposes package-local source text only to + `cargo-gamma-lib`; the vendored crate never enables it. - `no_std` compatibility. - The library target remains named `gamma_rt`. - Rustdoc is hidden, and the hand-written README warns downstream users not to @@ -71,5 +73,10 @@ census path that fills the buffer leaves census mode selected with no path the runtime can open, so nothing is written and nothing is sealed and the reader discards that binary's census exactly as it discards a truncated one. -The vendored standalone crate inherits the workspace edition and minimum Rust -version from the same manifest values used to build this crate. +The package owns the source bundle, edition, and minimum Rust version used to +write the vendored standalone crate. This keeps the coordinator's published +package self-contained while ensuring the copied runtime is the exact source +compiled as `cargo-gamma-rt`. The edition is deliberately explicit because +Cargo exposes no `CARGO_PKG_EDITION` compile-time variable, and a workspace +edition bump must not silently reinterpret source injected into another +repository. diff --git a/crates/cargo-gamma-rt/src/lib.rs b/crates/cargo-gamma-rt/src/lib.rs index 5210cb3f4..1603e6827 100644 --- a/crates/cargo-gamma-rt/src/lib.rs +++ b/crates/cargo-gamma-rt/src/lib.rs @@ -57,11 +57,12 @@ //! //! # Why this crate has no dependencies //! -//! It has zero dependencies, no features, no build script and no `std`, by design. Anything else -//! would perturb feature unification in *the user's* tree, which could change what their code -//! compiles to and therefore what their tests prove, or stop a `no_std` tree from building once -//! the shim is injected into it. Zero dependencies is a correctness requirement, not a -//! preference. +//! It has zero dependencies, no build script and no `std`, by design. Its empty features expose +//! only coordinator or repository test plumbing; the vendored crate enables none of them. +//! Anything else would perturb feature unification in *the user's* tree, which could change what +//! their code compiles to and therefore what their tests prove, or stop a `no_std` tree from +//! building once the shim is injected into it. Zero dependencies is a correctness requirement, +//! not a preference. //! //! For the same reason [`a`] must stay trivial. It is called at every mutation site of every //! execution of the suite, so its cost is multiplied by the whole population: a cached atomic load @@ -218,6 +219,26 @@ mod either; mod runtime; +#[cfg(feature = "embedding")] +#[doc(hidden)] +pub mod embedded { + /// Runtime source files embedded by the coordinator into each scratch workspace. + pub const SOURCES: [(&str, &str); 3] = [ + ("lib.rs", include_str!("lib.rs")), + ("either.rs", include_str!("either.rs")), + ("runtime.rs", include_str!("runtime.rs")), + ]; + + /// Edition used by the standalone vendored runtime. + /// + /// Cargo exposes no `CARGO_PKG_EDITION` variable. This stays explicit so a workspace edition + /// bump cannot silently reinterpret source injected into another repository. + pub const EDITION: &str = "2024"; + + /// Minimum compiler version used by the standalone vendored runtime. + pub const RUST_VERSION: &str = env!("CARGO_PKG_RUST_VERSION"); +} + #[doc(inline)] pub use either::Either; #[cfg(all(loom, feature = "loom"))] diff --git a/crates/cargo-gamma-unsafe/CHANGELOG.md b/crates/cargo-gamma-unsafe/CHANGELOG.md new file mode 100644 index 000000000..9bdb3282e --- /dev/null +++ b/crates/cargo-gamma-unsafe/CHANGELOG.md @@ -0,0 +1,14 @@ +# Changelog + +## [Unreleased] + +## [0.2.0] - 2026-09-03 + +- 🐛 Bug Fixes + + - harden discovery and process cleanup + - address hardening feedback ([#144](https://github.com/microsoft/ox-tools/pull/144)) + +## [0.1.0] - 2026-09-02 + +- Initial release. diff --git a/crates/cargo-gamma-unsafe/Cargo.toml b/crates/cargo-gamma-unsafe/Cargo.toml index 4444d5797..cab215b2b 100644 --- a/crates/cargo-gamma-unsafe/Cargo.toml +++ b/crates/cargo-gamma-unsafe/Cargo.toml @@ -4,7 +4,7 @@ [package] name = "cargo-gamma-unsafe" description = "The platform calls cargo-gamma cannot make safely, behind a safe interface" -version = "0.1.0" +version = "0.2.0" readme = "README.md" keywords = ["cargo", "mutation-testing", "testing"] categories = ["development-tools::testing"] diff --git a/crates/cargo-gamma-unsafe/README.md b/crates/cargo-gamma-unsafe/README.md index e523893e7..b44671d5d 100644 --- a/crates/cargo-gamma-unsafe/README.md +++ b/crates/cargo-gamma-unsafe/README.md @@ -13,11 +13,30 @@ -This is an implementation detail of the cargo-gamma tool. Do not take a dependency on this crate -as it may change in incompatible ways without warning. +Platform calls that [`cargo-gamma`][__link0] cannot make safely +are concentrated behind an interface that is safe to call. This crate is an implementation +detail of the tool; you should never need to depend on it directly. + +Two things the tool does have no safe expression in `std`: killing a whole process subtree (a +process group on Unix, a job object on Windows) and bounding what that subtree allocates (a +cgroup leaf on Linux, the same job object on Windows). Neither is a case of reaching for +`unsafe` to go faster — there is no safe version to prefer. + +Concentrating those calls here is what lets every other crate in the workspace carry +`#![forbid(unsafe_code)]`, which turns “we reviewed the unsafe code” into a property the +compiler checks on every build. `cargo-gamma-rt` is the one exception, and only because it is +injected into the dependency graph of the crate under test and so can depend on nothing at +all. + +Policy does not live here. What a memory ceiling *should* be is arithmetic on a baseline +measurement, and it stays in `cargo-gamma-lib` where it can be tested without a kernel. This +crate answers “what can the platform do, and do it”; its caller answers “what should we ask +for”.
This crate was developed as part of The Oxidizer Project. Browse this crate's source code. + + [__link0]: https://crates.io/crates/cargo-gamma diff --git a/crates/cargo-gamma-unsafe/src/cgroup.rs b/crates/cargo-gamma-unsafe/src/cgroup.rs index d2c3c051e..7aa3e377a 100644 --- a/crates/cargo-gamma-unsafe/src/cgroup.rs +++ b/crates/cargo-gamma-unsafe/src/cgroup.rs @@ -769,14 +769,32 @@ impl Cgroup { } /// Kills everything in the cgroup, including anything that left the process group. - pub fn kill(&self) { + /// + /// # Errors + /// + /// Returns the operating system's reason when the cgroup kill switch could not be written. + pub fn kill(&self) -> io::Result<()> { if let Some(kill) = self.kill.as_ref() { let fd = kill.as_raw_fd(); - // SAFETY: the descriptor is owned by `self`, remains open for this call, and the byte - // is static initialized storage whose exact length is supplied. - let _killed = unsafe { libc::write(fd, b"1".as_ptr().cast(), 1) }; + loop { + // SAFETY: the descriptor is owned by `self`, remains open for this call, and the + // byte is static initialized storage whose exact length is supplied. + let written = unsafe { libc::write(fd, b"1".as_ptr().cast(), 1) }; + + if written == 1 { + return Ok(()); + } + if written >= 0 { + return Err(io::Error::new(io::ErrorKind::WriteZero, "the cgroup kill switch accepted no byte")); + } + + let cause = io::Error::last_os_error(); + if cause.kind() != io::ErrorKind::Interrupted { + return Err(cause); + } + } } else { - let _killed = self.set("cgroup.kill", "1"); + fs::write(self.path.join("cgroup.kill"), "1") } } @@ -1431,7 +1449,7 @@ mod tests { let directory = tempfile::tempdir().expect("a temporary directory"); let path = directory.path(); - over(path).kill(); + over(path).kill().expect("the temporary cgroup kill switch is writable"); assert_eq!(fs::read_to_string(path.join("cgroup.kill")).expect("written"), "1"); } diff --git a/crates/cargo-gamma-unsafe/src/job.rs b/crates/cargo-gamma-unsafe/src/job.rs index 48e53557c..3c177ed66 100644 --- a/crates/cargo-gamma-unsafe/src/job.rs +++ b/crates/cargo-gamma-unsafe/src/job.rs @@ -10,6 +10,7 @@ use core::ffi::c_void; use core::mem; +use std::io; use std::os::windows::io::{AsRawHandle as _, FromRawHandle as _, OwnedHandle}; use std::process::{Child, Command}; use std::sync::{Mutex, Once}; @@ -512,8 +513,16 @@ impl Job { } /// Kills everything in the job. - pub fn terminate(&self) { - let _terminated = NATIVE_CALLS.terminate_job(self.handle); + /// + /// # Errors + /// + /// Returns the operating system's reason when the job could not be terminated. + pub fn terminate(&self) -> io::Result<()> { + if NATIVE_CALLS.terminate_job(self.handle) { + Ok(()) + } else { + Err(io::Error::last_os_error()) + } } } @@ -867,7 +876,7 @@ mod tests { "the child was not running to begin with, so its death proves nothing" ); - job.terminate(); + job.terminate().expect("the live test job can be terminated"); wait_for_end(&mut child); } @@ -1029,7 +1038,7 @@ mod tests { "a failed {call:?} did not leave the child suspended, so nothing was tested" ); - job.terminate(); + job.terminate().expect("the suspended test child remains reachable through its job"); wait_for_end(&mut child); @@ -1095,9 +1104,8 @@ mod tests { /// A termination that did not happen is not mistaken for one that did. /// - /// `terminate` returns nothing, so the only way to see that it worked is the subtree. This - /// asserts the injected failure really does keep the child alive — otherwise the test above it - /// proves nothing — and that a real termination afterwards still reaches it. + /// The error and the still-live child both show that the injected call did not terminate the + /// job; a later successful call proves the subtree remains reachable for cleanup. #[test] fn a_termination_that_fails_leaves_the_subtree_reachable() { let job = Job::create(None).expect("a job is created"); @@ -1106,14 +1114,15 @@ mod tests { let _armed = native_faults::arm(NativeCall::TerminateJob); - job.terminate(); + job.terminate().expect_err("the injected termination failure is reported"); assert!( child.try_wait().expect("the child's status can be read").is_none(), "the injected failure did not prevent the termination, so nothing was tested" ); - job.terminate(); + job.terminate() + .expect("the live test job remains terminable after the injected failure"); wait_for_end(&mut child); } diff --git a/crates/cargo-gamma-unsafe/src/lib.rs b/crates/cargo-gamma-unsafe/src/lib.rs index ab8f5a043..e5e2cd338 100644 --- a/crates/cargo-gamma-unsafe/src/lib.rs +++ b/crates/cargo-gamma-unsafe/src/lib.rs @@ -4,9 +4,9 @@ #![doc(hidden)] #![cfg_attr(coverage_nightly, feature(coverage_attribute))] -//! The platform calls [`cargo-gamma`](https://crates.io/crates/cargo-gamma) cannot make safely, -//! behind an interface that is safe to call. This crate is an implementation detail of the tool; -//! you should never need to depend on it directly. +//! Platform calls that [`cargo-gamma`](https://crates.io/crates/cargo-gamma) cannot make safely +//! are concentrated behind an interface that is safe to call. This crate is an implementation +//! detail of the tool; you should never need to depend on it directly. //! //! Two things the tool does have no safe expression in `std`: killing a whole process subtree (a //! process group on Unix, a job object on Windows) and bounding what that subtree allocates (a diff --git a/crates/cargo-gamma/CHANGELOG.md b/crates/cargo-gamma/CHANGELOG.md index 3f9cdf8f6..a6d062dc4 100644 --- a/crates/cargo-gamma/CHANGELOG.md +++ b/crates/cargo-gamma/CHANGELOG.md @@ -7,4 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.2.0] - 2026-09-03 + +- 🐛 Bug Fixes + + - harden discovery and process cleanup + - address hardening feedback ([#144](https://github.com/microsoft/ox-tools/pull/144)) + +## [0.1.0] - 2026-09-02 + - Initial release. diff --git a/crates/cargo-gamma/Cargo.toml b/crates/cargo-gamma/Cargo.toml index f928e042f..e91ece402 100644 --- a/crates/cargo-gamma/Cargo.toml +++ b/crates/cargo-gamma/Cargo.toml @@ -4,7 +4,7 @@ [package] name = "cargo-gamma" description = "Fast mutation testing for Rust" -version = "0.1.0" +version = "0.2.0" readme = "README.md" keywords = ["cargo", "cargo-plugin", "mutation-testing", "testing"] categories = ["command-line-utilities", "development-tools::testing"] diff --git a/crates/cargo-gamma/docs/TODO.md b/crates/cargo-gamma/docs/TODO.md index 5ba06b874..c0d074e97 100644 --- a/crates/cargo-gamma/docs/TODO.md +++ b/crates/cargo-gamma/docs/TODO.md @@ -5,9 +5,61 @@ deleted; this file is not a changelog or a record of rejected work. ## Contents +### Performance +- [P1](#p1) — Publish killers found after a file-learning fallback +- [P2](#p2) — Compare equal files correctly across short reads + ### Features - [F2](#f2) — Checkpoint and resume long-running campaigns +### Documentation +- [D1](#d1) — Correct the cgroup watch-state documentation + +### Testing +- [T1](#t1) — Isolate tests from the production interrupt registry + +## Performance + + +### P1 — Publish killers found after a file-learning fallback + +**Area:** `cargo-gamma-lib::exec::sweep` · **Priority:** Low · **Effort:** Small + +When a worker times out waiting for another file learner, or sees `Learning::Exhausted`, it runs +the full ordered judgement itself. Unlike the hinted and designated-learner paths, these fallback +paths return without publishing a killer they discover. Later mutants in the same file therefore +repeat the full binary order even though this run already found a reusable probe. + +- `crates/cargo-gamma-lib/src/exec/sweep.rs:746-749` and + `crates/cargo-gamma-lib/src/exec/sweep.rs:788-790` — the two normal paths publish or complete + learning +- `crates/cargo-gamma-lib/src/exec/sweep.rs:762-784` — timeout and exhausted-state fallbacks return + their judgement without either operation + +**Done when:** every fallback judgement publishes a newly found killer without overwriting an +already learned one, and a regression test starts from `InProgress`, forces the bounded wait to +expire, returns a killing judgement, and observes `Learning::Learned`. + +--- + + +### P2 — Compare equal files correctly across short reads + +**Area:** `cargo-gamma-lib::exec::sync` · **Priority:** Low · **Effort:** Small + +`same_contents` reads two files independently and treats unequal read lengths as unequal file +contents. `Read::read` may legally return a short non-EOF result, so identical files on a +filesystem that gives different chunk sizes can be needlessly recopied, changing timestamps and +invalidating Cargo fingerprints. + +- `crates/cargo-gamma-lib/src/exec/sync.rs:308-330` — independent reads are compared as though both + must fill equally sized chunks + +**Done when:** comparison handles independent short reads without misaligning bytes, and a unit +test uses two readers with different chunk schedules over identical content. + +--- + ## Features @@ -33,3 +85,58 @@ incomplete until the population finishes. **Done when:** interruption tests cover every publication boundary, an end-to-end resume test proves that a partial record saves work without changing the final score, and the configured checkpoint cadence places an explicit upper bound on progress at risk. + +--- + +## Documentation + + +### D1 — Correct the cgroup watch-state documentation + +**Area:** `cargo-gamma-unsafe::cgroup` · **Priority:** Low · **Effort:** Trivial + +The documentation on `Cgroup::is_watched` says the method records a published kill descriptor, +but the method only queries whether a watch exists. That description belongs to `watched_at`, +whose current one-line documentation omits the descriptor-lifetime invariant. + +- `crates/cargo-gamma-unsafe/src/cgroup.rs:794-805` — the two adjacent methods carry each other's + intended descriptions + +**Done when:** `is_watched` documents its boolean query and `watched_at` documents publication and +lifetime ownership. + +--- + +## Testing + + +### T1 — Isolate tests from the production interrupt registry + +**Area:** `cargo-gamma-unsafe` interrupt and cgroup tests · **Priority:** High · **Effort:** Medium +**Confidence:** High · **Scope:** five process-global test interactions — exhaustive +**Trigger:** libtest schedules production-handler tests before or concurrently with cgroup watch tests + +Two tests call the production signal handler directly, permanently latching the process-global +registry's interrupt state. Cgroup tests use that same registry with fabricated process-group IDs +41 and 42. Once interrupted, registering either ID immediately invokes the production +`kill_group`, and a concurrent handler sweep does the same. The test binary can therefore send a +real `SIGKILL` to an unrelated host process group that happens to own either numeric ID, while +later tests also inherit interrupt state they did not arrange. + +- `crates/cargo-gamma-unsafe/src/interrupt.rs:112-116` — registry interruption is deliberately + never cleared +- `crates/cargo-gamma-unsafe/src/interrupt.rs:193-211` and + `crates/cargo-gamma-unsafe/src/interrupt.rs:264-284` — claiming after interruption and sweeping + invoke the supplied killer +- `crates/cargo-gamma-unsafe/src/interrupt.rs:469-471` and + `crates/cargo-gamma-unsafe/src/interrupt.rs:575-581` — production paths supply a real + `kill(-group, SIGKILL)` +- `crates/cargo-gamma-unsafe/src/interrupt.rs:755-769` — tests call the production handler against + the global registry +- `crates/cargo-gamma-unsafe/src/cgroup.rs:913-914` and + `crates/cargo-gamma-unsafe/src/cgroup.rs:966-1050` — cgroup tests register IDs 41 and 42 through + that registry + +**Done when:** handler tests mutate only an isolated registry or run in child processes, cgroup +tests inject a recording killer instead of using the production registry, and order-randomized +parallel execution cannot signal a real process group or leak interrupt state between tests. diff --git a/scripts/release-crate.ps1 b/scripts/release-crate.ps1 index c118c4fd1..6a125e55c 100644 --- a/scripts/release-crate.ps1 +++ b/scripts/release-crate.ps1 @@ -455,6 +455,7 @@ function Update-CrateVersion { function Write-Changelog { param( [string]$crateName, + [string]$oldVersion, [string]$newVersion, [string]$crateFolder, [string]$changelogFile, @@ -463,22 +464,41 @@ function Write-Changelog { $tags = Invoke-GitCommand -Command "tag --list `"$crateName-v*`"" -ErrorMessage "Failed to retrieve git tags" $latestTag = $null + $initialCommit = $null + $previousReleaseCommit = $null if ($null -eq $tags -or $tags.Count -eq 0) { - Write-Warning "No tags found for crate '$crateName'. Generating changelog from all history." + Write-Warning "No tags found for crate '$crateName'. Generating changelog from the available repository history." } else { $filteredTags = @($tags | Where-Object { $_ -match "^${crateName}-v\d+\.\d+\.\d+$" }) if ($filteredTags.Count -gt 0) { $sortedTags = @($filteredTags | Sort-Object { [version]($_ -replace "${crateName}-v", '') }) $latestTag = $sortedTags[-1] + $previousReleaseCommit = Invoke-GitCommand -Command "rev-list -n 1 $latestTag" -ErrorMessage "Failed to resolve latest release tag" } else { - Write-Warning "No valid semantic version tags found for crate '$crateName'. Generating changelog from all history." + Write-Warning "No valid semantic version tags found for crate '$crateName'. Generating changelog from the available repository history." } } $currentDate = (Get-Date).ToString('yyyy-MM-dd') # Get commits since the latest tag (unreleased commits) - $range = if ($latestTag) { "$latestTag..HEAD" } else { "HEAD" } + if (-not $latestTag) { + $crateCommits = @(Invoke-GitCommand -Command "log --reverse --format=`"%H`" -- `"$crateFolder`"" -ErrorMessage "Failed to retrieve crate history") + if ($crateCommits.Count -gt 0) { + $initialCommit = $crateCommits[0] + $previousReleaseCommit = $initialCommit + } + } + + # A newly introduced crate may have shipped before its first release tag was created. Its + # introduction belongs to that initial version, not to the next release generated from HEAD. + $range = if ($latestTag) { + "$latestTag..HEAD" + } elseif ($initialCommit -and $oldVersion -ne "0.0.0") { + "$initialCommit..HEAD" + } else { + "HEAD" + } $rawCommits = Invoke-GitCommand -Command "log $range --pretty=format:`"%s`" -- `"$crateFolder`"" -ErrorMessage "Failed to retrieve git log for unreleased commits" if ($null -eq $rawCommits -or $rawCommits.Count -eq 0) { $rawCommits = @() @@ -506,12 +526,43 @@ function Write-Changelog { if (Test-Path $changelogFile) { $existingContent = Get-Content $changelogFile -Raw if ($existingContent) { - # Find the position after "# Changelog" header and any blank lines - # Insert the new version section there - $headerPattern = '^# Changelog\s*\r?\n(\r?\n)*' - if ($existingContent -match $headerPattern) { - $headerMatch = [regex]::Match($existingContent, $headerPattern) - $insertPosition = $headerMatch.Index + $headerMatch.Length + # Rerunning a release replaces that version's generated section rather than duplicating + # it. This also repairs output produced before initial-release history was separated. + $escapedNewVersion = [regex]::Escape($newVersion) + $targetSectionPattern = "(?ms)^## \[$escapedNewVersion\].*?(?=^## \[|\z)" + $existingContent = [regex]::Replace($existingContent, $targetSectionPattern, '').TrimEnd() + "`n" + $existingContent = [regex]::Replace($existingContent, '(?m)^# Changelog\s*\r?\n(?=## \[)', "# Changelog`n`n") + + $initialReleasePattern = '(?s)## \[Unreleased\]\s*-\s*Initial release\.\s*\z' + $wasInitialRelease = $existingContent -match $initialReleasePattern + + if ($wasInitialRelease -and $previousReleaseCommit) { + $initialDate = Invoke-GitCommand -Command "show -s --format=`"%cs`" $previousReleaseCommit" -ErrorMessage "Failed to retrieve initial release date" + $initialSection = "## [$oldVersion] - $initialDate`n`n- Initial release.`n" + $existingContent = [regex]::Replace($existingContent, $initialReleasePattern, $initialSection) + } + + $escapedOldVersion = [regex]::Escape($oldVersion) + $hasInitialVersion = $existingContent -match "(?m)^## \[$escapedOldVersion\]" + if ($oldVersion -ne "0.0.0" -and -not $hasInitialVersion -and $previousReleaseCommit) { + $initialDate = Invoke-GitCommand -Command "show -s --format=`"%cs`" $previousReleaseCommit" -ErrorMessage "Failed to retrieve initial release date" + $existingContent = $existingContent.TrimEnd() + "`n`n## [$oldVersion] - $initialDate`n`n- Initial release.`n" + } + + # Keep the explanatory preamble directly below the title and `Unreleased` first. + $unreleasedMatch = [regex]::Match($existingContent, '(?m)^## \[Unreleased\]\s*\r?\n(?:\r?\n)*') + $sectionMatch = [regex]::Match($existingContent, '(?m)^## \[') + if (-not $unreleasedMatch.Success) { + $insertPosition = if ($sectionMatch.Success) { $sectionMatch.Index } else { $existingContent.Length } + $separator = if ($insertPosition -eq $existingContent.Length) { "`n" } else { "" } + $existingContent = $existingContent.Substring(0, $insertPosition) + + $separator + "## [Unreleased]`n`n" + + $existingContent.Substring($insertPosition) + $unreleasedMatch = [regex]::Match($existingContent, '(?m)^## \[Unreleased\]\s*\r?\n(?:\r?\n)*') + } + + if ($unreleasedMatch.Success) { + $insertPosition = $unreleasedMatch.Index + $unreleasedMatch.Length $newContent = $existingContent.Substring(0, $insertPosition) + ($newVersionSection -join "`n") + "`n" + $existingContent.Substring($insertPosition) @@ -692,7 +743,7 @@ try { Exit 1 } - Write-Changelog -crateName $CrateName -newVersion $newVersion -crateFolder $crateFolder -changelogFile $changelogFile -prBaseUrl $prBaseUrl + Write-Changelog -crateName $CrateName -oldVersion $oldVersion -newVersion $newVersion -crateFolder $crateFolder -changelogFile $changelogFile -prBaseUrl $prBaseUrl Update-Readme -crateName $CrateName -crateFolder $crateFolder if (Test-SemverIncompatibleBump -oldVersion $oldVersion -newVersion $newVersion) {