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/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/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}"); +} diff --git a/crates/sandlock-core/src/cow/seccomp.rs b/crates/sandlock-core/src/cow/seccomp.rs index 026e5727..42c1ead8 100644 --- a/crates/sandlock-core/src/cow/seccomp.rs +++ b/crates/sandlock-core/src/cow/seccomp.rs @@ -1591,28 +1591,24 @@ 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(); - // 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() }); } @@ -3117,7 +3113,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 +3125,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 +3668,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 +3680,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 +3691,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 +3716,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")); } @@ -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::dry_run::ChangeKind::Added); + assert_eq!(for_path[0].kind, crate::result::ChangeKind::Modified); } #[test] @@ -5429,7 +5425,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 +5545,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 +5664,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 +5827,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(); @@ -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 @@ -6242,7 +6304,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..24eb502a 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,38 @@ pub enum ExitStatus { Killed, Timeout, } + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ChangeKind { + /// Exists in the branch but not in the workdir. + Added, + /// 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, +} + +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 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) }; +} 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/go/README.md b/go/README.md index 1db66635..9af3c541 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 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 `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..d5dd6b8b 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 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 } - -// 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) { diff --git a/python/README.md b/python/README.md index 3e9d6988..ce89234c 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 and directories 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..a628e0cc 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): @@ -118,33 +118,15 @@ 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.""" -@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..b1eeab7c 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,155 @@ 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_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() 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 +1072,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 +1081,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