From 094d86b9de63b9bc13cbe85cdbbe3dc97506f91e Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 9 Sep 2026 16:57:19 -0700 Subject: [PATCH 1/6] core: add BranchAction::Defer and report changes on every run A sandbox's COW branch was committed, aborted, or kept only inside Sandbox::drop, so a caller had no way to look at what a run changed and then decide (issue #215: an MCP tool-call proxy wants to hold a tool's writes pending an external approval). Keep left the upper behind with nothing to act on, and dry_run showed changes that could never be kept. Defer makes wait() leave the branch in the sandbox for commit()/abort(), with pending() and upper_dir() to inspect it, and dropping an undecided sandbox preserves the branch like Keep so nothing is published without a decision. wait() now settles the branch itself, reading the change set into RunResult::changes first, which makes dry_run a plain run with both actions set to Abort, so dry_run and DryRunResult are removed. Signed-off-by: Cong Wang --- crates/sandlock-core/src/cow/seccomp.rs | 34 +-- crates/sandlock-core/src/dry_run.rs | 45 ---- crates/sandlock-core/src/lib.rs | 4 +- crates/sandlock-core/src/pipeline.rs | 5 + crates/sandlock-core/src/profile.rs | 10 +- crates/sandlock-core/src/result.rs | 40 ++++ crates/sandlock-core/src/sandbox.rs | 180 +++++++++------- crates/sandlock-core/src/transaction.rs | 10 +- crates/sandlock-core/tests/integration.rs | 4 +- .../tests/integration/test_branch_action.rs | 203 ++++++++++++++++++ .../tests/integration/test_cow.rs | 12 +- .../tests/integration/test_dry_run.rs | 102 --------- .../tests/integration/test_handlers.rs | 2 +- .../tests/integration/test_transaction.rs | 109 ---------- 14 files changed, 385 insertions(+), 375 deletions(-) delete mode 100644 crates/sandlock-core/src/dry_run.rs create mode 100644 crates/sandlock-core/tests/integration/test_branch_action.rs delete mode 100644 crates/sandlock-core/tests/integration/test_dry_run.rs diff --git a/crates/sandlock-core/src/cow/seccomp.rs b/crates/sandlock-core/src/cow/seccomp.rs index 026e5727..2918e5b7 100644 --- a/crates/sandlock-core/src/cow/seccomp.rs +++ b/crates/sandlock-core/src/cow/seccomp.rs @@ -1591,8 +1591,8 @@ impl SeccompCowBranch { } /// List all filesystem changes in the COW layer. - pub fn changes(&self) -> Result, BranchError> { - use crate::dry_run::{Change, ChangeKind}; + pub fn changes(&self) -> Result, BranchError> { + use crate::result::{Change, ChangeKind}; let mut result = Vec::new(); @@ -3117,7 +3117,7 @@ mod tests { "the deletion was applied before the failure", ); - let mut outstanding: Vec<(crate::dry_run::ChangeKind, String)> = branch + let mut outstanding: Vec<(crate::result::ChangeKind, String)> = branch .changes() .unwrap() .into_iter() @@ -3129,8 +3129,8 @@ mod tests { vec![ // b.txt is "modified" because the obstructing symlink is still // there in the workdir; c.txt was never reached. - (crate::dry_run::ChangeKind::Modified, "b.txt".to_string()), - (crate::dry_run::ChangeKind::Added, "c.txt".to_string()), + (crate::result::ChangeKind::Modified, "b.txt".to_string()), + (crate::result::ChangeKind::Added, "c.txt".to_string()), ], "changes() after a partial merge must report the remainder only", ); @@ -3672,7 +3672,7 @@ mod tests { fs::write(&upper, "new content").unwrap(); let changes = branch.changes().unwrap(); assert_eq!(changes.len(), 1); - assert_eq!(changes[0].kind, crate::dry_run::ChangeKind::Added); + assert_eq!(changes[0].kind, crate::result::ChangeKind::Added); assert_eq!(changes[0].path, std::path::PathBuf::from("brand_new.txt")); } @@ -3684,7 +3684,7 @@ mod tests { fs::write(&upper, "modified content").unwrap(); let changes = branch.changes().unwrap(); assert_eq!(changes.len(), 1); - assert_eq!(changes[0].kind, crate::dry_run::ChangeKind::Modified); + assert_eq!(changes[0].kind, crate::result::ChangeKind::Modified); assert_eq!(changes[0].path, std::path::PathBuf::from("existing.txt")); } @@ -3695,7 +3695,7 @@ mod tests { branch.mark_deleted("existing.txt"); let changes = branch.changes().unwrap(); assert_eq!(changes.len(), 1); - assert_eq!(changes[0].kind, crate::dry_run::ChangeKind::Deleted); + assert_eq!(changes[0].kind, crate::result::ChangeKind::Deleted); assert_eq!(changes[0].path, std::path::PathBuf::from("existing.txt")); } @@ -3720,11 +3720,11 @@ mod tests { let mut changes = branch.changes().unwrap(); changes.sort_by(|a, b| a.path.cmp(&b.path)); assert_eq!(changes.len(), 3); - assert_eq!(changes[0].kind, crate::dry_run::ChangeKind::Modified); + assert_eq!(changes[0].kind, crate::result::ChangeKind::Modified); assert_eq!(changes[0].path, std::path::PathBuf::from("existing.txt")); - assert_eq!(changes[1].kind, crate::dry_run::ChangeKind::Added); + assert_eq!(changes[1].kind, crate::result::ChangeKind::Added); assert_eq!(changes[1].path, std::path::PathBuf::from("new.txt")); - assert_eq!(changes[2].kind, crate::dry_run::ChangeKind::Deleted); + assert_eq!(changes[2].kind, crate::result::ChangeKind::Deleted); assert_eq!(changes[2].path, std::path::PathBuf::from("subdir/nested.txt")); } @@ -4529,7 +4529,7 @@ mod tests { .filter(|c| c.path == std::path::Path::new("existing.txt")) .collect(); assert_eq!(for_path.len(), 1); - assert_eq!(for_path[0].kind, crate::dry_run::ChangeKind::Added); + assert_eq!(for_path[0].kind, crate::result::ChangeKind::Added); } #[test] @@ -5429,7 +5429,7 @@ mod tests { .changes() .unwrap() .iter() - .all(|c| c.kind != crate::dry_run::ChangeKind::Deleted), + .all(|c| c.kind != crate::result::ChangeKind::Deleted), "a whiteout the upper re-created must not be reported as a deletion", ); @@ -5549,7 +5549,7 @@ mod tests { .changes() .unwrap() .into_iter() - .filter(|c| c.kind == crate::dry_run::ChangeKind::Deleted) + .filter(|c| c.kind == crate::result::ChangeKind::Deleted) .map(|c| c.path) .collect::>(), vec![PathBuf::from("link/x.txt")], @@ -5668,7 +5668,7 @@ mod tests { .into_iter() .map(|c| (c.kind, c.path)) .collect::>(), - vec![(crate::dry_run::ChangeKind::Modified, PathBuf::from("f.txt"))], + vec![(crate::result::ChangeKind::Modified, PathBuf::from("f.txt"))], "precondition: the run reports the chmod as a recorded change", ); @@ -5831,7 +5831,7 @@ mod tests { /// reading a dry run or a recovery report is actually being told. #[test] fn changes_labels_an_entry_against_the_workdir_as_it_stands_now() { - use crate::dry_run::ChangeKind; + use crate::result::ChangeKind; let workdir = tempfile::tempdir().unwrap(); let storage = tempfile::tempdir().unwrap(); @@ -6242,7 +6242,7 @@ mod tests { /// run do" with half the truth. #[test] fn changes_on_a_kept_branch_still_reports_the_whole_change_set() { - use crate::dry_run::ChangeKind; + use crate::result::ChangeKind; let workdir = tempfile::tempdir().unwrap(); let storage = tempfile::tempdir().unwrap(); fs::write(workdir.path().join("gone.txt"), "still here").unwrap(); diff --git a/crates/sandlock-core/src/dry_run.rs b/crates/sandlock-core/src/dry_run.rs deleted file mode 100644 index dcda259a..00000000 --- a/crates/sandlock-core/src/dry_run.rs +++ /dev/null @@ -1,45 +0,0 @@ -use crate::result::RunResult; -use std::fmt; -use std::path::PathBuf; - -/// Kind of filesystem change detected by dry-run. -#[derive(Debug, Clone, PartialEq, Eq)] -pub enum ChangeKind { - /// File was created (exists in upper but not in workdir). - Added, - /// File was modified (exists in both, content differs). - Modified, - /// File was deleted. - Deleted, -} - -impl fmt::Display for ChangeKind { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - ChangeKind::Added => write!(f, "A"), - ChangeKind::Modified => write!(f, "M"), - ChangeKind::Deleted => write!(f, "D"), - } - } -} - -/// A single filesystem change detected by dry-run. -#[derive(Debug, Clone)] -pub struct Change { - pub kind: ChangeKind, - /// Path relative to workdir. - pub path: PathBuf, -} - -impl fmt::Display for Change { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{} {}", self.kind, self.path.display()) - } -} - -/// Result of a dry-run execution. -#[derive(Debug)] -pub struct DryRunResult { - pub run_result: RunResult, - pub changes: Vec, -} diff --git a/crates/sandlock-core/src/lib.rs b/crates/sandlock-core/src/lib.rs index b0771fd8..9c6220e5 100644 --- a/crates/sandlock-core/src/lib.rs +++ b/crates/sandlock-core/src/lib.rs @@ -32,7 +32,6 @@ pub mod image; pub mod fork; pub(crate) mod ca_inject; pub(crate) mod chroot; -pub mod dry_run; pub mod control; mod transparent_proxy; @@ -43,14 +42,13 @@ pub use protection::{Protection, ProtectionState, ProtectionPolicy, ProtectionSt pub use sandbox::{ BindPorts, Confinement, ConfinementBuilder, Process, Sandbox, SandboxBuilder, StdioMode, }; -pub use result::{RunResult, ExitStatus}; +pub use result::{Change, ChangeKind, ExitStatus, RunResult}; pub use pipeline::{Stage, Pipeline, Gather}; pub use transaction::{AbortReason, Transaction, TxnDisposition, TxnError, TxnOutcome}; // Recovery of COW branch storage that was preserved rather than reclaimed. The // rest of `cow` is internal; the `recovery` module is the backend-neutral path // these belong to, and the flat aliases here are kept for convenience. pub use recovery::{list_preserved, read_preserved, PreserveReason, PreservedBranch}; -pub use dry_run::{Change, ChangeKind, DryRunResult}; // Sectioned-profile parsing types: ProfileInput is the top-level deserialization // target; ProgramSpec carries [program].exec/args (not a Sandbox field). // format_net_rule renders a NetRule back into the --net-allow/--net-deny diff --git a/crates/sandlock-core/src/pipeline.rs b/crates/sandlock-core/src/pipeline.rs index 102b4f58..bcedae03 100644 --- a/crates/sandlock-core/src/pipeline.rs +++ b/crates/sandlock-core/src/pipeline.rs @@ -58,6 +58,7 @@ impl Stage { exit_status: ExitStatus::Timeout, stdout: None, stderr: None, + changes: Vec::new(), }), } } else { @@ -135,6 +136,7 @@ impl Pipeline { exit_status: ExitStatus::Timeout, stdout: None, stderr: None, + changes: Vec::new(), }), } } else { @@ -231,6 +233,7 @@ async fn run_pipeline(stages: Vec) -> Result { exit_status: ExitStatus::Killed, stdout: None, stderr: None, + changes: Vec::new(), }; for (i, mut sb) in sandboxes.into_iter().enumerate() { @@ -327,6 +330,7 @@ impl Gather { exit_status: ExitStatus::Timeout, stdout: None, stderr: None, + changes: Vec::new(), }), } } else { @@ -427,6 +431,7 @@ async fn run_gather( exit_status: ExitStatus::Killed, stdout: None, stderr: None, + changes: Vec::new(), }; for (i, mut sb) in sandboxes.into_iter().enumerate() { let result = sb.wait().await?; diff --git a/crates/sandlock-core/src/profile.rs b/crates/sandlock-core/src/profile.rs index 9e21b72c..1ac5a53d 100644 --- a/crates/sandlock-core/src/profile.rs +++ b/crates/sandlock-core/src/profile.rs @@ -107,10 +107,10 @@ pub struct FilesystemSection { /// Each entry has the form `"VIRTUAL:HOST"`, matching `--fs-mount` syntax. #[serde(skip_serializing_if = "Vec::is_empty")] pub mount: Vec, - /// One of `"commit"`, `"abort"`, `"keep"`. Maps to `Sandbox::on_exit`. + /// One of `"commit"`, `"abort"`, `"keep"`, `"defer"`. Maps to `Sandbox::on_exit`. #[serde(skip_serializing_if = "Option::is_none")] pub on_exit: Option, - /// One of `"commit"`, `"abort"`, `"keep"`. Maps to `Sandbox::on_error`. + /// One of `"commit"`, `"abort"`, `"keep"`, `"defer"`. Maps to `Sandbox::on_error`. #[serde(skip_serializing_if = "Option::is_none")] pub on_error: Option, } @@ -361,8 +361,9 @@ fn parse_branch_action(s: &str) -> Result BranchAction::Commit, "abort" => BranchAction::Abort, "keep" => BranchAction::Keep, + "defer" => BranchAction::Defer, other => return Err(SandlockError::Sandbox(SandboxError::Invalid( - format!("invalid branch action {other:?}; expected \"commit\" | \"abort\" | \"keep\""), + format!("invalid branch action {other:?}; expected \"commit\" | \"abort\" | \"keep\" | \"defer\""), ))), }) } @@ -406,13 +407,14 @@ fn parse_time_start(s: &str) -> Result { // Reverse serialization: Sandbox -> ProfileInput (and JSON/TOML) // ============================================================ -/// Render a `BranchAction` as the profile string form (`"commit"`/`"abort"`/`"keep"`). +/// Render a `BranchAction` as the profile string form. fn branch_action_str(a: &crate::sandbox::BranchAction) -> &'static str { use crate::sandbox::BranchAction; match a { BranchAction::Commit => "commit", BranchAction::Abort => "abort", BranchAction::Keep => "keep", + BranchAction::Defer => "defer", } } diff --git a/crates/sandlock-core/src/result.rs b/crates/sandlock-core/src/result.rs index 66e61254..4337c206 100644 --- a/crates/sandlock-core/src/result.rs +++ b/crates/sandlock-core/src/result.rs @@ -1,9 +1,15 @@ +use std::fmt; +use std::path::PathBuf; + /// The result of running a sandboxed process. #[derive(Debug, Clone)] pub struct RunResult { pub exit_status: ExitStatus, pub stdout: Option>, pub stderr: Option>, + /// What the run did to its COW branch, read before the branch action was + /// applied. Empty when the sandbox has no workdir. + pub changes: Vec, } impl RunResult { @@ -29,6 +35,7 @@ impl RunResult { exit_status: ExitStatus::Timeout, stdout: None, stderr: None, + changes: Vec::new(), } } @@ -47,3 +54,36 @@ pub enum ExitStatus { Killed, Timeout, } + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ChangeKind { + /// Exists in the branch but not in the workdir. + Added, + /// Opened for writing and exists in the workdir; the bytes are not compared. + Modified, + Deleted, +} + +impl fmt::Display for ChangeKind { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + ChangeKind::Added => write!(f, "A"), + ChangeKind::Modified => write!(f, "M"), + ChangeKind::Deleted => write!(f, "D"), + } + } +} + +/// One filesystem change a run made to its COW branch. +#[derive(Debug, Clone)] +pub struct Change { + pub kind: ChangeKind, + /// Relative to the workdir. + pub path: PathBuf, +} + +impl fmt::Display for Change { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{} {}", self.kind, self.path.display()) + } +} diff --git a/crates/sandlock-core/src/sandbox.rs b/crates/sandlock-core/src/sandbox.rs index 1e5a7c48..a1d19946 100644 --- a/crates/sandlock-core/src/sandbox.rs +++ b/crates/sandlock-core/src/sandbox.rs @@ -200,13 +200,17 @@ impl TryFrom<&Sandbox> for Confinement { } } -/// Action to take on branch exit. +/// What happens to the run's COW branch once the child has exited. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)] pub enum BranchAction { #[default] Commit, Abort, + /// Leave the branch on disk for recovery tooling. Keep, + /// Leave the branch in the sandbox for [`Sandbox::commit`] / + /// [`Sandbox::abort`]. Dropped undecided, it is preserved like `Keep`. + Defer, } // ============================================================ @@ -858,8 +862,9 @@ impl Sandbox { _ => None, }; if let Some(exit_status) = stopped { + let changes = self.settle_branch().await; let (stdout, stderr) = self.collect_pipe_drains().await; - return Ok(RunResult { exit_status, stdout, stderr }); + return Ok(RunResult { exit_status, stdout, stderr, changes }); } // Deliver EOF to a piped stdin the caller never took: otherwise a child @@ -927,20 +932,88 @@ impl Sandbox { let _ = h.await; } + let changes = self.settle_branch().await; + let (stdout, stderr) = self.collect_pipe_drains().await; + + Ok(RunResult { exit_status, stdout, stderr, changes }) + } + + /// Take the finished run's branch from the supervisor, read its change + /// set, and apply the exit action. `Defer` leaves the branch in place for + /// [`Self::commit`] / [`Self::abort`], so a repeat call finds it again and + /// only re-reads the changes. + async fn settle_branch(&mut self) -> Vec { // A transactional-pipeline stage leaves the branch in the shared COW - // state for the next stage / the coordinator's single commit — don't - // take it out (that would strip the upper from later stages) and don't - // let Drop commit/abort it (`seccomp_cow` stays None). - if self.rt().shared_cow.is_none() { - if let Some(ref cow_state) = self.rt().supervisor_cow.clone() { + // state for the next stage / the coordinator's single commit: taking it + // would strip the upper from later stages. + if self.rt().shared_cow.is_some() { + return Vec::new(); + } + if self.rt().seccomp_cow.is_none() { + if let Some(cow_state) = self.rt().supervisor_cow.clone() { let mut cow = cow_state.lock().await; self.rt_mut().seccomp_cow = cow.branch.take(); } } + let Some(mut branch) = self.rt_mut().seccomp_cow.take() else { + return Vec::new(); + }; + let changes = branch.changes().unwrap_or_default(); + match self.branch_action() { + BranchAction::Defer => self.rt_mut().seccomp_cow = Some(branch), + BranchAction::Keep => branch.keep(), + BranchAction::Abort => { let _ = branch.abort(); } + // commit() blocks up to DROP_COMMIT_LOCK_WAIT on a contended + // workdir, which must not stall the async worker. + BranchAction::Commit => { + let _ = tokio::task::spawn_blocking(move || branch.commit()).await; + } + } + changes + } - let (stdout, stderr) = self.collect_pipe_drains().await; + /// The action the exit status selects; `on_exit` until the child has + /// exited non-zero. + fn branch_action(&self) -> BranchAction { + let failed = self.runtime.as_ref().is_some_and(|rt| { + matches!(rt.state, RuntimeState::Stopped(ref s) if !matches!(s, crate::result::ExitStatus::Code(0))) + }); + if failed { self.on_error.clone() } else { self.on_exit.clone() } + } - Ok(RunResult { exit_status, stdout, stderr }) + /// Whether a [`BranchAction::Defer`] run has exited and is waiting for + /// [`Self::commit`] or [`Self::abort`]. + pub fn pending(&self) -> bool { + self.runtime.as_ref().is_some_and(|rt| { + rt.seccomp_cow.is_some() && matches!(rt.state, RuntimeState::Stopped(_)) + }) + } + + /// The pending branch's upper directory, laid out like the workdir and + /// holding the new bytes of every added or modified file. + pub fn upper_dir(&self) -> Option<&std::path::Path> { + if !self.pending() { return None; } + self.rt().seccomp_cow.as_ref().map(|b| b.upper_dir()) + } + + /// Merge the pending branch into the workdir. Blocks up to 5s on a + /// contended workdir; on timeout the branch is preserved and the error + /// names it. Last-writer-wins against anything that changed the workdir + /// since the run. + pub fn commit(&mut self) -> Result<(), crate::error::BranchError> { + self.take_pending()?.commit() + } + + /// Discard the pending branch. + pub fn abort(&mut self) -> Result<(), crate::error::BranchError> { + self.take_pending()?.abort() + } + + fn take_pending(&mut self) -> Result { + if !self.pending() { + return Err(crate::error::BranchError::Operation("no deferred branch to dispose".into())); + } + Ok(self.rt_mut().seccomp_cow.take().expect("pending() checked seccomp_cow")) } /// Join the capture-pipe drains, if this runtime still holds them. @@ -1346,42 +1419,6 @@ impl Sandbox { self.wait().await } - /// Dry-run: create, start, wait, collect filesystem changes, then abort. - /// - /// The branch action is forced to `Abort`, not `Keep`: a dry run must never - /// merge, and must not leave its upper on disk either — the changes are read - /// out of the branch here and returned, so nothing needs preserving. `Keep` - /// would additionally ask the branch to survive an abandoned run (`?` on - /// create/wait below), which for a dry run is a pure leak. - pub async fn dry_run( - &mut self, - cmd: &[&str], - ) -> Result { - self.on_exit = BranchAction::Abort; - self.on_error = BranchAction::Abort; - self.do_create(cmd, true).await?; - self.do_start()?; - let run_result = self.wait().await?; - let changes = self.collect_changes().await; - self.do_abort().await; - Ok(crate::dry_run::DryRunResult { run_result, changes }) - } - - /// Dry-run with inherited stdio. Same branch handling as [`Self::dry_run`]. - pub async fn dry_run_interactive( - &mut self, - cmd: &[&str], - ) -> Result { - self.on_exit = BranchAction::Abort; - self.on_error = BranchAction::Abort; - self.do_create(cmd, false).await?; - self.do_start()?; - let run_result = self.wait().await?; - let changes = self.collect_changes().await; - self.do_abort().await; - Ok(crate::dry_run::DryRunResult { run_result, changes }) - } - /// Create N COW clones of this sandbox. /// /// `fork()` requires `init_fn` and `work_fn` to be set on the sandbox (via @@ -1643,23 +1680,6 @@ impl Sandbox { // Internal: collect_changes / do_abort // ================================================================ - async fn collect_changes(&self) -> Vec { - if let Some(ref rt) = self.runtime { - if let Some(ref cow) = rt.seccomp_cow { - return cow.changes().unwrap_or_default(); - } - } - Vec::new() - } - - async fn do_abort(&mut self) { - if let Some(ref mut rt) = self.runtime { - if let Some(ref mut cow) = rt.seccomp_cow { - let _ = cow.abort(); - } - } - } - // ================================================================ // Internal: do_create (fork + policy install; child parks at the // ready_r read, awaiting do_start to release it to execve). @@ -2436,25 +2456,23 @@ impl Drop for Sandbox { if let Some(ParkedDrain::Running(h)) = slot { h.abort(); } } - let is_error = matches!( - rt.state, - RuntimeState::Stopped(ref s) if !matches!(s, crate::result::ExitStatus::Code(0)) - ); - let action = if is_error { &self.on_error } else { &self.on_exit }; - let action = action.clone(); - - if let Some(ref mut cow) = rt.seccomp_cow { - match action { - // NOTE: commit() is synchronous and blocks up to - // DROP_COMMIT_LOCK_WAIT (5s) on a contended workdir before - // deferring (bounded, no CPU spin). Do not drop a committing - // Sandbox on an async runtime worker. - BranchAction::Commit => { let _ = cow.commit(); } - BranchAction::Abort => { let _ = cow.abort(); } - // Mark kept so the branch's Drop backstop preserves the upper - // instead of cleaning it as an undisposed leak. - BranchAction::Keep => cow.keep(), - } + } + + // wait() settles the branch, so this only sees one it left behind: a + // deferred branch nobody decided on, or one a cancelled wait() never + // reached. + let action = self.branch_action(); + if let Some(cow) = self.runtime.as_mut().and_then(|rt| rt.seccomp_cow.as_mut()) { + match action { + // NOTE: commit() is synchronous and blocks up to + // DROP_COMMIT_LOCK_WAIT (5s) on a contended workdir before + // deferring (bounded, no CPU spin). Do not drop a committing + // Sandbox on an async runtime worker. + BranchAction::Commit => { let _ = cow.commit(); } + BranchAction::Abort => { let _ = cow.abort(); } + // The caller asked for the branch and never decided: preserve + // it so nothing is published and nothing is lost. + BranchAction::Keep | BranchAction::Defer => cow.keep(), } } } diff --git a/crates/sandlock-core/src/transaction.rs b/crates/sandlock-core/src/transaction.rs index 85175be0..cd296d9e 100644 --- a/crates/sandlock-core/src/transaction.rs +++ b/crates/sandlock-core/src/transaction.rs @@ -143,7 +143,7 @@ impl Transaction { /// /// The stages really execute — this predicts the filesystem effect on the /// workdir, not the effect of running the commands. Same contract as - /// [`Sandbox::dry_run`](crate::sandbox::Sandbox::dry_run) for one sandbox. + /// a `Sandbox` run with both branch actions set to `Abort`. /// The outcome's [`disposition`](TxnOutcome::disposition) is /// [`TxnDisposition::DryRun`] unless a stage failed or the transaction timed /// out first. @@ -392,7 +392,7 @@ pub struct TxnOutcome { /// The filesystem changes the shared upper held at the end of the run, i.e. /// what the commit merged (or, when not committed, what was discarded). /// Captured from the branch before it is disposed of. - pub changes: Vec, + pub changes: Vec, } impl TxnOutcome { @@ -613,7 +613,7 @@ async fn run_txn( /// What the commit phase did with the shared upper. struct Finished { /// The change set the upper held, read before it was disposed of. - changes: Vec, + changes: Vec, /// `None` when the upper was discarded rather than merged (a dry run, or an /// aborted run); otherwise the result of the locked commit. commit: Option>, @@ -908,7 +908,7 @@ mod tests { #[test] fn per_stage_branch_actions_are_rejected_unless_both_are_the_default() { let wd = tempfile::tempdir().unwrap(); - let actions = [BranchAction::Commit, BranchAction::Abort, BranchAction::Keep]; + let actions = [BranchAction::Commit, BranchAction::Abort, BranchAction::Keep, BranchAction::Defer]; for on_exit in &actions { for on_error in &actions { @@ -2069,7 +2069,7 @@ mod tests { .collect(); assert_eq!( paths, - vec![(crate::dry_run::ChangeKind::Added, std::path::PathBuf::from("a.txt"))], + vec![(crate::result::ChangeKind::Added, std::path::PathBuf::from("a.txt"))], "the discarded change set must still be reported" ); assert!( diff --git a/crates/sandlock-core/tests/integration.rs b/crates/sandlock-core/tests/integration.rs index a6c29ddb..5e9387a0 100644 --- a/crates/sandlock-core/tests/integration.rs +++ b/crates/sandlock-core/tests/integration.rs @@ -52,8 +52,8 @@ mod test_user_mapping; #[path = "integration/test_chroot.rs"] mod test_chroot; -#[path = "integration/test_dry_run.rs"] -mod test_dry_run; +#[path = "integration/test_branch_action.rs"] +mod test_branch_action; #[path = "integration/test_http_acl.rs"] mod test_http_acl; diff --git a/crates/sandlock-core/tests/integration/test_branch_action.rs b/crates/sandlock-core/tests/integration/test_branch_action.rs new file mode 100644 index 00000000..66c19cae --- /dev/null +++ b/crates/sandlock-core/tests/integration/test_branch_action.rs @@ -0,0 +1,203 @@ +//! Every run reports its change set, and `BranchAction::Defer` hands the +//! disposition to the caller instead of `Drop`. + +use sandlock_core::sandbox::BranchAction; +use sandlock_core::{ChangeKind, PreserveReason, Sandbox}; +use std::fs; +use std::path::{Path, PathBuf}; + +fn temp_dir(name: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!("sandlock-test-branch-{}-{}", name, std::process::id())); + let _ = fs::remove_dir_all(&dir); + let _ = fs::create_dir_all(&dir); + dir +} + +fn policy(workdir: &Path, storage: &Path) -> sandlock_core::SandboxBuilder { + Sandbox::builder() + .fs_read("/usr").fs_read("/lib").fs_read_if_exists("/lib64").fs_read("/bin").fs_read("/etc") + .fs_read("/proc") + .fs_write(workdir).workdir(workdir).cwd(workdir) + .fs_storage(storage) +} + +#[tokio::test] +async fn abort_reports_added_file_without_creating_it() { + let workdir = temp_dir("abort-add-wd"); + let storage = temp_dir("abort-add-st"); + let mut sb = policy(&workdir, &storage).on_exit(BranchAction::Abort).build().unwrap(); + + let result = sb.run(&["sh", "-c", "echo created > new.txt"]).await.unwrap(); + assert!(result.success(), "stderr={}", result.stderr_str().unwrap_or("")); + drop(sb); + + assert!(!workdir.join("new.txt").exists(), "aborted run must not write the workdir"); + assert!(result.changes.iter().any(|c| c.kind == ChangeKind::Added && c.path == Path::new("new.txt"))); + let _ = fs::remove_dir_all(&workdir); + let _ = fs::remove_dir_all(&storage); +} + +#[tokio::test] +async fn abort_reports_modified_file_without_changing_it() { + let workdir = temp_dir("abort-mod-wd"); + let storage = temp_dir("abort-mod-st"); + fs::write(workdir.join("data.txt"), "original").unwrap(); + let mut sb = policy(&workdir, &storage).on_exit(BranchAction::Abort).build().unwrap(); + + let result = sb.run(&["sh", "-c", "echo modified > data.txt"]).await.unwrap(); + assert!(result.success(), "stderr={}", result.stderr_str().unwrap_or("")); + drop(sb); + + assert_eq!(fs::read_to_string(workdir.join("data.txt")).unwrap(), "original"); + assert!(result.changes.iter().any(|c| c.kind == ChangeKind::Modified && c.path == Path::new("data.txt"))); + let _ = fs::remove_dir_all(&workdir); + let _ = fs::remove_dir_all(&storage); +} + +#[tokio::test] +async fn abort_reports_deleted_file_without_removing_it() { + let workdir = temp_dir("abort-del-wd"); + let storage = temp_dir("abort-del-st"); + fs::write(workdir.join("victim.txt"), "delete me").unwrap(); + let mut sb = policy(&workdir, &storage).on_exit(BranchAction::Abort).build().unwrap(); + + let result = sb.run(&["sh", "-c", "rm victim.txt"]).await.unwrap(); + assert!(result.success(), "stderr={}", result.stderr_str().unwrap_or("")); + drop(sb); + + assert!(workdir.join("victim.txt").exists()); + assert!(result.changes.iter().any(|c| c.kind == ChangeKind::Deleted && c.path == Path::new("victim.txt"))); + let _ = fs::remove_dir_all(&workdir); + let _ = fs::remove_dir_all(&storage); +} + +#[tokio::test] +async fn commit_reports_the_changes_it_merged() { + let workdir = temp_dir("commit-wd"); + let storage = temp_dir("commit-st"); + let mut sb = policy(&workdir, &storage).build().unwrap(); + + let result = sb.run(&["sh", "-c", "echo hi > out.txt"]).await.unwrap(); + assert!(result.success(), "stderr={}", result.stderr_str().unwrap_or("")); + drop(sb); + + assert_eq!(fs::read_to_string(workdir.join("out.txt")).unwrap(), "hi\n"); + assert!(result.changes.iter().any(|c| c.kind == ChangeKind::Added && c.path == Path::new("out.txt"))); + let _ = fs::remove_dir_all(&workdir); + let _ = fs::remove_dir_all(&storage); +} + +#[tokio::test] +async fn run_without_workdir_reports_no_changes() { + let mut sb = Sandbox::builder() + .fs_read("/usr").fs_read("/lib").fs_read_if_exists("/lib64").fs_read("/bin").fs_read("/etc") + .build() + .unwrap(); + let result = sb.run(&["true"]).await.unwrap(); + assert!(result.success()); + assert!(result.changes.is_empty()); + assert!(!sb.pending()); +} + +#[tokio::test] +async fn defer_holds_the_branch_until_commit() { + let workdir = temp_dir("defer-commit-wd"); + let storage = temp_dir("defer-commit-st"); + let mut sb = policy(&workdir, &storage).on_exit(BranchAction::Defer).build().unwrap(); + + let result = sb.run(&["sh", "-c", "echo hi > out.txt"]).await.unwrap(); + assert!(result.success(), "stderr={}", result.stderr_str().unwrap_or("")); + + assert!(sb.pending()); + assert!(!workdir.join("out.txt").exists(), "nothing lands before the caller decides"); + let upper = sb.upper_dir().expect("a pending branch has an upper").to_path_buf(); + assert_eq!(fs::read_to_string(upper.join("out.txt")).unwrap(), "hi\n"); + + sb.commit().unwrap(); + assert!(!sb.pending()); + assert!(sb.upper_dir().is_none()); + assert_eq!(fs::read_to_string(workdir.join("out.txt")).unwrap(), "hi\n"); + drop(sb); + assert!(sandlock_core::list_preserved(&storage).is_empty()); + let _ = fs::remove_dir_all(&workdir); + let _ = fs::remove_dir_all(&storage); +} + +#[tokio::test] +async fn defer_then_abort_discards() { + let workdir = temp_dir("defer-abort-wd"); + let storage = temp_dir("defer-abort-st"); + let mut sb = policy(&workdir, &storage).on_exit(BranchAction::Defer).build().unwrap(); + + let result = sb.run(&["sh", "-c", "echo hi > out.txt"]).await.unwrap(); + assert!(result.success(), "stderr={}", result.stderr_str().unwrap_or("")); + assert!(sb.pending()); + + sb.abort().unwrap(); + assert!(!sb.pending()); + assert!(!workdir.join("out.txt").exists()); + drop(sb); + assert!(sandlock_core::list_preserved(&storage).is_empty()); + let _ = fs::remove_dir_all(&workdir); + let _ = fs::remove_dir_all(&storage); +} + +#[tokio::test] +async fn defer_dropped_undecided_preserves_the_branch() { + let workdir = temp_dir("defer-drop-wd"); + let storage = temp_dir("defer-drop-st"); + { + let mut sb = policy(&workdir, &storage).on_exit(BranchAction::Defer).build().unwrap(); + let result = sb.run(&["sh", "-c", "echo hi > out.txt"]).await.unwrap(); + assert!(result.success(), "stderr={}", result.stderr_str().unwrap_or("")); + assert!(sb.pending()); + } + + assert!(!workdir.join("out.txt").exists()); + let preserved = sandlock_core::list_preserved(&storage); + assert_eq!(preserved.len(), 1); + assert_eq!(preserved[0].reason, PreserveReason::Kept); + assert_eq!(fs::read_to_string(preserved[0].upper.join("out.txt")).unwrap(), "hi\n"); + let _ = fs::remove_dir_all(&workdir); + let _ = fs::remove_dir_all(&storage); +} + +#[tokio::test] +async fn defer_on_exit_still_aborts_a_failed_run() { + let workdir = temp_dir("defer-fail-wd"); + let storage = temp_dir("defer-fail-st"); + let mut sb = policy(&workdir, &storage) + .on_exit(BranchAction::Defer) + .on_error(BranchAction::Abort) + .build() + .unwrap(); + + let result = sb.run(&["sh", "-c", "echo hi > out.txt; exit 3"]).await.unwrap(); + assert_eq!(result.code(), Some(3)); + assert!(result.changes.iter().any(|c| c.path == Path::new("out.txt"))); + + assert!(!sb.pending()); + assert!(sb.commit().is_err()); + drop(sb); + assert!(!workdir.join("out.txt").exists()); + assert!(sandlock_core::list_preserved(&storage).is_empty()); + let _ = fs::remove_dir_all(&workdir); + let _ = fs::remove_dir_all(&storage); +} + +#[tokio::test] +async fn commit_and_abort_need_a_pending_branch() { + let workdir = temp_dir("not-pending-wd"); + let storage = temp_dir("not-pending-st"); + let mut sb = policy(&workdir, &storage).build().unwrap(); + + assert!(sb.commit().is_err(), "nothing ran yet"); + assert!(sb.abort().is_err()); + + let result = sb.run(&["true"]).await.unwrap(); + assert!(result.success()); + assert!(sb.commit().is_err(), "Commit disposed the branch in wait()"); + assert!(sb.abort().is_err()); + let _ = fs::remove_dir_all(&workdir); + let _ = fs::remove_dir_all(&storage); +} diff --git a/crates/sandlock-core/tests/integration/test_cow.rs b/crates/sandlock-core/tests/integration/test_cow.rs index af6f7b8b..09f7d22e 100644 --- a/crates/sandlock-core/tests/integration/test_cow.rs +++ b/crates/sandlock-core/tests/integration/test_cow.rs @@ -1036,10 +1036,9 @@ fn cow_sandbox(workdir: &std::path::Path, on_exit: BranchAction) -> SandboxBuild /// A merge that STARTS and then fails leaves the change set on disk under a /// `MergeInterrupted` marker — the record that the workdir may have been touched -/// and must be reconciled, not merely re-committed. On the plain-`Sandbox` path -/// this is the only trace: the disposition runs in `Drop` and discards the -/// `commit()` error, so the caller — already holding its `RunResult` and -/// reporting a successful run — is never told. +/// and must be reconciled, not merely re-committed. `Defer` is what lets the +/// obstruction be planted between the run and its commit, and it is also the +/// one path where the caller is told: `Sandbox::commit` returns the error. /// /// The failure is planted so it lands INSIDE the merge, not at the lock: the /// workdir stays intact (so the commit flock opens) but a symlink sits where the @@ -1056,7 +1055,7 @@ async fn test_failed_merge_on_the_drop_path_leaves_the_upper_recoverable() { } { - let mut sb = cow_sandbox(&workdir, BranchAction::Commit) + let mut sb = cow_sandbox(&workdir, BranchAction::Defer) .fs_storage(&storage) .build() .unwrap(); @@ -1072,7 +1071,8 @@ async fn test_failed_merge_on_the_drop_path_leaves_the_upper_recoverable() { // by then. The child wrote `added.txt` into the upper (COW), so the base // path is free to plant here without the child having followed it. std::os::unix::fs::symlink("/dev/null", workdir.join("added.txt")).unwrap(); - // Dropped here: `Drop` commits, the merge fails, and the error is lost. + assert!(sb.commit().is_err(), "the obstructed merge must report its failure"); + assert!(!sb.pending(), "a failed commit disposes the branch by preserving it"); } let preserved = sandlock_core::list_preserved(&storage); diff --git a/crates/sandlock-core/tests/integration/test_dry_run.rs b/crates/sandlock-core/tests/integration/test_dry_run.rs deleted file mode 100644 index fe843789..00000000 --- a/crates/sandlock-core/tests/integration/test_dry_run.rs +++ /dev/null @@ -1,102 +0,0 @@ -use sandlock_core::{Sandbox}; -use sandlock_core::dry_run::ChangeKind; -use std::fs; -use std::path::PathBuf; - -fn temp_dir(name: &str) -> PathBuf { - let dir = std::env::temp_dir().join(format!("sandlock-test-dryrun-{}-{}", name, std::process::id())); - let _ = fs::create_dir_all(&dir); - dir -} - -#[tokio::test] -async fn test_dry_run_reports_added_file() { - let workdir = temp_dir("add"); - fs::write(workdir.join("existing.txt"), "hello").unwrap(); - - let policy = Sandbox::builder() - .fs_read("/usr").fs_read("/lib").fs_read_if_exists("/lib64").fs_read("/bin").fs_read("/etc") - .fs_read("/proc").fs_read("/dev") - .fs_write(&workdir) - .workdir(&workdir) - .build() - .unwrap(); - - let new_file = workdir.join("new.txt"); - let cmd = format!("echo created > {}", new_file.display()); - let result = policy.clone().dry_run(&["sh", "-c", &cmd]).await; - match result { - Ok(dr) => { - assert!(dr.run_result.success()); - assert!(!new_file.exists(), "new.txt should not exist after dry-run"); - let added: Vec<_> = dr.changes.iter() - .filter(|c| c.kind == ChangeKind::Added) - .collect(); - assert!(!added.is_empty(), "should report added file"); - } - Err(e) => eprintln!("Dry-run test skipped: {}", e), - } - - let _ = fs::remove_dir_all(&workdir); -} - -#[tokio::test] -async fn test_dry_run_reports_modified_file() { - let workdir = temp_dir("modify"); - fs::write(workdir.join("data.txt"), "original").unwrap(); - - let policy = Sandbox::builder() - .fs_read("/usr").fs_read("/lib").fs_read_if_exists("/lib64").fs_read("/bin").fs_read("/etc") - .fs_read("/proc").fs_read("/dev") - .fs_write(&workdir) - .workdir(&workdir) - .build() - .unwrap(); - - let cmd = format!("echo modified > {}/data.txt", workdir.display()); - let result = policy.clone().dry_run(&["sh", "-c", &cmd]).await; - match result { - Ok(dr) => { - assert!(dr.run_result.success()); - let content = fs::read_to_string(workdir.join("data.txt")).unwrap(); - assert_eq!(content, "original", "data.txt should be unchanged after dry-run"); - let modified: Vec<_> = dr.changes.iter() - .filter(|c| c.kind == ChangeKind::Modified) - .collect(); - assert!(!modified.is_empty(), "should report modified file"); - } - Err(e) => eprintln!("Dry-run test skipped: {}", e), - } - - let _ = fs::remove_dir_all(&workdir); -} - -#[tokio::test] -async fn test_dry_run_reports_deleted_file() { - let workdir = temp_dir("delete"); - fs::write(workdir.join("victim.txt"), "delete me").unwrap(); - - let policy = Sandbox::builder() - .fs_read("/usr").fs_read("/lib").fs_read_if_exists("/lib64").fs_read("/bin").fs_read("/etc") - .fs_read("/proc").fs_read("/dev") - .fs_write(&workdir) - .workdir(&workdir) - .build() - .unwrap(); - - let cmd = format!("rm {}/victim.txt", workdir.display()); - let result = policy.clone().dry_run(&["sh", "-c", &cmd]).await; - match result { - Ok(dr) => { - assert!(dr.run_result.success()); - assert!(workdir.join("victim.txt").exists(), "victim.txt should still exist after dry-run"); - let deleted: Vec<_> = dr.changes.iter() - .filter(|c| c.kind == ChangeKind::Deleted) - .collect(); - assert!(!deleted.is_empty(), "should report deleted file"); - } - Err(e) => eprintln!("Dry-run test skipped: {}", e), - } - - let _ = fs::remove_dir_all(&workdir); -} diff --git a/crates/sandlock-core/tests/integration/test_handlers.rs b/crates/sandlock-core/tests/integration/test_handlers.rs index 227ddee6..e6d0c30b 100644 --- a/crates/sandlock-core/tests/integration/test_handlers.rs +++ b/crates/sandlock-core/tests/integration/test_handlers.rs @@ -67,7 +67,7 @@ fn base_policy() -> sandlock_core::SandboxBuilder { // refuse to add the rule and the child exits before completing // confinement, surfacing as `pipe closed before 4 bytes read` // in the parent. Mirrors the convention used in upstream - // `test_dry_run`, `test_fork`, `test_netlink_virt`, `test_landlock`. + // `test_branch_action`, `test_fork`, `test_netlink_virt`, `test_landlock`. Sandbox::builder() .fs_read("/usr") .fs_read("/lib") diff --git a/crates/sandlock-core/tests/integration/test_transaction.rs b/crates/sandlock-core/tests/integration/test_transaction.rs index 9b4b13ec..43c63a71 100644 --- a/crates/sandlock-core/tests/integration/test_transaction.rs +++ b/crates/sandlock-core/tests/integration/test_transaction.rs @@ -1529,115 +1529,6 @@ async fn test_txn_dry_run_reports_the_failure_that_stopped_it_not_dry_run() { let _ = fs::remove_dir_all(&wd_t); } -/// `Sandbox::dry_run` overrides the caller's `on_exit`/`on_error`, so a dry run -/// cannot leave its upper on disk even when it is ABANDONED. -/// -/// `Keep` is the case the override exists for. On the ordinary path `dry_run` -/// aborts the branch itself, so the setting makes no difference; what it changes -/// is `keep_if_abandoned`, which asks the branch to survive a run that never -/// reaches its disposition. For a dry run that is a pure leak — the change set -/// is read out and returned, so nothing is left to recover — and the storage -/// would grow by one branch, with a `PRESERVED` marker inviting a sweep to merge -/// it, every time a dry run is cancelled. -/// -/// The abandonment is the shape a caller reaches by construction: `dry_run` -/// takes no timeout, so bounding it means wrapping it in one. -#[tokio::test] -async fn test_sandbox_dry_run_overrides_keep_so_an_abandoned_dry_run_leaves_no_upper() { - if !sandbox_available().await { - eprintln!("dry-run override test skipped: sandbox unavailable"); - return; - } - let workdir = temp_dir("dry-keep-wd"); - let storage = temp_dir("dry-keep-st"); - let mut sb = Sandbox::builder() - .fs_read("/usr").fs_read("/lib").fs_read_if_exists("/lib64").fs_read("/bin").fs_read("/etc") - .fs_read("/proc") - .fs_write(&workdir).workdir(&workdir).cwd(&workdir) - .fs_storage(&storage) - .on_exit(BranchAction::Keep) - .build() - .unwrap(); - - // Abandon the dry run once its write is in the upper, so there really is a - // change set that `Keep` could have asked to survive. - { - let dry = sb.dry_run(&["sh", "-c", "echo plan > a.txt && sleep 30"]); - tokio::pin!(dry); - let deadline = std::time::Instant::now() + Duration::from_secs(20); - while !upper_holds(&storage, "a.txt") { - tokio::select! { - early = &mut dry => panic!("the dry run finished before it could be abandoned: {early:?}"), - _ = tokio::time::sleep(Duration::from_millis(20)) => {} - } - assert!(std::time::Instant::now() < deadline, "the dry run never wrote into its upper"); - } - } // <-- the dry-run future is dropped here: the run is abandoned. - drop(sb); - - let deadline = std::time::Instant::now() + Duration::from_secs(20); - while branch_count(&storage) != 0 { - assert!( - std::time::Instant::now() < deadline, - "an abandoned dry run must not keep its upper, but {} branch(es) remain in {}", - branch_count(&storage), - storage.display(), - ); - tokio::time::sleep(Duration::from_millis(50)).await; - } - assert!( - sandlock_core::list_preserved(&storage).is_empty(), - "a dry run must not leave a marker inviting a sweep to merge its discarded changes", - ); - assert!(!workdir.join("a.txt").exists(), "a dry run must never merge"); - - let _ = fs::remove_dir_all(&workdir); - let _ = fs::remove_dir_all(&storage); -} - -/// The dry-run override is a mutation of the sandbox, not a per-call setting: a -/// `Sandbox` that has been dry-run carries `on_exit`/`on_error == Abort` -/// afterwards, and is therefore REJECTED as a transaction stage. -/// -/// This pins a sharp edge rather than approving of it. The caller never set a -/// branch action, so the rejection message — "stage 1 sets on_exit/on_error ... -/// leave them at their defaults" — accuses them of something they did not do. -#[tokio::test] -async fn test_txn_rejects_a_stage_policy_that_was_previously_dry_run() { - if !sandbox_available().await { - eprintln!("dry-run reuse test skipped: sandbox unavailable"); - return; - } - let workdir = temp_dir("dry-reuse"); - let mut policy = stage_policy(&workdir); - // A transaction built from this policy is valid before the dry run... - assert!( - Transaction::new([Stage::new(&policy, &["true"]), Stage::new(&policy, &["true"])]) - .run(None) - .await - .is_ok(), - "the policy must be a valid transaction stage to begin with", - ); - - policy.dry_run(&["true"]).await.expect("dry run should run"); - - // ...and rejected after it, with nothing about the stage list having changed. - let err = Transaction::new([Stage::new(&policy, &["true"]), Stage::new(&policy, &["true"])]) - .run(None) - .await - .expect_err("a dry-run sandbox carries on_exit=Abort and is no longer a valid stage"); - assert!( - matches!(err, TxnError::Invalid(_)), - "the rejection is a configuration error, got: {err:?}", - ); - assert!( - err.to_string().contains("on_exit/on_error"), - "the branch-action guardrail is what fires, got: {err}", - ); - - let _ = fs::remove_dir_all(&workdir); -} - /// `run`'s `timeout` bounds the STAGE phase only. A commit that has to wait out /// another transaction's merge is not cut short by it — the stages have all /// succeeded and the work is mergeable, so the wait is governed by From 62711ee97f7dd037ab42bfd4bed0b759add40186 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 9 Sep 2026 16:57:19 -0700 Subject: [PATCH 2/6] cli: rewire --dry-run onto the abort action Sandbox::dry_run is gone from the core: a dry run is a run whose branch actions are both Abort, and every RunResult now carries the change list. The flag keeps its behaviour and output format, it just sets the actions and prints result.changes instead of calling a dedicated entry point. Signed-off-by: Cong Wang --- crates/sandlock-cli/src/main.rs | 41 +++++++++++---------------- crates/sandlock-cli/tests/cli_test.rs | 25 ++++++++++++++++ 2 files changed, 41 insertions(+), 25 deletions(-) diff --git a/crates/sandlock-cli/src/main.rs b/crates/sandlock-cli/src/main.rs index 1885549b..d4ffdf7a 100644 --- a/crates/sandlock-cli/src/main.rs +++ b/crates/sandlock-cli/src/main.rs @@ -718,35 +718,15 @@ async fn run_command(args: RunArgs) -> Result { policy }; - let result = if args.dry_run { + if args.dry_run { if policy.workdir.is_none() { return Err(anyhow!("--dry-run requires --workdir")); } - let dr = if let Some(secs) = args.timeout { - match tokio::time::timeout( - std::time::Duration::from_secs(secs), - policy.dry_run_interactive(&cmd_strs), - ).await { - Ok(r) => r?, - Err(_) => { - eprintln!("sandlock: timeout after {}s", secs); - return Ok(124); - } - } - } else { - policy.dry_run_interactive(&cmd_strs).await? - }; + policy.on_exit = BranchAction::Abort; + policy.on_error = BranchAction::Abort; + } - if dr.changes.is_empty() { - eprintln!("sandlock: dry-run: no filesystem changes"); - } else { - eprintln!("sandlock: dry-run: filesystem changes:"); - for change in &dr.changes { - eprintln!("{}", change); - } - } - dr.run_result - } else if let Some(secs) = args.timeout { + let result = if let Some(secs) = args.timeout { match tokio::time::timeout( std::time::Duration::from_secs(secs), policy.run_interactive(&cmd_strs), @@ -761,6 +741,17 @@ async fn run_command(args: RunArgs) -> Result { policy.run_interactive(&cmd_strs).await? }; + if args.dry_run { + if result.changes.is_empty() { + eprintln!("sandlock: dry-run: no filesystem changes"); + } else { + eprintln!("sandlock: dry-run: filesystem changes:"); + for change in &result.changes { + eprintln!("{change}"); + } + } + } + if let Some(fd) = args.status_fd { use std::io::Write as _; use std::os::unix::io::FromRawFd; diff --git a/crates/sandlock-cli/tests/cli_test.rs b/crates/sandlock-cli/tests/cli_test.rs index 0e623b3d..d92fcf68 100644 --- a/crates/sandlock-cli/tests/cli_test.rs +++ b/crates/sandlock-cli/tests/cli_test.rs @@ -684,3 +684,28 @@ fn test_help_shows_ps_and_inspect() { "--help should NOT show 'list' command (renamed to 'ps')" ); } + +/// `--dry-run` is a run with both branch actions forced to `abort`: the +/// change list is reported on stderr and nothing lands in the workdir. +#[test] +fn test_dry_run_reports_changes_and_writes_nothing() { + let workdir = tempfile::tempdir().expect("tempdir"); + let sentinel = workdir.path().join("planned.txt"); + + let cmd = format!("echo planned > {}", sentinel.display()); + let output = sandlock_bin() + .args(args_for_host(&[ + "run", "--dry-run", + "-r", "/usr", "-r", "/lib", "-r", "/lib64", "-r", "/bin", "-r", "/etc", + "-w", workdir.path().to_str().unwrap(), + "--workdir", workdir.path().to_str().unwrap(), + "--", "sh", "-c", &cmd, + ])) + .output() + .expect("failed to run sandlock"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(output.status.success(), "sandlock exit={:?}, stderr: {stderr}", output.status.code()); + + assert!(!sentinel.exists(), "a dry run must not write the workdir"); + assert!(stderr.contains("A planned.txt"), "change list missing from stderr: {stderr}"); +} From 386f0d3d1548ae3376aaeb1bda262475946bf134 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 9 Sep 2026 16:57:19 -0700 Subject: [PATCH 3/6] ffi: expose deferred disposition and result changes The C ABI mirrors the core change: the branch action setters accept 3 for Defer, sandlock_result_* gains the change list accessors that the removed sandlock_dry_run_result_* type used to carry, and a handle whose run exited under Defer stays alive for sandlock_handle_commit/abort, with sandlock_handle_pending and sandlock_handle_upper_dir to inspect it. Freeing a pending handle preserves the branch, so a binding that crashes mid-decision never publishes. Header regenerated with cbindgen. Signed-off-by: Cong Wang --- Cargo.lock | 1 + crates/sandlock-ffi/Cargo.toml | 1 + crates/sandlock-ffi/include/sandlock.h | 147 ++++------- crates/sandlock-ffi/src/lib.rs | 334 ++++++++----------------- crates/sandlock-ffi/tests/defer.rs | 137 ++++++++++ 5 files changed, 297 insertions(+), 323 deletions(-) create mode 100644 crates/sandlock-ffi/tests/defer.rs diff --git a/Cargo.lock b/Cargo.lock index b1e608b2..a67705e6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1422,6 +1422,7 @@ dependencies = [ "libc", "sandlock-core", "serde_json", + "tempfile", "tokio", ] diff --git a/crates/sandlock-ffi/Cargo.toml b/crates/sandlock-ffi/Cargo.toml index eed79b4e..88d7d4eb 100644 --- a/crates/sandlock-ffi/Cargo.toml +++ b/crates/sandlock-ffi/Cargo.toml @@ -19,4 +19,5 @@ tokio = { version = "1", features = ["rt-multi-thread"] } [dev-dependencies] tokio = { version = "1", features = ["macros"] } +tempfile = "3" diff --git a/crates/sandlock-ffi/include/sandlock.h b/crates/sandlock-ffi/include/sandlock.h index 3ab0c067..0a38f53d 100644 --- a/crates/sandlock-ffi/include/sandlock.h +++ b/crates/sandlock-ffi/include/sandlock.h @@ -142,11 +142,6 @@ typedef struct sandlock_checkpoint_t sandlock_checkpoint_t; */ typedef struct sandlock_ctx_t sandlock_ctx_t; -/** - * Opaque dry-run result. - */ -typedef struct sandlock_dry_run_result_t sandlock_dry_run_result_t; - /** * Opaque handle for fork result (holds clone handles with pipes). */ @@ -404,7 +399,7 @@ sandlock_builder_t *sandlock_sandbox_builder_fs_mount_ro(sandlock_builder_t *b, /** * Set the COW branch action on successful exit. - * `action`: 0 = Commit, 1 = Abort, 2 = Keep. + * `action`: 0 = Commit, 1 = Abort, 2 = Keep, 3 = Defer. * * # Safety * `b` must be a valid builder pointer. @@ -413,7 +408,7 @@ sandlock_builder_t *sandlock_sandbox_builder_on_exit(sandlock_builder_t *b, uint /** * Set the COW branch action on error exit. - * `action`: 0 = Commit, 1 = Abort, 2 = Keep. + * `action`: 0 = Commit, 1 = Abort, 2 = Keep, 3 = Defer. * * # Safety * `b` must be a valid builder pointer. @@ -861,6 +856,42 @@ char *sandlock_handle_port_mappings(const sandlock_handle_t *h); */ void sandlock_handle_free(sandlock_handle_t *h); +/** + * Whether a `Defer` run has exited and is waiting for `sandlock_handle_commit` + * or `sandlock_handle_abort`. Freeing a pending handle preserves the branch. + * + * # Safety + * `h` must be a valid handle pointer. + */ +int sandlock_handle_pending(const sandlock_handle_t *h); + +/** + * The pending branch's upper directory, laid out like the workdir. Caller + * must free with `sandlock_string_free`; NULL when nothing is pending. + * + * # Safety + * `h` must be a valid handle pointer. + */ +char *sandlock_handle_upper_dir(const sandlock_handle_t *h); + +/** + * Merge the pending branch into the workdir. Blocks up to 5s on a contended + * workdir. Returns 0 on success, -1 when nothing is pending or the merge + * failed (a failed merge preserves the branch on disk). + * + * # Safety + * `h` must be a valid handle pointer. + */ +int sandlock_handle_commit(sandlock_handle_t *h); + +/** + * Discard the pending branch. Returns 0 on success, -1 when nothing is pending. + * + * # Safety + * `h` must be a valid handle pointer. + */ +int sandlock_handle_abort(sandlock_handle_t *h); + /** * Run a command with inherited stdio (interactive). Returns exit code. * @@ -940,115 +971,43 @@ const uint8_t *sandlock_result_stdout_bytes(const sandlock_result_t *r, uintptr_ const uint8_t *sandlock_result_stderr_bytes(const sandlock_result_t *r, uintptr_t *len); /** - * # Safety - * `r` must be null or a valid pointer from `sandlock_run`. - */ -void sandlock_result_free(sandlock_result_t *r); - -/** - * Free a string returned by `sandlock_result_stdout` or `sandlock_result_stderr`. - * - * # Safety - * `s` must be null or a pointer from a `sandlock_result_std*` function. - */ -void sandlock_string_free(char *s); - -/** - * Run a command in dry-run mode with captured stdout/stderr. - * - * # Safety - * `policy` must be a valid policy pointer. `name` may be NULL to - * auto-generate a sandbox name, or a valid NUL-terminated string. - * `argv` must point to `argc` C strings. - */ -sandlock_dry_run_result_t *sandlock_dry_run(const sandlock_sandbox_t *policy, - const char *name, - const char *const *argv, - unsigned int argc); - -/** - * Get the exit code from a dry-run result. - * - * # Safety - * `r` must be a valid dry-run result pointer. - */ -int sandlock_dry_run_result_exit_code(const sandlock_dry_run_result_t *r); - -/** - * Terminating reason of a dry-run result (parity with - * `sandlock_result_reason`). Returns `KILLED` for a null result. - * - * # Safety - * `r` must be null or a valid dry-run result pointer. - */ -sandlock_exit_reason sandlock_dry_run_result_reason(const sandlock_dry_run_result_t *r); - -/** - * Signal number for a `SIGNALED` dry-run result, or `-1` otherwise (parity - * with `sandlock_result_signal`). - * - * # Safety - * `r` must be null or a valid dry-run result pointer. - */ -int sandlock_dry_run_result_signal(const sandlock_dry_run_result_t *r); - -/** - * Check if the dry-run result indicates success. - * - * # Safety - * `r` must be a valid dry-run result pointer. - */ -bool sandlock_dry_run_result_success(const sandlock_dry_run_result_t *r); - -/** - * Get captured stdout bytes from a dry-run result. + * Number of filesystem changes the run made to its COW branch. * * # Safety - * `r` must be a valid dry-run result pointer. `len` must be a valid pointer. + * `r` must be a valid result pointer. */ -const uint8_t *sandlock_dry_run_result_stdout_bytes(const sandlock_dry_run_result_t *r, - uintptr_t *len); +uintptr_t sandlock_result_changes_len(const sandlock_result_t *r); /** - * Get captured stderr bytes from a dry-run result. + * Kind of the i-th change: 'A' (added), 'M' (modified), 'D' (deleted); 0 out of range. * * # Safety - * `r` must be a valid dry-run result pointer. `len` must be a valid pointer. + * `r` must be a valid result pointer. */ -const uint8_t *sandlock_dry_run_result_stderr_bytes(const sandlock_dry_run_result_t *r, - uintptr_t *len); +char sandlock_result_change_kind(const sandlock_result_t *r, uintptr_t i); /** - * Get the number of filesystem changes in a dry-run result. + * Workdir-relative path of the i-th change. Caller must free with + * `sandlock_string_free`; NULL out of range. * * # Safety - * `r` must be a valid dry-run result pointer. + * `r` must be a valid result pointer. */ -uintptr_t sandlock_dry_run_result_changes_len(const sandlock_dry_run_result_t *r); +char *sandlock_result_change_path(const sandlock_result_t *r, uintptr_t i); /** - * Get the kind of the i-th change: 'A' (added), 'M' (modified), 'D' (deleted). - * * # Safety - * `r` must be a valid dry-run result pointer. `i` must be < changes_len. - */ -char sandlock_dry_run_result_change_kind(const sandlock_dry_run_result_t *r, uintptr_t i); - -/** - * Get the path of the i-th change as a C string. Caller must free with `sandlock_string_free`. - * - * # Safety - * `r` must be a valid dry-run result pointer. `i` must be < changes_len. + * `r` must be null or a valid pointer from `sandlock_run`. */ -char *sandlock_dry_run_result_change_path(const sandlock_dry_run_result_t *r, uintptr_t i); +void sandlock_result_free(sandlock_result_t *r); /** - * Free a dry-run result. + * Free a string returned by `sandlock_result_stdout` or `sandlock_result_stderr`. * * # Safety - * `r` must be null or a valid dry-run result pointer. + * `s` must be null or a pointer from a `sandlock_result_std*` function. */ -void sandlock_dry_run_result_free(sandlock_dry_run_result_t *r); +void sandlock_string_free(char *s); /** * Create a new empty pipeline. diff --git a/crates/sandlock-ffi/src/lib.rs b/crates/sandlock-ffi/src/lib.rs index 4115d19e..f0c296a6 100644 --- a/crates/sandlock-ffi/src/lib.rs +++ b/crates/sandlock-ffi/src/lib.rs @@ -278,7 +278,7 @@ pub unsafe extern "C" fn sandlock_sandbox_builder_fs_mount_ro( } /// Set the COW branch action on successful exit. -/// `action`: 0 = Commit, 1 = Abort, 2 = Keep. +/// `action`: 0 = Commit, 1 = Abort, 2 = Keep, 3 = Defer. /// /// # Safety /// `b` must be a valid builder pointer. @@ -291,16 +291,12 @@ pub unsafe extern "C" fn sandlock_sandbox_builder_on_exit( return b; } let builder = *Box::from_raw(b); - let action = match action { - 1 => BranchAction::Abort, - 2 => BranchAction::Keep, - _ => BranchAction::Commit, - }; + let action = branch_action(action); Box::into_raw(Box::new(builder.on_exit(action))) } /// Set the COW branch action on error exit. -/// `action`: 0 = Commit, 1 = Abort, 2 = Keep. +/// `action`: 0 = Commit, 1 = Abort, 2 = Keep, 3 = Defer. /// /// # Safety /// `b` must be a valid builder pointer. @@ -313,12 +309,17 @@ pub unsafe extern "C" fn sandlock_sandbox_builder_on_error( return b; } let builder = *Box::from_raw(b); - let action = match action { + let action = branch_action(action); + Box::into_raw(Box::new(builder.on_error(action))) +} + +fn branch_action(discriminant: u8) -> BranchAction { + match discriminant { 1 => BranchAction::Abort, 2 => BranchAction::Keep, + 3 => BranchAction::Defer, _ => BranchAction::Commit, - }; - Box::into_raw(Box::new(builder.on_error(action))) + } } // ---------------------------------------------------------------- @@ -1465,6 +1466,67 @@ pub unsafe extern "C" fn sandlock_handle_free(h: *mut sandlock_handle_t) { } } +/// Whether a `Defer` run has exited and is waiting for `sandlock_handle_commit` +/// or `sandlock_handle_abort`. Freeing a pending handle preserves the branch. +/// +/// # Safety +/// `h` must be a valid handle pointer. +#[no_mangle] +pub unsafe extern "C" fn sandlock_handle_pending(h: *const sandlock_handle_t) -> c_int { + if h.is_null() { + return 0; + } + (*h).sandbox.pending() as c_int +} + +/// The pending branch's upper directory, laid out like the workdir. Caller +/// must free with `sandlock_string_free`; NULL when nothing is pending. +/// +/// # Safety +/// `h` must be a valid handle pointer. +#[no_mangle] +pub unsafe extern "C" fn sandlock_handle_upper_dir(h: *const sandlock_handle_t) -> *mut c_char { + if h.is_null() { + return ptr::null_mut(); + } + match (*h).sandbox.upper_dir() { + Some(p) => CString::new(p.to_string_lossy().as_bytes()).map(|s| s.into_raw()).unwrap_or(ptr::null_mut()), + None => ptr::null_mut(), + } +} + +/// Merge the pending branch into the workdir. Blocks up to 5s on a contended +/// workdir. Returns 0 on success, -1 when nothing is pending or the merge +/// failed (a failed merge preserves the branch on disk). +/// +/// # Safety +/// `h` must be a valid handle pointer. +#[no_mangle] +pub unsafe extern "C" fn sandlock_handle_commit(h: *mut sandlock_handle_t) -> c_int { + if h.is_null() { + return -1; + } + match (*h).sandbox.commit() { + Ok(()) => 0, + Err(_) => -1, + } +} + +/// Discard the pending branch. Returns 0 on success, -1 when nothing is pending. +/// +/// # Safety +/// `h` must be a valid handle pointer. +#[no_mangle] +pub unsafe extern "C" fn sandlock_handle_abort(h: *mut sandlock_handle_t) -> c_int { + if h.is_null() { + return -1; + } + match (*h).sandbox.abort() { + Ok(()) => 0, + Err(_) => -1, + } +} + /// Run a command with inherited stdio (interactive). Returns exit code. /// /// # Safety @@ -1681,254 +1743,73 @@ pub unsafe extern "C" fn sandlock_result_stderr_bytes( } } -/// # Safety -/// `r` must be null or a valid pointer from `sandlock_run`. -#[no_mangle] -pub unsafe extern "C" fn sandlock_result_free(r: *mut sandlock_result_t) { - if !r.is_null() { - drop(Box::from_raw(r)); - } -} - -/// Free a string returned by `sandlock_result_stdout` or `sandlock_result_stderr`. +/// Number of filesystem changes the run made to its COW branch. /// /// # Safety -/// `s` must be null or a pointer from a `sandlock_result_std*` function. +/// `r` must be a valid result pointer. #[no_mangle] -pub unsafe extern "C" fn sandlock_string_free(s: *mut c_char) { - if !s.is_null() { - drop(CString::from_raw(s)); - } -} - -// ---------------------------------------------------------------- -// Dry-run -// ---------------------------------------------------------------- - -/// Opaque dry-run result. -#[allow(non_camel_case_types)] -pub struct sandlock_dry_run_result_t { - _private: sandlock_core::DryRunResult, -} - -/// Run a command in dry-run mode with captured stdout/stderr. -/// -/// # Safety -/// `policy` must be a valid policy pointer. `name` may be NULL to -/// auto-generate a sandbox name, or a valid NUL-terminated string. -/// `argv` must point to `argc` C strings. -#[no_mangle] -pub unsafe extern "C" fn sandlock_dry_run( - policy: *const sandlock_sandbox_t, - name: *const c_char, - argv: *const *const c_char, - argc: c_uint, -) -> *mut sandlock_dry_run_result_t { - if policy.is_null() || argv.is_null() { - return ptr::null_mut(); - } - let policy = &(*policy)._private; - let name = match optional_name(name) { - Ok(name) => name, - Err(_) => return ptr::null_mut(), - }; - let args = read_argv(argv, argc); - let arg_refs: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); - - let mut sb = match name { - Some(ref n) => policy.clone().with_name(n.clone()), - None => policy.clone(), - }; - match with_runtime(|rt| rt.block_on(sb.dry_run(&arg_refs))) { - Some(Ok(result)) => Box::into_raw(Box::new(sandlock_dry_run_result_t { _private: result })), - _ => ptr::null_mut(), - } -} - -/// Get the exit code from a dry-run result. -/// -/// # Safety -/// `r` must be a valid dry-run result pointer. -#[no_mangle] -pub unsafe extern "C" fn sandlock_dry_run_result_exit_code( - r: *const sandlock_dry_run_result_t, -) -> c_int { - if r.is_null() { - return -1; - } - (*r)._private.run_result.code().unwrap_or(-1) as c_int -} - -/// Terminating reason of a dry-run result (parity with -/// `sandlock_result_reason`). Returns `KILLED` for a null result. -/// -/// # Safety -/// `r` must be null or a valid dry-run result pointer. -#[no_mangle] -pub unsafe extern "C" fn sandlock_dry_run_result_reason( - r: *const sandlock_dry_run_result_t, -) -> sandlock_exit_reason_t { - if r.is_null() { - return sandlock_exit_reason_t::Killed; - } - exit_reason(&(*r)._private.run_result.exit_status) -} - -/// Signal number for a `SIGNALED` dry-run result, or `-1` otherwise (parity -/// with `sandlock_result_signal`). -/// -/// # Safety -/// `r` must be null or a valid dry-run result pointer. -#[no_mangle] -pub unsafe extern "C" fn sandlock_dry_run_result_signal( - r: *const sandlock_dry_run_result_t, -) -> c_int { - if r.is_null() { - return -1; - } - exit_signal(&(*r)._private.run_result.exit_status) -} - -/// Check if the dry-run result indicates success. -/// -/// # Safety -/// `r` must be a valid dry-run result pointer. -#[no_mangle] -pub unsafe extern "C" fn sandlock_dry_run_result_success( - r: *const sandlock_dry_run_result_t, -) -> bool { - if r.is_null() { - return false; - } - (*r)._private.run_result.success() -} - -/// Get captured stdout bytes from a dry-run result. -/// -/// # Safety -/// `r` must be a valid dry-run result pointer. `len` must be a valid pointer. -#[no_mangle] -pub unsafe extern "C" fn sandlock_dry_run_result_stdout_bytes( - r: *const sandlock_dry_run_result_t, - len: *mut usize, -) -> *const u8 { - if r.is_null() { - if !len.is_null() { - *len = 0; - } - return ptr::null(); - } - match &(*r)._private.run_result.stdout { - Some(v) => { - *len = v.len(); - v.as_ptr() - } - None => { - *len = 0; - ptr::null() - } - } -} - -/// Get captured stderr bytes from a dry-run result. -/// -/// # Safety -/// `r` must be a valid dry-run result pointer. `len` must be a valid pointer. -#[no_mangle] -pub unsafe extern "C" fn sandlock_dry_run_result_stderr_bytes( - r: *const sandlock_dry_run_result_t, - len: *mut usize, -) -> *const u8 { - if r.is_null() { - if !len.is_null() { - *len = 0; - } - return ptr::null(); - } - match &(*r)._private.run_result.stderr { - Some(v) => { - *len = v.len(); - v.as_ptr() - } - None => { - *len = 0; - ptr::null() - } - } -} - -/// Get the number of filesystem changes in a dry-run result. -/// -/// # Safety -/// `r` must be a valid dry-run result pointer. -#[no_mangle] -pub unsafe extern "C" fn sandlock_dry_run_result_changes_len( - r: *const sandlock_dry_run_result_t, -) -> usize { +pub unsafe extern "C" fn sandlock_result_changes_len(r: *const sandlock_result_t) -> usize { if r.is_null() { return 0; } (*r)._private.changes.len() } -/// Get the kind of the i-th change: 'A' (added), 'M' (modified), 'D' (deleted). +/// Kind of the i-th change: 'A' (added), 'M' (modified), 'D' (deleted); 0 out of range. /// /// # Safety -/// `r` must be a valid dry-run result pointer. `i` must be < changes_len. +/// `r` must be a valid result pointer. #[no_mangle] -pub unsafe extern "C" fn sandlock_dry_run_result_change_kind( - r: *const sandlock_dry_run_result_t, - i: usize, -) -> c_char { +pub unsafe extern "C" fn sandlock_result_change_kind(r: *const sandlock_result_t, i: usize) -> c_char { if r.is_null() { return 0; } let changes = &(*r)._private.changes; - if i >= changes.len() { - return 0; - } - use sandlock_core::ChangeKind; - match changes[i].kind { - ChangeKind::Added => b'A' as c_char, - ChangeKind::Modified => b'M' as c_char, - ChangeKind::Deleted => b'D' as c_char, + match changes.get(i).map(|c| &c.kind) { + Some(sandlock_core::ChangeKind::Added) => b'A' as c_char, + Some(sandlock_core::ChangeKind::Modified) => b'M' as c_char, + Some(sandlock_core::ChangeKind::Deleted) => b'D' as c_char, + None => 0, } } -/// Get the path of the i-th change as a C string. Caller must free with `sandlock_string_free`. +/// Workdir-relative path of the i-th change. Caller must free with +/// `sandlock_string_free`; NULL out of range. /// /// # Safety -/// `r` must be a valid dry-run result pointer. `i` must be < changes_len. +/// `r` must be a valid result pointer. #[no_mangle] -pub unsafe extern "C" fn sandlock_dry_run_result_change_path( - r: *const sandlock_dry_run_result_t, - i: usize, -) -> *mut c_char { +pub unsafe extern "C" fn sandlock_result_change_path(r: *const sandlock_result_t, i: usize) -> *mut c_char { if r.is_null() { return ptr::null_mut(); } let changes = &(*r)._private.changes; - if i >= changes.len() { - return ptr::null_mut(); - } - let path = changes[i].path.to_string_lossy(); - match CString::new(path.as_bytes()) { - Ok(cs) => cs.into_raw(), - Err(_) => ptr::null_mut(), + match changes.get(i) { + Some(c) => CString::new(c.path.to_string_lossy().as_bytes()).map(|s| s.into_raw()).unwrap_or(ptr::null_mut()), + None => ptr::null_mut(), } } -/// Free a dry-run result. -/// /// # Safety -/// `r` must be null or a valid dry-run result pointer. +/// `r` must be null or a valid pointer from `sandlock_run`. #[no_mangle] -pub unsafe extern "C" fn sandlock_dry_run_result_free(r: *mut sandlock_dry_run_result_t) { +pub unsafe extern "C" fn sandlock_result_free(r: *mut sandlock_result_t) { if !r.is_null() { drop(Box::from_raw(r)); } } +/// Free a string returned by `sandlock_result_stdout` or `sandlock_result_stderr`. +/// +/// # Safety +/// `s` must be null or a pointer from a `sandlock_result_std*` function. +#[no_mangle] +pub unsafe extern "C" fn sandlock_string_free(s: *mut c_char) { + if !s.is_null() { + drop(CString::from_raw(s)); + } +} + // ---------------------------------------------------------------- // Pipeline // ---------------------------------------------------------------- @@ -2914,8 +2795,8 @@ mod tests { use sandlock_core::policy_fn::Verdict; use super::{ - exit_reason, exit_signal, sandlock_dry_run_result_reason, sandlock_dry_run_result_signal, - sandlock_exit_reason_t, sandlock_result_reason, sandlock_result_signal, + exit_reason, exit_signal, sandlock_exit_reason_t, sandlock_result_reason, + sandlock_result_signal, }; use sandlock_core::ExitStatus; @@ -2941,11 +2822,6 @@ mod tests { sandlock_exit_reason_t::Killed )); assert_eq!(sandlock_result_signal(std::ptr::null()), -1); - assert!(matches!( - sandlock_dry_run_result_reason(std::ptr::null()), - sandlock_exit_reason_t::Killed - )); - assert_eq!(sandlock_dry_run_result_signal(std::ptr::null()), -1); } } diff --git a/crates/sandlock-ffi/tests/defer.rs b/crates/sandlock-ffi/tests/defer.rs new file mode 100644 index 00000000..c8ac5c9b --- /dev/null +++ b/crates/sandlock-ffi/tests/defer.rs @@ -0,0 +1,137 @@ +//! The C ABI side of `BranchAction::Defer`: a handle that outlives its wait +//! holds the change set until `sandlock_handle_commit` / `_abort`. + +use std::ffi::{CStr, CString}; +use std::os::raw::{c_char, c_int, c_uint}; +use std::path::Path; +use std::ptr; + +use sandlock_ffi::{ + sandlock_create_for_run, sandlock_handle_abort, sandlock_handle_commit, sandlock_handle_free, + sandlock_handle_pending, sandlock_handle_upper_dir, sandlock_handle_wait, + sandlock_result_change_kind, sandlock_result_change_path, sandlock_result_changes_len, + sandlock_result_free, sandlock_result_success, sandlock_sandbox_build, + sandlock_sandbox_builder_cwd, sandlock_sandbox_builder_fs_read, + sandlock_sandbox_builder_fs_storage, sandlock_sandbox_builder_fs_write, + sandlock_sandbox_builder_new, sandlock_sandbox_builder_on_exit, + sandlock_sandbox_builder_workdir, sandlock_sandbox_free, sandlock_sandbox_t, sandlock_start, + sandlock_string_free, +}; + +const DEFER: u8 = 3; + +fn build_policy(workdir: &Path, storage: &Path, on_exit: u8) -> *mut sandlock_sandbox_t { + let mut b = sandlock_sandbox_builder_new(); + for p in ["/usr", "/lib", "/lib64", "/bin", "/etc", "/proc"] { + if p == "/lib64" && !Path::new("/lib64").exists() { continue; } + let c = CString::new(p).unwrap(); + b = unsafe { sandlock_sandbox_builder_fs_read(b, c.as_ptr()) }; + } + let wd = CString::new(workdir.to_str().unwrap()).unwrap(); + let st = CString::new(storage.to_str().unwrap()).unwrap(); + unsafe { + b = sandlock_sandbox_builder_fs_write(b, wd.as_ptr()); + b = sandlock_sandbox_builder_workdir(b, wd.as_ptr()); + b = sandlock_sandbox_builder_cwd(b, wd.as_ptr()); + b = sandlock_sandbox_builder_fs_storage(b, st.as_ptr()); + b = sandlock_sandbox_builder_on_exit(b, on_exit); + } + let mut err: c_int = 0; + let policy = unsafe { sandlock_sandbox_build(b, &mut err, ptr::null_mut()) }; + assert_eq!(err, 0, "policy build failed"); + policy +} + +fn argv(cmd: &[&str]) -> (Vec, Vec<*const c_char>) { + let owned: Vec = cmd.iter().map(|s| CString::new(*s).unwrap()).collect(); + let ptrs: Vec<*const c_char> = owned.iter().map(|c| c.as_ptr()).collect(); + (owned, ptrs) +} + +fn run_deferred(workdir: &Path, storage: &Path) -> *mut sandlock_ffi::sandlock_handle_t { + let policy = build_policy(workdir, storage, DEFER); + let (_owned, av) = argv(&["sh", "-c", "echo hi > out.txt"]); + let h = unsafe { sandlock_create_for_run(policy, ptr::null(), av.as_ptr(), av.len() as c_uint) }; + unsafe { sandlock_sandbox_free(policy) }; + assert!(!h.is_null(), "create failed"); + assert_eq!(unsafe { sandlock_start(h) }, 0); + let r = unsafe { sandlock_handle_wait(h) }; + assert!(!r.is_null(), "wait failed"); + assert!(unsafe { sandlock_result_success(r) }); + + assert_eq!(unsafe { sandlock_result_changes_len(r) }, 1); + assert_eq!(unsafe { sandlock_result_change_kind(r, 0) } as u8, b'A'); + let p = unsafe { sandlock_result_change_path(r, 0) }; + assert_eq!(unsafe { CStr::from_ptr(p) }.to_str().unwrap(), "out.txt"); + unsafe { sandlock_string_free(p) }; + unsafe { sandlock_result_free(r) }; + h +} + +#[test] +fn deferred_handle_commits_on_request() { + let workdir = tempfile::tempdir().unwrap(); + let storage = tempfile::tempdir().unwrap(); + let h = run_deferred(workdir.path(), storage.path()); + + assert_eq!(unsafe { sandlock_handle_pending(h) }, 1); + let upper = unsafe { sandlock_handle_upper_dir(h) }; + assert!(!upper.is_null()); + let upper_path = unsafe { CStr::from_ptr(upper) }.to_str().unwrap().to_string(); + unsafe { sandlock_string_free(upper) }; + assert_eq!(std::fs::read_to_string(Path::new(&upper_path).join("out.txt")).unwrap(), "hi\n"); + assert!(!workdir.path().join("out.txt").exists()); + + assert_eq!(unsafe { sandlock_handle_commit(h) }, 0); + assert_eq!(unsafe { sandlock_handle_pending(h) }, 0); + assert!(unsafe { sandlock_handle_upper_dir(h) }.is_null()); + assert_eq!(std::fs::read_to_string(workdir.path().join("out.txt")).unwrap(), "hi\n"); + assert_ne!(unsafe { sandlock_handle_commit(h) }, 0, "nothing left to commit"); + unsafe { sandlock_handle_free(h) }; +} + +#[test] +fn deferred_handle_aborts_on_request() { + let workdir = tempfile::tempdir().unwrap(); + let storage = tempfile::tempdir().unwrap(); + let h = run_deferred(workdir.path(), storage.path()); + + assert_eq!(unsafe { sandlock_handle_abort(h) }, 0); + assert_eq!(unsafe { sandlock_handle_pending(h) }, 0); + assert!(!workdir.path().join("out.txt").exists()); + unsafe { sandlock_handle_free(h) }; + assert!(sandlock_core::list_preserved(storage.path()).is_empty()); +} + +#[test] +fn freeing_a_pending_handle_preserves_the_branch() { + let workdir = tempfile::tempdir().unwrap(); + let storage = tempfile::tempdir().unwrap(); + let h = run_deferred(workdir.path(), storage.path()); + unsafe { sandlock_handle_free(h) }; + + assert!(!workdir.path().join("out.txt").exists()); + let preserved = sandlock_core::list_preserved(storage.path()); + assert_eq!(preserved.len(), 1); + assert_eq!(preserved[0].reason, sandlock_core::PreserveReason::Kept); +} + +#[test] +fn a_committing_handle_is_never_pending() { + let workdir = tempfile::tempdir().unwrap(); + let storage = tempfile::tempdir().unwrap(); + let policy = build_policy(workdir.path(), storage.path(), 0); + let (_owned, av) = argv(&["sh", "-c", "echo hi > out.txt"]); + let h = unsafe { sandlock_create_for_run(policy, ptr::null(), av.as_ptr(), av.len() as c_uint) }; + unsafe { sandlock_sandbox_free(policy) }; + assert_eq!(unsafe { sandlock_start(h) }, 0); + let r = unsafe { sandlock_handle_wait(h) }; + assert!(unsafe { sandlock_result_success(r) }); + assert_eq!(unsafe { sandlock_result_changes_len(r) }, 1); + unsafe { sandlock_result_free(r) }; + + assert_eq!(unsafe { sandlock_handle_pending(h) }, 0); + assert_ne!(unsafe { sandlock_handle_abort(h) }, 0); + assert_eq!(std::fs::read_to_string(workdir.path().join("out.txt")).unwrap(), "hi\n"); + unsafe { sandlock_handle_free(h) }; +} From ed2b3e31f5087c466a96fe5b3422bdafce5a2409 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 9 Sep 2026 16:57:20 -0700 Subject: [PATCH 4/6] python: add BranchAction.DEFER and Result.changes wait() frees the native handle on its way out, and that free is what used to run the merge, so Python needs to know up front that the caller wants to decide: DEFER keeps the handle parked on the Sandbox until commit()/abort() frees it, and pending/upper_dir expose it meanwhile. A pending sandbox refuses another run, and leaving the context preserves an undecided branch. Every Result now carries changes, which makes the old dry_run() a run with on_exit=ABORT, so it and DryRunResult go away. Signed-off-by: Cong Wang --- README.md | 14 ++- docs/sandbox-reference.md | 15 +-- python/README.md | 56 ++++++---- python/src/sandlock/__init__.py | 3 +- python/src/sandlock/_sdk.py | 84 ++++++++------- python/src/sandlock/sandbox.py | 176 +++++++++++++++++--------------- python/tests/test_sandbox.py | 157 +++++++++++++++++++--------- 7 files changed, 304 insertions(+), 201 deletions(-) diff --git a/README.md b/README.md index abc2c5c2..a61de5d9 100644 --- a/README.md +++ b/README.md @@ -246,7 +246,7 @@ sandlock run --no-supervisor -r /proc -r /usr -r /lib -r /lib64 -r /bin -r /etc ### Python API ```python -from sandlock import Sandbox, confine +from sandlock import Sandbox, BranchAction, confine sandbox = Sandbox( fs_writable=["/tmp/sandbox"], @@ -286,10 +286,18 @@ sb = Sandbox(port_remap=True, fs_readable=["/usr", "/lib", "/etc"], name="api.lo confine(Sandbox(fs_readable=["/usr", "/lib"], fs_writable=["/tmp"])) # Dry-run: see what files would change, then discard -sandbox = Sandbox(fs_writable=["."], workdir=".", fs_readable=["/usr", "/lib", "/bin", "/etc"]) -result = sandbox.dry_run(["make", "build"]) +sandbox = Sandbox(fs_writable=["."], workdir=".", fs_readable=["/usr", "/lib", "/bin", "/etc"], + on_exit=BranchAction.ABORT) +result = sandbox.run(["make", "build"]) for c in result.changes: print(f"{c.kind} {c.path}") # A=added, M=modified, D=deleted + +# Defer: inspect the changes, then commit or abort +sandbox = Sandbox(fs_writable=["."], workdir=".", fs_readable=["/usr", "/lib", "/bin", "/etc"], + on_exit=BranchAction.DEFER) +result = sandbox.run(["make", "build"]) +if sandbox.pending: + sandbox.commit() if approve(result.changes, sandbox.upper_dir) else sandbox.abort() ``` ### Pipeline diff --git a/docs/sandbox-reference.md b/docs/sandbox-reference.md index 7a18966f..f98d6964 100644 --- a/docs/sandbox-reference.md +++ b/docs/sandbox-reference.md @@ -484,7 +484,8 @@ does support them, the scopes remain enforced. class BranchAction(Enum): COMMIT = "commit" # Merge branch writes into the parent branch. ABORT = "abort" # Discard all branch writes. - KEEP = "keep" # Leave the branch as-is; caller decides. + KEEP = "keep" # Leave the branch on disk for recovery tooling. + DEFER = "defer" # Hold the branch for commit() / abort(). ``` ## Result types @@ -496,16 +497,8 @@ class Change: path: str # Path relative to workdir. ``` -```python -@dataclass -class DryRunResult: - success: bool - exit_code: int - stdout: bytes - stderr: bytes - changes: list[Change] - error: str | None -``` +Every `Result` carries `changes: list[Change]`, read from the COW branch +before the branch action is applied. Empty without a `workdir`. ## Helpers diff --git a/python/README.md b/python/README.md index 3e9d6988..39b69cf4 100644 --- a/python/README.md +++ b/python/README.md @@ -234,8 +234,8 @@ Sandlock always applies its default syscall blocklist. |-----------|------|---------|-------------| | `fs_storage` | `str \| None` | `None` | Storage directory for the seccomp COW upper layer / deltas | | `max_disk` | `str \| None` | `None` | Disk quota for COW storage (e.g. `"1G"`) | -| `on_exit` | `BranchAction` | `COMMIT` | `COMMIT`, `ABORT`, or `KEEP` | -| `on_error` | `BranchAction` | `ABORT` | `COMMIT`, `ABORT`, or `KEEP` | +| `on_exit` | `BranchAction` | `COMMIT` | `COMMIT`, `ABORT`, `KEEP`, or `DEFER` | +| `on_error` | `BranchAction` | `ABORT` | `COMMIT`, `ABORT`, `KEEP`, or `DEFER` | #### Protection opt-out @@ -307,17 +307,45 @@ Raises `RuntimeError` if no child has been created. Wait for the running process to finish and return its `Result`. -#### `sandbox.dry_run(cmd, timeout=None) -> DryRunResult` +#### Inspecting and deferring COW changes -Run a command in a temporary COW layer, then discard all writes. -Returns the list of filesystem changes that would have been made. +Every `Result` from a sandbox with a `workdir` carries `changes`, the list +of files the run added, modified, or deleted in its COW branch. A dry run +is a run whose branch action is `ABORT`: ```python -result = sandbox.dry_run(["sh", "-c", "echo hi > /tmp/out.txt"]) +sandbox = Sandbox(workdir=wd, on_exit=BranchAction.ABORT, ...) +result = sandbox.run(["sh", "-c", "echo hi > out.txt"]) for change in result.changes: - print(change.kind, change.path) # "A /tmp/out.txt" + print(change.kind, change.path) # "A out.txt" ``` +`BranchAction.DEFER` leaves the branch in the sandbox after the run exits so +the caller can inspect it and decide later. While pending, `upper_dir` holds +the new bytes of every added or modified file, laid out like `workdir`: + +```python +sandbox = Sandbox(workdir=wd, on_exit=BranchAction.DEFER, ...) # on_error stays ABORT +result = sandbox.run(["python3", "tool.py"]) +if sandbox.pending: + if approve(result.changes, sandbox.upper_dir): + sandbox.commit() + else: + sandbox.abort() +``` + +- `sandbox.pending -> bool`: True between a `DEFER` run's exit and `commit()` / `abort()`. +- `sandbox.upper_dir -> str | None`: the pending branch's upper directory. +- `sandbox.commit()`: merge into `workdir`. Blocks up to 5s on a workdir + another sandbox is merging into; raises `BranchError` if the merge fails, + leaving the branch preserved on disk. Last-writer-wins against anything + that changed the workdir since the run. +- `sandbox.abort()`: discard the branch. + +A pending sandbox refuses another `run()` until it is decided. Leaving a +`with` block, or letting the sandbox go away, preserves an undecided branch +on disk the same way `KEEP` does; nothing is ever merged without a decision. + #### `sandbox.run_interactive(cmd) -> int` Run with inherited stdio (no capture). Returns the exit code. @@ -384,16 +412,7 @@ Returned by `sandbox.run()`. | `stdout` | `bytes` | Captured standard output | | `stderr` | `bytes` | Captured standard error | | `error` | `str \| None` | Error message on failure | - -### DryRunResult - -Returned by `sandbox.dry_run()`. - -Same attributes as `Result`, plus: - -| Attribute | Type | Description | -|-----------|------|-------------| -| `changes` | `list[Change]` | Filesystem changes detected | +| `changes` | `list[Change]` | Filesystem changes the run made to its COW branch (empty without `workdir`) | ### Change @@ -653,7 +672,8 @@ from sandlock import SandlockError, SandboxError, SandboxRuntimeError - `BranchAction.COMMIT` -- merge writes on exit - `BranchAction.ABORT` -- discard writes -- `BranchAction.KEEP` -- leave branch as-is +- `BranchAction.KEEP` -- leave the branch on disk for recovery tooling +- `BranchAction.DEFER` -- hold the branch for `commit()` / `abort()` ### MCP integration diff --git a/python/src/sandlock/__init__.py b/python/src/sandlock/__init__.py index 80169fe3..557593b1 100644 --- a/python/src/sandlock/__init__.py +++ b/python/src/sandlock/__init__.py @@ -15,7 +15,7 @@ from .inputs import inputs from .handler import Handler, NotifAction, HandlerCtx, ExceptionPolicy from .sandbox import ( - Sandbox, BranchAction, parse_ports, Change, DryRunResult, StdioMode, Process, + Sandbox, BranchAction, parse_ports, Change, StdioMode, Process, ) from ._profile import load_profile, list_profiles from .exceptions import ( @@ -52,7 +52,6 @@ "BranchAction", "parse_ports", "Change", - "DryRunResult", "StdioMode", "Process", "Protection", diff --git a/python/src/sandlock/_sdk.py b/python/src/sandlock/_sdk.py index 00736c27..109d9f3b 100644 --- a/python/src/sandlock/_sdk.py +++ b/python/src/sandlock/_sdk.py @@ -12,7 +12,7 @@ from pathlib import Path from typing import Any, NamedTuple, Sequence -from .sandbox import Sandbox as PolicyDataclass +from .sandbox import Change, Sandbox as PolicyDataclass # ---------------------------------------------------------------- # Load the shared library @@ -314,6 +314,18 @@ def confine(policy: "PolicyDataclass") -> None: _lib.sandlock_handle_free.restype = None _lib.sandlock_handle_free.argtypes = [_c_handle_p] +_lib.sandlock_handle_pending.restype = ctypes.c_int +_lib.sandlock_handle_pending.argtypes = [_c_handle_p] + +_lib.sandlock_handle_upper_dir.restype = ctypes.c_void_p +_lib.sandlock_handle_upper_dir.argtypes = [_c_handle_p] + +_lib.sandlock_handle_commit.restype = ctypes.c_int +_lib.sandlock_handle_commit.argtypes = [_c_handle_p] + +_lib.sandlock_handle_abort.restype = ctypes.c_int +_lib.sandlock_handle_abort.argtypes = [_c_handle_p] + _lib.sandlock_handle_port_mappings.restype = ctypes.c_char_p _lib.sandlock_handle_port_mappings.argtypes = [_c_handle_p] @@ -355,41 +367,14 @@ def confine(policy: "PolicyDataclass") -> None: _lib.sandlock_result_free.restype = None _lib.sandlock_result_free.argtypes = [_c_result_p] -# Dry-run -_c_dry_run_p = ctypes.c_void_p - -_lib.sandlock_dry_run.restype = _c_dry_run_p -_lib.sandlock_dry_run.argtypes = [_c_policy_p, ctypes.c_char_p, ctypes.POINTER(ctypes.c_char_p), ctypes.c_uint] - -_lib.sandlock_dry_run_result_exit_code.restype = ctypes.c_int -_lib.sandlock_dry_run_result_exit_code.argtypes = [_c_dry_run_p] - -_lib.sandlock_dry_run_result_reason.restype = ctypes.c_uint -_lib.sandlock_dry_run_result_reason.argtypes = [_c_dry_run_p] - -_lib.sandlock_dry_run_result_signal.restype = ctypes.c_int -_lib.sandlock_dry_run_result_signal.argtypes = [_c_dry_run_p] - -_lib.sandlock_dry_run_result_success.restype = ctypes.c_bool -_lib.sandlock_dry_run_result_success.argtypes = [_c_dry_run_p] +_lib.sandlock_result_changes_len.restype = ctypes.c_size_t +_lib.sandlock_result_changes_len.argtypes = [_c_result_p] -_lib.sandlock_dry_run_result_stdout_bytes.restype = ctypes.c_void_p -_lib.sandlock_dry_run_result_stdout_bytes.argtypes = [_c_dry_run_p, ctypes.POINTER(ctypes.c_size_t)] +_lib.sandlock_result_change_kind.restype = ctypes.c_char +_lib.sandlock_result_change_kind.argtypes = [_c_result_p, ctypes.c_size_t] -_lib.sandlock_dry_run_result_stderr_bytes.restype = ctypes.c_void_p -_lib.sandlock_dry_run_result_stderr_bytes.argtypes = [_c_dry_run_p, ctypes.POINTER(ctypes.c_size_t)] - -_lib.sandlock_dry_run_result_changes_len.restype = ctypes.c_size_t -_lib.sandlock_dry_run_result_changes_len.argtypes = [_c_dry_run_p] - -_lib.sandlock_dry_run_result_change_kind.restype = ctypes.c_char -_lib.sandlock_dry_run_result_change_kind.argtypes = [_c_dry_run_p, ctypes.c_size_t] - -_lib.sandlock_dry_run_result_change_path.restype = ctypes.c_void_p -_lib.sandlock_dry_run_result_change_path.argtypes = [_c_dry_run_p, ctypes.c_size_t] - -_lib.sandlock_dry_run_result_free.restype = None -_lib.sandlock_dry_run_result_free.argtypes = [_c_dry_run_p] +_lib.sandlock_result_change_path.restype = ctypes.c_void_p +_lib.sandlock_result_change_path.argtypes = [_c_result_p, ctypes.c_size_t] # Pipeline _lib.sandlock_pipeline_new.restype = _c_pipeline_p @@ -756,6 +741,30 @@ def _read_result_bytes(result_p, fn) -> bytes: return ctypes.string_at(ptr, length.value) +def _read_result_changes(result_p) -> list: + """Read the change list from a result pointer.""" + changes = [] + for i in range(_lib.sandlock_result_changes_len(result_p)): + kind = _lib.sandlock_result_change_kind(result_p, i).decode("ascii") + path_p = _lib.sandlock_result_change_path(result_p, i) + path = "" + if path_p: + path = ctypes.string_at(path_p).decode("utf-8", "surrogateescape") + _lib.sandlock_string_free(ctypes.cast(path_p, ctypes.c_char_p)) + changes.append(Change(kind=kind, path=path)) + return changes + + +def _read_handle_string(fn, handle) -> str | None: + """Read and free a malloc'd C string returned for a handle.""" + ptr = fn(handle) + if not ptr: + return None + value = ctypes.string_at(ptr).decode("utf-8", "surrogateescape") + _lib.sandlock_string_free(ctypes.cast(ptr, ctypes.c_char_p)) + return value + + # ---------------------------------------------------------------- # Result # ---------------------------------------------------------------- @@ -791,6 +800,9 @@ class Result: ``None`` on an error raised before a native result was produced.""" signal: int = -1 """Signal number for a ``SIGNALED`` result, else ``-1``.""" + changes: list = field(default_factory=list) + """What the run did to its COW branch (:class:`Change` entries), read + before the branch action was applied. Empty without a ``workdir``.""" # ---------------------------------------------------------------- @@ -1071,8 +1083,8 @@ def _build_from_policy(policy: PolicyDataclass): for vp, hp in (policy.fs_mount or {}).items(): b = _b_fs_mount(b, _encode(str(vp)), _encode(str(hp))) - # COW branch actions (0=Commit, 1=Abort, 2=Keep) - _action_map = {"commit": 0, "abort": 1, "keep": 2} + # COW branch actions (0=Commit, 1=Abort, 2=Keep, 3=Defer) + _action_map = {"commit": 0, "abort": 1, "keep": 2, "defer": 3} on_exit_val = policy.on_exit.value if hasattr(policy.on_exit, 'value') else str(policy.on_exit) on_error_val = policy.on_error.value if hasattr(policy.on_error, 'value') else str(policy.on_error) b = _b_on_exit(b, _action_map.get(on_exit_val, 0)) diff --git a/python/src/sandlock/sandbox.py b/python/src/sandlock/sandbox.py index c7534a63..88818932 100644 --- a/python/src/sandlock/sandbox.py +++ b/python/src/sandlock/sandbox.py @@ -25,7 +25,6 @@ if TYPE_CHECKING: from ._notif_policy import NotifPolicy - from ._sdk import ExitReason # DryRunResult.reason annotation (runtime import is circular) # --- Memory size parsing (from branching/process/limits.py) --- @@ -99,7 +98,8 @@ class BranchAction(Enum): COMMIT = "commit" # Merge writes into parent branch ABORT = "abort" # Discard all writes - KEEP = "keep" # Leave branch as-is (caller decides) + KEEP = "keep" # Leave branch on disk for recovery tooling + DEFER = "defer" # Hold the branch for commit() / abort() class StdioMode(IntEnum): @@ -127,24 +127,6 @@ class Change: """Path relative to workdir.""" -@dataclass -class DryRunResult: - """Result of a dry-run execution.""" - - success: bool - exit_code: int = 0 - stdout: bytes = field(default=b"", repr=False) - stderr: bytes = field(default=b"", repr=False) - changes: list = field(default_factory=list) - error: str | None = None - # Appended after the original fields so positional construction is unchanged. - reason: "ExitReason | None" = None - """Why the process terminated (parity with ``Result.reason``); ``None`` on an - error raised before a native result was produced.""" - signal: int = -1 - """Signal number for a ``SIGNALED`` result, else ``-1``.""" - - @dataclass class Sandbox: """Sandbox configuration and runtime handle. @@ -447,6 +429,7 @@ def __post_init__(self): # Runtime state — not dataclass fields, not serialized self._native = None # _NativePolicy created lazily on first use self._handle = None # live sandbox handle during start()/run() + self._pending = None # handle kept after a DEFER run until commit()/abort() self._process = None # weakref to the live popen() Process; it OWNS its # own handle (see `_popen_process`), this is only a # non-owning busy marker @@ -537,6 +520,8 @@ def _check_not_running(self) -> None: first handle or alias a running popen() child.""" if self._live_handle() is not None: raise RuntimeError("sandbox is already running") + if self._pending is not None: + raise RuntimeError("sandbox has a pending branch; call commit() or abort() first") def _reject_if_popen(self) -> None: """Raise if the live child is driven by a :meth:`popen` :class:`Process`. @@ -574,6 +559,8 @@ def __exit__(self, exc_type, exc_val, exc_tb): except Exception: pass self._handle = None + # Freeing an undecided branch preserves it, same as KEEP. + self._release_pending() return False # ------------------------------------------------------------------ @@ -600,6 +587,71 @@ def is_running(self) -> bool: a live :meth:`popen` :class:`Process`).""" return self._live_handle() is not None + @property + def pending(self) -> bool: + """True between a ``DEFER`` run's exit and :meth:`commit` / :meth:`abort`.""" + return self._pending is not None + + @property + def upper_dir(self) -> str | None: + """Where the pending branch keeps the new bytes of every added or + modified file, laid out like ``workdir``. ``None`` unless pending.""" + if self._pending is None: + return None + from ._sdk import _lib, _read_handle_string + return _read_handle_string(_lib.sandlock_handle_upper_dir, self._pending) + + def commit(self) -> None: + """Merge the pending branch into ``workdir``. + + Blocks up to 5s on a workdir another sandbox is merging into. Raises + :class:`BranchError` if the merge fails; the branch is then preserved + on disk. Last-writer-wins against anything that changed the workdir + since the run. + """ + from ._sdk import _lib + from .exceptions import BranchError + + handle = self._require_pending() + try: + rc = _lib.sandlock_handle_commit(handle) + finally: + self._release_pending() + if rc != 0: + raise BranchError("commit failed; the change set is preserved on disk") + + def abort(self) -> None: + """Discard the pending branch.""" + from ._sdk import _lib + + handle = self._require_pending() + try: + _lib.sandlock_handle_abort(handle) + finally: + self._release_pending() + + def _require_pending(self): + if self._pending is None: + raise RuntimeError("no pending branch; only a DEFER run that has exited has one") + return self._pending + + def _release_pending(self) -> None: + if self._pending is None: + return + from ._sdk import _lib + try: + _lib.sandlock_handle_free(self._pending) + finally: + self._pending = None + + def _park_or_free(self, handle) -> None: + """After a wait: keep a handle whose branch is deferred, free any other.""" + from ._sdk import _lib + if _lib.sandlock_handle_pending(handle): + self._pending = handle + else: + _lib.sandlock_handle_free(handle) + # ------------------------------------------------------------------ # Execution methods # ------------------------------------------------------------------ @@ -619,7 +671,7 @@ def run(self, cmd: Sequence[str], timeout: float | None = None): killed and a timeout result is returned if exceeded. None means no timeout. """ - from ._sdk import _lib, _make_argv, _read_result_bytes, Result, ExitReason + from ._sdk import _lib, _make_argv, _read_result_bytes, _read_result_changes, Result, ExitReason self._check_not_running() @@ -641,14 +693,14 @@ def run(self, cmd: Sequence[str], timeout: float | None = None): self._handle = None return Result(success=False, exit_code=-1, error="sandlock_start failed") + handle, self._handle = self._handle, None try: # None -> wait forever (0). A finite timeout clamps up to 1ms so # timeout=0 / sub-ms don't collapse to 0 (= wait forever). timeout_ms = max(1, int(timeout * 1000)) if timeout is not None else 0 - result_p = _lib.sandlock_handle_wait_timeout(self._handle, timeout_ms) + result_p = _lib.sandlock_handle_wait_timeout(handle, timeout_ms) finally: - _lib.sandlock_handle_free(self._handle) - self._handle = None + self._park_or_free(handle) if not result_p: return Result(success=False, exit_code=-1, error="sandlock_handle_wait failed") @@ -659,6 +711,7 @@ def run(self, cmd: Sequence[str], timeout: float | None = None): signal = _lib.sandlock_result_signal(result_p) stdout = _read_result_bytes(result_p, _lib.sandlock_result_stdout_bytes) stderr = _read_result_bytes(result_p, _lib.sandlock_result_stderr_bytes) + changes = _read_result_changes(result_p) _lib.sandlock_result_free(result_p) return Result( @@ -668,6 +721,7 @@ def run(self, cmd: Sequence[str], timeout: float | None = None): signal=signal, stdout=stdout, stderr=stderr, + changes=changes, ) def run_with_handlers( @@ -718,6 +772,7 @@ def run_with_handlers( _lib, _make_argv, _read_result_bytes, + _read_result_changes, Result, ExitReason, ) @@ -844,6 +899,7 @@ def run_with_handlers( signal = _lib.sandlock_result_signal(result_p) stdout = _read_result_bytes(result_p, _lib.sandlock_result_stdout_bytes) stderr = _read_result_bytes(result_p, _lib.sandlock_result_stderr_bytes) + changes = _read_result_changes(result_p) _lib.sandlock_result_free(result_p) return Result( @@ -853,6 +909,7 @@ def run_with_handlers( signal=signal, stdout=stdout, stderr=stderr, + changes=changes, ) def create(self, cmd: Sequence[str]) -> None: @@ -918,17 +975,17 @@ def wait(self): :meth:`popen` :class:`Process` (wait on that Process instead — freeing its handle here would break it). """ - from ._sdk import _lib, _read_result_bytes, Result, ExitReason + from ._sdk import _lib, _read_result_bytes, _read_result_changes, Result, ExitReason self._reject_if_popen() if self._handle is None: raise RuntimeError("sandbox is not running") + handle, self._handle = self._handle, None try: - result_p = _lib.sandlock_handle_wait_timeout(self._handle, 0) + result_p = _lib.sandlock_handle_wait_timeout(handle, 0) finally: - _lib.sandlock_handle_free(self._handle) - self._handle = None + self._park_or_free(handle) if not result_p: return Result(success=False, exit_code=-1, error="sandlock_handle_wait failed") @@ -939,6 +996,7 @@ def wait(self): signal = _lib.sandlock_result_signal(result_p) stdout = _read_result_bytes(result_p, _lib.sandlock_result_stdout_bytes) stderr = _read_result_bytes(result_p, _lib.sandlock_result_stderr_bytes) + changes = _read_result_changes(result_p) _lib.sandlock_result_free(result_p) return Result( @@ -948,6 +1006,7 @@ def wait(self): signal=signal, stdout=stdout, stderr=stderr, + changes=changes, ) def popen( @@ -1033,61 +1092,6 @@ def popen( self._process = weakref.ref(proc) return proc - def dry_run(self, cmd: Sequence[str], timeout: float | None = None) -> "DryRunResult": - """Dry-run: run a command, collect filesystem changes, then discard. - - Args: - cmd: Command and arguments to execute. - timeout: Maximum execution time in seconds. None means no timeout. - - Returns: - DryRunResult with exit info and list of filesystem changes. - """ - from ._sdk import _lib, _make_argv, _read_result_bytes, ExitReason - - native = self._ensure_native() - argv, argc = _make_argv(list(cmd)) - result_p = _lib.sandlock_dry_run( - native.ptr, _encode(self._resolve_name()), argv, argc, - ) - - if not result_p: - return DryRunResult(success=False, exit_code=-1, error="sandlock_dry_run failed") - - try: - exit_code = _lib.sandlock_dry_run_result_exit_code(result_p) - success = _lib.sandlock_dry_run_result_success(result_p) - reason = ExitReason(_lib.sandlock_dry_run_result_reason(result_p)) - signal = _lib.sandlock_dry_run_result_signal(result_p) - stdout = _read_result_bytes(result_p, _lib.sandlock_dry_run_result_stdout_bytes) - stderr = _read_result_bytes(result_p, _lib.sandlock_dry_run_result_stderr_bytes) - - import ctypes - n = _lib.sandlock_dry_run_result_changes_len(result_p) - changes = [] - for i in range(n): - kind_byte = _lib.sandlock_dry_run_result_change_kind(result_p, i) - kind = kind_byte.decode("ascii") - path_p = _lib.sandlock_dry_run_result_change_path(result_p, i) - if path_p: - path = ctypes.c_char_p(path_p).value.decode("utf-8") - _lib.sandlock_string_free(ctypes.cast(path_p, ctypes.c_char_p)) - else: - path = "" - changes.append(Change(kind=kind, path=path)) - finally: - _lib.sandlock_dry_run_result_free(result_p) - - return DryRunResult( - success=bool(success), - exit_code=exit_code, - reason=reason, - signal=signal, - stdout=stdout, - stderr=stderr, - changes=changes, - ) - def run_interactive(self, cmd: Sequence[str]) -> int: """Run with inherited stdio. Returns exit code.""" from ._sdk import _lib, _make_argv @@ -1575,7 +1579,7 @@ def wait(self, timeout: float | None = None) -> "Result": ``stdout``/``stderr`` you have not drained can block the child on a full pipe and hang the wait forever; pass a ``timeout`` or drain first. """ - from ._sdk import _lib, Result, ExitReason + from ._sdk import _lib, _read_result_changes, Result, ExitReason # Reserve the handle under the lock so a concurrent kill()/pid sees a # consistent state, then run the blocking wait WITHOUT the lock so kill() @@ -1607,7 +1611,7 @@ def wait(self, timeout: float | None = None) -> "Result": result_p = _lib.sandlock_handle_wait_timeout(handle, timeout_ms) finally: with self._lock: - _lib.sandlock_handle_free(handle) + self._sandbox._park_or_free(handle) self._handle = None self._waiting = False # Release the sandbox's busy marker so it can be reused. Guard that it @@ -1627,9 +1631,11 @@ def wait(self, timeout: float | None = None) -> "Result": signal = _lib.sandlock_result_signal(result_p) # stdout/stderr were handed to the caller as fds, so the RunResult holds # none — read them off the streams, not the Result. + changes = _read_result_changes(result_p) _lib.sandlock_result_free(result_p) self._result = Result( success=bool(success), exit_code=exit_code, reason=reason, signal=signal, + changes=changes, ) return self._result diff --git a/python/tests/test_sandbox.py b/python/tests/test_sandbox.py index e78ea1b0..2d9174da 100644 --- a/python/tests/test_sandbox.py +++ b/python/tests/test_sandbox.py @@ -14,7 +14,9 @@ import pytest -from sandlock import Sandbox, Change, DryRunResult +from pathlib import Path + +from sandlock import Sandbox, BranchAction, Change _PYTHON_READABLE = list(dict.fromkeys([ @@ -719,75 +721,137 @@ def test_resume_not_running_raises(self): sb.resume() -class TestDryRun: - """Tests for Sandbox.dry_run().""" +class TestBranchAction: + """Every run reports its changes; DEFER hands the disposition to the caller.""" - def test_dry_run_reports_added_file(self, tmp_path): + def test_abort_reports_added_file_without_creating_it(self, tmp_path): workdir = tmp_path / "add" workdir.mkdir() - (workdir / "existing.txt").write_text("hello") - - p = _policy(fs_writable=[str(workdir)], workdir=str(workdir)) - result = p.dry_run( - ["sh", "-c", f"touch {workdir}/new.txt"] - ) + p = _policy(fs_writable=[str(workdir)], workdir=str(workdir), on_exit=BranchAction.ABORT) + result = p.run(["sh", "-c", f"touch {workdir}/new.txt"]) assert result.success - assert not (workdir / "new.txt").exists(), "new.txt should not exist after dry-run" - kinds = [c.kind for c in result.changes] - assert "A" in kinds + assert not (workdir / "new.txt").exists() + assert ("A", "new.txt") in [(c.kind, c.path) for c in result.changes] - def test_dry_run_reports_modified_file(self, tmp_path): + def test_abort_reports_modified_file_without_changing_it(self, tmp_path): workdir = tmp_path / "mod" workdir.mkdir() (workdir / "data.txt").write_text("original") - - p = _policy(fs_writable=[str(workdir)], workdir=str(workdir)) - result = p.dry_run( - ["sh", "-c", f"echo changed > {workdir}/data.txt"] - ) + p = _policy(fs_writable=[str(workdir)], workdir=str(workdir), on_exit=BranchAction.ABORT) + result = p.run(["sh", "-c", f"echo changed > {workdir}/data.txt"]) assert result.success assert (workdir / "data.txt").read_text() == "original" - kinds = [c.kind for c in result.changes] - assert "M" in kinds + assert ("M", "data.txt") in [(c.kind, c.path) for c in result.changes] - def test_dry_run_reports_deleted_file(self, tmp_path): + def test_abort_reports_deleted_file_without_removing_it(self, tmp_path): workdir = tmp_path / "del" workdir.mkdir() (workdir / "victim.txt").write_text("delete me") + p = _policy(fs_writable=[str(workdir)], workdir=str(workdir), on_exit=BranchAction.ABORT) + result = p.run(["sh", "-c", f"rm {workdir}/victim.txt"]) + assert result.success + assert (workdir / "victim.txt").exists() + assert ("D", "victim.txt") in [(c.kind, c.path) for c in result.changes] + def test_commit_reports_the_changes_it_merged(self, tmp_path): + workdir = tmp_path / "commit" + workdir.mkdir() p = _policy(fs_writable=[str(workdir)], workdir=str(workdir)) - result = p.dry_run( - ["sh", "-c", f"rm {workdir}/victim.txt"] - ) + result = p.run(["sh", "-c", f"echo hi > {workdir}/out.txt"]) assert result.success - assert (workdir / "victim.txt").exists(), "file should still exist after dry-run" - kinds = [c.kind for c in result.changes] - assert "D" in kinds + assert (workdir / "out.txt").read_text() == "hi\n" + assert [c for c in result.changes if isinstance(c, Change)] == result.changes + assert ("A", "out.txt") in [(c.kind, c.path) for c in result.changes] + assert not p.pending - def test_dry_run_no_changes(self, tmp_path): + def test_run_without_changes_reports_none(self, tmp_path): workdir = tmp_path / "noop" workdir.mkdir() - p = _policy(fs_writable=[str(workdir)], workdir=str(workdir)) - result = p.dry_run(["echo", "hello"]) + result = p.run(["echo", "hello"]) assert result.success assert result.changes == [] - def test_dry_run_returns_structured_result(self, tmp_path): - workdir = tmp_path / "struct" + def test_defer_holds_the_branch_until_commit(self, tmp_path): + workdir = tmp_path / "defer-commit" workdir.mkdir() - (workdir / "f.txt").write_text("x") + p = _policy(fs_writable=[str(workdir)], workdir=str(workdir), on_exit=BranchAction.DEFER) + result = p.run(["sh", "-c", f"echo hi > {workdir}/out.txt"]) + assert result.success + assert p.pending + assert not (workdir / "out.txt").exists() + assert (Path(p.upper_dir) / "out.txt").read_text() == "hi\n" - p = _policy(fs_writable=[str(workdir)], workdir=str(workdir)) - result = p.dry_run( - ["sh", "-c", f"echo y > {workdir}/f.txt; touch {workdir}/new.txt"] + p.commit() + assert not p.pending + assert p.upper_dir is None + assert (workdir / "out.txt").read_text() == "hi\n" + + def test_defer_then_abort_discards(self, tmp_path): + workdir = tmp_path / "defer-abort" + workdir.mkdir() + p = _policy(fs_writable=[str(workdir)], workdir=str(workdir), on_exit=BranchAction.DEFER) + p.run(["sh", "-c", f"echo hi > {workdir}/out.txt"]) + assert p.pending + p.abort() + assert not p.pending + assert not (workdir / "out.txt").exists() + + def test_defer_on_exit_still_aborts_a_failed_run(self, tmp_path): + workdir = tmp_path / "defer-fail" + workdir.mkdir() + p = _policy( + fs_writable=[str(workdir)], workdir=str(workdir), + on_exit=BranchAction.DEFER, on_error=BranchAction.ABORT, ) - assert isinstance(result, DryRunResult) - assert isinstance(result.changes, list) - for c in result.changes: - assert isinstance(c, Change) - assert c.kind in ("A", "M", "D") - assert isinstance(c.path, str) + result = p.run(["sh", "-c", f"echo hi > {workdir}/out.txt; exit 3"]) + assert result.exit_code == 3 + assert ("A", "out.txt") in [(c.kind, c.path) for c in result.changes] + assert not p.pending + assert not (workdir / "out.txt").exists() + + def test_commit_and_abort_need_a_pending_branch(self, tmp_path): + workdir = tmp_path / "not-pending" + workdir.mkdir() + p = _policy(fs_writable=[str(workdir)], workdir=str(workdir)) + with pytest.raises(RuntimeError): + p.commit() + with pytest.raises(RuntimeError): + p.abort() + p.run(["true"]) + with pytest.raises(RuntimeError): + p.commit() + + def test_a_pending_sandbox_refuses_another_run(self, tmp_path): + workdir = tmp_path / "busy" + workdir.mkdir() + p = _policy(fs_writable=[str(workdir)], workdir=str(workdir), on_exit=BranchAction.DEFER) + p.run(["sh", "-c", f"echo hi > {workdir}/out.txt"]) + assert p.pending + with pytest.raises(RuntimeError): + p.run(["true"]) + p.abort() + assert p.run(["true"]).success + + def test_leaving_the_context_releases_a_pending_branch(self, tmp_path): + workdir = tmp_path / "ctx" + workdir.mkdir() + with _policy(fs_writable=[str(workdir)], workdir=str(workdir), on_exit=BranchAction.DEFER) as p: + p.run(["sh", "-c", f"echo hi > {workdir}/out.txt"]) + assert p.pending + assert not p.pending + assert not (workdir / "out.txt").exists(), "an undecided branch is never published" + + def test_spawn_wait_defers_too(self, tmp_path): + workdir = tmp_path / "spawn" + workdir.mkdir() + p = _policy(fs_writable=[str(workdir)], workdir=str(workdir), on_exit=BranchAction.DEFER) + p.spawn(["sh", "-c", f"echo hi > {workdir}/out.txt"]) + result = p.wait() + assert result.success + assert p.pending + p.commit() + assert (workdir / "out.txt").read_text() == "hi\n" class TestNewPolicyFields: @@ -990,8 +1054,8 @@ def test_quota_none_is_unlimited(self, tmp_path): ) assert result.success - def test_quota_dry_run_enforced(self, tmp_path): - """Quota applies during dry_run (COW is always active).""" + def test_quota_enforced_on_an_aborting_run(self, tmp_path): + """Quota applies whatever the branch action (COW is always active).""" workdir = tmp_path / "dryquota" workdir.mkdir() (workdir / "big.bin").write_bytes(b"\x00" * 8192) @@ -999,8 +1063,9 @@ def test_quota_dry_run_enforced(self, tmp_path): fs_writable=[str(workdir)], workdir=str(workdir), max_disk="1K", + on_exit=BranchAction.ABORT, ) - result = p.dry_run( + result = p.run( ["sh", "-c", f"echo x >> {workdir}/big.bin"] ) assert not result.success From a9e3c6bebb480b8aba2cf187df59cc2fd91e1995 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 9 Sep 2026 16:57:20 -0700 Subject: [PATCH 5/6] go: add BranchActionDefer and Result.Changes Run frees its handle before returning, so it rejects Defer outright; the disposition lives on Process, whose Wait keeps the handle when the run exited under Defer and exposes Pending/UpperDir/Commit/Abort. Close on a pending Process preserves the branch. DryRun is removed along with the C entry points it wrapped; a dry run is Run with OnExit set to Abort and the Changes now on every Result. Signed-off-by: Cong Wang --- go/README.md | 17 +++-- go/sandbox.go | 23 +++---- go/sandlock_linux.go | 141 +++++++++++++++++++++----------------- go/sandlock_linux_test.go | 104 ++++++++++++++++++++++++++-- 4 files changed, 199 insertions(+), 86 deletions(-) diff --git a/go/README.md b/go/README.md index 1db66635..f189086d 100644 --- a/go/README.md +++ b/go/README.md @@ -85,8 +85,8 @@ func main() { `Sandbox` is a plain configuration struct; every field is optional and an unset field means "no restriction" unless noted. sandlock's default syscall blocklist is always applied. A `Sandbox` carries no runtime state, so it is safe to reuse -and share across goroutines — `Run`, `RunInteractive`, and `DryRun` build a -fresh native policy on each call. +and share across goroutines: `Run` and `RunInteractive` build a fresh native +policy on each call. | Group | Fields | |---|---| @@ -117,7 +117,6 @@ with `NetAllowBind`). ```go func (s *Sandbox) Run(ctx context.Context, cmd ...string) (*Result, error) func (s *Sandbox) RunInteractive(ctx context.Context, cmd ...string) (int, error) -func (s *Sandbox) DryRun(ctx context.Context, cmd ...string) (*DryRunResult, error) func (s *Sandbox) Spawn(cmd ...string) (*Process, error) func (s *Sandbox) Popen(stdio Stdio, cmd ...string) (*Process, error) ``` @@ -126,9 +125,9 @@ func (s *Sandbox) Popen(stdio Stdio, cmd ...string) (*Process, error) and returns a result with `ExitCode == -1`. `ctx` cancellation without a deadline does not preempt a running child. - **RunInteractive** inherits the caller's stdio and returns the exit code. -- **DryRun** runs against a temporary copy-on-write layer, reports the - filesystem `Changes` it would have made, and discards them. Requires - `Workdir`. +- Every `Result` from a sandbox with `Workdir` carries `Changes`, the files + the run added, modified, or deleted in its COW branch. A dry run is a run + with `OnExit: BranchActionAbort`. - **Spawn** starts a process without waiting, returning a `*Process`. - **Popen** is the streaming counterpart of Spawn: each stream set to `StdioPiped` is handed back on the `*Process` as an `*os.File` @@ -199,6 +198,12 @@ func (p *Process) Kill() error // SIGKILL func (p *Process) Ports() (map[int]int, error) // virtual→real, with PortRemap func (p *Process) Close() error // release the handle (kills if running), close piped streams +// BranchActionDefer only: after Wait, the change set stays on the Process. +func (p *Process) Pending() bool // exited under Defer and undecided +func (p *Process) UpperDir() string // new bytes of added/modified files, laid out like Workdir +func (p *Process) Commit() error // merge into Workdir (blocks up to 5s on a contended workdir) +func (p *Process) Abort() error // discard + // Popen only: caller-owned pipe ends, non-nil per stream wired StdioPiped. p.Stdin // *os.File p.Stdout // *os.File diff --git a/go/sandbox.go b/go/sandbox.go index cc3a5c3a..3db727a1 100644 --- a/go/sandbox.go +++ b/go/sandbox.go @@ -49,8 +49,11 @@ const ( BranchActionCommit // BranchActionAbort discards all of the branch's writes on exit. BranchActionAbort - // BranchActionKeep leaves the branch in place for the caller to handle. + // BranchActionKeep leaves the branch on disk for recovery tooling. BranchActionKeep + // BranchActionDefer holds the branch on the Process for Commit or Abort. + // Only Spawn and Popen can honor it; Run rejects it. + BranchActionDefer ) // SyscallCategory is the high-level category of an intercepted syscall event. @@ -170,8 +173,8 @@ const ( // is optional; an unset field means "no restriction" unless documented // otherwise. sandlock's default syscall blocklist is always applied. // -// A Sandbox value carries no runtime state: Run, RunInteractive, and DryRun -// build a fresh native policy on each call, so a single Sandbox may be reused +// A Sandbox value carries no runtime state: Run and RunInteractive build a +// fresh native policy on each call, so a single Sandbox may be reused // and shared across goroutines. Use Spawn for explicit process lifecycle // control, which returns an independent *Process handle. type Sandbox struct { @@ -304,6 +307,7 @@ type Result struct { Success bool // true when the process exited 0 Stdout []byte // captured standard output Stderr []byte // captured standard error + Changes []Change // what the run did to its COW branch; empty without Workdir } // StdioMode selects how one of a Popen'd process's standard streams is wired. @@ -329,7 +333,7 @@ type Stdio struct { Stderr StdioMode } -// ChangeKind classifies a filesystem change observed during a dry run. +// ChangeKind classifies a filesystem change a run made to its COW branch. type ChangeKind byte const ( @@ -338,15 +342,10 @@ const ( ChangeDeleted ChangeKind = 'D' ) -// Change is a single filesystem change detected by DryRun. +// Change is one filesystem change a run made to its COW branch. Modified +// means the file was opened for writing and exists in the workdir; the bytes +// are not compared. type Change struct { Kind ChangeKind // 'A' added, 'M' modified, 'D' deleted Path string // path relative to the working directory } - -// DryRunResult is the outcome of a dry run: a normal Result plus the list of -// filesystem changes the command would have made, all of which are discarded. -type DryRunResult struct { - Result - Changes []Change -} diff --git a/go/sandlock_linux.go b/go/sandlock_linux.go index 9769a45c..4044206b 100644 --- a/go/sandlock_linux.go +++ b/go/sandlock_linux.go @@ -585,6 +585,16 @@ func readResult(r *C.sandlock_result_t) *Result { } res.Stdout = readBytes(r, true) res.Stderr = readBytes(r, false) + count := int(C.sandlock_result_changes_len(r)) + for i := 0; i < count; i++ { + kind := byte(C.sandlock_result_change_kind(r, C.uintptr_t(i))) + var path string + if pc := C.sandlock_result_change_path(r, C.uintptr_t(i)); pc != nil { + path = C.GoString(pc) + C.sandlock_string_free(pc) + } + res.Changes = append(res.Changes, Change{Kind: ChangeKind(kind), Path: path}) + } return res } @@ -613,6 +623,9 @@ func (s *Sandbox) Run(ctx context.Context, cmd ...string) (*Result, error) { if len(cmd) == 0 { return nil, fmt.Errorf("sandlock: empty command") } + if s.OnExit == BranchActionDefer || s.OnError == BranchActionDefer { + return nil, fmt.Errorf("sandlock: BranchActionDefer needs a Process to decide on; use Spawn or Popen") + } policyPtr, err := s.buildPolicy() if err != nil { return nil, err @@ -676,63 +689,6 @@ func (s *Sandbox) RunInteractive(ctx context.Context, cmd ...string) (int, error return code, nil } -// DryRun executes cmd against a temporary copy-on-write layer, collects the -// filesystem changes it would have made, then discards them. It requires -// Workdir to be set. -func (s *Sandbox) DryRun(ctx context.Context, cmd ...string) (*DryRunResult, error) { - if err := ctx.Err(); err != nil { - return nil, err - } - if len(cmd) == 0 { - return nil, fmt.Errorf("sandlock: empty command") - } - policyPtr, err := s.buildPolicy() - if err != nil { - return nil, err - } - defer C.sandlock_sandbox_free(policyPtr) - - argv, err := cArgv(cmd) - if err != nil { - return nil, err - } - defer freeArgv(argv) - ap, ac := argvPtr(argv) - name := s.cName() - defer freeName(name) - - r := C.sandlock_dry_run(policyPtr, name, ap, ac) - if r == nil { - return nil, fmt.Errorf("sandlock: dry run failed (Workdir is required; check that readable paths exist)") - } - defer C.sandlock_dry_run_result_free(r) - - out := &DryRunResult{Result: Result{ - ExitCode: int(C.sandlock_dry_run_result_exit_code(r)), - Reason: ExitReason(C.sandlock_dry_run_result_reason(r)), - Signal: int(C.sandlock_dry_run_result_signal(r)), - Success: bool(C.sandlock_dry_run_result_success(r)), - }} - var n C.uintptr_t - if p := C.sandlock_dry_run_result_stdout_bytes(r, &n); p != nil && n > 0 { - out.Stdout = C.GoBytes(unsafe.Pointer(p), C.int(n)) - } - if p := C.sandlock_dry_run_result_stderr_bytes(r, &n); p != nil && n > 0 { - out.Stderr = C.GoBytes(unsafe.Pointer(p), C.int(n)) - } - count := int(C.sandlock_dry_run_result_changes_len(r)) - for i := 0; i < count; i++ { - kind := byte(C.sandlock_dry_run_result_change_kind(r, C.uintptr_t(i))) - var path string - if pc := C.sandlock_dry_run_result_change_path(r, C.uintptr_t(i)); pc != nil { - path = C.GoString(pc) - C.sandlock_string_free(pc) - } - out.Changes = append(out.Changes, Change{Kind: ChangeKind(kind), Path: path}) - } - return out, nil -} - // Confine applies the Sandbox's Landlock filesystem rules to the current // process, in place and irreversibly. Only filesystem fields are honored; // configuration that requires a supervisor or a fresh child (seccomp, @@ -794,6 +750,7 @@ type Process struct { h *C.sandlock_handle_t pid int waiting bool // a Wait owns the handle; other handle ops must defer to it + pending bool // the run exited under BranchActionDefer; Commit/Abort own the handle now // Caller-owned stdio for a process started by Popen. Each is non-nil only // for a stream wired StdioPiped; it owns the pipe fd (closing the file @@ -970,9 +927,15 @@ func (p *Process) Wait() (*Result, error) { p.mu.Lock() defer p.mu.Unlock() p.waiting = false - C.sandlock_handle_free(h) - p.h = nil - runtime.SetFinalizer(p, nil) + // A deferred branch keeps the handle alive until Commit/Abort/Close; the + // finalizer stays armed so a leaked Process still preserves it. + if C.sandlock_handle_pending(h) != 0 { + p.pending = true + } else { + C.sandlock_handle_free(h) + p.h = nil + runtime.SetFinalizer(p, nil) + } if r == nil { return nil, fmt.Errorf("sandlock: wait failed") } @@ -981,10 +944,65 @@ func (p *Process) Wait() (*Result, error) { return res, nil } +// Pending reports whether the process exited under BranchActionDefer and its +// change set is waiting for Commit or Abort. Close on a pending Process +// preserves the branch on disk, the same as BranchActionKeep. +func (p *Process) Pending() bool { + p.mu.Lock() + defer p.mu.Unlock() + return p.pending +} + +// UpperDir is the pending branch's upper directory, laid out like Workdir and +// holding the new bytes of every added or modified file. Empty unless Pending. +func (p *Process) UpperDir() string { + p.mu.Lock() + defer p.mu.Unlock() + if !p.pending { + return "" + } + c := C.sandlock_handle_upper_dir(p.h) + if c == nil { + return "" + } + defer C.sandlock_string_free(c) + return C.GoString(c) +} + +// Commit merges the pending branch into Workdir and releases the handle. It +// blocks up to 5s on a workdir another sandbox is merging into; a failed merge +// leaves the branch preserved on disk. Last-writer-wins against anything that +// changed the workdir since the run. +func (p *Process) Commit() error { + return p.dispose(func(h *C.sandlock_handle_t) C.int { return C.sandlock_handle_commit(h) }, "commit") +} + +// Abort discards the pending branch and releases the handle. +func (p *Process) Abort() error { + return p.dispose(func(h *C.sandlock_handle_t) C.int { return C.sandlock_handle_abort(h) }, "abort") +} + +func (p *Process) dispose(op func(*C.sandlock_handle_t) C.int, name string) error { + p.mu.Lock() + defer p.mu.Unlock() + if !p.pending { + return fmt.Errorf("sandlock: no pending branch to %s", name) + } + rc := op(p.h) + p.pending = false + C.sandlock_handle_free(p.h) + p.h = nil + runtime.SetFinalizer(p, nil) + if rc != 0 { + return fmt.Errorf("sandlock: %s failed; the change set is preserved on disk", name) + } + return nil +} + func (p *Process) signal(sig syscall.Signal) error { p.mu.Lock() defer p.mu.Unlock() - if p.h == nil || p.pid <= 0 { + if p.h == nil || p.pid <= 0 || p.pending { return ErrNotRunning } // The sandbox child leads its own process group; signal the whole group. @@ -1081,6 +1099,7 @@ func (p *Process) Close() error { } C.sandlock_handle_free(p.h) p.h = nil + p.pending = false runtime.SetFinalizer(p, nil) return nil } diff --git a/go/sandlock_linux_test.go b/go/sandlock_linux_test.go index 74d81bc1..b5cf8cab 100644 --- a/go/sandlock_linux_test.go +++ b/go/sandlock_linux_test.go @@ -228,26 +228,116 @@ func TestPolicyFnReceivesExecveArgv(t *testing.T) { } } -func TestDryRun(t *testing.T) { +func TestAbortReportsChangesAndWritesNothing(t *testing.T) { requireLandlock(t) dir := t.TempDir() sb := &sandlock.Sandbox{ FSReadable: rootfs, FSWritable: []string{dir}, Workdir: dir, + OnExit: sandlock.BranchActionAbort, } - res, err := sb.DryRun(context.Background(), "sh", "-c", "echo hi > "+dir+"/out.txt") + res, err := sb.Run(context.Background(), "sh", "-c", "echo hi > "+dir+"/out.txt") if err != nil { - t.Fatalf("DryRun: %v", err) + t.Fatalf("Run: %v", err) } if !res.Success { - t.Fatalf("dry run failed: exit=%d stderr=%q", res.ExitCode, res.Stderr) + t.Fatalf("run failed: exit=%d stderr=%q", res.ExitCode, res.Stderr) + } + if _, statErr := os.Stat(dir + "/out.txt"); statErr == nil { + t.Fatalf("an aborting run leaked a write to the host") + } + want := sandlock.Change{Kind: sandlock.ChangeAdded, Path: "out.txt"} + if len(res.Changes) != 1 || res.Changes[0] != want { + t.Fatalf("changes = %+v, want [%+v]", res.Changes, want) + } +} + +func TestRunRejectsDefer(t *testing.T) { + requireLandlock(t) + dir := t.TempDir() + sb := &sandlock.Sandbox{ + FSReadable: rootfs, + FSWritable: []string{dir}, + Workdir: dir, + OnExit: sandlock.BranchActionDefer, + } + if _, err := sb.Run(context.Background(), "true"); err == nil { + t.Fatal("Run must refuse a Defer policy: it has no Process to decide on") + } +} + +func TestDeferHoldsTheBranchForTheProcess(t *testing.T) { + requireLandlock(t) + dir := t.TempDir() + sb := &sandlock.Sandbox{ + FSReadable: rootfs, + FSWritable: []string{dir}, + Workdir: dir, + OnExit: sandlock.BranchActionDefer, + } + p, err := sb.Spawn("sh", "-c", "echo hi > "+dir+"/out.txt") + if err != nil { + t.Fatalf("Spawn: %v", err) + } + defer p.Close() + res, err := p.Wait() + if err != nil { + t.Fatalf("Wait: %v", err) + } + if !res.Success || len(res.Changes) != 1 { + t.Fatalf("unexpected result: %+v", res) + } + if !p.Pending() { + t.Fatal("a Defer run must be pending after Wait") + } + if _, statErr := os.Stat(dir + "/out.txt"); statErr == nil { + t.Fatal("nothing may land before Commit") + } + upper := p.UpperDir() + if body, readErr := os.ReadFile(upper + "/out.txt"); readErr != nil || string(body) != "hi\n" { + t.Fatalf("upper %q does not hold the write: %q, %v", upper, body, readErr) + } + if err := p.Commit(); err != nil { + t.Fatalf("Commit: %v", err) + } + if p.Pending() { + t.Fatal("Commit must clear pending") + } + if body, readErr := os.ReadFile(dir + "/out.txt"); readErr != nil || string(body) != "hi\n" { + t.Fatalf("workdir does not hold the committed write: %q, %v", body, readErr) + } + if err := p.Commit(); err == nil { + t.Fatal("a second Commit has nothing to merge and must fail") + } +} + +func TestDeferAbortDiscards(t *testing.T) { + requireLandlock(t) + dir := t.TempDir() + sb := &sandlock.Sandbox{ + FSReadable: rootfs, + FSWritable: []string{dir}, + Workdir: dir, + OnExit: sandlock.BranchActionDefer, + } + p, err := sb.Spawn("sh", "-c", "echo hi > "+dir+"/out.txt") + if err != nil { + t.Fatalf("Spawn: %v", err) + } + defer p.Close() + if _, err := p.Wait(); err != nil { + t.Fatalf("Wait: %v", err) + } + if err := p.Abort(); err != nil { + t.Fatalf("Abort: %v", err) + } + if p.Pending() { + t.Fatal("Abort must clear pending") } - // The write is discarded; the file must not exist on the host afterward. if _, statErr := os.Stat(dir + "/out.txt"); statErr == nil { - t.Fatalf("dry run leaked a write to the host") + t.Fatal("an aborted branch must not land") } - t.Logf("changes: %+v", res.Changes) } func TestProcessKillInterruptsWait(t *testing.T) { From cc5d5a79211f4e864ff374122dba1b83ce2c8c07 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Thu, 10 Sep 2026 09:55:24 -0700 Subject: [PATCH 6/6] cow: classify changes by comparing the trees, report empty dirs The change kind was derived from the whiteout set, so any tool that unlinks before it writes (sed -i, mv over a file, rm then recreate) reported the path as Added while its old bytes were still in the workdir. A caller diffing upper_dir against the workdir to review a deferred run then compared the new file against nothing. Modified now means the path exists on both sides, regardless of how it got there. Directories the run created were skipped by the upper walk even though commit creates them, so an empty directory was invisible to the caller deciding whether to commit. Signed-off-by: Cong Wang --- crates/sandlock-core/src/cow/seccomp.rs | 92 +++++++++++++++++++++---- crates/sandlock-core/src/result.rs | 4 +- go/README.md | 4 +- go/sandbox.go | 4 +- python/README.md | 2 +- python/src/sandlock/sandbox.py | 4 +- python/tests/test_sandbox.py | 18 +++++ 7 files changed, 105 insertions(+), 23 deletions(-) diff --git a/crates/sandlock-core/src/cow/seccomp.rs b/crates/sandlock-core/src/cow/seccomp.rs index 2918e5b7..42c1ead8 100644 --- a/crates/sandlock-core/src/cow/seccomp.rs +++ b/crates/sandlock-core/src/cow/seccomp.rs @@ -1596,23 +1596,19 @@ impl SeccompCowBranch { let mut result = Vec::new(); - // Walk upper directory for added/modified files + // The kind compares the two trees as they stand, not the branch's + // history: a whiteouted-then-recreated path still has its old bytes + // in the workdir, and that is what a caller diffing the sides needs. for entry in walkdir::WalkDir::new(&self.upper).min_depth(1) { let entry = entry.map_err(|e| BranchError::Operation(format!("walk: {}", e)))?; - if entry.file_type().is_dir() { + let rel = entry.path().strip_prefix(&self.upper).unwrap(); + let lower = self.workdir.join(rel).symlink_metadata().ok(); + // Copy-up recreates a modified file's parents in the upper; a + // directory the workdir already has is scaffolding, not a change. + if entry.file_type().is_dir() && lower.as_ref().is_some_and(|m| m.is_dir()) { continue; } - let rel = entry.path().strip_prefix(&self.upper).unwrap(); - let lower = self.workdir.join(rel); - // A covered path's lower entry is logically gone, so a re-created - // upper entry is an addition even though lower bytes still exist. - let kind = if self.deleted.covers(&rel.to_string_lossy()) { - ChangeKind::Added - } else if lower.exists() { - ChangeKind::Modified - } else { - ChangeKind::Added - }; + let kind = if lower.is_some() { ChangeKind::Modified } else { ChangeKind::Added }; result.push(Change { kind, path: rel.to_path_buf() }); } @@ -4523,13 +4519,13 @@ mod tests { let upper = branch.ensure_cow_copy("existing.txt").unwrap(); fs::write(&upper, "recreated").unwrap(); let changes = branch.changes().unwrap(); - // The recreated file is a single Added entry, not Deleted + Modified. + // The recreated file is a single Modified entry, not Deleted + Modified. let for_path: Vec<_> = changes .iter() .filter(|c| c.path == std::path::Path::new("existing.txt")) .collect(); assert_eq!(for_path.len(), 1); - assert_eq!(for_path[0].kind, crate::result::ChangeKind::Added); + assert_eq!(for_path[0].kind, crate::result::ChangeKind::Modified); } #[test] @@ -5851,6 +5847,72 @@ mod tests { ); } + /// A path that exists on both sides is Modified even when a whiteout + /// covers it: `sed -i`, `mv over` and `rm; recreate` all unlink first, + /// and a caller diffing the two trees needs the old bytes it can still + /// read in the workdir, not a claim that the file is new. + #[test] + fn changes_labels_a_recreated_entry_modified_while_the_workdir_still_has_it() { + use crate::result::ChangeKind; + let workdir = tempfile::tempdir().unwrap(); + let storage = tempfile::tempdir().unwrap(); + fs::write(workdir.path().join("f.txt"), "before").unwrap(); + + let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); + branch.mark_deleted("f.txt"); + fs::write(branch.upper.join("f.txt"), "after").unwrap(); + + let changes: Vec<_> = branch + .changes() + .unwrap() + .into_iter() + .map(|c| (c.kind, c.path.display().to_string())) + .collect(); + assert_eq!(changes, vec![(ChangeKind::Modified, "f.txt".to_string())]); + } + + /// The commit creates every directory the upper holds, so an empty one the + /// run made is a change and must be reported like any other addition. + #[test] + fn changes_reports_an_added_empty_directory() { + use crate::result::ChangeKind; + let workdir = tempfile::tempdir().unwrap(); + let storage = tempfile::tempdir().unwrap(); + + let branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); + fs::create_dir(branch.upper.join("newdir")).unwrap(); + + let changes: Vec<_> = branch + .changes() + .unwrap() + .into_iter() + .map(|c| (c.kind, c.path.display().to_string())) + .collect(); + assert_eq!(changes, vec![(ChangeKind::Added, "newdir".to_string())]); + } + + /// Copy-up recreates the parents of a modified file in the upper; those + /// mirror directories the workdir already has and are not changes. + #[test] + fn changes_skips_upper_directories_the_workdir_already_has() { + use crate::result::ChangeKind; + let workdir = tempfile::tempdir().unwrap(); + let storage = tempfile::tempdir().unwrap(); + fs::create_dir(workdir.path().join("sub")).unwrap(); + + let branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); + fs::create_dir(branch.upper.join("sub")).unwrap(); + fs::write(branch.upper.join("sub/a.txt"), "new").unwrap(); + + let changes: Vec<_> = branch + .changes() + .unwrap() + .into_iter() + .map(|c| (c.kind, c.path.display().to_string())) + .collect(); + assert_eq!(changes, vec![(ChangeKind::Added, "sub/a.txt".to_string())]); + } + // ---- Names, symlinks and the confined path helpers ---- /// `safe_rel` normalises the spellings that name the same entry, rejects an diff --git a/crates/sandlock-core/src/result.rs b/crates/sandlock-core/src/result.rs index 4337c206..24eb502a 100644 --- a/crates/sandlock-core/src/result.rs +++ b/crates/sandlock-core/src/result.rs @@ -59,8 +59,10 @@ pub enum ExitStatus { pub enum ChangeKind { /// Exists in the branch but not in the workdir. Added, - /// Opened for writing and exists in the workdir; the bytes are not compared. + /// Exists on both sides; the bytes are not compared, so a rewrite with + /// identical contents, a mode change, or a rename over the path all count. Modified, + /// Exists in the workdir but not in the branch. Deleted, } diff --git a/go/README.md b/go/README.md index f189086d..9af3c541 100644 --- a/go/README.md +++ b/go/README.md @@ -125,8 +125,8 @@ func (s *Sandbox) Popen(stdio Stdio, cmd ...string) (*Process, error) and returns a result with `ExitCode == -1`. `ctx` cancellation without a deadline does not preempt a running child. - **RunInteractive** inherits the caller's stdio and returns the exit code. -- Every `Result` from a sandbox with `Workdir` carries `Changes`, the files - the run added, modified, or deleted in its COW branch. A dry run is a run +- Every `Result` from a sandbox with `Workdir` carries `Changes`, the files and + directories the run added, modified, or deleted in its COW branch. A dry run is a run with `OnExit: BranchActionAbort`. - **Spawn** starts a process without waiting, returning a `*Process`. - **Popen** is the streaming counterpart of Spawn: each stream set to diff --git a/go/sandbox.go b/go/sandbox.go index 3db727a1..d5dd6b8b 100644 --- a/go/sandbox.go +++ b/go/sandbox.go @@ -343,8 +343,8 @@ const ( ) // Change is one filesystem change a run made to its COW branch. Modified -// means the file was opened for writing and exists in the workdir; the bytes -// are not compared. +// means the path exists on both sides; the bytes are not compared, so a +// rename over an existing file counts. type Change struct { Kind ChangeKind // 'A' added, 'M' modified, 'D' deleted Path string // path relative to the working directory diff --git a/python/README.md b/python/README.md index 39b69cf4..ce89234c 100644 --- a/python/README.md +++ b/python/README.md @@ -310,7 +310,7 @@ Wait for the running process to finish and return its `Result`. #### Inspecting and deferring COW changes Every `Result` from a sandbox with a `workdir` carries `changes`, the list -of files the run added, modified, or deleted in its COW branch. A dry run +of files and directories the run added, modified, or deleted in its COW branch. A dry run is a run whose branch action is `ABORT`: ```python diff --git a/python/src/sandlock/sandbox.py b/python/src/sandlock/sandbox.py index 88818932..a628e0cc 100644 --- a/python/src/sandlock/sandbox.py +++ b/python/src/sandlock/sandbox.py @@ -118,10 +118,10 @@ class StdioMode(IntEnum): @dataclass(frozen=True) class Change: - """A single filesystem change detected by dry-run.""" + """A single filesystem change a run made to its COW branch.""" kind: str - """Change kind: A=added, M=modified, D=deleted.""" + """A=added, M=modified (exists on both sides, bytes not compared), D=deleted.""" path: str """Path relative to workdir.""" diff --git a/python/tests/test_sandbox.py b/python/tests/test_sandbox.py index 2d9174da..b1eeab7c 100644 --- a/python/tests/test_sandbox.py +++ b/python/tests/test_sandbox.py @@ -753,6 +753,24 @@ def test_abort_reports_deleted_file_without_removing_it(self, tmp_path): assert (workdir / "victim.txt").exists() assert ("D", "victim.txt") in [(c.kind, c.path) for c in result.changes] + def test_rename_over_an_existing_file_reports_modified(self, tmp_path): + workdir = tmp_path / "rename-over" + workdir.mkdir() + (workdir / "data.txt").write_text("original") + p = _policy(fs_writable=[str(workdir)], workdir=str(workdir), on_exit=BranchAction.ABORT) + result = p.run(["sh", "-c", f"cd {workdir} && echo changed > tmp && mv tmp data.txt"]) + assert result.success, result + assert [(c.kind, c.path) for c in result.changes] == [("M", "data.txt")] + + def test_added_empty_directory_is_reported(self, tmp_path): + workdir = tmp_path / "empty-dir" + workdir.mkdir() + p = _policy(fs_writable=[str(workdir)], workdir=str(workdir), on_exit=BranchAction.ABORT) + result = p.run(["mkdir", str(workdir / "newdir")]) + assert result.success, result + assert [(c.kind, c.path) for c in result.changes] == [("A", "newdir")] + assert not (workdir / "newdir").exists() + def test_commit_reports_the_changes_it_merged(self, tmp_path): workdir = tmp_path / "commit" workdir.mkdir()