From dd814dd318e3b53cf493a8bc9cdffe7bd302b137 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Tue, 13 Oct 2026 15:04:11 -0700 Subject: [PATCH 01/23] sandbox: keep background jobs supervised after main exit Waiting for the main process also tore down supervision and killed background jobs. Embedders could not observe command completion while keeping a service started by that command alive. Make wait return the main ExitStatus and retain the session. Add shutdown to terminate remaining jobs, reap children, collect output and settle COW changes. Expose a snapshot of tracked process groups, and have run wait and shut down for callers needing a complete result. Update the internal callers and keep complete-result FFI adapters working until the bindings expose the new lifecycle. Use a private subreaper to retain ancestry for supervised syscalls and reap orphaned descendants without changing the embedder's subreaper setting. Pin the main PID until the supervisor obtains its pidfd, and use a shutdown channel to clean up descendants outside tracked groups. Cover repeated and cancelled waits, background output and COW writes, embedder child ownership, descriptor exhaustion and descendant cleanup. The 124 affected integration tests, the reaper regression and workspace checks pass. Fixes #292 Signed-off-by: Cong Wang --- crates/sandlock-cli/src/learn.rs | 6 +- crates/sandlock-core/src/lib.rs | 1 + crates/sandlock-core/src/pgroup.rs | 15 + crates/sandlock-core/src/pipeline.rs | 6 +- crates/sandlock-core/src/reaper.rs | 304 ++++++++++++++++++ crates/sandlock-core/src/sandbox.rs | 224 ++++++++----- crates/sandlock-core/src/transaction.rs | 3 +- .../tests/integration/test_branch_action.rs | 40 +++ .../tests/integration/test_control.rs | 6 +- .../tests/integration/test_fork.rs | 10 +- .../tests/integration/test_policy_fn.rs | 5 +- .../tests/integration/test_popen.rs | 36 ++- .../tests/integration/test_process_groups.rs | 277 +++++++++++++++- .../tests/integration/test_resource.rs | 6 +- .../tests/integration/test_restore.rs | 2 +- .../tests/integration/test_sandbox.rs | 8 +- crates/sandlock-ffi/src/lib.rs | 8 +- crates/sandlock-oci/src/supervisor.rs | 4 +- 18 files changed, 826 insertions(+), 135 deletions(-) create mode 100644 crates/sandlock-core/src/reaper.rs diff --git a/crates/sandlock-cli/src/learn.rs b/crates/sandlock-cli/src/learn.rs index 95668ca1e..1652fccca 100644 --- a/crates/sandlock-cli/src/learn.rs +++ b/crates/sandlock-cli/src/learn.rs @@ -632,7 +632,7 @@ pub async fn run(args: LearnArgs) -> Result<()> { match tokio::time::timeout(deadline, sandbox.wait()).await { Ok(r) => { let result = r.map_err(|e| anyhow!("sandbox error: {e}"))?; - require_clean_exit(&result.exit_status)?; + require_clean_exit(&result)?; false } Err(_elapsed) => { @@ -647,10 +647,12 @@ pub async fn run(args: LearnArgs) -> Result<()> { } else { let result = sandbox.wait().await .map_err(|e| anyhow!("sandbox error: {e}"))?; - require_clean_exit(&result.exit_status)?; + require_clean_exit(&result)?; false }; + sandbox.shutdown().await.map_err(|e| anyhow!("sandbox error: {e}"))?; + if timed_out { eprintln!("sandlock learn: writing partial profile from observations before timeout"); } diff --git a/crates/sandlock-core/src/lib.rs b/crates/sandlock-core/src/lib.rs index a88475585..5630a4efd 100644 --- a/crates/sandlock-core/src/lib.rs +++ b/crates/sandlock-core/src/lib.rs @@ -16,6 +16,7 @@ pub mod protection; pub mod seccomp; pub(crate) mod resource; pub(crate) mod pgroup; +pub(crate) mod reaper; pub(crate) mod network; pub mod context; pub(crate) mod resolved; diff --git a/crates/sandlock-core/src/pgroup.rs b/crates/sandlock-core/src/pgroup.rs index 0edd30f92..a7cbca9c1 100644 --- a/crates/sandlock-core/src/pgroup.rs +++ b/crates/sandlock-core/src/pgroup.rs @@ -52,6 +52,21 @@ impl ProcessGroups { .unwrap_or(false) } + pub fn live(&self) -> io::Result> { + let leaders = self.leaders.lock() + .map_err(|_| io::Error::other("process group table poisoned"))?; + let mut groups = Vec::new(); + for (&pgid, leader) in leaders.iter() { + match pidfd_signal_group(leader, 0) { + Ok(()) => groups.push(pgid), + Err(e) if e.raw_os_error() == Some(libc::ESRCH) => {} + Err(e) => return Err(e), + } + } + groups.sort_unstable(); + Ok(groups) + } + /// Signal every group, returning how many had members. Holding the lock /// for the whole sweep keeps a group recorded meanwhile from being /// skipped: its creator is still in a group this sweep reaches. diff --git a/crates/sandlock-core/src/pipeline.rs b/crates/sandlock-core/src/pipeline.rs index 1c41d5a57..f018c1b5d 100644 --- a/crates/sandlock-core/src/pipeline.rs +++ b/crates/sandlock-core/src/pipeline.rs @@ -247,7 +247,8 @@ async fn run_pipeline(stages: Vec) -> Result { }; for (i, mut sb) in sandboxes.into_iter().enumerate() { - let result = sb.wait().await?; + sb.wait().await?; + let result = sb.shutdown().await?; if i == n - 1 { last_result.exit_status = result.exit_status; } @@ -459,7 +460,8 @@ async fn run_gather( changes: Vec::new(), }; for (i, mut sb) in sandboxes.into_iter().enumerate() { - let result = sb.wait().await?; + sb.wait().await?; + let result = sb.shutdown().await?; if i == total - 1 { last_result.exit_status = result.exit_status; } diff --git a/crates/sandlock-core/src/reaper.rs b/crates/sandlock-core/src/reaper.rs new file mode 100644 index 000000000..592c0e786 --- /dev/null +++ b/crates/sandlock-core/src/reaper.rs @@ -0,0 +1,304 @@ +use std::io; +use std::os::fd::{AsRawFd, FromRawFd, OwnedFd}; + +pub(crate) struct Reaper { + pub pid: i32, + pub main_pidfd: OwnedFd, + status: OwnedFd, + command: Option, +} + +impl Reaper { + pub fn shutdown(&mut self) { + self.command.take(); + } + + pub async fn main_status(&self) -> io::Result { + let fd = tokio::io::unix::AsyncFd::new(self.status.try_clone()?)?; + loop { + let mut ready = fd.readable().await?; + let result = ready.try_io(|fd| { + let mut status = 0i32; + let n = unsafe { + libc::read( + fd.get_ref().as_raw_fd(), + (&mut status as *mut i32).cast(), + 4, + ) + }; + match n { + 4 => Ok(status), + -1 => Err(io::Error::last_os_error()), + _ => Err(io::Error::new( + io::ErrorKind::UnexpectedEof, + "session reaper lost main status", + )), + } + }); + match result { + Ok(Err(e)) if e.kind() == io::ErrorKind::Interrupted => continue, + Ok(result) => return result, + Err(_) => {} + } + } + } +} + +/// A private subreaper keeps orphans below the supervisor for Yama checks, +/// without changing the embedding process's child-adoption policy. +pub(crate) fn fork() -> io::Result<(i32, Option)> { + let mut pipe = [0; 2]; + if unsafe { libc::pipe2(pipe.as_mut_ptr(), libc::O_CLOEXEC) } < 0 { + return Err(io::Error::last_os_error()); + } + let read = unsafe { OwnedFd::from_raw_fd(pipe[0]) }; + let write = unsafe { OwnedFd::from_raw_fd(pipe[1]) }; + if unsafe { libc::pipe2(pipe.as_mut_ptr(), libc::O_CLOEXEC) } < 0 { + return Err(io::Error::last_os_error()); + } + let pinned_r = unsafe { OwnedFd::from_raw_fd(pipe[0]) }; + let pinned_w = unsafe { OwnedFd::from_raw_fd(pipe[1]) }; + let owner = unsafe { libc::getpid() }; + let reaper = crate::control::fork_without_control_fds(); + if reaper < 0 { + return Err(io::Error::last_os_error()); + } + if reaper == 0 { + unsafe { + let mut blocked = std::mem::zeroed(); + let mut previous_mask = std::mem::zeroed(); + libc::sigfillset(&mut blocked); + libc::sigprocmask(libc::SIG_SETMASK, &blocked, &mut previous_mask); + let mut previous_chld = std::mem::zeroed(); + let mut default_chld: libc::sigaction = std::mem::zeroed(); + default_chld.sa_sigaction = libc::SIG_DFL; + libc::sigaction(libc::SIGCHLD, &default_chld, &mut previous_chld); + if libc::prctl(libc::PR_SET_CHILD_SUBREAPER, 1, 0, 0, 0) != 0 + || libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGKILL) != 0 + || libc::getppid() != owner + { + libc::_exit(127); + } + let parent = libc::getpid(); + let child = libc::fork(); + if child < 0 { + libc::_exit(127); + } + if child == 0 { + if libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGKILL) != 0 + || libc::getppid() != parent + { + libc::_exit(127); + } + libc::sigaction(libc::SIGCHLD, &previous_chld, std::ptr::null_mut()); + libc::sigprocmask(libc::SIG_SETMASK, &previous_mask, std::ptr::null_mut()); + drop(read); + drop(write); + drop(pinned_r); + drop(pinned_w); + return Ok((0, None)); + } + let fd = write.as_raw_fd(); + let gate = pinned_r.as_raw_fd(); + let lo = fd.min(gate) as u32; + let hi = fd.max(gate) as u32; + if lo > 0 { + libc::syscall(libc::SYS_close_range, 0u32, lo - 1, 0u32); + } + if hi > lo + 1 { + libc::syscall(libc::SYS_close_range, lo + 1, hi - 1, 0u32); + } + libc::syscall(libc::SYS_close_range, hi + 1, u32::MAX, 0u32); + if !send(fd, child) { + libc::_exit(127); + } + // Keep the main PID pinned as a child until the supervisor has a pidfd. + let mut ack = 0i32; + let acknowledged = loop { + let n = libc::read(gate, (&mut ack as *mut i32).cast(), 4); + if n >= 0 { + break n == 4; + } + if io::Error::last_os_error().raw_os_error() != Some(libc::EINTR) { + libc::_exit(127); + } + }; + supervise(child, fd, gate, !acknowledged); + } + } + drop(write); + drop(pinned_r); + let child = loop { + match crate::context::read_u32_fd(read.as_raw_fd()) { + Err(e) if e.kind() == io::ErrorKind::Interrupted => continue, + result => break result, + } + }; + match child { + Ok(pid) if pid > 0 => { + let main_pidfd = match crate::sys::syscall::pidfd_open(pid, 0) { + Ok(fd) => fd, + Err(e) => { + unsafe { libc::kill(pid as i32, libc::SIGKILL) }; + drop(pinned_w); + reap(reaper); + return Err(e); + } + }; + unsafe { send(pinned_w.as_raw_fd(), 1) }; + unsafe { libc::fcntl(read.as_raw_fd(), libc::F_SETFL, libc::O_NONBLOCK) }; + Ok(( + pid as i32, + Some(Reaper { + pid: reaper, + main_pidfd, + status: read, + command: Some(pinned_w), + }), + )) + } + _ => { + drop(pinned_w); + reap(reaper); + Err(io::Error::other("session reaper failed to start")) + } + } +} + +unsafe fn supervise(main: i32, status_fd: i32, command_fd: i32, mut stopping: bool) -> ! { + let mut mask = std::mem::zeroed(); + libc::sigemptyset(&mut mask); + libc::sigaddset(&mut mask, libc::SIGCHLD); + let signal_fd = libc::signalfd(-1, &mask, libc::SFD_CLOEXEC | libc::SFD_NONBLOCK); + let mut poll = [ + libc::pollfd { + fd: command_fd, + events: libc::POLLIN, + revents: 0, + }, + libc::pollfd { + fd: signal_fd, + events: libc::POLLIN, + revents: 0, + }, + ]; + loop { + if stopping { + kill_children(); + } + loop { + let mut status = 0; + let pid = libc::waitpid(-1, &mut status, libc::WNOHANG); + if pid == 0 { + break; + } + if pid < 0 { + if io::Error::last_os_error().raw_os_error() == Some(libc::EINTR) { + continue; + } + libc::_exit(0); + } + if pid == main && !send(status_fd, status) { + stopping = true; + } + } + // A killed parent may have just transferred more children to us. + if stopping { + kill_children(); + } + libc::poll(poll.as_mut_ptr(), 2, if signal_fd < 0 { 10 } else { -1 }); + if poll[0].revents != 0 { + stopping = true; + poll[0].fd = -1; + } + if poll[1].revents != 0 { + let mut signal: libc::signalfd_siginfo = std::mem::zeroed(); + libc::read( + signal_fd, + (&mut signal as *mut libc::signalfd_siginfo).cast(), + std::mem::size_of_val(&signal), + ); + } + } +} + +unsafe fn kill_children() { + let fd = libc::open( + b"/proc/thread-self/children\0".as_ptr().cast(), + libc::O_RDONLY | libc::O_CLOEXEC, + ); + if fd < 0 { + return; + } + let mut buffer = [0u8; 4096]; + let mut pid = 0i32; + loop { + let n = libc::read(fd, buffer.as_mut_ptr().cast(), buffer.len()); + if n <= 0 { + break; + } + for &byte in &buffer[..n as usize] { + if byte.is_ascii_digit() { + pid = pid * 10 + (byte - b'0') as i32; + } else if pid > 0 { + // Only this process reaps these children, so their PIDs cannot be reused here. + libc::kill(pid, libc::SIGKILL); + pid = 0; + } + } + } + if pid > 0 { + libc::kill(pid, libc::SIGKILL); + } + libc::close(fd); +} + +fn reap(pid: i32) { + loop { + if unsafe { libc::waitpid(pid, std::ptr::null_mut(), 0) } >= 0 { + return; + } + if io::Error::last_os_error().raw_os_error() != Some(libc::EINTR) { + return; + } + } +} + +unsafe fn send(fd: i32, value: i32) -> bool { + loop { + if libc::write(fd, (&value as *const i32).cast(), 4) == 4 { + return true; + } + if io::Error::last_os_error().raw_os_error() != Some(libc::EINTR) { + return false; + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn reports_an_exit_before_the_caller_starts_waiting() { + let (pid, reaper) = fork().unwrap(); + if pid == 0 { + unsafe { libc::_exit(42) } + } + let reaper = reaper.unwrap(); + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + let status = reaper.main_status().await.unwrap(); + assert!(libc::WIFEXITED(status)); + assert_eq!(libc::WEXITSTATUS(status), 42); + let mut ready = libc::pollfd { + fd: reaper.main_pidfd.as_raw_fd(), + events: libc::POLLIN, + revents: 0, + }; + assert_eq!(unsafe { libc::poll(&mut ready, 1, 0) }, 1); + assert_eq!( + unsafe { libc::waitpid(reaper.pid, std::ptr::null_mut(), 0) }, + reaper.pid + ); + } +} diff --git a/crates/sandlock-core/src/sandbox.rs b/crates/sandlock-core/src/sandbox.rs index 840d6e224..5877581bb 100644 --- a/crates/sandlock-core/src/sandbox.rs +++ b/crates/sandlock-core/src/sandbox.rs @@ -287,6 +287,7 @@ struct Runtime { name: String, state: RuntimeState, child_pid: Option, + reaper: Option, pidfd: Option, groups: Arc, notif_handle: Option>, @@ -325,7 +326,7 @@ struct Runtime { /// Set when this sandbox is a stage of a [`Transaction`](crate::transaction::Transaction) /// that shares one COW upper across all stages. When present, `do_create_stdio` /// reuses this `CowState` (instead of building its own branch), and neither - /// `wait()` nor `Drop` take/commit/abort the branch — the transaction + /// `shutdown()` nor `Drop` take/commit/abort the branch — the transaction /// coordinator owns the single commit/abort. See [`SharedCow`]. #[cfg(feature = "cow")] shared_cow: Option, @@ -356,6 +357,7 @@ enum RuntimeState { Created, Running, Paused, + MainExited(crate::result::ExitStatus), Stopped(crate::result::ExitStatus), } @@ -892,14 +894,18 @@ impl Sandbox { /// Send SIGSTOP to the sandbox's process groups. pub fn pause(&mut self) -> Result<(), crate::error::SandlockError> { self.signal_live_groups(libc::SIGSTOP)?; - self.rt_mut().state = RuntimeState::Paused; + if !matches!(self.rt().state, RuntimeState::MainExited(_)) { + self.rt_mut().state = RuntimeState::Paused; + } Ok(()) } /// Send SIGCONT to the sandbox's process groups. pub fn resume(&mut self) -> Result<(), crate::error::SandlockError> { self.signal_live_groups(libc::SIGCONT)?; - self.rt_mut().state = RuntimeState::Running; + if !matches!(self.rt().state, RuntimeState::MainExited(_)) { + self.rt_mut().state = RuntimeState::Running; + } Ok(()) } @@ -938,35 +944,29 @@ impl Sandbox { } } - /// Wait for the child process to exit. - /// - /// Dropping the returned future does not cost the captured output: the pipe - /// drains belong to the `Sandbox`, so a caller that cancels this `wait()` - /// (a `timeout` or `select!` around it) and calls `wait()` again still gets - /// what the child wrote. That holds wherever the cancellation lands — - /// before the drains start, while the child is still running, or while the - /// final join is blocked because a descendant is still holding the write - /// end open past the child's own exit. - pub async fn wait(&mut self) -> Result { + /// Snapshot of the sandbox's remaining process group IDs, sorted by ID. + /// Membership may change as soon as this returns. + pub fn process_groups(&self) -> Result, crate::error::SandlockError> { use crate::error::SandboxRuntimeError; - use crate::result::RunResult; + let rt = self.runtime.as_ref() + .filter(|rt| rt.child_pid.is_some()) + .ok_or(SandboxRuntimeError::NotRunning)?; + Ok(rt.groups.live().map_err(SandboxRuntimeError::Io)?) + } - let pid = self.rt().child_pid.ok_or(SandboxRuntimeError::NotRunning)?; - - // Already reaped: hand back the same status again, plus whatever the - // drains collected. They outlive the `wait()` that started them, so a - // second call still reports the output of a first one that was - // cancelled — including one cancelled while joining them here, since a - // join only unparks a drain once it has completed. A `wait()` that ran - // to the end took them with it, and both streams are then `None`. - let stopped = match self.rt().state { - RuntimeState::Stopped(ref es) => Some(es.clone()), - _ => 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, changes }); + /// Wait only for the main process, keeping background jobs supervised. + /// + /// Returns the same status on repeated calls. Capture pipes keep draining + /// until `shutdown()` collects them; this does not wait for EOF + /// or settle the COW branch. Untaken piped stdin is closed here. + /// Call `shutdown()` to terminate remaining jobs and collect their output. + pub async fn wait(&mut self) -> Result { + use crate::error::SandboxRuntimeError; + let rt = self.runtime.as_ref().ok_or(SandboxRuntimeError::NotRunning)?; + let pid = rt.child_pid.ok_or(SandboxRuntimeError::NotRunning)?; + match &rt.state { + RuntimeState::MainExited(status) | RuntimeState::Stopped(status) => return Ok(status.clone()), + _ => {} } // Deliver EOF to a piped stdin the caller never took: otherwise a child @@ -987,7 +987,7 @@ impl Sandbox { // // The drains are parked in the runtime rather than in this future: a // caller that cancels `wait()` (a `timeout` or `select!` around it) must - // be able to `wait()` again and still be given the output. The + // be able to `shutdown()` later and still be given the output. The // `is_none()` guard is what makes that second call reuse them instead of // starting a second reader. A stream the caller took through // `Process::take_stdout`/`take_stderr` is `None` here and stays theirs. @@ -1002,37 +1002,66 @@ impl Sandbox { } } - // Wait for the top-level child to exit. Prefer the child's pidfd via - // `AsyncFd`: pidfd readiness fires only on *exit*, so — unlike a - // `waitpid` loop — it never consumes the child's ptrace-stops, which - // the `policy_fn` fork-tracking worker reaps (`waitpid` with any flags - // reaps a tracee's ptrace-stops, so a concurrent `waitpid` here would - // race the worker for fork events and hang it). Mirrors - // `spawn_pid_watcher`. Falls back to a blocking `waitpid` only when no - // pidfd is available (kernel without `pidfd_open`). - let exit_status = match self.rt_mut().pidfd.take() { - Some(pidfd) => wait_child_exit_via_pidfd(pidfd, pid).await, - None => wait_child_exit_blocking(pid).await, + let exit_status = if let Some(reaper) = &self.rt().reaper { + sandbox_wait_status_to_exit(reaper.main_status().await.map_err(SandboxRuntimeError::Io)?) + } else { + match self.rt().pidfd.as_ref() { + Some(pidfd) => { + let pidfd = pidfd.try_clone().map_err(SandboxRuntimeError::Io)?; + wait_child_exit_via_pidfd(pidfd, pid).await + } + None => wait_child_exit_blocking(pid).await, + } }; - // The sandbox ends with its main process: descendants left behind - // would run on with every supervised syscall failing, holding the - // caller's stdio. - let _ = self.rt().groups.signal(libc::SIGKILL); - - self.rt_mut().state = RuntimeState::Stopped(exit_status.clone()); + self.rt_mut().state = RuntimeState::MainExited(exit_status.clone()); if self.rt().tty_foreground_taken { sandbox_restore_tty_foreground(pid); self.rt_mut().tty_foreground_taken = false; } + Ok(exit_status) + } + + /// Kill remaining jobs, reap owned children, and release supervision. + /// Returns the main process's status and collected output, settling COW + /// only after all tracked groups have exited. Safe to call repeatedly. + pub async fn shutdown(&mut self) -> Result { + use crate::error::SandboxRuntimeError; + use crate::result::RunResult; + if let Some(reaper) = self.runtime.as_mut().and_then(|rt| rt.reaper.as_mut()) { + reaper.shutdown(); + } + self.kill()?; + let exit_status = self.wait().await?; + loop { + if self.rt().groups.signal(libc::SIGKILL).map_err(SandboxRuntimeError::Io)? == 0 { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + if let Some(reaper) = &self.rt().reaper { + loop { + let ret = unsafe { libc::waitpid(reaper.pid, std::ptr::null_mut(), libc::WNOHANG) }; + if ret > 0 { break; } + if ret < 0 { + let e = std::io::Error::last_os_error(); + if e.raw_os_error() == Some(libc::EINTR) { continue; } + return Err(SandboxRuntimeError::Io(e).into()); + } + tokio::time::sleep(std::time::Duration::from_millis(1)).await; + } + } + self.rt_mut().reaper = None; + self.rt_mut().state = RuntimeState::Stopped(exit_status.clone()); let rt = self.rt_mut(); rt.control_claim = None; + rt.http_acl_handle = None; if let Some(h) = rt.notif_handle.take() { h.abort(); } rt.policy_fn_worker = None; if let Some(h) = rt.throttle_handle.take() { h.abort(); } if let Some(h) = rt.loadavg_handle.take() { h.abort(); } - // Finish shutting down the old endpoint before wait() returns. + // Finish shutting down the old endpoint before releasing the name. if let Some(h) = rt.control_handle.take() { h.abort(); let _ = h.await; @@ -1083,7 +1112,7 @@ impl Sandbox { /// 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))) + matches!(rt.state, RuntimeState::MainExited(ref s) | RuntimeState::Stopped(ref s) if !matches!(s, crate::result::ExitStatus::Code(0))) }); if failed { self.on_error.clone() } else { self.on_exit.clone() } } @@ -1156,7 +1185,7 @@ impl Sandbox { /// completes; see `finish_parked_drain`. /// /// `None` means the stream was never piped, was taken by the caller, or was - /// already collected by an earlier `wait()`; a drain that panicked or was + /// already collected by an earlier `shutdown()`; a drain that panicked or was /// aborted reports empty bytes rather than failing the run. async fn collect_pipe_drains(&mut self) -> (Option>, Option>) { // Finish both first — each stores its bytes into the runtime as it @@ -1174,7 +1203,7 @@ impl Sandbox { /// supervisor + rlimits + landlock + COW + network/HTTP proxies). /// The child is parked between policy install and `execve`; call /// `start()` to release it. Stdout/stderr are captured for later - /// retrieval via `wait()`. + /// retrieval via `shutdown()`. pub async fn create(&mut self, cmd: &[&str]) -> Result<(), crate::error::SandlockError> { self.do_create(cmd, true).await } @@ -1515,7 +1544,8 @@ impl Sandbox { ) -> Result { self.do_create(cmd, true).await?; self.do_start()?; - self.wait().await + self.wait().await?; + self.shutdown().await } /// Run with inherited stdio (interactive mode). @@ -1525,7 +1555,8 @@ impl Sandbox { ) -> Result { self.do_create(cmd, false).await?; self.do_start()?; - self.wait().await + self.wait().await?; + self.shutdown().await } /// One-shot run with user-supplied syscall handlers. @@ -1544,7 +1575,8 @@ impl Sandbox { self.rt_mut().handlers = pending; self.do_create(cmd, true).await?; self.do_start()?; - self.wait().await + self.wait().await?; + self.shutdown().await } /// Interactive-stdio counterpart of `run_with_handlers`. @@ -1563,7 +1595,8 @@ impl Sandbox { self.rt_mut().handlers = pending; self.do_create(cmd, false).await?; self.do_start()?; - self.wait().await + self.wait().await?; + self.shutdown().await } /// Create N COW clones of this sandbox. @@ -1687,6 +1720,7 @@ impl Sandbox { crate::result::ExitStatus::Killed }), child_pid: Some(clone_pid), + reaper: None, pidfd: None, groups: Arc::new(crate::pgroup::ProcessGroups::new()), notif_handle: None, @@ -1766,7 +1800,8 @@ impl Sandbox { unsafe { libc::close(stdin_fds[0]) }; let _ = write_handle.await; - reducer.wait().await + reducer.wait().await?; + reducer.shutdown().await } /// Whether named (pathname) `AF_UNIX` connects should be gated by the @@ -1792,6 +1827,7 @@ impl Sandbox { name, state: RuntimeState::Created, child_pid: None, + reaper: None, pidfd: None, groups: Arc::new(crate::pgroup::ProcessGroups::new()), notif_handle: None, @@ -2050,10 +2086,6 @@ impl Sandbox { None }; - // Capture our PID before fork so the child can detect parent death - // without assuming PID 1 is always init (wrong in containers). - let parent_pid = unsafe { libc::getpid() }; - // Interactive (fully inherited) stdio on a terminal: the child will // take the tty foreground group, and this process must take it back // once the child is reaped. @@ -2085,12 +2117,11 @@ impl Sandbox { }; let child_publisher = control_socket.as_ref().and_then(|s| s.claim.as_ref()).map(|c| c.child_publisher()); - let pid = crate::control::fork_without_control_fds(); - if pid < 0 { - return Err(SandboxRuntimeError::Fork(std::io::Error::last_os_error()).into()); - } + let (pid, reaper) = crate::reaper::fork().map_err(SandboxRuntimeError::Fork)?; + self.rt_mut().reaper = reaper; if pid == 0 { + let parent_pid = unsafe { libc::getppid() }; // ===== CHILD PROCESS ===== // Publish only after the process group exists, before confinement denies IPC. if unsafe { libc::setpgid(0, 0) } != 0 { @@ -2190,14 +2221,12 @@ impl Sandbox { // State remains `Created` until `do_start` writes ready_w to release // the child to execve. - let pidfd = match syscall::pidfd_open(pid as u32, 0) { - Ok(fd) => Some(fd), - Err(_) => None, - }; + let pidfd = Some(self.rt().reaper.as_ref().expect("parent has a reaper") + .main_pidfd.try_clone().map_err(SandboxRuntimeError::Io)?); // The child's first act is setpgid(0, 0), so its pid names its group. - if let Some(leader) = pidfd.as_ref().and_then(|pfd| pfd.try_clone().ok()) { - self.rt_mut().groups.track(pid, leader); - } + let leader = pidfd.as_ref().expect("reaper pins the child") + .try_clone().map_err(SandboxRuntimeError::Io)?; + self.rt_mut().groups.track(pid, leader); let notif_fd_num = read_u32_fd(pipes.notif_r.as_raw_fd()) .map_err(|e| SandboxRuntimeError::Child(format!("read notif fd from child: {}", e)))?; @@ -2534,9 +2563,9 @@ impl Sandbox { /// the process is killed and reaped when that `Sandbox` is dropped, or eagerly /// via [`Process::kill`]. /// -/// A `Process` that is dropped without [`Process::wait`] leaves the child -/// running until the `Sandbox` is dropped — call `wait` (or `kill`) to end it. -#[must_use = "a Process is a live confined child; call wait() (or kill()) or it runs until the Sandbox is dropped"] +/// Dropping a `Process` leaves the session owned by the `Sandbox`. Call +/// [`Process::shutdown`] to terminate its jobs and collect captured output. +#[must_use = "a Process is a live confined child; call shutdown() to end the session or it runs until the Sandbox is dropped"] pub struct Process<'a> { sandbox: &'a mut Sandbox, } @@ -2579,14 +2608,21 @@ impl Process<'_> { self.sandbox.kill() } - /// Wait for the child to exit. Any `Piped` stdout/stderr the caller did not - /// take is drained into the returned `RunResult`; taken streams are `None` - /// because the caller owns them. An untaken piped stdin is closed here so the - /// child sees EOF; a *taken* stdin the caller must close itself first (see - /// [`Process::take_stdin`]) or this blocks forever. - pub async fn wait(self) -> Result { + /// Wait for the main process while background jobs remain supervised. + pub async fn wait(&mut self) -> Result { self.sandbox.wait().await } + + /// Snapshot of the sandbox's remaining process group IDs. + pub fn process_groups(&self) -> Result, crate::error::SandlockError> { + self.sandbox.process_groups() + } + + /// Terminate remaining jobs and collect the main status and untaken output. + pub async fn shutdown(self) -> Result { + self.sandbox.shutdown().await + } + } /// Hand the terminal's foreground process group back to this process after @@ -2618,14 +2654,21 @@ impl Drop for Sandbox { fn drop(&mut self) { if let Some(ref mut rt) = self.runtime { if let Some(worker) = &rt.policy_fn_worker { worker.cancel(); } + let _ = rt.groups.signal(libc::SIGKILL); + if let Some(reaper) = &mut rt.reaper { + reaper.shutdown(); + use std::os::fd::AsRawFd; + unsafe { libc::syscall(libc::SYS_pidfd_send_signal, reaper.main_pidfd.as_raw_fd(), libc::SIGKILL, 0, 0) }; + } if let Some(pid) = rt.child_pid { if matches!(rt.state, RuntimeState::Created | RuntimeState::Running | RuntimeState::Paused) { - let _ = rt.groups.signal(libc::SIGKILL); // The waitpid below must not hang on a child whose group // was never recorded. Unreaped, its pid is still ours. - unsafe { libc::kill(pid, libc::SIGKILL) }; - let mut status: i32 = 0; - unsafe { libc::waitpid(pid, &mut status, 0) }; + if rt.reaper.is_none() { + unsafe { libc::kill(pid, libc::SIGKILL) }; + let mut status: i32 = 0; + unsafe { libc::waitpid(pid, &mut status, 0) }; + } } if rt.tty_foreground_taken { sandbox_restore_tty_foreground(pid); @@ -2633,6 +2676,13 @@ impl Drop for Sandbox { } } + if let Some(reaper) = rt.reaper.take() { + loop { + if unsafe { libc::waitpid(reaper.pid, std::ptr::null_mut(), 0) } >= 0 { break; } + if std::io::Error::last_os_error().raw_os_error() != Some(libc::EINTR) { break; } + } + } + if let Some(h) = rt.notif_handle.take() { h.abort(); } rt.policy_fn_worker = None; if let Some(h) = rt.throttle_handle.take() { h.abort(); } @@ -2649,8 +2699,8 @@ impl Drop for Sandbox { } - // wait() settles the branch, so this only sees one it left behind: a - // deferred branch nobody decided on, or one a cancelled wait() never + // shutdown() settles the branch, so this only sees one it left behind: a + // deferred branch nobody decided on, or one a cancelled shutdown() never // reached. let action = self.branch_action(); #[cfg(feature = "cow")] diff --git a/crates/sandlock-core/src/transaction.rs b/crates/sandlock-core/src/transaction.rs index 14d8fc9ed..9a9dd76e5 100644 --- a/crates/sandlock-core/src/transaction.rs +++ b/crates/sandlock-core/src/transaction.rs @@ -738,7 +738,8 @@ async fn drive_txn_stages( let drain = tokio::task::spawn_blocking(move || drain_capped_tee(read_fd, STAGE_STDERR_CAP, tee)); - let mut result = sb.wait().await.map_err(at)?; + sb.wait().await.map_err(at)?; + let mut result = sb.shutdown().await.map_err(at)?; // wait() returns stderr: None on the inherit path; the captured bytes are // authoritative. result.stderr = Some(drain.await.unwrap_or_default()); diff --git a/crates/sandlock-core/tests/integration/test_branch_action.rs b/crates/sandlock-core/tests/integration/test_branch_action.rs index 6dae271f9..da32d1c0f 100644 --- a/crates/sandlock-core/tests/integration/test_branch_action.rs +++ b/crates/sandlock-core/tests/integration/test_branch_action.rs @@ -201,3 +201,43 @@ async fn commit_and_abort_need_a_pending_branch() { let _ = fs::remove_dir_all(&workdir); let _ = fs::remove_dir_all(&storage); } + +#[tokio::test] +async fn background_changes_are_committed_only_at_shutdown() { + use std::time::Duration; + let workdir = tempfile::tempdir().unwrap(); + let storage = tempfile::tempdir().unwrap(); + let control = tempfile::tempdir().unwrap(); + let ready = control.path().join("ready"); + let release = control.path().join("release"); + let done = control.path().join("done"); + let script = format!(r#" +import os, time +if os.fork() == 0: + open({ready:?}, "w").close() + while not os.path.exists({release:?}): + time.sleep(0.01) + open("late.txt", "w").write("background change") + open({done:?}, "w").close() + while True: + time.sleep(1) +while not os.path.exists({ready:?}): + time.sleep(0.01) +os._exit(0) +"#); + let mut sb = policy(workdir.path(), storage.path()) + .fs_write(control.path()).build().unwrap(); + sb.spawn(&["python3", "-c", &script]).await.unwrap(); + assert_eq!(sb.wait().await.unwrap(), sandlock_core::ExitStatus::Code(0)); + fs::write(release, "go").unwrap(); + tokio::time::timeout(Duration::from_secs(10), async { + while !done.exists() { + tokio::time::sleep(Duration::from_millis(10)).await; + } + }).await.unwrap(); + assert!(!workdir.path().join("late.txt").exists()); + let result = sb.shutdown().await.unwrap(); + assert!(result.success()); + assert_eq!(fs::read_to_string(workdir.path().join("late.txt")).unwrap(), "background change"); + assert!(result.changes.iter().any(|c| c.path == Path::new("late.txt"))); +} diff --git a/crates/sandlock-core/tests/integration/test_control.rs b/crates/sandlock-core/tests/integration/test_control.rs index a0eb34bf3..2aff35eb9 100644 --- a/crates/sandlock-core/tests/integration/test_control.rs +++ b/crates/sandlock-core/tests/integration/test_control.rs @@ -748,7 +748,7 @@ async fn test_control_parked_child_does_not_pin_other_names() { parked.create(&["true"]).await.unwrap(); first.start().unwrap(); - first.wait().await.unwrap(); + async { first.wait().await?; first.shutdown().await }.await.unwrap(); let again = policy .clone() @@ -756,7 +756,7 @@ async fn test_control_parked_child_does_not_pin_other_names() { .run(&["true"]) .await; parked.start().unwrap(); - let _ = parked.wait().await; + let _ = async { parked.wait().await?; parked.shutdown().await }.await; assert!(again.is_ok(), "a parked sibling must not pin the name: {:?}", again.err()); } @@ -773,7 +773,7 @@ async fn test_control_child_pid_is_published_before_start() { assert_eq!(pids.supervisor, std::process::id() as i32); } sb.start().unwrap(); - assert!(sb.wait().await.unwrap().success()); + assert!(async { sb.wait().await?; sb.shutdown().await }.await.unwrap().success()); } // ============================================================ diff --git a/crates/sandlock-core/tests/integration/test_fork.rs b/crates/sandlock-core/tests/integration/test_fork.rs index 363f91198..53355edcc 100644 --- a/crates/sandlock-core/tests/integration/test_fork.rs +++ b/crates/sandlock-core/tests/integration/test_fork.rs @@ -29,7 +29,8 @@ async fn test_fork_basic() { // Each clone is a live Sandbox — wait for all for c in clones.iter_mut() { - let r = c.wait().await.unwrap(); + c.wait().await.unwrap(); + let r = c.shutdown().await.unwrap(); assert!(r.success()); } @@ -61,7 +62,7 @@ async fn test_fork_cow_sharing() { }); let mut clones = sb.fork(3).await.unwrap(); - for c in clones.iter_mut() { let _ = c.wait().await; } + for c in clones.iter_mut() { let _ = async { c.wait().await?; c.shutdown().await }.await; } for i in 0..3u32 { let content = std::fs::read_to_string(out_dir.join(format!("{}", i))).unwrap_or_default(); @@ -87,7 +88,7 @@ async fn test_fork_clone_id_env() { }); let mut clones = sb.fork(3).await.unwrap(); - for c in clones.iter_mut() { let _ = c.wait().await; } + for c in clones.iter_mut() { let _ = async { c.wait().await?; c.shutdown().await }.await; } for i in 0..3u32 { let content = std::fs::read_to_string(out_dir.join(format!("{}", i))).unwrap_or_default(); @@ -142,7 +143,8 @@ async fn test_fork_clone_exit_status() { // All clones should have succeeded (already finished) for (i, c) in clones.iter_mut().enumerate() { - let r = c.wait().await.unwrap(); + c.wait().await.unwrap(); + let r = c.shutdown().await.unwrap(); assert!(r.success(), "clone {} should succeed", i); } diff --git a/crates/sandlock-core/tests/integration/test_policy_fn.rs b/crates/sandlock-core/tests/integration/test_policy_fn.rs index b613f5057..ac4d96f02 100644 --- a/crates/sandlock-core/tests/integration/test_policy_fn.rs +++ b/crates/sandlock-core/tests/integration/test_policy_fn.rs @@ -881,7 +881,8 @@ async fn unlimited_approval_survives_old_five_second_limit() { .unwrap(); let mut sandbox = policy.clone(); sandbox.spawn(&["/bin/sleep", "0.1"]).await.unwrap(); - let result = sandbox.wait().await.unwrap(); + sandbox.wait().await.unwrap(); + let result = sandbox.shutdown().await.unwrap(); assert!( result.success(), "unlimited approval must accept a late verdict" @@ -945,7 +946,7 @@ async fn shutdown_cancels_unlimited_approval() { }) .await .expect("kill must cancel before wait or drop"); - tokio::time::timeout(Duration::from_secs(2), sandbox.wait()) + tokio::time::timeout(Duration::from_secs(2), async { sandbox.wait().await?; sandbox.shutdown().await }) .await .unwrap() .unwrap(); diff --git a/crates/sandlock-core/tests/integration/test_popen.rs b/crates/sandlock-core/tests/integration/test_popen.rs index ace53d226..ad0151c74 100644 --- a/crates/sandlock-core/tests/integration/test_popen.rs +++ b/crates/sandlock-core/tests/integration/test_popen.rs @@ -44,7 +44,8 @@ async fn test_popen_pipe_roundtrip() { stdout.read_to_string(&mut out).unwrap(); assert_eq!(out, "hello\n"); - let res = child.wait().await.unwrap(); + child.wait().await.unwrap(); + let res = child.shutdown().await.unwrap(); assert!(res.success()); } @@ -62,7 +63,7 @@ async fn test_popen_stdout_only() { let mut out = String::new(); stdout.read_to_string(&mut out).unwrap(); assert_eq!(out, "hi\n"); - assert!(child.wait().await.unwrap().success()); + assert!(async { child.wait().await?; child.shutdown().await }.await.unwrap().success()); } /// Null stdout is actually wired to `/dev/null`, not left inherited. The child @@ -71,7 +72,7 @@ async fn test_popen_stdout_only() { #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn test_popen_null_stdout_is_dev_null() { let mut sb = base().with_name("popen-null"); - let child = sb + let mut child = sb .popen( // Stat the *shell's* fd 1 via $$ — `$(...)` would rebind fd 1 for the // inner stat itself, so /proc/self/fd/1 there is the substitution pipe. @@ -82,7 +83,8 @@ async fn test_popen_null_stdout_is_dev_null() { ) .await .unwrap(); - let res = child.wait().await.unwrap(); + child.wait().await.unwrap(); + let res = child.shutdown().await.unwrap(); assert!(res.success(), "fd 1 was not /dev/null (1:3): exit={:?}", res.exit_status); } @@ -100,7 +102,7 @@ async fn test_popen_stderr_piped() { let mut out = String::new(); stderr.read_to_string(&mut out).unwrap(); assert_eq!(out, "oops\n"); - assert!(child.wait().await.unwrap().success()); + assert!(async { child.wait().await?; child.shutdown().await }.await.unwrap().success()); } /// A piped stdin the caller never takes must still reach EOF at `wait`, or a @@ -109,12 +111,13 @@ async fn test_popen_stderr_piped() { #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn test_popen_untaken_stdin_reaches_eof() { let mut sb = base().with_name("popen-stdin-eof"); - let child = sb + let mut child = sb .popen(&["cat"], StdioMode::Piped, StdioMode::Null, StdioMode::Inherit) .await .unwrap(); // Deliberately do not take stdin. cat reads to EOF; wait must deliver it. - let res = child.wait().await.unwrap(); + child.wait().await.unwrap(); + let res = child.shutdown().await.unwrap(); assert!(res.success(), "cat did not exit — untaken stdin never EOF'd"); } @@ -178,7 +181,8 @@ async fn test_wait_kills_descendants_the_main_process_left_behind() { let bg_pid: i32 = line.trim().parse().expect("background pid"); assert!(unsafe { libc::kill(bg_pid, 0) } == 0, "background job should be alive"); - let res = child.wait().await.unwrap(); + child.wait().await.unwrap(); + let res = child.shutdown().await.unwrap(); assert!(res.success()); // A killed orphan stays a zombie until init/subreaper reaps it. @@ -225,18 +229,18 @@ async fn test_popen_all_three_piped() { stderr.read_to_string(&mut err).unwrap(); assert_eq!(out, "data\n"); assert_eq!(err, "err\n"); - assert!(child.wait().await.unwrap().success()); + assert!(async { child.wait().await?; child.shutdown().await }.await.unwrap().success()); } /// Null stdin delivers immediate EOF: `cat` reading /dev/null exits 0 at once. #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn test_popen_null_stdin_is_eof() { let mut sb = base().with_name("popen-null-stdin"); - let child = sb + let mut child = sb .popen(&["cat"], StdioMode::Null, StdioMode::Null, StdioMode::Inherit) .await .unwrap(); - assert!(child.wait().await.unwrap().success()); + assert!(async { child.wait().await?; child.shutdown().await }.await.unwrap().success()); } /// Explicit Process::kill terminates the child; wait still collects a @@ -249,7 +253,8 @@ async fn test_popen_explicit_kill_then_wait() { .await .unwrap(); child.kill().expect("kill"); - let res = child.wait().await.unwrap(); + child.wait().await.unwrap(); + let res = child.shutdown().await.unwrap(); assert!(!res.success(), "a killed process must not report success"); } @@ -258,11 +263,11 @@ async fn test_popen_explicit_kill_then_wait() { #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn test_popen_rejects_second_spawn() { let mut sb = base().with_name("popen-twice"); - let first = sb + let mut first = sb .popen(&["echo", "first"], StdioMode::Inherit, StdioMode::Inherit, StdioMode::Inherit) .await .unwrap(); - first.wait().await.unwrap(); // consume the Process → releases the &mut borrow + async { first.wait().await?; first.shutdown().await }.await.unwrap(); // consume the Process → releases the &mut borrow let second = sb .popen(&["echo", "second"], StdioMode::Inherit, StdioMode::Inherit, StdioMode::Inherit) @@ -293,7 +298,8 @@ async fn test_popen_taken_stdout_is_not_drained_by_wait() { stdout.read_to_string(&mut out).unwrap(); assert_eq!(out, "out", "the taken stream must still carry the child's output"); - let res = child.wait().await.unwrap(); + child.wait().await.unwrap(); + let res = child.shutdown().await.unwrap(); assert!(res.success()); assert!(res.stdout.is_none(), "a taken stdout must be None in the RunResult"); assert_eq!( diff --git a/crates/sandlock-core/tests/integration/test_process_groups.rs b/crates/sandlock-core/tests/integration/test_process_groups.rs index bdc3b6aaf..68883d493 100644 --- a/crates/sandlock-core/tests/integration/test_process_groups.rs +++ b/crates/sandlock-core/tests/integration/test_process_groups.rs @@ -115,9 +115,9 @@ import os, signal pid = os.fork() if pid == 0: os.setsid() + open("{}.tmp", "w").write(str(os.getpid())) + os.rename("{}.tmp", "{}") signal.pause() -open("{}.tmp", "w").write(str(pid)) -os.rename("{}.tmp", "{}") signal.pause() "#, pid_file.display(), @@ -156,7 +156,10 @@ async fn test_pause_stops_a_process_that_left_the_main_group() { let state = || { std::fs::read_to_string(format!("/proc/{leader}/stat")) .ok() - .and_then(|s| s.rsplit_once(") ").map(|(_, rest)| rest.chars().next().unwrap())) + .and_then(|s| { + s.rsplit_once(") ") + .map(|(_, rest)| rest.chars().next().unwrap()) + }) }; let mut stopped = false; for _ in 0..300 { @@ -166,7 +169,10 @@ async fn test_pause_stops_a_process_that_left_the_main_group() { } std::thread::sleep(std::time::Duration::from_millis(10)); } - assert!(stopped, "session leader {leader} kept running through pause()"); + assert!( + stopped, + "session leader {leader} kept running through pause()" + ); sb.resume().unwrap(); sb.kill().unwrap(); @@ -203,6 +209,265 @@ os.rename("{0}.tmp", "{0}") .unwrap(); assert!(result.success()); - let leader: i32 = std::fs::read_to_string(&pid_file).unwrap().trim().parse().unwrap(); - assert!(wait_gone(leader), "session leader {leader} outlived the main process"); + let leader: i32 = std::fs::read_to_string(&pid_file) + .unwrap() + .trim() + .parse() + .unwrap(); + assert!( + wait_gone(leader), + "session leader {leader} outlived the main process" + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_wait_keeps_background_jobs_supervised() { + use sandlock_core::result::ExitStatus; + use std::time::Duration; + + let mut subreaper_before = 0i32; + assert_eq!( + unsafe { libc::prctl(libc::PR_GET_CHILD_SUBREAPER, &mut subreaper_before) }, + 0 + ); + let dir = tempfile::tempdir().unwrap(); + let ready = dir.path().join("ready"); + let release = dir.path().join("release"); + let done = dir.path().join("done"); + let script = format!( + r#" +import os, time +pid = os.fork() +if pid == 0: + os.setsid() + open({ready:?}, "w").write(str(os.getpid())) + while not os.path.exists({release:?}): + time.sleep(0.01) + print("background output", flush=True) + open({done:?}, "w").write("supervised") + while True: + time.sleep(1) +while not os.path.exists({ready:?}): + time.sleep(0.01) +os._exit(7) +"# + ); + let mut sb = policy(); + sb.fs_writable.push(dir.path().to_path_buf()); + sb.spawn(&["python3", "-c", &script]).await.unwrap(); + assert_eq!( + tokio::time::timeout(Duration::from_secs(10), sb.wait()) + .await + .unwrap() + .unwrap(), + ExitStatus::Code(7) + ); + let mut subreaper_after = 0i32; + assert_eq!( + unsafe { libc::prctl(libc::PR_GET_CHILD_SUBREAPER, &mut subreaper_after) }, + 0 + ); + assert_eq!(subreaper_before, subreaper_after); + assert_eq!(sb.wait().await.unwrap(), ExitStatus::Code(7)); + assert!(!sb.is_running()); + let leader: i32 = std::fs::read_to_string(&ready).unwrap().parse().unwrap(); + assert_eq!(sb.process_groups().unwrap(), vec![leader]); + sb.pause().unwrap(); + sb.resume().unwrap(); + assert_eq!(sb.wait().await.unwrap(), ExitStatus::Code(7)); + std::fs::write(release, "go").unwrap(); + tokio::time::timeout(Duration::from_secs(10), async { + while !done.exists() { + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .expect("background job lost supervision"); + let result = tokio::time::timeout(Duration::from_secs(10), sb.shutdown()) + .await + .unwrap() + .unwrap(); + assert_eq!(result.exit_status, ExitStatus::Code(7)); + assert_eq!(result.stdout_str(), Some("background output")); + assert!(sb.process_groups().unwrap().is_empty()); + assert!(wait_gone(leader)); + assert_eq!( + sb.shutdown().await.unwrap().exit_status, + ExitStatus::Code(7) + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_drop_after_wait_cleans_background_jobs() { + use std::time::Duration; + let mut sb = policy(); + let leader = spawn_with_session_leader(&mut sb).await; + unsafe { libc::kill(sb.pid().unwrap(), libc::SIGTERM) }; + tokio::time::timeout(Duration::from_secs(10), sb.wait()) + .await + .unwrap() + .unwrap(); + drop(sb); + assert!(wait_gone(leader)); +} + +#[tokio::test] +async fn test_wait_without_a_child_returns_an_error() { + let mut sb = policy(); + assert!(sb.wait().await.is_err()); + assert!(sb.shutdown().await.is_err()); + assert!(sb.process_groups().is_err()); +} + +#[tokio::test] +async fn test_cancelled_wait_preserves_status_and_output() { + use sandlock_core::result::ExitStatus; + use std::time::Duration; + let mut sb = policy(); + sb.spawn(&["sh", "-c", "echo before; sleep 0.2; echo after; exit 23"]) + .await + .unwrap(); + assert!(tokio::time::timeout(Duration::from_millis(20), sb.wait()) + .await + .is_err()); + assert_eq!(sb.wait().await.unwrap(), ExitStatus::Code(23)); + let result = sb.shutdown().await.unwrap(); + assert_eq!(result.exit_status, ExitStatus::Code(23)); + assert_eq!(result.stdout_str(), Some("before\nafter")); +} + +#[test] +fn test_shutdown_reaps_adopted_jobs_without_reaping_unrelated_children() { + const CHILD: &str = "SANDLOCK_TEST_BACKGROUND_SUBREAPER"; + if std::env::var_os(CHILD).is_none() { + let status = std::process::Command::new(std::env::current_exe().unwrap()) + .args(["test_process_groups::test_shutdown_reaps_adopted_jobs_without_reaping_unrelated_children", "--exact", "--nocapture"]) + .env(CHILD, "1") + .status().unwrap(); + assert!(status.success()); + return; + } + assert_eq!( + unsafe { libc::prctl(libc::PR_SET_CHILD_SUBREAPER, 1, 0, 0, 0) }, + 0 + ); + let mut unrelated = std::process::Command::new("sh") + .args(["-c", "exit 29"]) + .spawn() + .unwrap(); + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + rt.block_on(async { + let mut sb = policy(); + let leader = spawn_with_session_leader(&mut sb).await; + unsafe { libc::kill(sb.pid().unwrap(), libc::SIGTERM) }; + sb.wait().await.unwrap(); + tokio::time::timeout(std::time::Duration::from_secs(10), sb.shutdown()) + .await + .unwrap() + .unwrap(); + assert_eq!( + unsafe { libc::waitpid(leader, std::ptr::null_mut(), libc::WNOHANG) }, + -1 + ); + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::ECHILD) + ); + assert!(wait_gone(leader)); + }); + assert_eq!(unrelated.wait().unwrap().code(), Some(29)); +} + +#[test] +fn test_creation_under_fd_pressure_does_not_leave_children() { + const CHILD: &str = "SANDLOCK_TEST_BACKGROUND_FD_PRESSURE"; + if std::env::var_os(CHILD).is_none() { + let mut child = std::process::Command::new(std::env::current_exe().unwrap()) + .args([ + "test_process_groups::test_creation_under_fd_pressure_does_not_leave_children", + "--exact", + "--nocapture", + ]) + .env(CHILD, "1") + .spawn() + .unwrap(); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(15); + loop { + if let Some(status) = child.try_wait().unwrap() { + assert!(status.success()); + return; + } + if std::time::Instant::now() >= deadline { + child.kill().unwrap(); + let _ = child.wait(); + panic!("sandbox creation or cleanup hung under fd pressure"); + } + std::thread::sleep(std::time::Duration::from_millis(10)); + } + } + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + let mut limit: libc::rlimit = unsafe { std::mem::zeroed() }; + assert_eq!( + unsafe { libc::getrlimit(libc::RLIMIT_NOFILE, &mut limit) }, + 0 + ); + for budget in [16, 20, 24, 28, 32] { + let mut sb = policy(); + let constrained = libc::rlimit { + rlim_cur: budget, + rlim_max: limit.rlim_max, + }; + assert_eq!( + unsafe { libc::setrlimit(libc::RLIMIT_NOFILE, &constrained) }, + 0 + ); + let _ = rt.block_on(sb.create(&["sleep", "30"])); + drop(sb); + assert_eq!(unsafe { libc::setrlimit(libc::RLIMIT_NOFILE, &limit) }, 0); + assert_eq!( + unsafe { libc::waitpid(-1, std::ptr::null_mut(), libc::WNOHANG) }, + -1 + ); + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::ECHILD) + ); + } +} + +#[tokio::test] +async fn test_shutdown_reaps_untracked_groups_without_syscall_supervision() { + let dir = tempfile::tempdir().unwrap(); + let ready = dir.path().join("ready"); + let script = format!( + r#" +import os, signal, time +if os.fork() == 0: + os.setsid() + open({ready:?}, "w").write(str(os.getpid())) + signal.pause() +while not os.path.exists({ready:?}): + time.sleep(0.01) +"# + ); + let mut sb = policy(); + sb.no_supervisor = true; + sb.fs_writable.push(dir.path().to_path_buf()); + sb.spawn(&["python3", "-c", &script]).await.unwrap(); + sb.wait().await.unwrap(); + let leader: i32 = std::fs::read_to_string(ready).unwrap().parse().unwrap(); + let cleanup = tokio::time::timeout(std::time::Duration::from_secs(5), sb.shutdown()).await; + if cleanup.is_err() { + unsafe { libc::kill(leader, libc::SIGKILL) }; + } + cleanup + .expect("shutdown must also clean untracked descendants") + .unwrap(); + assert!(wait_gone(leader)); } diff --git a/crates/sandlock-core/tests/integration/test_resource.rs b/crates/sandlock-core/tests/integration/test_resource.rs index 8371d21df..32b182794 100644 --- a/crates/sandlock-core/tests/integration/test_resource.rs +++ b/crates/sandlock-core/tests/integration/test_resource.rs @@ -492,7 +492,8 @@ async fn test_spawn_and_kill() { sb.start().unwrap(); sb.kill().unwrap(); - let result = sb.wait().await.unwrap(); + sb.wait().await.unwrap(); + let result = sb.shutdown().await.unwrap(); assert!( matches!(result.exit_status, ExitStatus::Signal(_) | ExitStatus::Killed), "expected Signal or Killed, got {:?}", @@ -681,7 +682,8 @@ async fn test_pause_resume() { sb.resume().expect("resume should succeed"); sb.kill().unwrap(); - let result = sb.wait().await.unwrap(); + sb.wait().await.unwrap(); + let result = sb.shutdown().await.unwrap(); // Process should have been killed cleanly after pause/resume cycle assert!( diff --git a/crates/sandlock-core/tests/integration/test_restore.rs b/crates/sandlock-core/tests/integration/test_restore.rs index 7c28733a4..41d50e0b7 100644 --- a/crates/sandlock-core/tests/integration/test_restore.rs +++ b/crates/sandlock-core/tests/integration/test_restore.rs @@ -168,7 +168,7 @@ async fn test_restore_glibc_vdso_program_resumes() { // Clean up before asserting so a failure never leaks the child/files. let _ = sb2.kill(); - let exit = sb2.wait().await.map(|r| r.exit_status); + let exit = sb2.wait().await; let _ = std::fs::remove_dir_all(&tmp); assert!( diff --git a/crates/sandlock-core/tests/integration/test_sandbox.rs b/crates/sandlock-core/tests/integration/test_sandbox.rs index fce2666dd..74ce40c9c 100644 --- a/crates/sandlock-core/tests/integration/test_sandbox.rs +++ b/crates/sandlock-core/tests/integration/test_sandbox.rs @@ -180,7 +180,7 @@ async fn test_nested_sandbox() { // separately verifying that running Sandbox::run after another // Sandbox::run works (the seccomp filter stacks). outer_sb.kill().unwrap(); - let _ = outer_sb.wait().await; + let _ = async { outer_sb.wait().await?; outer_sb.shutdown().await }.await; // Sequential sandboxes: first sandbox applies Landlock + seccomp, // second sandbox from the same parent gets EBUSY on seccomp @@ -900,12 +900,12 @@ async fn test_cancelled_wait_keeps_the_capture_for_the_next_wait() { sb.spawn(&["sh", "-c", "printf hello; sleep 5"]).await.unwrap(); // The child sleeps well past this, so the first wait is cancelled mid-flight. - let first = tokio::time::timeout(std::time::Duration::from_millis(500), sb.wait()).await; + let first = tokio::time::timeout(std::time::Duration::from_millis(500), async { sb.wait().await?; sb.shutdown().await }).await; assert!(first.is_err(), "the child sleeps 5s: this wait must be cancelled, not complete"); sb.kill().unwrap(); - let result = tokio::time::timeout(std::time::Duration::from_secs(30), sb.wait()) + let result = tokio::time::timeout(std::time::Duration::from_secs(30), async { sb.wait().await?; sb.shutdown().await }) .await .expect("the second wait() hung") .unwrap(); @@ -945,7 +945,7 @@ fn test_capture_wait_does_not_park_blocking_pool_threads() { let mut quick = capture_policy().with_name("drain-quick"); - let idle_wait = idle.wait(); + let idle_wait = async { idle.wait().await?; idle.shutdown().await }; let quick_run = quick.run(&["echo", "hi"]); tokio::pin!(idle_wait); tokio::pin!(quick_run); diff --git a/crates/sandlock-ffi/src/lib.rs b/crates/sandlock-ffi/src/lib.rs index fa0c91c44..dd787fbaf 100644 --- a/crates/sandlock-ffi/src/lib.rs +++ b/crates/sandlock-ffi/src/lib.rs @@ -1440,7 +1440,7 @@ pub unsafe extern "C" fn sandlock_handle_wait(h: *mut sandlock_handle_t) -> *mut return ptr::null_mut(); } let h = &mut *h; - match block_on_runtime(&h.runtime, h.sandbox.wait()) { + match block_on_runtime(&h.runtime, async { h.sandbox.wait().await?; h.sandbox.shutdown().await }) { Some(Ok(result)) => Box::into_raw(Box::new(sandlock_result_t { _private: result })), _ => ptr::null_mut(), } @@ -1464,7 +1464,7 @@ pub unsafe extern "C" fn sandlock_handle_wait_timeout( if timeout_ms == 0 { // No timeout -- same as sandlock_handle_wait. - return match block_on_runtime(&h.runtime, h.sandbox.wait()) { + return match block_on_runtime(&h.runtime, async { h.sandbox.wait().await?; h.sandbox.shutdown().await }) { Some(Ok(result)) => Box::into_raw(Box::new(sandlock_result_t { _private: result })), _ => ptr::null_mut(), }; @@ -1472,7 +1472,7 @@ pub unsafe extern "C" fn sandlock_handle_wait_timeout( let dur = Duration::from_millis(timeout_ms); match block_on_runtime(&h.runtime, async { - tokio::time::timeout(dur, h.sandbox.wait()).await + tokio::time::timeout(dur, async { h.sandbox.wait().await?; h.sandbox.shutdown().await }).await }) { Some(Ok(Ok(result))) => Box::into_raw(Box::new(sandlock_result_t { _private: result })), Some(Ok(Err(_))) | None => ptr::null_mut(), @@ -2607,7 +2607,7 @@ pub unsafe extern "C" fn sandlock_wait(sb: *mut Sandbox) -> c_int { } let sb = &mut *sb; - match with_runtime(|rt| rt.block_on(sb.wait())) { + match with_runtime(|rt| rt.block_on(async { sb.wait().await?; sb.shutdown().await })) { Some(Ok(r)) => r.code().unwrap_or(-1), _ => -1, } diff --git a/crates/sandlock-oci/src/supervisor.rs b/crates/sandlock-oci/src/supervisor.rs index 9a14e515f..0c04d4422 100644 --- a/crates/sandlock-oci/src/supervisor.rs +++ b/crates/sandlock-oci/src/supervisor.rs @@ -907,7 +907,7 @@ async fn serve_running( RunningCmd::Continue => {} RunningCmd::Shutdown => { let _ = sandbox.kill(); - return exit_info_from(sandbox.wait().await); + return exit_info_from(sandbox.shutdown().await); } } } @@ -928,7 +928,7 @@ async fn serve_running( async fn reap_and_collapse( sandbox: &mut sandlock_core::Sandbox, ) -> Option { - let info = exit_info_from(sandbox.wait().await); + let info = exit_info_from(sandbox.shutdown().await); let _ = sandbox.signal(libc::SIGKILL); info } From 9580016caa5b5c40512dbf6ead32dbe85efb5ade Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Tue, 13 Oct 2026 15:13:08 -0700 Subject: [PATCH 02/23] ffi: separate main-process waits from session shutdown The C wait adapters still collected a complete result and released supervision, preventing bindings from retaining background jobs after the main process exited. Return status-only results from handle waits and add explicit shutdown to collect output and filesystem changes. Leave the session alive when a wait times out so callers can retry or shut it down. Expose tracked process groups and pause/resume operations through the C API. Update complete-result test callers and regenerate the C header. Add coverage for retained background jobs, repeated waits and recovery after a wait timeout. The complete FFI test suite passes. Signed-off-by: Cong Wang --- crates/sandlock-ffi/include/sandlock.h | 59 +++++++++--- crates/sandlock-ffi/src/lib.rs | 119 +++++++++++++++++-------- crates/sandlock-ffi/tests/changes.rs | 5 +- crates/sandlock-ffi/tests/defer.rs | 8 +- crates/sandlock-ffi/tests/popen.rs | 63 +++++++++++++ crates/sandlock-ffi/tests/restore.rs | 8 +- 6 files changed, 203 insertions(+), 59 deletions(-) diff --git a/crates/sandlock-ffi/include/sandlock.h b/crates/sandlock-ffi/include/sandlock.h index 22d83f5c1..79b5e820b 100644 --- a/crates/sandlock-ffi/include/sandlock.h +++ b/crates/sandlock-ffi/include/sandlock.h @@ -70,7 +70,7 @@ enum sandlock_exit_reason */ SANDLOCK_EXIT_REASON_KILLED = 2, /** - * Killed by sandlock because it exceeded its timeout. + * A run or wait exceeded its timeout. A wait timeout leaves the session alive. */ SANDLOCK_EXIT_REASON_TIMEOUT = 3, }; @@ -879,8 +879,8 @@ int32_t sandlock_handle_pid(const sandlock_handle_t *h); /** * Send SIGKILL to the handle's entire process group. Idempotent: a process * that already exited is not an error. Returns 0 on success, -1 on error. - * The handle remains valid; call `sandlock_handle_wait` to collect the exit - * status and `sandlock_handle_free` to release it. + * The handle remains valid; call `sandlock_handle_shutdown` to reap jobs and + * collect output, then `sandlock_handle_free` to release it. * * # Safety * `h` must be a valid handle from `sandlock_create` / `sandlock_popen`. @@ -888,27 +888,62 @@ int32_t sandlock_handle_pid(const sandlock_handle_t *h); int sandlock_handle_kill(sandlock_handle_t *h); /** - * Wait for the sandbox to exit. Returns a result handle with stdout/stderr. + * Pause all tracked live process groups. Returns 0 on success, -1 on error. * - * For a `sandlock_popen` handle, close a piped `out_stdin_fd` and drain any - * piped `out_stdout_fd`/`out_stderr_fd` *before* calling this, or a child that - * reads to EOF or fills a pipe buffer never exits and this blocks forever. + * # Safety + * `h` must be a valid handle with no concurrent handle operations. + */ +int sandlock_handle_pause(sandlock_handle_t *h); + +/** + * Resume all tracked live process groups. Returns 0 on success, -1 on error. * * # Safety - * `h` must be a valid handle from `sandlock_create` / `sandlock_popen`. + * `h` must be a valid handle with no concurrent handle operations. + */ +int sandlock_handle_resume(sandlock_handle_t *h); + +/** + * Wait for the main process, retaining supervision of background jobs. + * The returned result contains only exit status. Collect output and filesystem + * changes with `sandlock_handle_shutdown`. Free each result separately. + * Close caller-owned stdin and drain caller-owned output pipes while waiting. + * + * # Safety + * `h` must be a valid handle with no concurrent handle operations. */ sandlock_result_t *sandlock_handle_wait(sandlock_handle_t *h); /** - * Wait for the sandbox to exit with a timeout in milliseconds. - * Returns a result handle, or null on error. On timeout the sandbox is - * killed and a result with `ExitStatus::Timeout` is returned. + * Wait for the main process with a timeout in milliseconds (0 means unlimited). + * Returns a status-only result, or null on error. A timeout returns + * the timeout reason without killing jobs or releasing supervision; + * callers may wait again or shut down the session. * * # Safety - * `h` must be a valid handle from `sandlock_create`. + * `h` must be a valid handle with no concurrent handle operations. */ sandlock_result_t *sandlock_handle_wait_timeout(sandlock_handle_t *h, uint64_t timeout_ms); +/** + * Terminate remaining jobs with SIGKILL, reap them, and release supervision. + * Returns the main status, captured output, and filesystem changes, or null + * on error. The handle remains valid until `sandlock_handle_free`. + * + * # Safety + * `h` must be a valid handle with no concurrent handle operations. + */ +sandlock_result_t *sandlock_handle_shutdown(sandlock_handle_t *h); + +/** + * Return a sorted snapshot of tracked live process group IDs as a JSON array. + * Free the returned string with `sandlock_string_free`. Returns null on error. + * + * # Safety + * `h` must be a valid handle with no concurrent handle operations. + */ +char *sandlock_handle_process_groups(const sandlock_handle_t *h); + /** * Get port mappings as a JSON string (e.g. `{"80":9001,"443":9002}`). * Returns a C string that must be freed with `sandlock_string_free`. diff --git a/crates/sandlock-ffi/src/lib.rs b/crates/sandlock-ffi/src/lib.rs index dd787fbaf..cc8937134 100644 --- a/crates/sandlock-ffi/src/lib.rs +++ b/crates/sandlock-ffi/src/lib.rs @@ -1409,8 +1409,8 @@ pub unsafe extern "C" fn sandlock_handle_pid(h: *const sandlock_handle_t) -> i32 /// Send SIGKILL to the handle's entire process group. Idempotent: a process /// that already exited is not an error. Returns 0 on success, -1 on error. -/// The handle remains valid; call `sandlock_handle_wait` to collect the exit -/// status and `sandlock_handle_free` to release it. +/// The handle remains valid; call `sandlock_handle_shutdown` to reap jobs and +/// collect output, then `sandlock_handle_free` to release it. /// /// # Safety /// `h` must be a valid handle from `sandlock_create` / `sandlock_popen`. @@ -1426,62 +1426,103 @@ pub unsafe extern "C" fn sandlock_handle_kill(h: *mut sandlock_handle_t) -> c_in 0 } -/// Wait for the sandbox to exit. Returns a result handle with stdout/stderr. +/// Pause all tracked live process groups. Returns 0 on success, -1 on error. /// -/// For a `sandlock_popen` handle, close a piped `out_stdin_fd` and drain any -/// piped `out_stdout_fd`/`out_stderr_fd` *before* calling this, or a child that -/// reads to EOF or fills a pipe buffer never exits and this blocks forever. +/// # Safety +/// `h` must be a valid handle with no concurrent handle operations. +#[no_mangle] +pub unsafe extern "C" fn sandlock_handle_pause(h: *mut sandlock_handle_t) -> c_int { + if h.is_null() { return -1; } + match (*h).sandbox.pause() { Ok(()) => 0, Err(_) => -1 } +} + +/// Resume all tracked live process groups. Returns 0 on success, -1 on error. /// /// # Safety -/// `h` must be a valid handle from `sandlock_create` / `sandlock_popen`. +/// `h` must be a valid handle with no concurrent handle operations. +#[no_mangle] +pub unsafe extern "C" fn sandlock_handle_resume(h: *mut sandlock_handle_t) -> c_int { + if h.is_null() { return -1; } + match (*h).sandbox.resume() { Ok(()) => 0, Err(_) => -1 } +} + +/// Wait for the main process, retaining supervision of background jobs. +/// The returned result contains only exit status. Collect output and filesystem +/// changes with `sandlock_handle_shutdown`. Free each result separately. +/// Close caller-owned stdin and drain caller-owned output pipes while waiting. +/// +/// # Safety +/// `h` must be a valid handle with no concurrent handle operations. #[no_mangle] pub unsafe extern "C" fn sandlock_handle_wait(h: *mut sandlock_handle_t) -> *mut sandlock_result_t { + sandlock_handle_wait_timeout(h, 0) +} + +/// Wait for the main process with a timeout in milliseconds (0 means unlimited). +/// Returns a status-only result, or null on error. A timeout returns +/// the timeout reason without killing jobs or releasing supervision; +/// callers may wait again or shut down the session. +/// +/// # Safety +/// `h` must be a valid handle with no concurrent handle operations. +#[no_mangle] +pub unsafe extern "C" fn sandlock_handle_wait_timeout( + h: *mut sandlock_handle_t, + timeout_ms: u64, +) -> *mut sandlock_result_t { if h.is_null() { return ptr::null_mut(); } let h = &mut *h; - match block_on_runtime(&h.runtime, async { h.sandbox.wait().await?; h.sandbox.shutdown().await }) { - Some(Ok(result)) => Box::into_raw(Box::new(sandlock_result_t { _private: result })), + let status = block_on_runtime(&h.runtime, async { + if timeout_ms == 0 { + h.sandbox.wait().await + } else { + match tokio::time::timeout(Duration::from_millis(timeout_ms), h.sandbox.wait()).await { + Ok(result) => result, + Err(_) => Ok(ExitStatus::Timeout), + } + } + }); + match status { + Some(Ok(exit_status)) => Box::into_raw(Box::new(sandlock_result_t { + _private: RunResult { exit_status, stdout: None, stderr: None, changes: Vec::new() }, + })), _ => ptr::null_mut(), } } -/// Wait for the sandbox to exit with a timeout in milliseconds. -/// Returns a result handle, or null on error. On timeout the sandbox is -/// killed and a result with `ExitStatus::Timeout` is returned. +/// Terminate remaining jobs with SIGKILL, reap them, and release supervision. +/// Returns the main status, captured output, and filesystem changes, or null +/// on error. The handle remains valid until `sandlock_handle_free`. /// /// # Safety -/// `h` must be a valid handle from `sandlock_create`. +/// `h` must be a valid handle with no concurrent handle operations. #[no_mangle] -pub unsafe extern "C" fn sandlock_handle_wait_timeout( - h: *mut sandlock_handle_t, - timeout_ms: u64, -) -> *mut sandlock_result_t { +pub unsafe extern "C" fn sandlock_handle_shutdown(h: *mut sandlock_handle_t) -> *mut sandlock_result_t { if h.is_null() { return ptr::null_mut(); } let h = &mut *h; - - if timeout_ms == 0 { - // No timeout -- same as sandlock_handle_wait. - return match block_on_runtime(&h.runtime, async { h.sandbox.wait().await?; h.sandbox.shutdown().await }) { - Some(Ok(result)) => Box::into_raw(Box::new(sandlock_result_t { _private: result })), - _ => ptr::null_mut(), - }; + match block_on_runtime(&h.runtime, h.sandbox.shutdown()) { + Some(Ok(result)) => Box::into_raw(Box::new(sandlock_result_t { _private: result })), + _ => ptr::null_mut(), } +} - let dur = Duration::from_millis(timeout_ms); - match block_on_runtime(&h.runtime, async { - tokio::time::timeout(dur, async { h.sandbox.wait().await?; h.sandbox.shutdown().await }).await - }) { - Some(Ok(Ok(result))) => Box::into_raw(Box::new(sandlock_result_t { _private: result })), - Some(Ok(Err(_))) | None => ptr::null_mut(), - Some(Err(_)) => { - // Timeout -- kill the process and return a timeout result. - let _ = h.sandbox.kill(); - let result = RunResult::timeout(); - Box::into_raw(Box::new(sandlock_result_t { _private: result })) - } +/// Return a sorted snapshot of tracked live process group IDs as a JSON array. +/// Free the returned string with `sandlock_string_free`. Returns null on error. +/// +/// # Safety +/// `h` must be a valid handle with no concurrent handle operations. +#[no_mangle] +pub unsafe extern "C" fn sandlock_handle_process_groups(h: *const sandlock_handle_t) -> *mut c_char { + if h.is_null() { + return ptr::null_mut(); + } + match (*h).sandbox.process_groups() { + Ok(groups) => CString::new(serde_json::to_string(&groups).unwrap()).unwrap().into_raw(), + Err(_) => ptr::null_mut(), } } @@ -1639,7 +1680,7 @@ pub enum sandlock_exit_reason_t { Signaled = 1, /// Killed with no recoverable signal number. Killed = 2, - /// Killed by sandlock because it exceeded its timeout. + /// A run or wait exceeded its timeout. A wait timeout leaves the session alive. Timeout = 3, } @@ -2607,8 +2648,8 @@ pub unsafe extern "C" fn sandlock_wait(sb: *mut Sandbox) -> c_int { } let sb = &mut *sb; - match with_runtime(|rt| rt.block_on(async { sb.wait().await?; sb.shutdown().await })) { - Some(Ok(r)) => r.code().unwrap_or(-1), + match with_runtime(|rt| rt.block_on(sb.wait())) { + Some(Ok(ExitStatus::Code(code))) => code, _ => -1, } } diff --git a/crates/sandlock-ffi/tests/changes.rs b/crates/sandlock-ffi/tests/changes.rs index 562d7ce0b..ee0e238f9 100644 --- a/crates/sandlock-ffi/tests/changes.rs +++ b/crates/sandlock-ffi/tests/changes.rs @@ -10,7 +10,7 @@ use std::ptr; use sandlock_ffi::{ sandlock_create_for_run, sandlock_entry_kind_t, sandlock_entry_t, sandlock_handle_free, - sandlock_handle_wait, + sandlock_handle_shutdown, sandlock_handle_wait, sandlock_result_change_entry, sandlock_result_change_path, sandlock_result_change_target, sandlock_result_changes_len, sandlock_result_free, sandlock_result_success, sandlock_sandbox_build, sandlock_sandbox_builder_cwd, @@ -94,7 +94,8 @@ fn both_sides_of_every_change_are_readable() { unsafe { sandlock_sandbox_free(policy) }; assert!(!h.is_null()); assert_eq!(unsafe { sandlock_start(h) }, 0); - let r = unsafe { sandlock_handle_wait(h) }; + unsafe { sandlock_result_free(sandlock_handle_wait(h)) }; + let r = unsafe { sandlock_handle_shutdown(h) }; assert!(unsafe { sandlock_result_success(r) }); let n = unsafe { sandlock_result_changes_len(r) }; diff --git a/crates/sandlock-ffi/tests/defer.rs b/crates/sandlock-ffi/tests/defer.rs index 381c1c451..50f9a4fab 100644 --- a/crates/sandlock-ffi/tests/defer.rs +++ b/crates/sandlock-ffi/tests/defer.rs @@ -10,7 +10,7 @@ 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_handle_pending, sandlock_handle_upper_dir, sandlock_handle_shutdown, sandlock_handle_wait, sandlock_result_change_entry, 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, @@ -57,7 +57,8 @@ fn run_deferred(workdir: &Path, storage: &Path) -> *mut sandlock_ffi::sandlock_h 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) }; + unsafe { sandlock_result_free(sandlock_handle_wait(h)) }; + let r = unsafe { sandlock_handle_shutdown(h) }; assert!(!r.is_null(), "wait failed"); assert!(unsafe { sandlock_result_success(r) }); @@ -131,7 +132,8 @@ fn a_committing_handle_is_never_pending() { 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) }; + unsafe { sandlock_result_free(sandlock_handle_wait(h)) }; + let r = unsafe { sandlock_handle_shutdown(h) }; assert!(unsafe { sandlock_result_success(r) }); assert_eq!(unsafe { sandlock_result_changes_len(r) }, 1); unsafe { sandlock_result_free(r) }; diff --git a/crates/sandlock-ffi/tests/popen.rs b/crates/sandlock-ffi/tests/popen.rs index a0740c4e7..8197739f0 100644 --- a/crates/sandlock-ffi/tests/popen.rs +++ b/crates/sandlock-ffi/tests/popen.rs @@ -410,3 +410,66 @@ fn popen_piped_stream_with_null_out_ptr_is_closed_not_leaked() { stream with a null out-pointer is leaking its fd instead of being closed" ); } + +#[test] +fn wait_retains_background_session_until_shutdown() { + use sandlock_ffi::*; + let policy = build_policy(); + let (_owned, av) = argv(&["sh", "-c", "sleep 60 & echo background; exit 7"]); + unsafe { + let h = sandlock_create(policy, ptr::null(), av.as_ptr(), av.len() as c_uint); + assert!(!h.is_null()); + assert_eq!(sandlock_start(h), 0); + for _ in 0..2 { + let status = sandlock_handle_wait_timeout(h, 5000); + assert!(!status.is_null()); + assert_eq!(sandlock_result_exit_code(status), 7); + let mut len = 0; + sandlock_result_stdout_bytes(status, &mut len); + assert_eq!(len, 0); + sandlock_result_free(status); + } + let groups = sandlock_handle_process_groups(h); + assert!(!groups.is_null()); + let groups: Vec = { + let parsed = serde_json::from_slice(std::ffi::CStr::from_ptr(groups).to_bytes()).unwrap(); + sandlock_string_free(groups); + parsed + }; + assert!(!groups.is_empty()); + let result = sandlock_handle_shutdown(h); + assert!(!result.is_null()); + assert_eq!(sandlock_result_exit_code(result), 7); + let mut len = 0; + let bytes = sandlock_result_stdout_bytes(result, &mut len); + assert_eq!(std::slice::from_raw_parts(bytes, len), b"background\n"); + sandlock_result_free(result); + let groups = sandlock_handle_process_groups(h); + assert_eq!(std::ffi::CStr::from_ptr(groups).to_bytes(), b"[]"); + sandlock_string_free(groups); + sandlock_handle_free(h); + sandlock_sandbox_free(policy); + } +} + +#[test] +fn wait_timeout_leaves_main_alive_for_a_later_wait() { + use sandlock_ffi::*; + let policy = build_policy(); + let (_owned, av) = argv(&["sh", "-c", "sleep 0.2; exit 9"]); + unsafe { + let h = sandlock_create(policy, ptr::null(), av.as_ptr(), av.len() as c_uint); + assert!(!h.is_null()); + assert_eq!(sandlock_start(h), 0); + let status = sandlock_handle_wait_timeout(h, 1); + assert!(!status.is_null()); + assert!(matches!(sandlock_result_reason(status), sandlock_exit_reason_t::Timeout)); + sandlock_result_free(status); + let status = sandlock_handle_wait_timeout(h, 5000); + assert_eq!(sandlock_result_exit_code(status), 9); + sandlock_result_free(status); + sandlock_result_free(sandlock_handle_shutdown(h)); + sandlock_handle_free(h); + sandlock_sandbox_free(policy); + } +} diff --git a/crates/sandlock-ffi/tests/restore.rs b/crates/sandlock-ffi/tests/restore.rs index 2542174bb..3a0c51000 100644 --- a/crates/sandlock-ffi/tests/restore.rs +++ b/crates/sandlock-ffi/tests/restore.rs @@ -16,7 +16,7 @@ use sandlock_ffi::{ sandlock_checkpoint_free, sandlock_handle_checkpoint, sandlock_handle_free, sandlock_handle_kill, sandlock_handle_pid, sandlock_handle_restore_skipped_fd, sandlock_handle_restore_skipped_len, sandlock_handle_restore_skipped_path, - sandlock_handle_wait, sandlock_result_free, sandlock_restore_interactive, + sandlock_handle_shutdown, sandlock_handle_wait, sandlock_result_free, sandlock_restore_interactive, sandlock_sandbox_build, sandlock_sandbox_builder_fs_read, sandlock_sandbox_builder_fs_write, sandlock_sandbox_builder_new, sandlock_sandbox_free, sandlock_sandbox_t, sandlock_start, sandlock_string_free, @@ -149,7 +149,8 @@ fn restore_interactive_resumes_via_c_abi() { assert!(baseline > 2, "counter should have advanced, got {baseline}"); assert_eq!(unsafe { sandlock_handle_kill(h) }, 0); - let r = unsafe { sandlock_handle_wait(h) }; + unsafe { sandlock_result_free(sandlock_handle_wait(h)) }; + let r = unsafe { sandlock_handle_shutdown(h) }; if !r.is_null() { unsafe { sandlock_result_free(r) }; } @@ -197,7 +198,8 @@ fn restore_interactive_resumes_via_c_abi() { // Clean up before asserting so a failure never leaks the child/files. unsafe { sandlock_handle_kill(h2); - let r2 = sandlock_handle_wait(h2); + sandlock_result_free(sandlock_handle_wait(h2)); + let r2 = sandlock_handle_shutdown(h2); if !r2.is_null() { sandlock_result_free(r2); } From fbcccfd509fe54eae52033e1bede2f86a975e5ea Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Tue, 13 Oct 2026 15:17:35 -0700 Subject: [PATCH 03/23] python: retain sandbox sessions after waiting for main Python wait freed the native handle and returned the complete result, so background jobs could not survive the main process's exit. Return ExitStatus from Sandbox and Process waits, retaining the session until shutdown collects output and filesystem changes. Expose process group inspection. Raise TimeoutError without terminating jobs when a wait expires, while keeping run timeout cleanup and result reporting. Serialize native handle access and use bounded waits so another thread can signal jobs without aliasing the native wait. Coordinate context exit with an active wait, clean up abandoned sessions, and clear cached status when restoring a new process. Keep deferred branches available for commit or abort after shutdown. Update the Python API documentation and cover session reuse, timeout recovery, concurrent cleanup and restored status. All 139 lifecycle, sandbox and checkpoint regression tests pass. Signed-off-by: Cong Wang --- python/README.md | 31 ++- python/src/sandlock/__init__.py | 3 +- python/src/sandlock/_sdk.py | 18 ++ python/src/sandlock/sandbox.py | 476 +++++++++++++++++--------------- python/tests/test_checkpoint.py | 5 + python/tests/test_lifecycle.py | 6 +- python/tests/test_popen.py | 26 +- python/tests/test_sandbox.py | 5 +- python/tests/test_session.py | 108 ++++++++ 9 files changed, 441 insertions(+), 237 deletions(-) create mode 100644 python/tests/test_session.py diff --git a/python/README.md b/python/README.md index edf37a3a1..3ae7f7a69 100644 --- a/python/README.md +++ b/python/README.md @@ -331,7 +331,7 @@ result = sandbox.run(["python3", "-c", "print(42)"], timeout=10.0) #### `sandbox.spawn(cmd) -> None` Spawn `cmd` without waiting. Use `pid`, `pause()`, `resume()`, `kill()`, -and `wait()` to manage the process lifecycle. +`wait()`, and `shutdown()` to manage the session. Raises `RuntimeError` if a process is already running. @@ -353,9 +353,34 @@ Release a previously `create()`d child to `execve`. Raises `RuntimeError` if no child has been created. -#### `sandbox.wait() -> Result` +#### `sandbox.wait(timeout=None) -> ExitStatus` -Wait for the running process to finish and return its `Result`. +Wait for the main process while background jobs remain supervised. The status +contains `success`, `exit_code`, `reason`, and `signal`. Repeated waits return +the same status. A timeout raises `TimeoutError` and leaves the session alive. + +#### `sandbox.process_groups() -> list[int]` + +Return a sorted snapshot of tracked live process group IDs. + +#### `sandbox.shutdown() -> Result` + +Terminate remaining jobs with SIGKILL, reap them, collect captured output, and +apply the configured filesystem branch action. `run()` waits and shuts down +automatically; its timeout still terminates the session. A sandbox remains +busy after `wait()` until shutdown or context exit. + +```python +with sandbox: + sandbox.spawn(["sh", "-c", "sleep 60 &"]) + status = sandbox.wait() + groups = sandbox.process_groups() + result = sandbox.shutdown() +``` + +A `Process` returned by `popen()` exposes the same wait, process-group, and +shutdown methods. Its piped output remains caller-owned. Exiting its context +terminates the session and closes its streams. #### Inspecting and deferring COW changes diff --git a/python/src/sandlock/__init__.py b/python/src/sandlock/__init__.py index e4fd45bb1..044517a57 100644 --- a/python/src/sandlock/__init__.py +++ b/python/src/sandlock/__init__.py @@ -7,7 +7,7 @@ from ._version import __version__ from ._sdk import ( - Stage, Pipeline, Result, ExitReason, SyscallEvent, PolicyContext, Checkpoint, SkippedFd, + Stage, Pipeline, Result, ExitStatus, ExitReason, SyscallEvent, PolicyContext, Checkpoint, SkippedFd, NamedStage, Gather, GatherPipeline, Protection, landlock_abi_version, min_landlock_abi, confine, pull_image, @@ -46,6 +46,7 @@ "Pipeline", "Result", "ExitReason", + "ExitStatus", "SyscallEvent", "PolicyContext", "Checkpoint", diff --git a/python/src/sandlock/_sdk.py b/python/src/sandlock/_sdk.py index 01b973d8b..59dbfd2b8 100644 --- a/python/src/sandlock/_sdk.py +++ b/python/src/sandlock/_sdk.py @@ -319,6 +319,15 @@ def confine(policy: "PolicyDataclass") -> None: _lib.sandlock_handle_wait_timeout.restype = _c_result_p _lib.sandlock_handle_wait_timeout.argtypes = [_c_handle_p, ctypes.c_uint64] +_lib.sandlock_handle_shutdown.restype = _c_result_p +_lib.sandlock_handle_shutdown.argtypes = [_c_handle_p] +_lib.sandlock_handle_process_groups.restype = ctypes.c_void_p +_lib.sandlock_handle_process_groups.argtypes = [_c_handle_p] +for _name in ("sandlock_handle_pause", "sandlock_handle_resume"): + getattr(_lib, _name).restype = ctypes.c_int + getattr(_lib, _name).argtypes = [_c_handle_p] + + _lib.sandlock_handle_free.restype = None _lib.sandlock_handle_free.argtypes = [_c_handle_p] @@ -907,6 +916,15 @@ class ExitReason(IntEnum): """Killed by sandlock because it exceeded its timeout.""" +@dataclass(frozen=True) +class ExitStatus: + """Main-process status returned by wait(), without ending the session.""" + success: bool + exit_code: int + reason: ExitReason + signal: int = -1 + + @dataclass class Result: """Result of a sandboxed command.""" diff --git a/python/src/sandlock/sandbox.py b/python/src/sandlock/sandbox.py index 2e1393135..d209380a6 100644 --- a/python/src/sandlock/sandbox.py +++ b/python/src/sandlock/sandbox.py @@ -12,6 +12,7 @@ import itertools import os import re +import threading import weakref from dataclasses import dataclass, field from enum import Enum, IntEnum @@ -534,6 +535,10 @@ def __post_init__(self): # own handle (see `_popen_process`), this is only a # non-owning busy marker self._restore_skipped = [] # SkippedFd entries from the last restore + self._lock = threading.RLock() + self._idle = threading.Condition(self._lock) + self._waiting = False + self._status = None @staticmethod def _next_name() -> str: @@ -613,29 +618,29 @@ def _reject_if_popen(self) -> None: "sandbox" ) + def __del__(self): + try: + self.__exit__(None, None, None) + except Exception: + pass + def __enter__(self): return self def __exit__(self, exc_type, exc_val, exc_tb): - # Reap a still-running popen() Process (it owns its own handle) so leaving - # the sandbox context never strands a confined child. proc = self._popen_process() - if proc is not None and proc._handle is not None: - try: - proc.kill() - except Exception: - pass - try: - proc.wait() - except Exception: - pass - if self._handle is not None: - from ._sdk import _lib - try: - _lib.sandlock_handle_free(self._handle) - except Exception: - pass - self._handle = None + if proc is not None: + proc.__exit__(exc_type, exc_val, exc_tb) + with self._lock: + if self._handle is not None: + from ._sdk import _lib + if self._waiting: + _lib.sandlock_handle_kill(self._handle) + while self._waiting: + self._idle.wait() + if self._handle is not None: + _lib.sandlock_handle_free(self._handle) + self._handle = None # Freeing an undecided branch preserves it, same as KEEP. self._release_pending() return False @@ -651,17 +656,17 @@ def pid(self) -> int | None: proc = self._popen_process() if proc is not None: # The Process owns the handle; its pid is lock-guarded and cached, so - # reading it can't race a concurrent wait() freeing the handle. + # reading it can't race a concurrent shutdown() freeing the handle. return proc.pid - if self._handle is None: - return None from ._sdk import _lib - return _lib.sandlock_handle_pid(self._handle) or None + with self._lock: + if self._handle is None: + return None + return _lib.sandlock_handle_pid(self._handle) or None @property def is_running(self) -> bool: - """True if a process is currently running in this sandbox (lifecycle or - a live :meth:`popen` :class:`Process`).""" + """True while a session is retained, including after the main exits.""" return self._live_handle() is not None @property @@ -722,7 +727,7 @@ def _release_pending(self) -> None: self._pending = None def _park_or_free(self, handle) -> None: - """After a wait: keep a handle whose branch is deferred, free any other.""" + """Retain a deferred branch after shutdown; otherwise release the handle.""" from ._sdk import _lib if _lib.sandlock_handle_pending(handle): self._pending = handle @@ -772,10 +777,12 @@ def run(self, cmd: Sequence[str], timeout: float | None = None): 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(handle, timeout_ms) + timed_out = False + try: + _wait_status(handle, timeout) + except TimeoutError: + timed_out = True + result_p = _lib.sandlock_handle_shutdown(handle) finally: self._park_or_free(handle) @@ -786,6 +793,8 @@ def run(self, cmd: Sequence[str], timeout: float | None = None): success = _lib.sandlock_result_success(result_p) reason = ExitReason(_lib.sandlock_result_reason(result_p)) signal = _lib.sandlock_result_signal(result_p) + if timed_out: + exit_code, success, reason, signal = -1, False, ExitReason.TIMEOUT, -1 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) @@ -1009,6 +1018,7 @@ def create(self, cmd: Sequence[str]) -> None: argv, argc = _make_argv(list(cmd)) resolved_name = self._resolve_name() + self._status = None self._handle = _lib.sandlock_create( native.ptr, _encode(resolved_name), argv, argc, ) @@ -1024,12 +1034,15 @@ def start(self) -> None: """ from ._sdk import _lib - if self._handle is None: - raise RuntimeError("sandbox has not been created") - if _lib.sandlock_start(self._handle) != 0: - _lib.sandlock_handle_free(self._handle) - self._handle = None - raise RuntimeError("sandlock_start failed") + with self._lock: + if self._waiting: + raise RuntimeError("lifecycle operation already in progress") + if self._handle is None: + raise RuntimeError("sandbox has not been created") + if _lib.sandlock_start(self._handle) != 0: + _lib.sandlock_handle_free(self._handle) + self._handle = None + raise RuntimeError("sandlock_start failed") def spawn(self, cmd: Sequence[str]) -> None: """Spawn ``cmd`` in the sandbox without waiting for it to finish. @@ -1044,47 +1057,56 @@ def spawn(self, cmd: Sequence[str]) -> None: self.create(cmd) self.start() - def wait(self): - """Wait for the running process to finish and return its Result. + def wait(self, timeout: float | None = None) -> "ExitStatus": + """Wait for the main process, retaining supervision of background jobs. - Raises: - RuntimeError: If the sandbox is not running, or if it is running a - :meth:`popen` :class:`Process` (wait on that Process instead — - freeing its handle here would break it). + A timeout raises TimeoutError and leaves the session alive. Call + shutdown() to terminate remaining jobs and collect output and changes. """ - 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 + with self._lock: + if self._handle is None: + raise RuntimeError("sandbox is not running") + if self._waiting: + raise RuntimeError("lifecycle operation already in progress") + if self._status is not None: + return self._status + handle = self._handle + self._waiting = True try: - result_p = _lib.sandlock_handle_wait_timeout(handle, 0) + self._status = _wait_status(handle, timeout, self._lock) + return self._status finally: - self._park_or_free(handle) - - if not result_p: - return Result(success=False, exit_code=-1, error="sandlock_handle_wait failed") + with self._lock: + self._waiting = False + self._idle.notify_all() - exit_code = _lib.sandlock_result_exit_code(result_p) - success = _lib.sandlock_result_success(result_p) - reason = ExitReason(_lib.sandlock_result_reason(result_p)) - 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) + def process_groups(self) -> list[int]: + """Return a sorted snapshot of tracked live process group IDs.""" + self._reject_if_popen() + with self._lock: + if self._handle is None or self._waiting: + raise RuntimeError("session is unavailable") + return _read_process_groups(self._handle) - return Result( - success=bool(success), - exit_code=exit_code, - reason=reason, - signal=signal, - stdout=stdout, - stderr=stderr, - changes=changes, - ) + def shutdown(self) -> "Result": + """SIGKILL remaining jobs, reap them, and collect output and changes.""" + from ._sdk import _lib + self._reject_if_popen() + with self._lock: + if self._handle is None or self._waiting: + raise RuntimeError("session is unavailable") + handle = self._handle + self._waiting = True + try: + with self._lock: + return _take_result(_lib.sandlock_handle_shutdown(handle)) + finally: + with self._lock: + self._park_or_free(handle) + self._handle = None + self._waiting = False + self._idle.notify_all() def popen( self, @@ -1288,46 +1310,26 @@ def reduce(self, cmd: list[str], fork_result: "ForkResult") -> "Result": # Lifecycle methods # ------------------------------------------------------------------ - def pause(self) -> None: - """Send SIGSTOP to the sandbox process group. - - Raises RuntimeError if a :meth:`popen` Process owns the child (manage it - through the Process).""" - import signal + def _signal(self, name: str) -> None: + from ._sdk import _lib self._reject_if_popen() - pid = self.pid - if pid is None: - raise RuntimeError("sandbox is not running") - import os - os.killpg(pid, signal.SIGSTOP) + with self._lock: + if self._handle is None: + raise RuntimeError("sandbox is not running") + if getattr(_lib, name)(self._handle) != 0: + raise RuntimeError("sandbox signal failed") - def resume(self) -> None: - """Send SIGCONT to the sandbox process group. + def pause(self) -> None: + """Send SIGSTOP to the tracked live process groups.""" + self._signal("sandlock_handle_pause") - Raises RuntimeError if a :meth:`popen` Process owns the child.""" - import signal - self._reject_if_popen() - pid = self.pid - if pid is None: - raise RuntimeError("sandbox is not running") - import os - os.killpg(pid, signal.SIGCONT) + def resume(self) -> None: + """Send SIGCONT to the tracked live process groups.""" + self._signal("sandlock_handle_resume") def kill(self) -> None: - """Send SIGKILL to the sandbox process group. - - Raises RuntimeError if a :meth:`popen` Process owns the child (call - ``proc.kill()`` instead).""" - import signal - self._reject_if_popen() - pid = self.pid - if pid is None: - raise RuntimeError("sandbox is not running") - import os - try: - os.killpg(pid, signal.SIGKILL) - except ProcessLookupError: - pass + """Send SIGKILL to the tracked live process groups.""" + self._signal("sandlock_handle_kill") def ports(self) -> dict[int, int]: """Return current port mappings {virtual_port: real_port}. @@ -1340,11 +1342,12 @@ def ports(self) -> dict[int, int]: proc = self._popen_process() if proc is not None: # The Process owns the handle; its ports() reads it under the lock so - # this can't race a concurrent wait() freeing it. + # this can't race a concurrent shutdown() freeing it. return proc.ports() - if self._handle is None: - return {} - return _read_port_mappings(self._handle) + with self._lock: + if self._handle is None or self._waiting: + return {} + return _read_port_mappings(self._handle) def checkpoint( self, @@ -1373,9 +1376,12 @@ def checkpoint( # would fight the streaming Process for it, so reject rather than reach # past it — consistent with wait/pause/resume/kill. self._reject_if_popen() - if self._handle is None: - raise RuntimeError("sandbox is not running (use start() or run() first)") - ptr = _lib.sandlock_handle_checkpoint(self._handle) + with self._lock: + if self._waiting: + raise RuntimeError("lifecycle operation already in progress") + if self._handle is None: + raise RuntimeError("sandbox is not running (use start() or run() first)") + ptr = _lib.sandlock_handle_checkpoint(self._handle) if not ptr: raise RuntimeError("checkpoint capture failed") cp = Checkpoint(ptr) @@ -1423,9 +1429,10 @@ def restore_interactive(self, cp: "Checkpoint") -> None: ) if not handle: raise RuntimeError("checkpoint restore failed") + self._status = None self._handle = handle # Copy the skipped-fd diagnostics out eagerly: the native entries live - # on the handle, which wait() frees. + # on the handle, which shutdown() frees. skipped = [] for i in range(_lib.sandlock_handle_restore_skipped_len(handle)): fd = _lib.sandlock_handle_restore_skipped_fd(handle, i) @@ -1535,7 +1542,7 @@ class Process: ``.stdin`` / ``.stdout`` / ``.stderr`` are unbuffered binary file objects for streams opened with :attr:`StdioMode.PIPED`, else ``None``. The process - keeps running until :meth:`wait` (or :meth:`kill`) is called, or the + remains supervised until :meth:`shutdown` is called, or the ``Process`` context manager exits. Prefer the context manager, which closes the streams and reaps the child even on error:: @@ -1554,9 +1561,10 @@ def __init__(self, sandbox: "Sandbox", handle, stdin_fd: int, stdout_fd: int, st self._sandbox = sandbox # _lock guards _handle (and the _waiting reservation) so kill()/pid on - # one thread can't observe or act on a handle wait() is freeing on + # one thread can't observe or act on a handle shutdown() is freeing on # another — the "kill from another thread while wait() blocks" pattern. - self._lock = threading.Lock() + self._lock = threading.RLock() + self._idle = threading.Condition(self._lock) self._waiting = False # The handle and pid are set only AFTER the fds are wrapped below: if an # fdopen raises, this Process never takes ownership, so popen()'s caller @@ -1564,6 +1572,7 @@ def __init__(self, sandbox: "Sandbox", handle, stdin_fd: int, stdout_fd: int, st self._handle = None self._pid = -1 self._result = None + self._status = None self.stdin = self.stdout = self.stderr = None # os.fdopen takes ownership of each fd: closing the stream closes the fd, # so a Process that is closed/exited never leaks the pipe ends. Wrap the @@ -1590,147 +1599,113 @@ def __init__(self, sandbox: "Sandbox", handle, stdin_fd: int, stdout_fd: int, st pass raise self.stdin, self.stdout, self.stderr = opened - # Take ownership now that construction can no longer fail. Cache the pid - # so kill()/pid never dereference the handle (avoiding a race with a - # concurrent wait() free) — they signal by pid, like the sandbox and the - # Go binding. + # Take ownership only after every stream has been wrapped successfully. self._pid = _lib.sandlock_handle_pid(handle) or -1 self._handle = handle @property def pid(self) -> int | None: - """The child PID while running, else ``None`` (after :meth:`wait`).""" + """The child PID while running, else ``None`` (after :meth:`shutdown`).""" with self._lock: return self._pid if self._handle is not None and self._pid > 0 else None def ports(self) -> dict[int, int]: """Current virtual→real port mappings while running, else ``{}`` (needs ``port_remap``). Reads the handle under the lock, and reports ``{}`` - while a :meth:`wait` holds it, so it can't race the wait's free.""" + while a lifecycle operation holds it.""" with self._lock: if self._handle is None or self._waiting: return {} return _read_port_mappings(self._handle) def kill(self) -> None: - """SIGKILL the child's entire process group by pid (like the sandbox and - the Go binding), so a kill from another thread is safe while :meth:`wait` - blocks on the handle. Idempotent: a child that already exited is not an - error, and after :meth:`wait` has reaped it this is a no-op. + """SIGKILL tracked live groups; usable from another thread during wait.""" + from ._sdk import _lib + with self._lock: + if self._handle is None: + return + if _lib.sandlock_handle_kill(self._handle) != 0: + raise RuntimeError("sandbox kill failed") - Raises: - RuntimeError: If the process has no pid (never a valid popen child). - OSError: If the kill genuinely fails (e.g. EPERM) — a - ``ProcessLookupError`` (already-exited group) is swallowed. - """ - import os - import signal + def wait(self, timeout: float | None = None) -> "ExitStatus": + """Wait for the main process while retaining the session. + Closes piped stdin. Drain caller-owned output concurrently if needed. + A timeout raises TimeoutError without terminating jobs. Repeated waits + return the cached main status. + """ with self._lock: + if self._waiting: + raise RuntimeError("lifecycle operation already in progress") + if self._status is not None: + return self._status if self._handle is None: - return # already reaped by wait() — no-op - if self._pid <= 0: - raise RuntimeError("process has no pid") - pid = self._pid - try: - os.killpg(pid, signal.SIGKILL) - except ProcessLookupError: - pass # the group already exited — idempotent no-op - - def wait(self, timeout: float | None = None) -> "Result": - """Wait for the child to exit and return its :class:`Result`. + raise RuntimeError("process is not running") + handle = self._handle + self._waiting = True + try: + if self.stdin is not None and not self.stdin.closed: + try: + self.stdin.close() + except OSError: + pass + self._status = _wait_status(handle, timeout, self._lock) + return self._status + finally: + with self._lock: + self._waiting = False + self._idle.notify_all() - A still-open piped ``stdin`` is closed first so a child that reads to EOF - can exit; piped ``stdout``/``stderr`` stay open for the caller to finish - reading. Frees the underlying handle; a second call returns the cached - result. + def process_groups(self) -> list[int]: + """Return a sorted snapshot of tracked live process group IDs.""" + with self._lock: + if self._handle is None or self._waiting: + raise RuntimeError("session is unavailable") + return _read_process_groups(self._handle) - The returned ``Result`` carries no ``stdout``/``stderr`` (those were - streamed to you as fds — read them off ``.stdout``/``.stderr``). + def shutdown(self) -> "Result": + """Terminate and reap remaining jobs, returning the complete result. - Args: - timeout: Maximum seconds to wait. On timeout the child is killed and - a non-success ``Result`` is returned (same semantics as - :meth:`Sandbox.run`'s ``timeout``) — the wait never hangs. ``None`` - (default) waits indefinitely. Note: without a ``timeout``, a piped - ``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. + Caller-owned output streams remain open. A repeated call returns the + cached result; a deferred branch remains available on the Sandbox. """ - 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() - # (which signals by pid, not through the handle) can interrupt it. + from ._sdk import _lib, ExitStatus with self._lock: - handle = self._handle - if handle is None: - if self._result is not None: - return self._result - raise RuntimeError("process is not running") if self._waiting: - raise RuntimeError("wait already in progress") + raise RuntimeError("lifecycle operation already in progress") + if self._result is not None: + return self._result + if self._handle is None: + raise RuntimeError("process is not running") + handle = self._handle self._waiting = True - - # Deliver EOF to a still-open piped stdin so a reader child can exit and - # the wait below does not block forever. - if self.stdin is not None and not self.stdin.closed: - try: - self.stdin.close() - except OSError: - pass - try: - # None -> wait forever (the FFI treats 0 as "no timeout"). A finite - # timeout maps to milliseconds, clamped up to 1ms so timeout=0 and - # sub-millisecond values don't collapse to 0 and wait forever — the - # opposite of the caller's intent. - timeout_ms = max(1, int(timeout * 1000)) if timeout is not None else 0 - result_p = _lib.sandlock_handle_wait_timeout(handle, timeout_ms) + with self._lock: + self._result = _take_result(_lib.sandlock_handle_shutdown(handle)) + r = self._result + self._status = ExitStatus(r.success, r.exit_code, r.reason, r.signal) + return r finally: with self._lock: 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 - # still points at us (the weakref may already be dead during __del__). + self._idle.notify_all() if self._sandbox._popen_process() is self: self._sandbox._process = None - if not result_p: - self._result = Result( - success=False, exit_code=-1, error="sandlock_handle_wait failed" - ) - return self._result - - exit_code = _lib.sandlock_result_exit_code(result_p) - success = _lib.sandlock_result_success(result_p) - reason = ExitReason(_lib.sandlock_result_reason(result_p)) - 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 - def __enter__(self) -> "Process": return self def __exit__(self, exc_type, exc_val, exc_tb) -> None: - # Terminate and reap a still-running child so no confined process is left - # behind, then close every stream we own. - if self._handle is not None: - try: - self.kill() - except Exception: - pass - try: - self.wait() - except Exception: - pass + with self._lock: + if self._handle is not None: + if self._waiting: + self.kill() + while self._waiting: + self._idle.wait() + if self._handle is not None: + self.shutdown() for stream in (self.stdin, self.stdout, self.stderr): if stream is not None and not stream.closed: try: @@ -1739,10 +1714,10 @@ def __exit__(self, exc_type, exc_val, exc_tb) -> None: pass def __del__(self): - # Safety net for a Process dropped without wait()/context: a live handle + # Safety net for a Process dropped without shutdown()/context: a live handle # would otherwise leak the confined child + its runtime and wedge the # Sandbox in "already running". Reap it, warning like subprocess.Popen so - # the missing wait()/`with` is visible. __del__ must never raise, and may + # the missing shutdown()/`with` is visible. __del__ must never raise, and may # run during interpreter shutdown when imports/globals are gone — so this # is strictly best-effort inside a broad guard. try: @@ -1751,13 +1726,72 @@ def __del__(self): import warnings warnings.warn( - "Process was not waited on or used as a context manager; " + "Process was not shut down or used as a context manager; " "reaping the confined child. Use `with sandbox.popen(...)` or " - "call wait().", + "call shutdown().", ResourceWarning, stacklevel=2, ) - self.kill() - self.wait() + self.shutdown() except Exception: pass + + +def _wait_status(handle, timeout, lock=None): + import contextlib + import time + from ._sdk import _lib, ExitStatus, ExitReason + deadline = time.monotonic() + max(0, timeout) if timeout is not None else None + while True: + remaining = max(0, deadline - time.monotonic()) if deadline is not None else None + interval = min(remaining, 0.05) if remaining is not None and lock is not None else remaining + if interval is None and lock is not None: + interval = 0.05 + timeout_ms = max(1, int(interval * 1000)) if interval is not None else 0 + # Bounded waits let another thread signal through the same handle safely. + with lock if lock is not None else contextlib.nullcontext(): + ptr = _lib.sandlock_handle_wait_timeout(handle, timeout_ms) + if not ptr: + raise RuntimeError("sandlock_handle_wait failed") + try: + reason = ExitReason(_lib.sandlock_result_reason(ptr)) + if reason != ExitReason.TIMEOUT: + return ExitStatus(bool(_lib.sandlock_result_success(ptr)), + _lib.sandlock_result_exit_code(ptr), reason, + _lib.sandlock_result_signal(ptr)) + finally: + _lib.sandlock_result_free(ptr) + if deadline is not None and time.monotonic() >= deadline: + raise TimeoutError("main process has not exited") + time.sleep(0) + + +def _read_process_groups(handle): + import ctypes + import json + from ._sdk import _lib + ptr = _lib.sandlock_handle_process_groups(handle) + if not ptr: + raise RuntimeError("process group inspection failed") + try: + return json.loads(ctypes.string_at(ptr)) + finally: + _lib.sandlock_string_free(ctypes.cast(ptr, ctypes.c_char_p)) + + +def _take_result(ptr): + from ._sdk import _lib, _read_result_bytes, _read_result_changes, Result, ExitReason + if not ptr: + raise RuntimeError("sandbox shutdown failed") + try: + return Result( + success=bool(_lib.sandlock_result_success(ptr)), + exit_code=_lib.sandlock_result_exit_code(ptr), + reason=ExitReason(_lib.sandlock_result_reason(ptr)), + signal=_lib.sandlock_result_signal(ptr), + stdout=_read_result_bytes(ptr, _lib.sandlock_result_stdout_bytes), + stderr=_read_result_bytes(ptr, _lib.sandlock_result_stderr_bytes), + changes=_read_result_changes(ptr), + ) + finally: + _lib.sandlock_result_free(ptr) diff --git a/python/tests/test_checkpoint.py b/python/tests/test_checkpoint.py index a1c856b36..ba6bf3d99 100644 --- a/python/tests/test_checkpoint.py +++ b/python/tests/test_checkpoint.py @@ -261,9 +261,14 @@ def test_restore_resumes_counter(self, tmp_dir): counter.write_text("0\n") sb2 = _policy(**policy_kw) + sb2.spawn(["sh", "-c", "exit 7"]) + assert sb2.wait().exit_code == 7 + sb2.shutdown() assert sb2.restore_skipped == [] ret = sb2.restore_interactive(cp) assert ret is None + with pytest.raises(TimeoutError): + sb2.wait(timeout=0.01) skipped = sb2.restore_skipped assert isinstance(skipped, list) for s in skipped: diff --git a/python/tests/test_lifecycle.py b/python/tests/test_lifecycle.py index 423c0aa7d..713a78309 100644 --- a/python/tests/test_lifecycle.py +++ b/python/tests/test_lifecycle.py @@ -70,7 +70,8 @@ class TestSpawn: def test_spawn_then_wait_returns_result(self): with _policy() as sb: sb.spawn(["sh", "-c", "echo hello; exit 0"]) - result = sb.wait() + assert sb.wait().success + result = sb.shutdown() assert result.exit_code == 0 assert b"hello" in result.stdout @@ -102,7 +103,8 @@ def test_create_then_start_runs_command(self): with _policy() as sb: sb.create(["sh", "-c", "echo two-step"]) sb.start() - result = sb.wait() + assert sb.wait().success + result = sb.shutdown() assert result.exit_code == 0 assert b"two-step" in result.stdout diff --git a/python/tests/test_popen.py b/python/tests/test_popen.py index 0283d87eb..cd8633e0c 100644 --- a/python/tests/test_popen.py +++ b/python/tests/test_popen.py @@ -149,7 +149,7 @@ def test_wait_is_idempotent_and_cached(): proc = _policy().popen(["echo", "hi"], stdout=StdioMode.PIPED) assert proc.stdout.read() == b"hi\n" first = proc.wait() - second = proc.wait() # handle already freed → cached result, no double-free + second = proc.wait() # cached status, session remains alive assert first is second assert first.exit_code == 0 @@ -181,7 +181,7 @@ def test_popen_rejects_second_process_on_same_sandbox(): proc.wait() -def test_wait_timeout_kills_and_returns_nonsuccess(): +def test_wait_timeout_leaves_session_alive(): # A child that never exits within the timeout: wait(timeout) must RETURN (not # hang), killing the child and yielding a non-success result — parity with # Sandbox.run(timeout=...). Run under a watchdog so a regression (ignored @@ -191,7 +191,9 @@ def test_wait_timeout_kills_and_returns_nonsuccess(): box = {} def _run(): - box["result"] = proc.wait(timeout=0.5) + with pytest.raises(TimeoutError): + proc.wait(timeout=0.5) + box["timed_out"] = True t = threading.Thread(target=_run, daemon=True) t.start() @@ -201,7 +203,9 @@ def _run(): t.join(timeout=5) pytest.fail("wait(timeout) did not return — timeout was ignored") - assert not box["result"].success, "a timed-out (killed) process is not success" + assert box["timed_out"] + assert proc.process_groups() + proc.shutdown() def test_wait_without_timeout_completes_for_quick_child(): @@ -256,7 +260,8 @@ def test_stale_process_does_not_alias_reused_sandbox(): sandbox = _policy() p1 = sandbox.popen(["echo", "one"], stdout=StdioMode.PIPED) assert p1.stdout.read() == b"one\n" - p1.wait() # frees p1's handle, releases the sandbox + p1.wait() + p1.shutdown() p2 = sandbox.popen(["sleep", "100"]) try: @@ -267,7 +272,7 @@ def test_stale_process_does_not_alias_reused_sandbox(): assert p2.pid is not None, "p2 must still be alive" finally: p2.kill() - p2.wait() + p2.shutdown() assert not sandbox.is_running @@ -300,7 +305,7 @@ def test_sandbox_pid_and_ports_delegate_to_popen_process(): assert sandbox.ports() == proc.ports() # both {} without port_remap finally: proc.kill() - proc.wait() + proc.shutdown() assert sandbox.pid is None assert sandbox.ports() == {} @@ -314,7 +319,9 @@ def test_wait_timeout_zero_does_not_wait_forever(): box = {} def _run(): - box["result"] = proc.wait(timeout=0) + with pytest.raises(TimeoutError): + proc.wait(timeout=0) + box["timed_out"] = True t = threading.Thread(target=_run, daemon=True) t.start() @@ -324,7 +331,8 @@ def _run(): t.join(timeout=5) pytest.fail("wait(timeout=0) hung — 0 collapsed to wait-forever") - assert not box["result"].success + assert box["timed_out"] + proc.shutdown() def test_abandoned_process_is_reaped_by_del(): diff --git a/python/tests/test_sandbox.py b/python/tests/test_sandbox.py index e2b5624b4..6c9fe9f09 100644 --- a/python/tests/test_sandbox.py +++ b/python/tests/test_sandbox.py @@ -795,6 +795,8 @@ def test_pid_available_during_run(self): assert pid > 0 sb.wait() + assert sb.pid == pid + sb.shutdown() assert sb.pid is None def test_pause_not_running_raises(self): @@ -1008,7 +1010,8 @@ def test_spawn_wait_defers_too(self, tmp_path): 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 p.wait().success + result = p.shutdown() assert result.success assert p.pending p.commit() diff --git a/python/tests/test_session.py b/python/tests/test_session.py new file mode 100644 index 000000000..749abe361 --- /dev/null +++ b/python/tests/test_session.py @@ -0,0 +1,108 @@ +"""Session lifetime is independent of the main process's exit.""" +import os + +import pytest + +from sandlock import Sandbox, ExitStatus, ExitReason, StdioMode, BranchAction + + +def policy(**kwargs): + return Sandbox(fs_readable=["/usr", "/lib", "/lib64", "/bin", "/etc", "/proc", "/dev"], **kwargs) + + +def test_wait_retains_jobs_and_shutdown_collects_output(): + with policy() as sb: + sb.spawn(["sh", "-c", "sleep 60 & echo retained; exit 7"]) + status = sb.wait(timeout=5) + assert isinstance(status, ExitStatus) + assert status.exit_code == 7 + assert sb.wait() == status + assert sb.is_running + assert sb.process_groups() + with pytest.raises(RuntimeError, match="already running"): + sb.spawn(["true"]) + result = sb.shutdown() + assert result.exit_code == 7 + assert result.stdout == b"retained\n" + assert not sb.is_running + assert sb.run(["true"]).success + + +def test_wait_timeout_retains_main_for_retry(): + with policy() as sb: + sb.spawn(["sh", "-c", "sleep 0.2; exit 9"]) + with pytest.raises(TimeoutError): + sb.wait(timeout=0) + assert sb.process_groups() + assert sb.wait(timeout=5).exit_code == 9 + assert sb.shutdown().exit_code == 9 + + +def test_popen_context_cleans_background_jobs_after_wait(): + with policy().popen(["sh", "-c", "sleep 60 & echo $!"], stdout=StdioMode.PIPED) as proc: + child = int(proc.stdout.readline()) + assert proc.wait(timeout=5).success + assert proc.process_groups() + os.kill(child, 0) + with pytest.raises(ProcessLookupError): + os.kill(child, 0) + + +def test_shutdown_defers_cow_until_jobs_finish(tmp_path): + with policy(fs_writable=[str(tmp_path)], workdir=str(tmp_path), on_exit=BranchAction.DEFER) as sb: + sb.spawn(["sh", "-c", f"echo data > {tmp_path}/out"]) + assert sb.wait(timeout=5).success + assert not sb.pending + assert not (tmp_path / "out").exists() + result = sb.shutdown() + assert result.changes + assert sb.pending + sb.commit() + assert (tmp_path / "out").read_text() == "data\n" + + +def test_run_timeout_terminates_session(): + with policy() as sb: + result = sb.run(["sleep", "60"], timeout=0.01) + assert result.reason == ExitReason.TIMEOUT + assert not result.success + assert not sb.is_running + +@pytest.mark.parametrize("streaming", [False, True]) +def test_context_exit_interrupts_wait_and_releases_session(streaming): + import threading + import time + sb = policy() + if streaming: + owner = sb.popen(["sleep", "60"]) + else: + sb.spawn(["sleep", "60"]) + owner = sb + outcomes = [] + worker = threading.Thread(target=lambda: outcomes.append(owner.wait()), daemon=True) + worker.start() + time.sleep(0.1) + owner.__exit__(None, None, None) + worker.join(5) + assert not worker.is_alive() + assert outcomes and not outcomes[0].success + assert not sb.is_running + assert sb.run(["true"]).success + + +def test_wait_rejects_conflicting_start_and_checkpoint(): + import threading + import time + with policy() as sb: + sb.spawn(["sleep", "60"]) + worker = threading.Thread(target=sb.wait, daemon=True) + worker.start() + time.sleep(0.1) + try: + for op in (sb.start, sb.checkpoint): + with pytest.raises(RuntimeError, match="already in progress"): + op() + finally: + sb.kill() + worker.join(5) + assert not worker.is_alive() From 57a6c74bb4741abbda4786b75b5a51351d8902bd Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Tue, 13 Oct 2026 15:17:44 -0700 Subject: [PATCH 04/23] go: retain process sessions until explicit shutdown Process.Wait released the native handle after collecting a result, ending background supervision when the main command finished. Return a cached ExitStatus from Wait and add Shutdown for the complete result. Expose ProcessGroups and apply signals to tracked groups through the native API. Have Run wait and shut down, preserving timeout results and deferred branch ownership. Serialize bounded native waits with signals and handle cleanup so Kill and Close can interrupt a blocked Wait. Let native handle cleanup reap abandoned sessions instead of signalling a cached numeric process group that could have been reused after the main process exited. Update the Go API documentation and add session lifetime, concurrent close, run timeout and finalizer regressions. All 55 Go tests pass with the race detector enabled. Signed-off-by: Cong Wang --- go/README.md | 16 ++- go/sandbox.go | 8 ++ go/sandlock_linux.go | 234 +++++++++++++++++++++++--------------- go/sandlock_linux_test.go | 15 ++- go/session_linux_test.go | 99 ++++++++++++++++ 5 files changed, 273 insertions(+), 99 deletions(-) create mode 100644 go/session_linux_test.go diff --git a/go/README.md b/go/README.md index b9b7c8bdf..09cf7c410 100644 --- a/go/README.md +++ b/go/README.md @@ -139,8 +139,8 @@ func (s *Sandbox) Popen(stdio Stdio, cmd ...string) (*Process, error) `Stdio` inherits all three (identical to Spawn). Close a piped `Stdin` before `Wait` (or let `Wait` close it for you) so a reader child sees EOF; drain a piped `Stdout`/`Stderr` before `Wait`, or `Kill` from another goroutine to - interrupt a blocked `Wait`. `Wait` returns a `Result` with the exit status - only — a Popen'd process sends piped output to the `Stdout`/`Stderr` fields + interrupt a blocked `Wait`. `Wait` returns an `ExitStatus` and retains the session. + A Popen'd process sends piped output to the `Stdout`/`Stderr` fields (inherited/null streams go to the parent fd or `/dev/null`), so (unlike `Run`) `Result.Stdout`/`Result.Stderr` are always empty. @@ -260,14 +260,16 @@ sb := &sandlock.Sandbox{ ```go func (p *Process) Pid() int -func (p *Process) Wait() (*Result, error) +func (p *Process) Wait() (*ExitStatus, error) +func (p *Process) Shutdown() (*Result, error) +func (p *Process) ProcessGroups() ([]int, error) func (p *Process) Pause() error // SIGSTOP to the process group func (p *Process) Resume() error // SIGCONT 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. +// BranchActionDefer only: after Shutdown, 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) @@ -279,6 +281,12 @@ p.Stdout // *os.File p.Stderr // *os.File ``` +`Wait` reports the main process's status while background jobs remain supervised. +`ProcessGroups` returns a sorted snapshot of tracked live groups. `Shutdown` +terminates remaining jobs with SIGKILL, reaps them, and returns captured output +and filesystem changes. `Run` performs both steps automatically. Use `Close` +when no result is needed; it also closes caller-owned streams. + ### Confine the current process ```go diff --git a/go/sandbox.go b/go/sandbox.go index 28d6c2fad..5d48bba73 100644 --- a/go/sandbox.go +++ b/go/sandbox.go @@ -321,6 +321,14 @@ const ( ReasonTimeout ExitReason = 3 ) +// ExitStatus describes the main process without ending its sandbox session. +type ExitStatus struct { + ExitCode int + Reason ExitReason + Signal int + Success bool +} + // Result is the outcome of a captured run. type Result struct { ExitCode int // process exit code, or -1 if terminated abnormally diff --git a/go/sandlock_linux.go b/go/sandlock_linux.go index 91ebe7e20..773bb785e 100644 --- a/go/sandlock_linux.go +++ b/go/sandlock_linux.go @@ -731,13 +731,23 @@ func (s *Sandbox) Run(ctx context.Context, cmd ...string) (*Result, error) { C.sandlock_handle_free(h) return nil, fmt.Errorf("sandlock: failed to start sandbox") } - r := C.sandlock_handle_wait_timeout(h, timeoutMs(ctx)) + status := C.sandlock_handle_wait_timeout(h, timeoutMs(ctx)) + if status == nil { + C.sandlock_handle_free(h) + return nil, fmt.Errorf("sandlock: wait failed") + } + timedOut := ExitReason(C.sandlock_result_reason(status)) == ReasonTimeout + C.sandlock_result_free(status) + r := C.sandlock_handle_shutdown(h) C.sandlock_handle_free(h) if r == nil { - return nil, fmt.Errorf("sandlock: wait failed") + return nil, fmt.Errorf("sandlock: shutdown failed") } res := readResult(r) C.sandlock_result_free(r) + if timedOut { + res.ExitCode, res.Reason, res.Signal, res.Success = -1, ReasonTimeout, -1, false + } return res, nil } @@ -852,21 +862,19 @@ func SyscallNr(name string) (int, error) { return int(nr), nil } -// Process is a live sandboxed process started by Spawn. It supports PID -// inspection, pause/resume/kill via the process group, and Wait. A Process -// holds at most one running command; create separate Spawns for concurrency. -// -// The underlying FFI handle is not safe for concurrent access, so all handle -// operations are serialized. Pause/Resume/Kill act on the OS process group by -// PID and touch no handle state, so they remain usable while Wait blocks on -// the handle — that is how Kill interrupts a blocked Wait. Ports, by contrast, -// reads the handle and is reported as empty while a Wait is in flight. +// Process owns a sandbox session started by Spawn or Popen. Wait reports the +// main process's status; Shutdown terminates remaining jobs and collects output. +// Close releases the session and caller-owned streams without returning output. +// Handle operations are serialized, including signals during a blocked Wait. type Process struct { - mu sync.Mutex - 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 + mu sync.Mutex + h *C.sandlock_handle_t + pid int + waiting bool // a Wait reserves the session between bounded native calls + closeRequested bool + status *ExitStatus + result *Result + 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 @@ -980,7 +988,7 @@ func (s *Sandbox) Popen(stdio Stdio, cmd ...string) (*Process, error) { } // finalize is the SetFinalizer cleanup for a Process abandoned without -// Wait/Close. It can only run once the Process is unreachable, which implies +// Shutdown/Close. It can only run once the Process is unreachable, which implies // no Wait is in flight (a blocked Wait keeps the Process reachable), so the // handle is not concurrently borrowed and is safe to free here. The piped // streams are intentionally left alone: once the Process is unreachable so are @@ -991,9 +999,6 @@ func (p *Process) finalize() { if p.h == nil { return } - if p.pid > 0 { - _ = syscall.Kill(-p.pid, syscall.SIGKILL) - } C.sandlock_handle_free(p.h) p.h = nil } @@ -1005,59 +1010,115 @@ func (p *Process) Pid() int { return p.pid } -// Wait blocks until the process exits, returns its Result, and releases the -// handle. After Wait the Process is no longer running. -// -// The Result carries the exit status only. Unlike Run, a Popen'd process sends -// any StdioPiped output to the caller through the Stdout/Stderr fields (and -// StdioInherit/StdioNull output to the inherited fd or /dev/null), so it is -// never captured into Result.Stdout/Result.Stderr — read piped bytes off -// p.Stdout/p.Stderr, not off the Result. -// -// The blocking native wait runs without holding the mutex so that Kill (and -// Pause/Resume), which signal the process group by PID, can run concurrently -// and interrupt it. The waiting flag reserves exclusive use of the handle for -// the duration, so no other handle operation aliases it. -// -// Wait closes a still-open piped Stdin to deliver EOF, so do not write to Stdin -// from another goroutine while Wait runs — the write would race a closing fd. -func (p *Process) Wait() (*Result, error) { +// Wait returns the main process's status, retaining background supervision. +// It closes piped Stdin; caller-owned output must be drained concurrently if +// needed. Repeated waits return the cached status. Kill can interrupt a wait. +func (p *Process) Wait() (*ExitStatus, error) { p.mu.Lock() - if p.h == nil || p.waiting { + if p.waiting { + p.mu.Unlock() + return nil, ErrNotRunning + } + if p.status != nil { + status := *p.status + p.mu.Unlock() + return &status, nil + } + if p.h == nil { p.mu.Unlock() return nil, ErrNotRunning } - h := p.h - stdin := p.Stdin p.waiting = true + if p.Stdin != nil { + _ = p.Stdin.Close() + } p.mu.Unlock() - - // Deliver EOF to a still-open piped stdin so a reader child (e.g. cat) can - // exit; otherwise the native wait blocks forever. Harmless if already closed. - if stdin != nil { - _ = stdin.Close() + defer func() { + p.mu.Lock() + defer p.mu.Unlock() + p.waiting = false + if p.closeRequested { + C.sandlock_handle_free(p.h) + p.h = nil + runtime.SetFinalizer(p, nil) + } + }() + for { + p.mu.Lock() + // Bounded waits let signals use the same handle without aliasing Rust borrows. + r := C.sandlock_handle_wait_timeout(p.h, 50) + if r == nil { + p.mu.Unlock() + return nil, fmt.Errorf("sandlock: wait failed") + } + status := ExitStatus{ + ExitCode: int(C.sandlock_result_exit_code(r)), + Reason: ExitReason(C.sandlock_result_reason(r)), + Signal: int(C.sandlock_result_signal(r)), + Success: bool(C.sandlock_result_success(r)), + } + C.sandlock_result_free(r) + if status.Reason != ReasonTimeout { + p.status = &status + p.mu.Unlock() + copy := status + return ©, nil + } + p.mu.Unlock() + runtime.Gosched() } +} - r := C.sandlock_handle_wait(h) - +// Shutdown sends SIGKILL to remaining jobs, reaps them, and collects output and +// filesystem changes. Caller-owned output streams stay open. A deferred branch +// stays available for Commit or Abort. Repeated calls return the cached result. +func (p *Process) Shutdown() (*Result, error) { p.mu.Lock() defer p.mu.Unlock() - p.waiting = false - // 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 { + if p.waiting { + return nil, ErrNotRunning + } + if p.result != nil { + return p.result, nil + } + if p.h == nil { + return nil, ErrNotRunning + } + r := C.sandlock_handle_shutdown(p.h) + if r == nil { + return nil, fmt.Errorf("sandlock: shutdown failed") + } + p.result = readResult(r) + C.sandlock_result_free(r) + p.status = &ExitStatus{ExitCode: p.result.ExitCode, Reason: p.result.Reason, + Signal: p.result.Signal, Success: p.result.Success} + if C.sandlock_handle_pending(p.h) != 0 { p.pending = true } else { - C.sandlock_handle_free(h) + C.sandlock_handle_free(p.h) p.h = nil runtime.SetFinalizer(p, nil) } - if r == nil { - return nil, fmt.Errorf("sandlock: wait failed") + return p.result, nil +} + +// ProcessGroups returns a sorted snapshot of tracked live process group IDs. +func (p *Process) ProcessGroups() ([]int, error) { + p.mu.Lock() + defer p.mu.Unlock() + if p.h == nil { + return nil, ErrNotRunning } - res := readResult(r) - C.sandlock_result_free(r) - return res, nil + raw := C.sandlock_handle_process_groups(p.h) + if raw == nil { + return nil, fmt.Errorf("sandlock: process group inspection failed") + } + defer C.sandlock_string_free(raw) + var groups []int + if err := json.Unmarshal([]byte(C.GoString(raw)), &groups); err != nil { + return nil, err + } + return groups, nil } // Pending reports whether the process exited under BranchActionDefer and its @@ -1118,45 +1179,35 @@ func (p *Process) dispose(op func(*C.sandlock_handle_t) C.int, name string) erro func (p *Process) signal(sig syscall.Signal) error { p.mu.Lock() defer p.mu.Unlock() - if p.h == nil || p.pid <= 0 || p.pending { + if p.h == nil || p.pending { + if sig == syscall.SIGKILL { + return nil + } return ErrNotRunning } - // The sandbox child leads its own process group; signal the whole group. - return syscall.Kill(-p.pid, sig) + var rc C.int + switch sig { + case syscall.SIGSTOP: + rc = C.sandlock_handle_pause(p.h) + case syscall.SIGCONT: + rc = C.sandlock_handle_resume(p.h) + case syscall.SIGKILL: + rc = C.sandlock_handle_kill(p.h) + } + if rc != 0 { + return fmt.Errorf("sandlock: signal failed") + } + return nil } -// Pause sends SIGSTOP to the sandbox process group. +// Pause sends SIGSTOP to the tracked live process groups. func (p *Process) Pause() error { return p.signal(syscall.SIGSTOP) } -// Resume sends SIGCONT to the sandbox process group. +// Resume sends SIGCONT to the tracked live process groups. func (p *Process) Resume() error { return p.signal(syscall.SIGCONT) } -// Kill sends SIGKILL to the sandbox process group. It is idempotent: a process -// that already exited (ESRCH) or was already reaped by Wait (ErrNotRunning) is -// not an error — killing it is a no-op success, matching the FFI -// (sandlock_handle_kill returns 0) and the Python binding (silent no-op after -// wait()). This keeps the "Kill from another goroutine" escape hatch race-free: -// a Kill that fires just as Wait reaps the child still returns nil. -func (p *Process) Kill() error { - err := p.signal(syscall.SIGKILL) - if err == syscall.ESRCH { - return nil // child already exited (still unreaped) — no-op - } - if err == ErrNotRunning { - // signal() returns ErrNotRunning for two causes: the handle was already - // reaped by Wait (h == nil) — an idempotent no-op — or a live handle with - // no pid. Only the reaped case is a success; a missing pid is a genuine - // error we must surface (and must never turn into killpg(-0), which would - // signal our own process group). - p.mu.Lock() - reaped := p.h == nil - p.mu.Unlock() - if reaped { - return nil - } - } - return err -} +// Kill sends SIGKILL to the tracked live process groups. It is idempotent. +func (p *Process) Kill() error { return p.signal(syscall.SIGKILL) } // Ports returns the current virtual-to-real TCP port mappings while the // process is running. It is non-empty only when PortRemap is enabled and at @@ -1206,10 +1257,9 @@ func (p *Process) Close() error { return nil } if p.waiting { - // A Wait owns the handle and will free it; just kill the process - // group by PID to unblock that Wait, without touching the handle. - if p.pid > 0 { - _ = syscall.Kill(-p.pid, syscall.SIGKILL) + p.closeRequested = true + if C.sandlock_handle_kill(p.h) != 0 { + return fmt.Errorf("sandlock: kill failed") } return nil } diff --git a/go/sandlock_linux_test.go b/go/sandlock_linux_test.go index d1c261377..3f0d461b9 100644 --- a/go/sandlock_linux_test.go +++ b/go/sandlock_linux_test.go @@ -393,7 +393,13 @@ func TestDeferHoldsTheBranchForTheProcess(t *testing.T) { t.Fatalf("Spawn: %v", err) } defer p.Close() - res, err := p.Wait() + if _, err := p.Wait(); err != nil { + t.Fatal(err) + } + if p.Pending() { + t.Fatal("wait must not settle the branch") + } + res, err := p.Shutdown() if err != nil { t.Fatalf("Wait: %v", err) } @@ -401,7 +407,7 @@ func TestDeferHoldsTheBranchForTheProcess(t *testing.T) { t.Fatalf("unexpected result: %+v", res) } if !p.Pending() { - t.Fatal("a Defer run must be pending after Wait") + t.Fatal("a Defer run must be pending after Shutdown") } if _, statErr := os.Stat(dir + "/out.txt"); statErr == nil { t.Fatal("nothing may land before Commit") @@ -441,6 +447,9 @@ func TestDeferAbortDiscards(t *testing.T) { if _, err := p.Wait(); err != nil { t.Fatalf("Wait: %v", err) } + if _, err := p.Shutdown(); err != nil { + t.Fatal(err) + } if err := p.Abort(); err != nil { t.Fatalf("Abort: %v", err) } @@ -556,7 +565,7 @@ func TestPopenWaitClosesUnclosedStdin(t *testing.T) { // stdout concurrently so a full pipe can never be the reason cat blocks. go io.ReadAll(p.Stdout) - done := make(chan *sandlock.Result, 1) + done := make(chan *sandlock.ExitStatus, 1) go func() { res, _ := p.Wait() done <- res diff --git a/go/session_linux_test.go b/go/session_linux_test.go new file mode 100644 index 000000000..7f4e772aa --- /dev/null +++ b/go/session_linux_test.go @@ -0,0 +1,99 @@ +//go:build linux + +package sandlock_test + +import ( + "context" + "fmt" + "github.com/multikernel/sandlock/go" + "runtime" + "testing" + "time" +) + +func TestSessionWaitRetainsBackgroundJobs(t *testing.T) { + requireLandlock(t) + sb := &sandlock.Sandbox{FSReadable: rootfs} + p, err := sb.Spawn("sh", "-c", "sleep 60 & echo retained; exit 7") + if err != nil { + t.Fatal(err) + } + defer p.Close() + status, err := p.Wait() + if err != nil || status.ExitCode != 7 { + t.Fatalf("wait: %+v, %v", status, err) + } + again, err := p.Wait() + if err != nil || *again != *status { + t.Fatalf("repeat wait: %+v, %v", again, err) + } + groups, err := p.ProcessGroups() + if err != nil || len(groups) == 0 { + t.Fatalf("groups: %v, %v", groups, err) + } + result, err := p.Shutdown() + if err != nil || result.ExitCode != 7 || string(result.Stdout) != "retained\n" { + t.Fatalf("shutdown: %+v, %v", result, err) + } +} + +func TestSessionCloseInterruptsWait(t *testing.T) { + requireLandlock(t) + sb := &sandlock.Sandbox{FSReadable: rootfs} + p, err := sb.Spawn("sleep", "60") + if err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + go func() { _, err := p.Wait(); done <- err }() + time.Sleep(100 * time.Millisecond) + if err := p.Close(); err != nil { + t.Fatal(err) + } + select { + case err := <-done: + if err != nil { + t.Fatal(err) + } + case <-time.After(5 * time.Second): + t.Fatal("close did not release wait") + } +} + +func TestSessionRunTimeoutStillCleansUp(t *testing.T) { + requireLandlock(t) + sb := &sandlock.Sandbox{FSReadable: rootfs} + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Millisecond) + defer cancel() + result, err := sb.Run(ctx, "sleep", "60") + if err != nil || result.Reason != sandlock.ReasonTimeout || result.Success { + t.Fatalf("run: %+v, %v", result, err) + } +} + +func TestSessionFinalizerCleansJobsAfterMainExit(t *testing.T) { + requireLandlock(t) + child := func() int { + sb := &sandlock.Sandbox{FSReadable: rootfs} + p, err := sb.Popen(sandlock.Stdio{Stdout: sandlock.StdioPiped}, "sh", "-c", "sleep 60 & echo $!") + if err != nil { + t.Fatal(err) + } + var pid int + if _, err := fmt.Fscanln(p.Stdout, &pid); err != nil { + t.Fatal(err) + } + if status, err := p.Wait(); err != nil || !status.Success { + t.Fatalf("wait: %+v, %v", status, err) + } + return pid + }() + deadline := time.Now().Add(5 * time.Second) + for !procDead(child) { + runtime.GC() + if time.Now().After(deadline) { + t.Fatal("finalizer left background job alive") + } + time.Sleep(20 * time.Millisecond) + } +} From 9d20daeed0bc3f687b057b4eaf21ddf80e86d676 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Tue, 13 Oct 2026 15:19:12 -0700 Subject: [PATCH 05/23] docs: describe sandbox session lifetime Main-process completion and session completion are now separate events. The sandbox reference needs to explain when background supervision, captured output and staged filesystem changes remain active. Document the shared create, wait, signal, shutdown and run semantics without binding-specific examples. Explain timeout behavior, tracked group snapshots, output ownership and COW settlement at session completion. Clarify that shutdown uses SIGKILL and that releasing the owning handle also cleans up its jobs. Signed-off-by: Cong Wang --- docs/sandbox-reference.md | 50 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/docs/sandbox-reference.md b/docs/sandbox-reference.md index 8b580a4db..6d1a3fb6b 100644 --- a/docs/sandbox-reference.md +++ b/docs/sandbox-reference.md @@ -480,6 +480,54 @@ The two `allow_degraded` calls let the sandbox build on Linux kernels below 6.12, where the v6 IPC scopes are unavailable. On a kernel that does support them, the scopes remain enforced. +## Session lifecycle + +A sandbox session owns the main process launched by the caller and its +background jobs. The main process exiting does not end the session: +remaining jobs continue under the configured supervision until shutdown. + +The operations below describe the shared lifecycle. Method spelling and +return types vary by binding. + +| Operation | Behavior | +| :-------- | :------- | +| Create and start | Create a confined main process, then release it to execute the command. Spawn combines these steps. | +| Wait | Wait for the main process and return its exit status, retaining the session and background supervision. Repeated waits return the same main status. | +| Inspect process groups | Return a sorted snapshot of tracked live process group IDs. Membership can change immediately after inspection. | +| Pause, resume, kill | Send SIGSTOP, SIGCONT, or SIGKILL to tracked live process groups. Sending a signal does not collect the session's final result. | +| Shutdown | Send SIGKILL to remaining jobs, reap the session's children, release supervision, and return the main status, captured output, and filesystem changes. | +| Run | Start a command, wait for its main process, then shut down the session and return the complete result. | + +For a command that launches a background service, wait can report the +launcher's exit while the service remains available. Keep the session +alive while using the service, then shut it down. Run ends the session +as soon as the main process finishes, terminating any remaining jobs. +Shutdown can also be called before the main process exits to terminate +the whole session; it does not perform a graceful termination sequence. + +Where a wait timeout is supported, expiration leaves the session alive. +The caller can wait again or shut down. A run timeout terminates the +session and reports a timeout result. + +### Output and filesystem changes + +Wait returns the main status without waiting for output EOF: background +jobs may still hold output pipes open and write to them. Captured output +continues draining and is collected at shutdown. With streaming stdio, +caller-owned output streams must be drained as needed to prevent a full +pipe from blocking the workload; their bytes are not included in the +collected result. + +COW writes remain staged while the session is active, including writes +made by background jobs after the main process exits. Shutdown collects +the change set and applies the configured branch action after the jobs +have exited. The main process's status selects `on_exit` or `on_error`. +With `DEFER`, the branch becomes pending at session completion and stays +available for an explicit commit or abort. + +Releasing the owning session handle also terminates and reaps its jobs. +Use explicit shutdown when the caller needs the complete result. + ## Enumerations ### `BranchAction` @@ -547,7 +595,7 @@ parse_ports([80, "443", "8000-8005"]) so a dynamic override can never erase it. 3. **Seccomp COW with `workdir`.** When `workdir` is set, the seccomp-based COW path intercepts writes under `workdir` and stages - them in an upper layer, committed or aborted on exit per `on_exit` / + them in an upper layer, committed or aborted at session completion per `on_exit` / `on_error`. Staging covers regular files, directories, and symlinks. A FIFO, socket, or device node under `workdir` is a kernel object: opening it or changing its metadata goes to the kernel under the From 6c496338f203b8b4f020a78f861a85a5fd6850b1 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Tue, 13 Oct 2026 15:59:09 -0700 Subject: [PATCH 06/23] sandbox: reap sessions without pidfd group signals Shutdown asked the private reaper to terminate descendants, then also sent pidfd group signals. Kernels such as Linux 6.8 reject that flag with EINVAL, causing even a completed echo command to fail under a degraded policy. Use the reaper's shutdown channel for sessions it owns and wait for it to finish reaping before releasing supervision or settling COW. Keep policy callback cancellation and the legacy cleanup path for sessions without a reaper. Skip group signalling on repeated shutdown as well. Add a subprocess regression that rejects pidfd group signals through seccomp. Cover complete runs, repeated shutdown and cleanup of a child that outlives the main process in a separate session. All 87 selected protection, process-group, policy, streaming and COW tests pass. Signed-off-by: Cong Wang --- crates/sandlock-core/src/sandbox.rs | 18 ++- .../tests/integration/test_protection.rs | 132 ++++++++++++++++++ 2 files changed, 144 insertions(+), 6 deletions(-) diff --git a/crates/sandlock-core/src/sandbox.rs b/crates/sandlock-core/src/sandbox.rs index 5877581bb..bda6a8b73 100644 --- a/crates/sandlock-core/src/sandbox.rs +++ b/crates/sandlock-core/src/sandbox.rs @@ -1028,16 +1028,22 @@ impl Sandbox { pub async fn shutdown(&mut self) -> Result { use crate::error::SandboxRuntimeError; use crate::result::RunResult; - if let Some(reaper) = self.runtime.as_mut().and_then(|rt| rt.reaper.as_mut()) { + let rt = self.runtime.as_mut().ok_or(SandboxRuntimeError::NotRunning)?; + let signal_groups = rt.reaper.is_none() && !matches!(rt.state, RuntimeState::Stopped(_)); + if let Some(reaper) = &mut rt.reaper { + // The reaper owns descendant cleanup even on kernels without pidfd group signals. reaper.shutdown(); + if let Some(worker) = &rt.policy_fn_worker { worker.cancel(); } } - self.kill()?; + if signal_groups { self.kill()?; } let exit_status = self.wait().await?; - loop { - if self.rt().groups.signal(libc::SIGKILL).map_err(SandboxRuntimeError::Io)? == 0 { - break; + if signal_groups { + loop { + if self.rt().groups.signal(libc::SIGKILL).map_err(SandboxRuntimeError::Io)? == 0 { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(10)).await; } - tokio::time::sleep(std::time::Duration::from_millis(10)).await; } if let Some(reaper) = &self.rt().reaper { loop { diff --git a/crates/sandlock-core/tests/integration/test_protection.rs b/crates/sandlock-core/tests/integration/test_protection.rs index d12ed2b98..df46be652 100644 --- a/crates/sandlock-core/tests/integration/test_protection.rs +++ b/crates/sandlock-core/tests/integration/test_protection.rs @@ -534,3 +534,135 @@ async fn degraded_policy_confines_and_runs_below_v6() { "degraded-mode confinement must still execute the child" ); } + +#[test] +fn shutdown_without_pidfd_group_signals() { + const CHILD: &str = "SANDLOCK_TEST_NO_PIDFD_GROUP_SIGNALS"; + if std::env::var_os(CHILD).is_none() { + let mut child = std::process::Command::new(std::env::current_exe().unwrap()) + .args([ + "test_protection::shutdown_without_pidfd_group_signals", + "--exact", + "--nocapture", + ]) + .env(CHILD, "1") + .spawn() + .unwrap(); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(20); + loop { + if let Some(status) = child.try_wait().unwrap() { + assert!(status.success()); + return; + } + if std::time::Instant::now() >= deadline { + child.kill().unwrap(); + child.wait().unwrap(); + panic!("shutdown hung without pidfd group signals"); + } + std::thread::sleep(std::time::Duration::from_millis(10)); + } + } + + // Emulate kernels that reject PIDFD_SIGNAL_PROCESS_GROUP, including 6.8. + let filter = [ + libc::sock_filter { + code: 0x20, + jt: 0, + jf: 0, + k: 0, + }, + libc::sock_filter { + code: 0x15, + jt: 0, + jf: 3, + k: libc::SYS_pidfd_send_signal as u32, + }, + libc::sock_filter { + code: 0x20, + jt: 0, + jf: 0, + k: 16 + 3 * 8, + }, + libc::sock_filter { + code: 0x45, + jt: 0, + jf: 1, + k: 1 << 2, + }, + libc::sock_filter { + code: 0x06, + jt: 0, + jf: 0, + k: libc::SECCOMP_RET_ERRNO | libc::EINVAL as u32, + }, + libc::sock_filter { + code: 0x06, + jt: 0, + jf: 0, + k: libc::SECCOMP_RET_ALLOW, + }, + ]; + let program = libc::sock_fprog { + len: filter.len() as u16, + filter: filter.as_ptr() as *mut _, + }; + unsafe { + assert_eq!(libc::prctl(libc::PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0), 0); + assert_eq!( + libc::prctl(libc::PR_SET_SECCOMP, libc::SECCOMP_MODE_FILTER, &program), + 0 + ); + } + let dir = tempfile::tempdir().unwrap(); + let ready = dir.path().join("ready"); + let mut builder = sandlock_core::Sandbox::builder(); + for protection in Protection::all() { + builder = builder.allow_degraded(protection); + } + let mut sb = builder + .fs_read("/usr") + .fs_read("/lib") + .fs_read_if_exists("/lib64") + .fs_read("/bin") + .fs_read("/etc") + .fs_write(dir.path()) + .build() + .unwrap(); + let policy = sb.clone(); + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap() + .block_on(async { + let result = sb.run(&["echo", "ok"]).await.unwrap(); + assert!(result.success()); + assert_eq!(result.stdout.as_deref(), Some(b"ok\n".as_slice())); + assert!(sb.shutdown().await.unwrap().success()); + let mut sb = policy; + let script = format!( + r#" +import os, signal, time +if os.fork() == 0: + os.setsid() + open({ready:?}, "w").write(str(os.getpid())) + signal.pause() +while not os.path.exists({ready:?}): + time.sleep(0.01) +os._exit(7) +"# + ); + sb.spawn(&["python3", "-c", &script]).await.unwrap(); + assert_eq!(sb.wait().await.unwrap(), sandlock_core::ExitStatus::Code(7)); + let child: i32 = std::fs::read_to_string(&ready).unwrap().parse().unwrap(); + let result = tokio::time::timeout(std::time::Duration::from_secs(5), sb.shutdown()) + .await + .unwrap() + .unwrap(); + assert_eq!(result.code(), Some(7)); + assert_eq!(unsafe { libc::kill(child, 0) }, -1); + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::ESRCH) + ); + }); +} From 63fac0b370b6e590a46a5542c9ecf4597431cb94 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Tue, 13 Oct 2026 15:59:27 -0700 Subject: [PATCH 07/23] python: declare the native kill handle type The lifecycle changes routed kill through sandlock_handle_kill without registering its ctypes signature. The default integer conversion can truncate a native handle to 32 bits, crashing checkpoint and lifecycle tests on hosts where allocations use higher addresses. Declare the handle argument as c_void_p alongside pause and resume. Add a regression that checks pointer-width argument handling and kills a live sandbox from a fresh interpreter. The full Python suite passes: 659 tests passed and 15 skipped. Signed-off-by: Cong Wang --- python/src/sandlock/_sdk.py | 2 +- python/tests/test_session.py | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/python/src/sandlock/_sdk.py b/python/src/sandlock/_sdk.py index 59dbfd2b8..576024437 100644 --- a/python/src/sandlock/_sdk.py +++ b/python/src/sandlock/_sdk.py @@ -323,7 +323,7 @@ def confine(policy: "PolicyDataclass") -> None: _lib.sandlock_handle_shutdown.argtypes = [_c_handle_p] _lib.sandlock_handle_process_groups.restype = ctypes.c_void_p _lib.sandlock_handle_process_groups.argtypes = [_c_handle_p] -for _name in ("sandlock_handle_pause", "sandlock_handle_resume"): +for _name in ("sandlock_handle_pause", "sandlock_handle_resume", "sandlock_handle_kill"): getattr(_lib, _name).restype = ctypes.c_int getattr(_lib, _name).argtypes = [_c_handle_p] diff --git a/python/tests/test_session.py b/python/tests/test_session.py index 749abe361..84601c6e9 100644 --- a/python/tests/test_session.py +++ b/python/tests/test_session.py @@ -106,3 +106,23 @@ def test_wait_rejects_conflicting_start_and_checkpoint(): sb.kill() worker.join(5) assert not worker.is_alive() + + +def test_kill_in_fresh_interpreter(): + import ctypes + from sandlock._sdk import _lib + assert _lib.sandlock_handle_kill.argtypes == [ctypes.c_void_p] + import subprocess + import sys + result = subprocess.run( + [sys.executable, "-c", """ +from sandlock import Sandbox +with Sandbox(fs_readable=["/usr", "/lib", "/lib64", "/bin", "/etc"]) as sb: + sb.spawn(["sleep", "60"]) + sb.kill() + assert not sb.wait().success + sb.shutdown() +"""], + capture_output=True, timeout=10, + ) + assert result.returncode == 0, result.stderr.decode() From 786d9f1dc7e2f6a6c683acfbf090d2c17f7201a8 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 14 Oct 2026 11:35:05 -0700 Subject: [PATCH 08/23] sandbox: track session processes without scanning procfs The session reaper killed leftover descendants by reading /proc/thread-self/children. That needs CONFIG_PROC_CHILDREN and a usable procfs, and when the read failed the reaper waited forever, hanging shutdown and Drop. Every supervised process is in a tracked process group, since setsid and setpgid are trapped, so the leader pidfds already reach all of them. Let the reaper only reap: it reports the main status along with whether anything else remains, and exits once it has no children. Its exit is a kernel-stamped sign that the session is empty, and shutdown awaits it through a pidfd instead of polling waitpid. Jobs outside tracked groups (new groups under no_supervisor, or any job on kernels without pidfd group signals) cannot be killed without procfs. Shutdown asks the reaper to finish, releasing them still confined, and stops the capture drains so a released writer cannot hold the result hostage. Finish is a byte, not EOF, because children parked before exec may hold a copy of the pipe. Signed-off-by: Cong Wang --- crates/sandlock-core/src/pgroup.rs | 2 +- crates/sandlock-core/src/reaper.rs | 279 ++++++++++-------- crates/sandlock-core/src/sandbox.rs | 84 +++--- .../tests/integration/test_process_groups.rs | 15 +- .../tests/integration/test_protection.rs | 14 +- docs/sandbox-reference.md | 9 +- 6 files changed, 233 insertions(+), 170 deletions(-) diff --git a/crates/sandlock-core/src/pgroup.rs b/crates/sandlock-core/src/pgroup.rs index a7cbca9c1..5a010a6c9 100644 --- a/crates/sandlock-core/src/pgroup.rs +++ b/crates/sandlock-core/src/pgroup.rs @@ -103,7 +103,7 @@ impl Default for ProcessGroups { } } -fn has_exited(pidfd: RawFd) -> bool { +pub(crate) fn has_exited(pidfd: RawFd) -> bool { let mut pfd = libc::pollfd { fd: pidfd, events: libc::POLLIN, diff --git a/crates/sandlock-core/src/reaper.rs b/crates/sandlock-core/src/reaper.rs index 592c0e786..c454a8893 100644 --- a/crates/sandlock-core/src/reaper.rs +++ b/crates/sandlock-core/src/reaper.rs @@ -3,31 +3,31 @@ use std::os::fd::{AsRawFd, FromRawFd, OwnedFd}; pub(crate) struct Reaper { pub pid: i32, + pub pidfd: OwnedFd, pub main_pidfd: OwnedFd, status: OwnedFd, - command: Option, + finish: Option, + /// Set once the main process is known to have been the last one left. + pub empty: bool, } -impl Reaper { - pub fn shutdown(&mut self) { - self.command.take(); - } +pub(crate) struct MainExit { + pub status: i32, + pub last: bool, +} - pub async fn main_status(&self) -> io::Result { +impl Reaper { + pub async fn main_exit(&self) -> io::Result { let fd = tokio::io::unix::AsyncFd::new(self.status.try_clone()?)?; loop { let mut ready = fd.readable().await?; let result = ready.try_io(|fd| { - let mut status = 0i32; + let mut message = [0i32; 2]; let n = unsafe { - libc::read( - fd.get_ref().as_raw_fd(), - (&mut status as *mut i32).cast(), - 4, - ) + libc::read(fd.get_ref().as_raw_fd(), message.as_mut_ptr().cast(), 8) }; match n { - 4 => Ok(status), + 8 => Ok(MainExit { status: message[0], last: message[1] != 0 }), -1 => Err(io::Error::last_os_error()), _ => Err(io::Error::new( io::ErrorKind::UnexpectedEof, @@ -42,10 +42,30 @@ impl Reaper { } } } + + /// Collect what has exited, then exit. Children still alive are not in + /// any tracked group, so nothing could kill them; they are released. + pub fn finish(&mut self) { + // A byte, not EOF: children parked before exec may hold a copy of this fd. + if let Some(fd) = self.finish.take() { + unsafe { send(fd.as_raw_fd(), &[0]) }; + } + } + + pub async fn join(&self) -> io::Result<()> { + crate::sandbox::wait_child_exit_via_pidfd(self.pidfd.try_clone()?, self.pid).await; + Ok(()) + } + + pub fn join_blocking(self) { + reap(self.pid); + } } /// A private subreaper keeps orphans below the supervisor for Yama checks, -/// without changing the embedding process's child-adoption policy. +/// without changing the embedding process's child-adoption policy. It never +/// kills: the supervisor kills through the tracked process groups, and the +/// reaper exits once nothing is left to reap or it is told to finish. pub(crate) fn fork() -> io::Result<(i32, Option)> { let mut pipe = [0; 2]; if unsafe { libc::pipe2(pipe.as_mut_ptr(), libc::O_CLOEXEC) } < 0 { @@ -79,6 +99,13 @@ pub(crate) fn fork() -> io::Result<(i32, Option)> { { libc::_exit(127); } + let mut chld = std::mem::zeroed(); + libc::sigemptyset(&mut chld); + libc::sigaddset(&mut chld, libc::SIGCHLD); + let signal_fd = libc::signalfd(-1, &chld, libc::SFD_CLOEXEC | libc::SFD_NONBLOCK); + if signal_fd < 0 { + libc::_exit(127); + } let parent = libc::getpid(); let child = libc::fork(); if child < 0 { @@ -100,16 +127,17 @@ pub(crate) fn fork() -> io::Result<(i32, Option)> { } let fd = write.as_raw_fd(); let gate = pinned_r.as_raw_fd(); - let lo = fd.min(gate) as u32; - let hi = fd.max(gate) as u32; - if lo > 0 { - libc::syscall(libc::SYS_close_range, 0u32, lo - 1, 0u32); - } - if hi > lo + 1 { - libc::syscall(libc::SYS_close_range, lo + 1, hi - 1, 0u32); + let mut keep = [fd, gate, signal_fd]; + keep.sort_unstable(); + let mut next = 0u32; + for kept in keep { + if kept as u32 > next { + libc::syscall(libc::SYS_close_range, next, kept as u32 - 1, 0u32); + } + next = kept as u32 + 1; } - libc::syscall(libc::SYS_close_range, hi + 1, u32::MAX, 0u32); - if !send(fd, child) { + libc::syscall(libc::SYS_close_range, next, u32::MAX, 0u32); + if !send(fd, &[child]) { libc::_exit(127); } // Keep the main PID pinned as a child until the supervisor has a pidfd. @@ -120,10 +148,13 @@ pub(crate) fn fork() -> io::Result<(i32, Option)> { break n == 4; } if io::Error::last_os_error().raw_os_error() != Some(libc::EINTR) { - libc::_exit(127); + break false; } }; - supervise(child, fd, gate, !acknowledged); + if !acknowledged { + libc::kill(child, libc::SIGKILL); + } + supervise(child, fd, gate, signal_fd); } } drop(write); @@ -134,123 +165,80 @@ pub(crate) fn fork() -> io::Result<(i32, Option)> { result => break result, } }; - match child { - Ok(pid) if pid > 0 => { - let main_pidfd = match crate::sys::syscall::pidfd_open(pid, 0) { - Ok(fd) => fd, - Err(e) => { - unsafe { libc::kill(pid as i32, libc::SIGKILL) }; - drop(pinned_w); - reap(reaper); - return Err(e); - } - }; - unsafe { send(pinned_w.as_raw_fd(), 1) }; + let opened = match child { + Ok(pid) if pid > 0 => crate::sys::syscall::pidfd_open(pid, 0) + .and_then(|main| Ok((pid as i32, main, crate::sys::syscall::pidfd_open(reaper as u32, 0)?))), + _ => Err(io::Error::other("session reaper failed to start")), + }; + match opened { + Ok((pid, main_pidfd, pidfd)) => { + unsafe { send(pinned_w.as_raw_fd(), &[1]) }; unsafe { libc::fcntl(read.as_raw_fd(), libc::F_SETFL, libc::O_NONBLOCK) }; Ok(( - pid as i32, + pid, Some(Reaper { pid: reaper, + pidfd, main_pidfd, status: read, - command: Some(pinned_w), + finish: Some(pinned_w), + empty: false, }), )) } - _ => { + Err(e) => { + // Without the ack the reaper kills its pinned child itself. drop(pinned_w); reap(reaper); - Err(io::Error::other("session reaper failed to start")) + Err(e) } } } -unsafe fn supervise(main: i32, status_fd: i32, command_fd: i32, mut stopping: bool) -> ! { - let mut mask = std::mem::zeroed(); - libc::sigemptyset(&mut mask); - libc::sigaddset(&mut mask, libc::SIGCHLD); - let signal_fd = libc::signalfd(-1, &mask, libc::SFD_CLOEXEC | libc::SFD_NONBLOCK); +unsafe fn supervise(main: i32, status_fd: i32, finish_fd: i32, signal_fd: i32) -> ! { let mut poll = [ - libc::pollfd { - fd: command_fd, - events: libc::POLLIN, - revents: 0, - }, - libc::pollfd { - fd: signal_fd, - events: libc::POLLIN, - revents: 0, - }, + libc::pollfd { fd: finish_fd, events: libc::POLLIN, revents: 0 }, + libc::pollfd { fd: signal_fd, events: libc::POLLIN, revents: 0 }, ]; + let mut finishing = false; loop { - if stopping { - kill_children(); - } - loop { + let alive = loop { let mut status = 0; - let pid = libc::waitpid(-1, &mut status, libc::WNOHANG); - if pid == 0 { - break; - } - if pid < 0 { - if io::Error::last_os_error().raw_os_error() == Some(libc::EINTR) { - continue; + match libc::waitpid(-1, &mut status, libc::WNOHANG | libc::__WALL) { + 0 => break true, + pid if pid == main => { + let last = !has_children(); + send(status_fd, &[status, last as i32]); } - libc::_exit(0); - } - if pid == main && !send(status_fd, status) { - stopping = true; + pid if pid > 0 => {} + _ if io::Error::last_os_error().raw_os_error() == Some(libc::EINTR) => {} + // No child means no descendant: the session is over. + _ => break false, } + }; + if !alive || finishing { + libc::_exit(0); } - // A killed parent may have just transferred more children to us. - if stopping { - kill_children(); - } - libc::poll(poll.as_mut_ptr(), 2, if signal_fd < 0 { 10 } else { -1 }); + libc::poll(poll.as_mut_ptr(), 2, -1); if poll[0].revents != 0 { - stopping = true; - poll[0].fd = -1; + finishing = true; } if poll[1].revents != 0 { - let mut signal: libc::signalfd_siginfo = std::mem::zeroed(); - libc::read( - signal_fd, - (&mut signal as *mut libc::signalfd_siginfo).cast(), - std::mem::size_of_val(&signal), - ); + let mut info: libc::signalfd_siginfo = std::mem::zeroed(); + libc::read(signal_fd, (&mut info as *mut libc::signalfd_siginfo).cast(), std::mem::size_of_val(&info)); } } } -unsafe fn kill_children() { - let fd = libc::open( - b"/proc/thread-self/children\0".as_ptr().cast(), - libc::O_RDONLY | libc::O_CLOEXEC, - ); - if fd < 0 { - return; - } - let mut buffer = [0u8; 4096]; - let mut pid = 0i32; +unsafe fn has_children() -> bool { loop { - let n = libc::read(fd, buffer.as_mut_ptr().cast(), buffer.len()); - if n <= 0 { - break; - } - for &byte in &buffer[..n as usize] { - if byte.is_ascii_digit() { - pid = pid * 10 + (byte - b'0') as i32; - } else if pid > 0 { - // Only this process reaps these children, so their PIDs cannot be reused here. - libc::kill(pid, libc::SIGKILL); - pid = 0; - } + match libc::waitpid(-1, std::ptr::null_mut(), libc::WNOHANG | libc::__WALL) { + 0 => return true, + pid if pid > 0 => {} + _ if io::Error::last_os_error().raw_os_error() == Some(libc::EINTR) => {} + _ => return false, } } - if pid > 0 { - libc::kill(pid, libc::SIGKILL); - } - libc::close(fd); } fn reap(pid: i32) { @@ -264,9 +252,10 @@ fn reap(pid: i32) { } } -unsafe fn send(fd: i32, value: i32) -> bool { +unsafe fn send(fd: i32, values: &[i32]) -> bool { + let len = std::mem::size_of_val(values); loop { - if libc::write(fd, (&value as *const i32).cast(), 4) == 4 { + if libc::write(fd, values.as_ptr().cast(), len) == len as isize { return true; } if io::Error::last_os_error().raw_os_error() != Some(libc::EINTR) { @@ -287,18 +276,70 @@ mod tests { } let reaper = reaper.unwrap(); tokio::time::sleep(std::time::Duration::from_millis(20)).await; - let status = reaper.main_status().await.unwrap(); - assert!(libc::WIFEXITED(status)); - assert_eq!(libc::WEXITSTATUS(status), 42); + let exit = reaper.main_exit().await.unwrap(); + assert!(libc::WIFEXITED(exit.status)); + assert_eq!(libc::WEXITSTATUS(exit.status), 42); + assert!(exit.last); let mut ready = libc::pollfd { fd: reaper.main_pidfd.as_raw_fd(), events: libc::POLLIN, revents: 0, }; assert_eq!(unsafe { libc::poll(&mut ready, 1, 0) }, 1); - assert_eq!( - unsafe { libc::waitpid(reaper.pid, std::ptr::null_mut(), 0) }, - reaper.pid - ); + reaper.join().await.unwrap(); + } + + #[tokio::test] + async fn outlives_the_main_process_until_orphans_are_reaped() { + let (pid, reaper) = fork().unwrap(); + if pid == 0 { + unsafe { + if libc::fork() == 0 { + libc::usleep(300_000); + libc::_exit(0); + } + libc::_exit(7); + } + } + let reaper = reaper.unwrap(); + let exit = reaper.main_exit().await.unwrap(); + assert_eq!(libc::WEXITSTATUS(exit.status), 7); + assert!(!exit.last); + let mut ready = libc::pollfd { + fd: reaper.pidfd.as_raw_fd(), + events: libc::POLLIN, + revents: 0, + }; + assert_eq!(unsafe { libc::poll(&mut ready, 1, 50) }, 0); + reaper.join().await.unwrap(); + } + + #[tokio::test] + async fn finish_releases_children_still_running() { + let mut pipe = [0; 2]; + assert_eq!(unsafe { libc::pipe(pipe.as_mut_ptr()) }, 0); + let (pid, reaper) = fork().unwrap(); + if pid == 0 { + unsafe { + let orphan = libc::fork(); + if orphan == 0 { + libc::pause(); + libc::_exit(0); + } + libc::write(pipe[1], (&orphan as *const i32).cast(), 4); + libc::_exit(0); + } + } + let mut orphan = 0i32; + assert_eq!(unsafe { libc::read(pipe[0], (&mut orphan as *mut i32).cast(), 4) }, 4); + unsafe { + libc::close(pipe[0]); + libc::close(pipe[1]); + } + let mut reaper = reaper.unwrap(); + assert!(!reaper.main_exit().await.unwrap().last); + reaper.finish(); + reaper.join().await.unwrap(); + assert_eq!(unsafe { libc::kill(orphan, libc::SIGKILL) }, 0); } } diff --git a/crates/sandlock-core/src/sandbox.rs b/crates/sandlock-core/src/sandbox.rs index bda6a8b73..c9cb567e9 100644 --- a/crates/sandlock-core/src/sandbox.rs +++ b/crates/sandlock-core/src/sandbox.rs @@ -303,6 +303,9 @@ struct Runtime { // the runtime rather than to the `wait()` that started them, so a cancelled // `wait()` takes neither the reader nor what it has already produced. stdout_drain: Option, + /// Ends the drains once the reaper is gone: what still holds the write + /// ends was released and may never close them. + drain_cut: tokio_util::sync::CancellationToken, stderr_drain: Option, // Parent-held write end of a piped stdin (popen). The caller takes it via // `Process::take_stdin`; closing it signals EOF to the child. @@ -993,17 +996,19 @@ impl Sandbox { // `Process::take_stdout`/`take_stderr` is `None` here and stays theirs. if self.rt().stdout_drain.is_none() { if let Some(fd) = self.rt_mut()._stdout_read.take() { - self.rt_mut().stdout_drain = sandbox_spawn_pipe_drain(fd); + self.rt_mut().stdout_drain = sandbox_spawn_pipe_drain(fd, self.rt().drain_cut.clone()); } } if self.rt().stderr_drain.is_none() { if let Some(fd) = self.rt_mut()._stderr_read.take() { - self.rt_mut().stderr_drain = sandbox_spawn_pipe_drain(fd); + self.rt_mut().stderr_drain = sandbox_spawn_pipe_drain(fd, self.rt().drain_cut.clone()); } } let exit_status = if let Some(reaper) = &self.rt().reaper { - sandbox_wait_status_to_exit(reaper.main_status().await.map_err(SandboxRuntimeError::Io)?) + let exit = reaper.main_exit().await.map_err(SandboxRuntimeError::Io)?; + if let Some(reaper) = &mut self.rt_mut().reaper { reaper.empty = exit.last; } + sandbox_wait_status_to_exit(exit.status) } else { match self.rt().pidfd.as_ref() { Some(pidfd) => { @@ -1028,16 +1033,19 @@ impl Sandbox { pub async fn shutdown(&mut self) -> Result { use crate::error::SandboxRuntimeError; use crate::result::RunResult; - let rt = self.runtime.as_mut().ok_or(SandboxRuntimeError::NotRunning)?; - let signal_groups = rt.reaper.is_none() && !matches!(rt.state, RuntimeState::Stopped(_)); - if let Some(reaper) = &mut rt.reaper { - // The reaper owns descendant cleanup even on kernels without pidfd group signals. - reaper.shutdown(); - if let Some(worker) = &rt.policy_fn_worker { worker.cancel(); } - } - if signal_groups { self.kill()?; } + use std::os::fd::AsRawFd; + let rt = self.runtime.as_ref().ok_or(SandboxRuntimeError::NotRunning)?; + // A main process that already exited delivers its status promptly, + // and that status says whether it left anything behind to kill. + if rt.reaper.as_ref().is_some_and(|r| crate::pgroup::has_exited(r.main_pidfd.as_raw_fd())) { + self.wait().await?; + } + let rt = self.rt(); + let empty = matches!(rt.state, RuntimeState::Stopped(_)) + || rt.reaper.as_ref().is_some_and(|r| r.empty || crate::pgroup::has_exited(r.pidfd.as_raw_fd())); + if !empty { self.kill()?; } let exit_status = self.wait().await?; - if signal_groups { + if !empty { loop { if self.rt().groups.signal(libc::SIGKILL).map_err(SandboxRuntimeError::Io)? == 0 { break; @@ -1045,17 +1053,10 @@ impl Sandbox { tokio::time::sleep(std::time::Duration::from_millis(10)).await; } } - if let Some(reaper) = &self.rt().reaper { - loop { - let ret = unsafe { libc::waitpid(reaper.pid, std::ptr::null_mut(), libc::WNOHANG) }; - if ret > 0 { break; } - if ret < 0 { - let e = std::io::Error::last_os_error(); - if e.raw_os_error() == Some(libc::EINTR) { continue; } - return Err(SandboxRuntimeError::Io(e).into()); - } - tokio::time::sleep(std::time::Duration::from_millis(1)).await; - } + if let Some(reaper) = &mut self.rt_mut().reaper { + reaper.finish(); + reaper.join().await.map_err(SandboxRuntimeError::Io)?; + self.rt().drain_cut.cancel(); } self.rt_mut().reaper = None; self.rt_mut().state = RuntimeState::Stopped(exit_status.clone()); @@ -1736,6 +1737,7 @@ impl Sandbox { _stdout_read: None, _stderr_read: None, stdout_drain: None, + drain_cut: tokio_util::sync::CancellationToken::new(), stderr_drain: None, _stdin_write: None, #[cfg(feature = "cow")] @@ -1845,6 +1847,7 @@ impl Sandbox { _stdout_read: None, _stderr_read: None, stdout_drain: None, + drain_cut: tokio_util::sync::CancellationToken::new(), stderr_drain: None, _stdin_write: None, #[cfg(feature = "cow")] @@ -2661,10 +2664,14 @@ impl Drop for Sandbox { if let Some(ref mut rt) = self.runtime { if let Some(worker) = &rt.policy_fn_worker { worker.cancel(); } let _ = rt.groups.signal(libc::SIGKILL); - if let Some(reaper) = &mut rt.reaper { - reaper.shutdown(); + if let Some(reaper) = &rt.reaper { use std::os::fd::AsRawFd; unsafe { libc::syscall(libc::SYS_pidfd_send_signal, reaper.main_pidfd.as_raw_fd(), libc::SIGKILL, 0, 0) }; + // The reaper collects the zombies that keep a swept group + // counted, so the branch below settles after the jobs are gone. + while matches!(rt.groups.signal(libc::SIGKILL), Ok(n) if n > 0) { + std::thread::sleep(std::time::Duration::from_millis(10)); + } } if let Some(pid) = rt.child_pid { if matches!(rt.state, RuntimeState::Created | RuntimeState::Running | RuntimeState::Paused) { @@ -2682,11 +2689,9 @@ impl Drop for Sandbox { } } - if let Some(reaper) = rt.reaper.take() { - loop { - if unsafe { libc::waitpid(reaper.pid, std::ptr::null_mut(), 0) } >= 0 { break; } - if std::io::Error::last_os_error().raw_os_error() != Some(libc::EINTR) { break; } - } + if let Some(mut reaper) = rt.reaper.take() { + reaper.finish(); + reaper.join_blocking(); } if let Some(h) = rt.notif_handle.take() { h.abort(); } @@ -2919,7 +2924,10 @@ enum ParkedDrain { /// dropping output. The conversion can only fail if the fd is not a readable /// pipe, which cannot happen for the pipes `do_create_stdio` creates; if it /// somehow did, the stream is left uncaptured rather than the run failing. -fn sandbox_spawn_pipe_drain(fd: std::os::fd::OwnedFd) -> Option { +fn sandbox_spawn_pipe_drain( + fd: std::os::fd::OwnedFd, + cut: tokio_util::sync::CancellationToken, +) -> Option { let mut rx = match tokio::net::unix::pipe::Receiver::from_owned_fd(fd) { Ok(rx) => rx, Err(_) => return None, @@ -2927,7 +2935,17 @@ fn sandbox_spawn_pipe_drain(fd: std::os::fd::OwnedFd) -> Option { Some(ParkedDrain::Running(tokio::spawn(async move { use tokio::io::AsyncReadExt; let mut buf = Vec::new(); - let _ = rx.read_to_end(&mut buf).await; + loop { + buf.reserve(8192); + tokio::select! { + biased; + _ = cut.cancelled() => { + while matches!(rx.try_read_buf(&mut buf), Ok(n) if n > 0) {} + break; + } + n = rx.read_buf(&mut buf) => if !matches!(n, Ok(n) if n > 0) { break }, + } + } buf }))) } @@ -3002,7 +3020,7 @@ fn sandbox_wait_status_to_exit(status: i32) -> crate::result::ExitStatus { /// already exited, it does not consume the child's ptrace-stops the way a /// `waitpid`-loop would — so it doesn't race the `policy_fn` fork-tracking /// worker. Falls back to the blocking waiter on any pidfd/`AsyncFd` error. -async fn wait_child_exit_via_pidfd( +pub(crate) async fn wait_child_exit_via_pidfd( pidfd: std::os::unix::io::OwnedFd, pid: libc::pid_t, ) -> crate::result::ExitStatus { diff --git a/crates/sandlock-core/tests/integration/test_process_groups.rs b/crates/sandlock-core/tests/integration/test_process_groups.rs index 68883d493..f1574f627 100644 --- a/crates/sandlock-core/tests/integration/test_process_groups.rs +++ b/crates/sandlock-core/tests/integration/test_process_groups.rs @@ -442,7 +442,7 @@ fn test_creation_under_fd_pressure_does_not_leave_children() { } #[tokio::test] -async fn test_shutdown_reaps_untracked_groups_without_syscall_supervision() { +async fn test_shutdown_releases_untracked_groups_without_syscall_supervision() { let dir = tempfile::tempdir().unwrap(); let ready = dir.path().join("ready"); let script = format!( @@ -462,12 +462,11 @@ while not os.path.exists({ready:?}): sb.spawn(&["python3", "-c", &script]).await.unwrap(); sb.wait().await.unwrap(); let leader: i32 = std::fs::read_to_string(ready).unwrap().parse().unwrap(); - let cleanup = tokio::time::timeout(std::time::Duration::from_secs(5), sb.shutdown()).await; - if cleanup.is_err() { - unsafe { libc::kill(leader, libc::SIGKILL) }; - } - cleanup - .expect("shutdown must also clean untracked descendants") - .unwrap(); + // Without a supervisor nothing records the new group, so shutdown + // releases its leader instead of waiting on it forever. + let shutdown = tokio::time::timeout(std::time::Duration::from_secs(5), sb.shutdown()).await; + let alive = unsafe { libc::kill(leader, libc::SIGKILL) } == 0; + shutdown.expect("shutdown must not wait on untracked descendants").unwrap(); + assert!(alive); assert!(wait_gone(leader)); } diff --git a/crates/sandlock-core/tests/integration/test_protection.rs b/crates/sandlock-core/tests/integration/test_protection.rs index df46be652..fb2a8226a 100644 --- a/crates/sandlock-core/tests/integration/test_protection.rs +++ b/crates/sandlock-core/tests/integration/test_protection.rs @@ -654,15 +654,13 @@ os._exit(7) sb.spawn(&["python3", "-c", &script]).await.unwrap(); assert_eq!(sb.wait().await.unwrap(), sandlock_core::ExitStatus::Code(7)); let child: i32 = std::fs::read_to_string(&ready).unwrap().parse().unwrap(); - let result = tokio::time::timeout(std::time::Duration::from_secs(5), sb.shutdown()) + // Only group signals reach a job in another session, so shutdown + // reports that it cannot end the session instead of hanging. + assert!(tokio::time::timeout(std::time::Duration::from_secs(5), sb.shutdown()) .await .unwrap() - .unwrap(); - assert_eq!(result.code(), Some(7)); - assert_eq!(unsafe { libc::kill(child, 0) }, -1); - assert_eq!( - std::io::Error::last_os_error().raw_os_error(), - Some(libc::ESRCH) - ); + .is_err()); + drop(sb); + assert_eq!(unsafe { libc::kill(child, libc::SIGKILL) }, 0); }); } diff --git a/docs/sandbox-reference.md b/docs/sandbox-reference.md index 6d1a3fb6b..374183e29 100644 --- a/docs/sandbox-reference.md +++ b/docs/sandbox-reference.md @@ -259,7 +259,7 @@ fields on `Sandbox`. | `clean_env` | `clean_env` | `bool` | `False` | When `True`, start with a minimal environment (`PATH`, `HOME`, `USER`, `TERM`, `LANG`) instead of inheriting the parent's. | | `no_coredump` | `no_coredump` | `bool` | `False` | Apply `prctl(PR_SET_DUMPABLE, 0)`. Disables core dumps and restricts `/proc/` access from other processes. Breaks `gdb`, `strace`, and `perf`. | | `no_huge_pages` | `no_huge_pages` | `bool` | `False` | Disable transparent huge pages via `prctl(PR_SET_THP_DISABLE)`. | -| `no_supervisor` | `no_supervisor` | `bool` | `False` | Skip the seccomp user-notification supervisor. The sandbox runs with Landlock + a kernel-only deny filter, without IP allowlisting, resource limits, COW, chroot mediation, `/proc` virtualization, or custom handlers. Required when nesting inside another sandlock (the kernel only allows one `SECCOMP_FILTER_FLAG_NEW_LISTENER` per task). | +| `no_supervisor` | `no_supervisor` | `bool` | `False` | Skip the seccomp user-notification supervisor. The sandbox runs with Landlock + a kernel-only deny filter, without IP allowlisting, resource limits, COW, chroot mediation, `/proc` virtualization, or custom handlers. Required at one level when nesting sandlock, inner or outer (the kernel allows only one `SECCOMP_FILTER_FLAG_NEW_LISTENER` per filter chain). | ## Variable expansion @@ -528,6 +528,13 @@ available for an explicit commit or abort. Releasing the owning session handle also terminates and reaps its jobs. Use explicit shutdown when the caller needs the complete result. +Jobs are terminated through their process groups, which requires Linux +6.9 or later. On older kernels, shutdown fails if any job outlives the +main process, and releasing the handle leaves those jobs running +without supervision. With `no_supervisor`, a job that moves to a new +process group or session is not tracked; shutdown releases it instead +of terminating it, still confined by Landlock and the deny filter. + ## Enumerations ### `BranchAction` From 53854dbc57732e40b869fd0c06e2fb6d15ab2eec Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 14 Oct 2026 11:47:11 -0700 Subject: [PATCH 09/23] pgroup: refuse group changes that cannot be tracked Kill reaches the sandbox only through recorded process groups. When pidfd_open failed for a new leader, or the parent check could not confirm the target, setsid and setpgid were let through unrecorded, so a group could form that no kill or shutdown would ever reach. Fail closed instead: return the pidfd_open error, and ESRCH for a target that is not the caller or a live child of it, matching what the kernel itself would report for a non-child. Signed-off-by: Cong Wang --- crates/sandlock-core/src/pgroup.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/crates/sandlock-core/src/pgroup.rs b/crates/sandlock-core/src/pgroup.rs index 5a010a6c9..dbf8d26a7 100644 --- a/crates/sandlock-core/src/pgroup.rs +++ b/crates/sandlock-core/src/pgroup.rs @@ -159,16 +159,19 @@ fn track_new_group( caller: i32, leader: i32, ) -> NotifAction { - let Ok(pidfd) = pidfd_open(leader as u32, 0) else { - return NotifAction::Continue; + // An unrecorded group is out of reach of kill, so refuse rather than allow it. + let pidfd = match pidfd_open(leader as u32, 0) { + Ok(pidfd) => pidfd, + Err(e) => return NotifAction::Errno(e.raw_os_error().unwrap_or(libc::EAGAIN)), }; // The kernel moves only the caller or one of its children. Anything - // else fails there with ESRCH, and recording it would aim later signals - // at a process outside the sandbox. The caller is parked in this + // else fails there with ESRCH, so answer that here: recording it would + // aim later signals outside the sandbox, and letting a misread through + // could form a group nobody recorded. The caller is parked in this // syscall, and a leader still alive after the read kept its number // throughout, so neither pid was recycled under the check. if leader != caller && (read_ppid(leader) != Some(caller) || has_exited(pidfd.as_raw_fd())) { - return NotifAction::Continue; + return NotifAction::Errno(libc::ESRCH); } if id_valid(notif_fd, notif.id).is_err() { return NotifAction::Continue; From 339290275ab21799bc567f8de69f466e36ec4ef3 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 14 Oct 2026 11:49:12 -0700 Subject: [PATCH 10/23] reaper: outlive the thread that created the session PDEATHSIG fires when the parent thread exits, not the parent process. The FFI forks the reaper on the caller's thread, so a binding that created a sandbox from a short-lived thread lost the session when that thread ended: the reaper was killed, the main process followed, and background jobs were released unsupervised. Watch a pidfd of the owning process instead, opened before checking that the owner is still the parent. When the owner dies the reaper kills the main process, still its unreaped child, and finishes. The main process keeps PDEATHSIG, since the reaper is single-threaded. Signed-off-by: Cong Wang --- crates/sandlock-core/src/reaper.rs | 48 ++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 5 deletions(-) diff --git a/crates/sandlock-core/src/reaper.rs b/crates/sandlock-core/src/reaper.rs index c454a8893..7056d207d 100644 --- a/crates/sandlock-core/src/reaper.rs +++ b/crates/sandlock-core/src/reaper.rs @@ -93,8 +93,11 @@ pub(crate) fn fork() -> io::Result<(i32, Option)> { let mut default_chld: libc::sigaction = std::mem::zeroed(); default_chld.sa_sigaction = libc::SIG_DFL; libc::sigaction(libc::SIGCHLD, &default_chld, &mut previous_chld); + // Watch the owning process rather than set PDEATHSIG, which fires + // when the forking thread exits and would end the session with it. + let owner_fd = libc::syscall(libc::SYS_pidfd_open, owner, 0) as i32; if libc::prctl(libc::PR_SET_CHILD_SUBREAPER, 1, 0, 0, 0) != 0 - || libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGKILL) != 0 + || owner_fd < 0 || libc::getppid() != owner { libc::_exit(127); @@ -127,7 +130,7 @@ pub(crate) fn fork() -> io::Result<(i32, Option)> { } let fd = write.as_raw_fd(); let gate = pinned_r.as_raw_fd(); - let mut keep = [fd, gate, signal_fd]; + let mut keep = [fd, gate, signal_fd, owner_fd]; keep.sort_unstable(); let mut next = 0u32; for kept in keep { @@ -154,7 +157,7 @@ pub(crate) fn fork() -> io::Result<(i32, Option)> { if !acknowledged { libc::kill(child, libc::SIGKILL); } - supervise(child, fd, gate, signal_fd); + supervise(child, fd, gate, signal_fd, owner_fd); } } drop(write); @@ -195,18 +198,21 @@ pub(crate) fn fork() -> io::Result<(i32, Option)> { } } -unsafe fn supervise(main: i32, status_fd: i32, finish_fd: i32, signal_fd: i32) -> ! { +unsafe fn supervise(main: i32, status_fd: i32, finish_fd: i32, signal_fd: i32, owner_fd: i32) -> ! { let mut poll = [ libc::pollfd { fd: finish_fd, events: libc::POLLIN, revents: 0 }, libc::pollfd { fd: signal_fd, events: libc::POLLIN, revents: 0 }, + libc::pollfd { fd: owner_fd, events: libc::POLLIN, revents: 0 }, ]; let mut finishing = false; + let mut main_alive = true; loop { let alive = loop { let mut status = 0; match libc::waitpid(-1, &mut status, libc::WNOHANG | libc::__WALL) { 0 => break true, pid if pid == main => { + main_alive = false; let last = !has_children(); send(status_fd, &[status, last as i32]); } @@ -219,10 +225,17 @@ unsafe fn supervise(main: i32, status_fd: i32, finish_fd: i32, signal_fd: i32) - if !alive || finishing { libc::_exit(0); } - libc::poll(poll.as_mut_ptr(), 2, -1); + libc::poll(poll.as_mut_ptr(), 3, -1); if poll[0].revents != 0 { finishing = true; } + if poll[2].revents != 0 { + // Unreaped, the main pid cannot have been reused. + if main_alive { + libc::kill(main, libc::SIGKILL); + } + finishing = true; + } if poll[1].revents != 0 { let mut info: libc::signalfd_siginfo = std::mem::zeroed(); libc::read(signal_fd, (&mut info as *mut libc::signalfd_siginfo).cast(), std::mem::size_of_val(&info)); @@ -314,6 +327,31 @@ mod tests { reaper.join().await.unwrap(); } + #[tokio::test] + async fn outlives_the_thread_that_created_it() { + let (pid, reaper) = std::thread::spawn(|| { + let (pid, reaper) = fork().unwrap(); + if pid == 0 { + unsafe { + libc::pause(); + libc::_exit(0); + } + } + (pid, reaper) + }) + .join() + .unwrap(); + let mut reaper = reaper.unwrap(); + std::thread::sleep(std::time::Duration::from_millis(50)); + for fd in [&reaper.pidfd, &reaper.main_pidfd] { + let mut ready = libc::pollfd { fd: fd.as_raw_fd(), events: libc::POLLIN, revents: 0 }; + assert_eq!(unsafe { libc::poll(&mut ready, 1, 0) }, 0); + } + unsafe { libc::kill(pid, libc::SIGKILL) }; + reaper.finish(); + reaper.join().await.unwrap(); + } + #[tokio::test] async fn finish_releases_children_still_running() { let mut pipe = [0; 2]; From 3a0f0377be6556b83fce66f0012118224d1bb943 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 14 Oct 2026 11:56:14 -0700 Subject: [PATCH 11/23] ffi: expose a pollable main-exit descriptor Bindings could only wait through the handle, which a blocking wait borrows mutably. To let another thread signal the session meanwhile, they waited in 50ms slices under a lock, adding latency and spinning the runtime for every slice. Add sandlock_handle_main_exit_fd: it prepares the wait (closing untaken stdin and starting the output drains, so a chatty child cannot fill its pipe) and returns a pidfd duplicate that turns readable once the main process exits. A binding blocks on it without the handle and then calls sandlock_handle_wait, which no longer blocks. Signed-off-by: Cong Wang --- crates/sandlock-core/src/sandbox.rs | 44 ++++++++++++++++++-------- crates/sandlock-ffi/include/sandlock.h | 11 +++++++ crates/sandlock-ffi/src/lib.rs | 21 ++++++++++++ 3 files changed, 63 insertions(+), 13 deletions(-) diff --git a/crates/sandlock-core/src/sandbox.rs b/crates/sandlock-core/src/sandbox.rs index c9cb567e9..546b9c746 100644 --- a/crates/sandlock-core/src/sandbox.rs +++ b/crates/sandlock-core/src/sandbox.rs @@ -957,21 +957,21 @@ impl Sandbox { Ok(rt.groups.live().map_err(SandboxRuntimeError::Io)?) } - /// Wait only for the main process, keeping background jobs supervised. - /// - /// Returns the same status on repeated calls. Capture pipes keep draining - /// until `shutdown()` collects them; this does not wait for EOF - /// or settle the COW branch. Untaken piped stdin is closed here. - /// Call `shutdown()` to terminate remaining jobs and collect their output. - pub async fn wait(&mut self) -> Result { + /// A descriptor that turns readable once the main process has exited. + /// Blocking on it needs no `&mut self`, so other threads can signal the + /// session meanwhile; `wait()` then returns without blocking. + pub fn main_exit_fd(&mut self) -> Result { use crate::error::SandboxRuntimeError; - let rt = self.runtime.as_ref().ok_or(SandboxRuntimeError::NotRunning)?; - let pid = rt.child_pid.ok_or(SandboxRuntimeError::NotRunning)?; - match &rt.state { - RuntimeState::MainExited(status) | RuntimeState::Stopped(status) => return Ok(status.clone()), - _ => {} - } + let pidfd = self.runtime.as_ref() + .and_then(|rt| rt.pidfd.as_ref()) + .ok_or(SandboxRuntimeError::NotRunning)? + .try_clone() + .map_err(SandboxRuntimeError::Io)?; + self.prepare_wait(); + Ok(pidfd) + } + fn prepare_wait(&mut self) { // Deliver EOF to a piped stdin the caller never took: otherwise a child // that reads stdin (e.g. `cat`) blocks forever and this wait never // returns. A taken stdin is already None here (the caller owns it). @@ -1004,6 +1004,24 @@ impl Sandbox { self.rt_mut().stderr_drain = sandbox_spawn_pipe_drain(fd, self.rt().drain_cut.clone()); } } + } + + /// Wait only for the main process, keeping background jobs supervised. + /// + /// Returns the same status on repeated calls. Capture pipes keep draining + /// until `shutdown()` collects them; this does not wait for EOF + /// or settle the COW branch. Untaken piped stdin is closed here. + /// Call `shutdown()` to terminate remaining jobs and collect their output. + pub async fn wait(&mut self) -> Result { + use crate::error::SandboxRuntimeError; + let rt = self.runtime.as_ref().ok_or(SandboxRuntimeError::NotRunning)?; + let pid = rt.child_pid.ok_or(SandboxRuntimeError::NotRunning)?; + match &rt.state { + RuntimeState::MainExited(status) | RuntimeState::Stopped(status) => return Ok(status.clone()), + _ => {} + } + + self.prepare_wait(); let exit_status = if let Some(reaper) = &self.rt().reaper { let exit = reaper.main_exit().await.map_err(SandboxRuntimeError::Io)?; diff --git a/crates/sandlock-ffi/include/sandlock.h b/crates/sandlock-ffi/include/sandlock.h index 79b5e820b..fbf210cff 100644 --- a/crates/sandlock-ffi/include/sandlock.h +++ b/crates/sandlock-ffi/include/sandlock.h @@ -925,6 +925,17 @@ sandlock_result_t *sandlock_handle_wait(sandlock_handle_t *h); */ sandlock_result_t *sandlock_handle_wait_timeout(sandlock_handle_t *h, uint64_t timeout_ms); +/** + * Prepare to wait and return a new descriptor that becomes readable once the + * main process exits, or -1 on error. Poll it without holding the handle, so + * other threads can signal the session, then call `sandlock_handle_wait`, + * which no longer blocks. The caller closes the descriptor. + * + * # Safety + * `h` must be a valid handle with no concurrent handle operations. + */ +int sandlock_handle_main_exit_fd(sandlock_handle_t *h); + /** * Terminate remaining jobs with SIGKILL, reap them, and release supervision. * Returns the main status, captured output, and filesystem changes, or null diff --git a/crates/sandlock-ffi/src/lib.rs b/crates/sandlock-ffi/src/lib.rs index cc8937134..4d618fe31 100644 --- a/crates/sandlock-ffi/src/lib.rs +++ b/crates/sandlock-ffi/src/lib.rs @@ -1492,6 +1492,27 @@ pub unsafe extern "C" fn sandlock_handle_wait_timeout( } } +/// Prepare to wait and return a new descriptor that becomes readable once the +/// main process exits, or -1 on error. Poll it without holding the handle, so +/// other threads can signal the session, then call `sandlock_handle_wait`, +/// which no longer blocks. The caller closes the descriptor. +/// +/// # Safety +/// `h` must be a valid handle with no concurrent handle operations. +#[no_mangle] +pub unsafe extern "C" fn sandlock_handle_main_exit_fd(h: *mut sandlock_handle_t) -> c_int { + if h.is_null() { + return -1; + } + let h = &mut *h; + // Starting the output drains spawns tasks on the handle's runtime. + let _runtime = h.runtime.enter(); + match h.sandbox.main_exit_fd() { + Ok(fd) => std::os::fd::IntoRawFd::into_raw_fd(fd), + Err(_) => -1, + } +} + /// Terminate remaining jobs with SIGKILL, reap them, and release supervision. /// Returns the main status, captured output, and filesystem changes, or null /// on error. The handle remains valid until `sandlock_handle_free`. From 9e57a018eef26f40efdfa162df821cd3992ad6eb Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 14 Oct 2026 11:56:14 -0700 Subject: [PATCH 12/23] python: wait on the main-exit descriptor outside the lock Waits polled the native handle in 50ms slices so kill could interleave. Poll the main-exit descriptor without holding the lock instead, then collect the status, which returns at once. Since no native call now holds the handle across a wait, process_groups and ports no longer refuse while another thread waits. Signed-off-by: Cong Wang --- python/src/sandlock/_sdk.py | 3 +- python/src/sandlock/sandbox.py | 60 ++++++++++++++++++---------------- python/tests/test_session.py | 17 ++++++++++ 3 files changed, 50 insertions(+), 30 deletions(-) diff --git a/python/src/sandlock/_sdk.py b/python/src/sandlock/_sdk.py index 576024437..ef0241b8d 100644 --- a/python/src/sandlock/_sdk.py +++ b/python/src/sandlock/_sdk.py @@ -323,7 +323,8 @@ def confine(policy: "PolicyDataclass") -> None: _lib.sandlock_handle_shutdown.argtypes = [_c_handle_p] _lib.sandlock_handle_process_groups.restype = ctypes.c_void_p _lib.sandlock_handle_process_groups.argtypes = [_c_handle_p] -for _name in ("sandlock_handle_pause", "sandlock_handle_resume", "sandlock_handle_kill"): +for _name in ("sandlock_handle_pause", "sandlock_handle_resume", "sandlock_handle_kill", + "sandlock_handle_main_exit_fd"): getattr(_lib, _name).restype = ctypes.c_int getattr(_lib, _name).argtypes = [_c_handle_p] diff --git a/python/src/sandlock/sandbox.py b/python/src/sandlock/sandbox.py index d209380a6..d0e3449f5 100644 --- a/python/src/sandlock/sandbox.py +++ b/python/src/sandlock/sandbox.py @@ -1085,7 +1085,7 @@ def process_groups(self) -> list[int]: """Return a sorted snapshot of tracked live process group IDs.""" self._reject_if_popen() with self._lock: - if self._handle is None or self._waiting: + if self._handle is None: raise RuntimeError("session is unavailable") return _read_process_groups(self._handle) @@ -1345,7 +1345,7 @@ def ports(self) -> dict[int, int]: # this can't race a concurrent shutdown() freeing it. return proc.ports() with self._lock: - if self._handle is None or self._waiting: + if self._handle is None: return {} return _read_port_mappings(self._handle) @@ -1611,10 +1611,9 @@ def pid(self) -> int | None: def ports(self) -> dict[int, int]: """Current virtual→real port mappings while running, else ``{}`` (needs - ``port_remap``). Reads the handle under the lock, and reports ``{}`` - while a lifecycle operation holds it.""" + ``port_remap``). Reads the handle under the lock.""" with self._lock: - if self._handle is None or self._waiting: + if self._handle is None: return {} return _read_port_mappings(self._handle) @@ -1659,7 +1658,7 @@ def wait(self, timeout: float | None = None) -> "ExitStatus": def process_groups(self) -> list[int]: """Return a sorted snapshot of tracked live process group IDs.""" with self._lock: - if self._handle is None or self._waiting: + if self._handle is None: raise RuntimeError("session is unavailable") return _read_process_groups(self._handle) @@ -1739,31 +1738,34 @@ def __del__(self): def _wait_status(handle, timeout, lock=None): import contextlib - import time + import math + import os + import select from ._sdk import _lib, ExitStatus, ExitReason - deadline = time.monotonic() + max(0, timeout) if timeout is not None else None - while True: - remaining = max(0, deadline - time.monotonic()) if deadline is not None else None - interval = min(remaining, 0.05) if remaining is not None and lock is not None else remaining - if interval is None and lock is not None: - interval = 0.05 - timeout_ms = max(1, int(interval * 1000)) if interval is not None else 0 - # Bounded waits let another thread signal through the same handle safely. - with lock if lock is not None else contextlib.nullcontext(): - ptr = _lib.sandlock_handle_wait_timeout(handle, timeout_ms) - if not ptr: - raise RuntimeError("sandlock_handle_wait failed") - try: - reason = ExitReason(_lib.sandlock_result_reason(ptr)) - if reason != ExitReason.TIMEOUT: - return ExitStatus(bool(_lib.sandlock_result_success(ptr)), - _lib.sandlock_result_exit_code(ptr), reason, - _lib.sandlock_result_signal(ptr)) - finally: - _lib.sandlock_result_free(ptr) - if deadline is not None and time.monotonic() >= deadline: + guard = lock if lock is not None else contextlib.nullcontext() + with guard: + fd = _lib.sandlock_handle_main_exit_fd(handle) + if fd < 0: + raise RuntimeError("sandlock_handle_main_exit_fd failed") + # Block outside the lock so other threads can signal the session meanwhile. + try: + poller = select.poll() + poller.register(fd, select.POLLIN) + if not poller.poll(None if timeout is None else math.ceil(max(0, timeout) * 1000)): raise TimeoutError("main process has not exited") - time.sleep(0) + finally: + os.close(fd) + with guard: + ptr = _lib.sandlock_handle_wait(handle) + if not ptr: + raise RuntimeError("sandlock_handle_wait failed") + try: + return ExitStatus(bool(_lib.sandlock_result_success(ptr)), + _lib.sandlock_result_exit_code(ptr), + ExitReason(_lib.sandlock_result_reason(ptr)), + _lib.sandlock_result_signal(ptr)) + finally: + _lib.sandlock_result_free(ptr) def _read_process_groups(handle): diff --git a/python/tests/test_session.py b/python/tests/test_session.py index 84601c6e9..7008e41a0 100644 --- a/python/tests/test_session.py +++ b/python/tests/test_session.py @@ -38,6 +38,23 @@ def test_wait_timeout_retains_main_for_retry(): assert sb.shutdown().exit_code == 9 +def test_handle_stays_usable_while_another_thread_waits(): + import threading + import time + with policy() as sb: + sb.spawn(["sleep", "60"]) + statuses = [] + waiter = threading.Thread(target=lambda: statuses.append(sb.wait())) + waiter.start() + time.sleep(0.1) + assert sb.process_groups() + sb.kill() + waiter.join(timeout=5) + assert not waiter.is_alive() + assert statuses[0].reason == ExitReason.KILLED + sb.shutdown() + + def test_popen_context_cleans_background_jobs_after_wait(): with policy().popen(["sh", "-c", "sleep 60 & echo $!"], stdout=StdioMode.PIPED) as proc: child = int(proc.stdout.readline()) From 3d1980eb9b50f4acdf00bbacdde09da23e1131af Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 14 Oct 2026 11:56:15 -0700 Subject: [PATCH 13/23] go: wait on the main-exit descriptor outside the lock Process.Wait looped over 50ms native waits so Kill and Close could take the handle in between. Block in poll on the main-exit descriptor without the mutex, retrying on EINTR from Go's runtime signals, then collect the status. Ports no longer reports empty during a Wait. Signed-off-by: Cong Wang --- go/sandlock_linux.go | 67 +++++++++++++++++++++++++++----------------- 1 file changed, 41 insertions(+), 26 deletions(-) diff --git a/go/sandlock_linux.go b/go/sandlock_linux.go index 773bb785e..7b20c7d62 100644 --- a/go/sandlock_linux.go +++ b/go/sandlock_linux.go @@ -10,9 +10,22 @@ package sandlock // // The C declarations come from the cbindgen-generated header, so the cgo // prototypes stay in lock-step with crates/sandlock-ffi automatically. +#include +#include #include +#include #include "sandlock.h" +// Go's runtime signals interrupt poll on cgo threads, so retry on EINTR. +static int sandlock_go_await_readable(int fd) { + struct pollfd p = { .fd = fd, .events = POLLIN }; + int r; + do { + r = poll(&p, 1, -1); + } while (r < 0 && errno == EINTR); + return r; +} + extern int32_t goPolicyCallback(sandlock_event_t *event, sandlock_ctx_t *ctx, void *user_data); extern void goPolicyDrop(void *user_data); */ @@ -1043,30 +1056,34 @@ func (p *Process) Wait() (*ExitStatus, error) { runtime.SetFinalizer(p, nil) } }() - for { - p.mu.Lock() - // Bounded waits let signals use the same handle without aliasing Rust borrows. - r := C.sandlock_handle_wait_timeout(p.h, 50) - if r == nil { - p.mu.Unlock() - return nil, fmt.Errorf("sandlock: wait failed") - } - status := ExitStatus{ - ExitCode: int(C.sandlock_result_exit_code(r)), - Reason: ExitReason(C.sandlock_result_reason(r)), - Signal: int(C.sandlock_result_signal(r)), - Success: bool(C.sandlock_result_success(r)), - } - C.sandlock_result_free(r) - if status.Reason != ReasonTimeout { - p.status = &status - p.mu.Unlock() - copy := status - return ©, nil - } - p.mu.Unlock() - runtime.Gosched() + p.mu.Lock() + fd := C.sandlock_handle_main_exit_fd(p.h) + p.mu.Unlock() + if fd < 0 { + return nil, fmt.Errorf("sandlock: wait failed") + } + // Block without the lock so Kill and Close can reach the handle meanwhile. + ready := C.sandlock_go_await_readable(fd) + C.close(fd) + if ready < 0 { + return nil, fmt.Errorf("sandlock: wait failed") } + p.mu.Lock() + defer p.mu.Unlock() + r := C.sandlock_handle_wait(p.h) + if r == nil { + return nil, fmt.Errorf("sandlock: wait failed") + } + status := ExitStatus{ + ExitCode: int(C.sandlock_result_exit_code(r)), + Reason: ExitReason(C.sandlock_result_reason(r)), + Signal: int(C.sandlock_result_signal(r)), + Success: bool(C.sandlock_result_success(r)), + } + C.sandlock_result_free(r) + p.status = &status + copy := status + return ©, nil } // Shutdown sends SIGKILL to remaining jobs, reaps them, and collects output and @@ -1215,9 +1232,7 @@ func (p *Process) Kill() error { return p.signal(syscall.SIGKILL) } func (p *Process) Ports() (map[int]int, error) { p.mu.Lock() defer p.mu.Unlock() - // While a Wait holds the handle, reading port mappings would alias it; - // report empty rather than touch the handle concurrently. - if p.h == nil || p.waiting { + if p.h == nil { return map[int]int{}, nil } c := C.sandlock_handle_port_mappings(p.h) From 4783e8cbd8a3a8de5e024a2b94d3036fa1ecb696 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 14 Oct 2026 11:57:14 -0700 Subject: [PATCH 14/23] sandbox: await the reaper instead of re-signalling on a timer Shutdown and Drop re-sent SIGKILL every 10ms until no tracked group had members left. One sweep is enough: the kernel restarts a fork that races a group kill, and a process killed while parked in a trapped setsid never completes it. With every supervised process in a swept group, the reaper exits on its own once the last one is reaped, and its pidfd reports that. Only without a supervisor, or when the sweep itself failed, can jobs outside the sweep remain; finish the reaper right away then. That mode has no COW branch that must outwait them. Clones still poll, since no reaper reports the end of their descendants. Signed-off-by: Cong Wang --- crates/sandlock-core/src/sandbox.rs | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/crates/sandlock-core/src/sandbox.rs b/crates/sandlock-core/src/sandbox.rs index 546b9c746..5b239d82e 100644 --- a/crates/sandlock-core/src/sandbox.rs +++ b/crates/sandlock-core/src/sandbox.rs @@ -1063,7 +1063,17 @@ impl Sandbox { || rt.reaper.as_ref().is_some_and(|r| r.empty || crate::pgroup::has_exited(r.pidfd.as_raw_fd())); if !empty { self.kill()?; } let exit_status = self.wait().await?; - if !empty { + let no_supervisor = self.no_supervisor; + if let Some(reaper) = &mut self.rt_mut().reaper { + // Supervised, every process was in a swept group, so the reaper + // exits on its own once the last is reaped. Only without a + // supervisor can untracked jobs remain, and there is no COW then + // that must outwait them. + if no_supervisor { reaper.finish(); } + reaper.join().await.map_err(SandboxRuntimeError::Io)?; + self.rt().drain_cut.cancel(); + } else if !empty { + // Nothing reports the end of a clone's descendants. loop { if self.rt().groups.signal(libc::SIGKILL).map_err(SandboxRuntimeError::Io)? == 0 { break; @@ -1071,11 +1081,6 @@ impl Sandbox { tokio::time::sleep(std::time::Duration::from_millis(10)).await; } } - if let Some(reaper) = &mut self.rt_mut().reaper { - reaper.finish(); - reaper.join().await.map_err(SandboxRuntimeError::Io)?; - self.rt().drain_cut.cancel(); - } self.rt_mut().reaper = None; self.rt_mut().state = RuntimeState::Stopped(exit_status.clone()); @@ -2681,15 +2686,10 @@ impl Drop for Sandbox { fn drop(&mut self) { if let Some(ref mut rt) = self.runtime { if let Some(worker) = &rt.policy_fn_worker { worker.cancel(); } - let _ = rt.groups.signal(libc::SIGKILL); + let swept = rt.groups.signal(libc::SIGKILL); if let Some(reaper) = &rt.reaper { use std::os::fd::AsRawFd; unsafe { libc::syscall(libc::SYS_pidfd_send_signal, reaper.main_pidfd.as_raw_fd(), libc::SIGKILL, 0, 0) }; - // The reaper collects the zombies that keep a swept group - // counted, so the branch below settles after the jobs are gone. - while matches!(rt.groups.signal(libc::SIGKILL), Ok(n) if n > 0) { - std::thread::sleep(std::time::Duration::from_millis(10)); - } } if let Some(pid) = rt.child_pid { if matches!(rt.state, RuntimeState::Created | RuntimeState::Running | RuntimeState::Paused) { @@ -2708,7 +2708,8 @@ impl Drop for Sandbox { } if let Some(mut reaper) = rt.reaper.take() { - reaper.finish(); + // As in shutdown(): only jobs no sweep reached can keep it alive. + if self.no_supervisor || swept.is_err() { reaper.finish(); } reaper.join_blocking(); } From f07b7c9ddee33cffbb2c9614d92fe793cf71d5d0 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 14 Oct 2026 12:01:21 -0700 Subject: [PATCH 15/23] control: tolerate a refused peer closing before the request is sent server_closes_on_another_uid_without_answering flaked with EPIPE: the server checks SO_PEERCRED on accept and closes a foreign peer at once, which can land before the client finishes writing its request, and the helper unwrapped the write. Ignore write errors in the helper. A server that answers still has to produce the response the own-uid test checks, so nothing is masked. Signed-off-by: Cong Wang --- crates/sandlock-core/src/control.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/sandlock-core/src/control.rs b/crates/sandlock-core/src/control.rs index 9601a8e75..61ce3d854 100644 --- a/crates/sandlock-core/src/control.rs +++ b/crates/sandlock-core/src/control.rs @@ -825,8 +825,8 @@ mod tests { fn raw_info_request(name: &str) -> Vec { let mut s = UnixStream::connect_addr(&socket_addr(name).unwrap()).unwrap(); let body = br#"{"v":1,"verb":"info","args":{}}"#; - s.write_all(&(body.len() as u32).to_be_bytes()).unwrap(); - s.write_all(body).unwrap(); + // A server refusing the peer may close before the request is written. + let _ = s.write_all(&(body.len() as u32).to_be_bytes()).and_then(|()| s.write_all(body)); s.set_read_timeout(Some(std::time::Duration::from_secs(2))).unwrap(); let mut out = Vec::new(); let _ = s.read_to_end(&mut out); From 61618974441aef3030ccddfca1890968b50d0412 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 14 Oct 2026 12:54:51 -0700 Subject: [PATCH 16/23] python: wait natively on one-shot run handles Sandbox.run creates its handle with sandlock_create_for_run, whose current_thread runtime advances the supervisor only inside a native call. Polling the main-exit descriptor from Python starved it: trapped syscalls were never answered, the child never exited, and the Python suite hung in CI. Wait natively in run, as the Go binding does; nothing else shares that handle. Make sandlock_handle_main_exit_fd refuse current_thread handles so the same mistake fails instead of hanging. Signed-off-by: Cong Wang --- crates/sandlock-ffi/include/sandlock.h | 3 ++- crates/sandlock-ffi/src/lib.rs | 8 +++++++- python/src/sandlock/sandbox.py | 22 ++++++++++++---------- 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/crates/sandlock-ffi/include/sandlock.h b/crates/sandlock-ffi/include/sandlock.h index fbf210cff..4a1442026 100644 --- a/crates/sandlock-ffi/include/sandlock.h +++ b/crates/sandlock-ffi/include/sandlock.h @@ -929,7 +929,8 @@ sandlock_result_t *sandlock_handle_wait_timeout(sandlock_handle_t *h, uint64_t t * Prepare to wait and return a new descriptor that becomes readable once the * main process exits, or -1 on error. Poll it without holding the handle, so * other threads can signal the session, then call `sandlock_handle_wait`, - * which no longer blocks. The caller closes the descriptor. + * which no longer blocks. The caller closes the descriptor. Handles from + * `sandlock_create_for_run` are refused: wait on those natively. * * # Safety * `h` must be a valid handle with no concurrent handle operations. diff --git a/crates/sandlock-ffi/src/lib.rs b/crates/sandlock-ffi/src/lib.rs index 4d618fe31..de91396f2 100644 --- a/crates/sandlock-ffi/src/lib.rs +++ b/crates/sandlock-ffi/src/lib.rs @@ -1495,7 +1495,8 @@ pub unsafe extern "C" fn sandlock_handle_wait_timeout( /// Prepare to wait and return a new descriptor that becomes readable once the /// main process exits, or -1 on error. Poll it without holding the handle, so /// other threads can signal the session, then call `sandlock_handle_wait`, -/// which no longer blocks. The caller closes the descriptor. +/// which no longer blocks. The caller closes the descriptor. Handles from +/// `sandlock_create_for_run` are refused: wait on those natively. /// /// # Safety /// `h` must be a valid handle with no concurrent handle operations. @@ -1505,6 +1506,11 @@ pub unsafe extern "C" fn sandlock_handle_main_exit_fd(h: *mut sandlock_handle_t) return -1; } let h = &mut *h; + // A current_thread runtime runs the supervisor only inside block_on, so + // a caller blocked on the descriptor would starve it and never return. + if h.runtime.handle().runtime_flavor() == tokio::runtime::RuntimeFlavor::CurrentThread { + return -1; + } // Starting the output drains spawns tasks on the handle's runtime. let _runtime = h.runtime.enter(); match h.sandbox.main_exit_fd() { diff --git a/python/src/sandlock/sandbox.py b/python/src/sandlock/sandbox.py index d0e3449f5..ae5eee61a 100644 --- a/python/src/sandlock/sandbox.py +++ b/python/src/sandlock/sandbox.py @@ -10,6 +10,7 @@ import inspect import itertools +import math import os import re import threading @@ -777,11 +778,14 @@ def run(self, cmd: Sequence[str], timeout: float | None = None): handle, self._handle = self._handle, None try: - timed_out = False - try: - _wait_status(handle, timeout) - except TimeoutError: - timed_out = True + # This handle's runtime advances only inside native calls, so + # wait natively; no other thread shares it. + status_p = _lib.sandlock_handle_wait_timeout( + handle, 0 if timeout is None else max(1, math.ceil(timeout * 1000))) + if not status_p: + raise RuntimeError("sandlock_handle_wait failed") + timed_out = ExitReason(_lib.sandlock_result_reason(status_p)) == ExitReason.TIMEOUT + _lib.sandlock_result_free(status_p) result_p = _lib.sandlock_handle_shutdown(handle) finally: self._park_or_free(handle) @@ -1736,14 +1740,12 @@ def __del__(self): pass -def _wait_status(handle, timeout, lock=None): - import contextlib +def _wait_status(handle, timeout, lock): import math import os import select from ._sdk import _lib, ExitStatus, ExitReason - guard = lock if lock is not None else contextlib.nullcontext() - with guard: + with lock: fd = _lib.sandlock_handle_main_exit_fd(handle) if fd < 0: raise RuntimeError("sandlock_handle_main_exit_fd failed") @@ -1755,7 +1757,7 @@ def _wait_status(handle, timeout, lock=None): raise TimeoutError("main process has not exited") finally: os.close(fd) - with guard: + with lock: ptr = _lib.sandlock_handle_wait(handle) if not ptr: raise RuntimeError("sandlock_handle_wait failed") From 46898b7c103c8a0696f4f2659e21ee2eebaf288a Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 14 Oct 2026 14:28:09 -0700 Subject: [PATCH 17/23] ffi: report an expired wait without a timeout exit status A wait that timed out returned a result whose reason was TIMEOUT, the same reason a run reports after killing the process for exceeding its timeout. One reason then meant both "still running" and "killed", and a caller had to know which call produced it. Return 1 from sandlock_handle_wait_timeout when the timeout elapses and hand the status back through an out-pointer, so TIMEOUT only ever describes a process that was killed. Signed-off-by: Cong Wang --- crates/sandlock-ffi/include/sandlock.h | 15 ++++++---- crates/sandlock-ffi/src/lib.rs | 41 ++++++++++++++++---------- crates/sandlock-ffi/tests/popen.rs | 13 ++++---- go/sandlock_linux.go | 7 +++-- python/src/sandlock/_sdk.py | 5 ++-- python/src/sandlock/sandbox.py | 16 ++++++---- 6 files changed, 58 insertions(+), 39 deletions(-) diff --git a/crates/sandlock-ffi/include/sandlock.h b/crates/sandlock-ffi/include/sandlock.h index 4a1442026..015c8c335 100644 --- a/crates/sandlock-ffi/include/sandlock.h +++ b/crates/sandlock-ffi/include/sandlock.h @@ -70,7 +70,7 @@ enum sandlock_exit_reason */ SANDLOCK_EXIT_REASON_KILLED = 2, /** - * A run or wait exceeded its timeout. A wait timeout leaves the session alive. + * A run exceeded its timeout and was killed. */ SANDLOCK_EXIT_REASON_TIMEOUT = 3, }; @@ -916,14 +916,17 @@ sandlock_result_t *sandlock_handle_wait(sandlock_handle_t *h); /** * Wait for the main process with a timeout in milliseconds (0 means unlimited). - * Returns a status-only result, or null on error. A timeout returns - * the timeout reason without killing jobs or releasing supervision; - * callers may wait again or shut down the session. + * Returns 0 and stores a status-only result in `*out` once main exits, 1 if + * the timeout elapsed first, or -1 on error. A timeout leaves the session + * untouched: callers may wait again or shut it down. * * # Safety - * `h` must be a valid handle with no concurrent handle operations. + * `h` must be a valid handle with no concurrent handle operations, and + * `out` must be a valid pointer. */ -sandlock_result_t *sandlock_handle_wait_timeout(sandlock_handle_t *h, uint64_t timeout_ms); +int sandlock_handle_wait_timeout(sandlock_handle_t *h, + uint64_t timeout_ms, + sandlock_result_t **out); /** * Prepare to wait and return a new descriptor that becomes readable once the diff --git a/crates/sandlock-ffi/src/lib.rs b/crates/sandlock-ffi/src/lib.rs index de91396f2..3c9826bbe 100644 --- a/crates/sandlock-ffi/src/lib.rs +++ b/crates/sandlock-ffi/src/lib.rs @@ -1455,40 +1455,49 @@ pub unsafe extern "C" fn sandlock_handle_resume(h: *mut sandlock_handle_t) -> c_ /// `h` must be a valid handle with no concurrent handle operations. #[no_mangle] pub unsafe extern "C" fn sandlock_handle_wait(h: *mut sandlock_handle_t) -> *mut sandlock_result_t { - sandlock_handle_wait_timeout(h, 0) + let mut out = ptr::null_mut(); + sandlock_handle_wait_timeout(h, 0, &mut out); + out } /// Wait for the main process with a timeout in milliseconds (0 means unlimited). -/// Returns a status-only result, or null on error. A timeout returns -/// the timeout reason without killing jobs or releasing supervision; -/// callers may wait again or shut down the session. +/// Returns 0 and stores a status-only result in `*out` once main exits, 1 if +/// the timeout elapsed first, or -1 on error. A timeout leaves the session +/// untouched: callers may wait again or shut it down. /// /// # Safety -/// `h` must be a valid handle with no concurrent handle operations. +/// `h` must be a valid handle with no concurrent handle operations, and +/// `out` must be a valid pointer. #[no_mangle] pub unsafe extern "C" fn sandlock_handle_wait_timeout( h: *mut sandlock_handle_t, timeout_ms: u64, -) -> *mut sandlock_result_t { - if h.is_null() { - return ptr::null_mut(); + out: *mut *mut sandlock_result_t, +) -> c_int { + if h.is_null() || out.is_null() { + return -1; } + *out = ptr::null_mut(); let h = &mut *h; let status = block_on_runtime(&h.runtime, async { if timeout_ms == 0 { - h.sandbox.wait().await + h.sandbox.wait().await.map(Some) } else { match tokio::time::timeout(Duration::from_millis(timeout_ms), h.sandbox.wait()).await { - Ok(result) => result, - Err(_) => Ok(ExitStatus::Timeout), + Ok(result) => result.map(Some), + Err(_) => Ok(None), } } }); match status { - Some(Ok(exit_status)) => Box::into_raw(Box::new(sandlock_result_t { - _private: RunResult { exit_status, stdout: None, stderr: None, changes: Vec::new() }, - })), - _ => ptr::null_mut(), + Some(Ok(Some(exit_status))) => { + *out = Box::into_raw(Box::new(sandlock_result_t { + _private: RunResult { exit_status, stdout: None, stderr: None, changes: Vec::new() }, + })); + 0 + } + Some(Ok(None)) => 1, + _ => -1, } } @@ -1707,7 +1716,7 @@ pub enum sandlock_exit_reason_t { Signaled = 1, /// Killed with no recoverable signal number. Killed = 2, - /// A run or wait exceeded its timeout. A wait timeout leaves the session alive. + /// A run exceeded its timeout and was killed. Timeout = 3, } diff --git a/crates/sandlock-ffi/tests/popen.rs b/crates/sandlock-ffi/tests/popen.rs index 8197739f0..83be4ba1e 100644 --- a/crates/sandlock-ffi/tests/popen.rs +++ b/crates/sandlock-ffi/tests/popen.rs @@ -421,8 +421,8 @@ fn wait_retains_background_session_until_shutdown() { assert!(!h.is_null()); assert_eq!(sandlock_start(h), 0); for _ in 0..2 { - let status = sandlock_handle_wait_timeout(h, 5000); - assert!(!status.is_null()); + let mut status = ptr::null_mut(); + assert_eq!(sandlock_handle_wait_timeout(h, 5000, &mut status), 0); assert_eq!(sandlock_result_exit_code(status), 7); let mut len = 0; sandlock_result_stdout_bytes(status, &mut len); @@ -461,11 +461,10 @@ fn wait_timeout_leaves_main_alive_for_a_later_wait() { let h = sandlock_create(policy, ptr::null(), av.as_ptr(), av.len() as c_uint); assert!(!h.is_null()); assert_eq!(sandlock_start(h), 0); - let status = sandlock_handle_wait_timeout(h, 1); - assert!(!status.is_null()); - assert!(matches!(sandlock_result_reason(status), sandlock_exit_reason_t::Timeout)); - sandlock_result_free(status); - let status = sandlock_handle_wait_timeout(h, 5000); + let mut status = ptr::null_mut(); + assert_eq!(sandlock_handle_wait_timeout(h, 1, &mut status), 1); + assert!(status.is_null()); + assert_eq!(sandlock_handle_wait_timeout(h, 5000, &mut status), 0); assert_eq!(sandlock_result_exit_code(status), 9); sandlock_result_free(status); sandlock_result_free(sandlock_handle_shutdown(h)); diff --git a/go/sandlock_linux.go b/go/sandlock_linux.go index 7b20c7d62..ca1722368 100644 --- a/go/sandlock_linux.go +++ b/go/sandlock_linux.go @@ -744,12 +744,13 @@ func (s *Sandbox) Run(ctx context.Context, cmd ...string) (*Result, error) { C.sandlock_handle_free(h) return nil, fmt.Errorf("sandlock: failed to start sandbox") } - status := C.sandlock_handle_wait_timeout(h, timeoutMs(ctx)) - if status == nil { + var status *C.sandlock_result_t + rc := C.sandlock_handle_wait_timeout(h, timeoutMs(ctx), &status) + if rc < 0 { C.sandlock_handle_free(h) return nil, fmt.Errorf("sandlock: wait failed") } - timedOut := ExitReason(C.sandlock_result_reason(status)) == ReasonTimeout + timedOut := rc == 1 C.sandlock_result_free(status) r := C.sandlock_handle_shutdown(h) C.sandlock_handle_free(h) diff --git a/python/src/sandlock/_sdk.py b/python/src/sandlock/_sdk.py index ef0241b8d..a1dc8d15e 100644 --- a/python/src/sandlock/_sdk.py +++ b/python/src/sandlock/_sdk.py @@ -316,8 +316,9 @@ def confine(policy: "PolicyDataclass") -> None: _lib.sandlock_handle_wait.restype = _c_result_p _lib.sandlock_handle_wait.argtypes = [_c_handle_p] -_lib.sandlock_handle_wait_timeout.restype = _c_result_p -_lib.sandlock_handle_wait_timeout.argtypes = [_c_handle_p, ctypes.c_uint64] +_lib.sandlock_handle_wait_timeout.restype = ctypes.c_int +_lib.sandlock_handle_wait_timeout.argtypes = [ + _c_handle_p, ctypes.c_uint64, ctypes.POINTER(_c_result_p)] _lib.sandlock_handle_shutdown.restype = _c_result_p _lib.sandlock_handle_shutdown.argtypes = [_c_handle_p] diff --git a/python/src/sandlock/sandbox.py b/python/src/sandlock/sandbox.py index ae5eee61a..0b2db28af 100644 --- a/python/src/sandlock/sandbox.py +++ b/python/src/sandlock/sandbox.py @@ -754,7 +754,11 @@ 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, _read_result_changes, Result, ExitReason + import ctypes + from ._sdk import ( + _c_result_p, _lib, _make_argv, _read_result_bytes, _read_result_changes, + Result, ExitReason, + ) self._check_not_running() @@ -780,11 +784,13 @@ def run(self, cmd: Sequence[str], timeout: float | None = None): try: # This handle's runtime advances only inside native calls, so # wait natively; no other thread shares it. - status_p = _lib.sandlock_handle_wait_timeout( - handle, 0 if timeout is None else max(1, math.ceil(timeout * 1000))) - if not status_p: + status_p = _c_result_p() + rc = _lib.sandlock_handle_wait_timeout( + handle, 0 if timeout is None else max(1, math.ceil(timeout * 1000)), + ctypes.byref(status_p)) + if rc < 0: raise RuntimeError("sandlock_handle_wait failed") - timed_out = ExitReason(_lib.sandlock_result_reason(status_p)) == ExitReason.TIMEOUT + timed_out = rc == 1 _lib.sandlock_result_free(status_p) result_p = _lib.sandlock_handle_shutdown(handle) finally: From 0d8f2cf1ef9df39bb68bc0d83680c1475f2a84e0 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Thu, 15 Oct 2026 09:05:18 -0700 Subject: [PATCH 18/23] context: drop the parent-death check the reaper already makes The sandbox child is now forked by the session reaper, which sets PR_SET_PDEATHSIG and checks getppid() itself before returning into do_create. confine_child then captured getppid() right after that fork and compared it again, a check that could never fail, and set the same death signal a second time. Signed-off-by: Cong Wang --- crates/sandlock-core/src/context.rs | 18 ------------------ crates/sandlock-core/src/sandbox.rs | 2 -- 2 files changed, 20 deletions(-) diff --git a/crates/sandlock-core/src/context.rs b/crates/sandlock-core/src/context.rs index 7a2b5a22d..6780cd60b 100644 --- a/crates/sandlock-core/src/context.rs +++ b/crates/sandlock-core/src/context.rs @@ -237,10 +237,6 @@ pub(crate) struct ChildSpawnArgs<'a> { /// Merged into the child's BPF notif list so the kernel actually /// raises USER_NOTIF for them. pub extra_syscalls: &'a [u32], - /// PID of the parent process captured before fork. Used to detect - /// parent death in the child without assuming PID 1 is always init - /// (incorrect in containers where the entrypoint runs as PID 1). - pub parent_pid: libc::pid_t, /// Make the child the terminal's foreground process group before exec. /// Only interactive (fully inherited) stdio wants this; a captured or /// piped run taking the foreground demotes the embedding process to a @@ -291,7 +287,6 @@ pub(crate) fn confine_child(args: ChildSpawnArgs<'_>) -> ! { keep_fds, sandbox_name, extra_syscalls, - parent_pid, foreground, } = args; // Helper: abort child on error. Includes the OS error automatically. @@ -319,19 +314,6 @@ pub(crate) fn confine_child(args: ChildSpawnArgs<'_>) -> ! { } } - // 2. Die if parent exits - if unsafe { libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGKILL) } != 0 { - fail!("prctl(PR_SET_PDEATHSIG)"); - } - - // 3. Check parent didn't die between fork and prctl. - // Compare against the actual parent PID captured before fork rather than - // hardcoding 1, since containers often run the entrypoint as PID 1 and a - // child forked from it legitimately has getppid() == 1. - if unsafe { libc::getppid() } != parent_pid { - fail!("parent died before confinement"); - } - // 4. Optional: disable ASLR if sandbox.no_randomize_memory { const ADDR_NO_RANDOMIZE: libc::c_ulong = 0x0040000; diff --git a/crates/sandlock-core/src/sandbox.rs b/crates/sandlock-core/src/sandbox.rs index 5b239d82e..55400a3ac 100644 --- a/crates/sandlock-core/src/sandbox.rs +++ b/crates/sandlock-core/src/sandbox.rs @@ -2153,7 +2153,6 @@ impl Sandbox { self.rt_mut().reaper = reaper; if pid == 0 { - let parent_pid = unsafe { libc::getppid() }; // ===== CHILD PROCESS ===== // Publish only after the process group exists, before confinement denies IPC. if unsafe { libc::setpgid(0, 0) } != 0 { @@ -2235,7 +2234,6 @@ impl Sandbox { keep_fds: &gather_keep_fds, sandbox_name: Some(sandbox_name.as_str()), extra_syscalls: &extra_syscalls, - parent_pid, foreground, }); } From 9ef6674f9f87e22cb2cee5b51f56a725fa8156bf Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Thu, 15 Oct 2026 09:10:02 -0700 Subject: [PATCH 19/23] python: share one session object between Sandbox and Process Sandbox and Process each carried their own lock, idle condition, waiting flag, cached status and copies of the same wait, shutdown and close logic. Move that into _Session, which both now own. shutdown also both reserved the handle and held the lock across the native call. The FFI forbids concurrent handle calls, so the lock is the part that matters; the reservation only made a concurrent wait fail. Keep the lock, and reserve only around wait, the one call that blocks outside it. run() reuses the shared result decoding instead of a copy of it. Signed-off-by: Cong Wang --- python/src/sandlock/sandbox.py | 336 +++++++++++++------------------- python/tests/test_checkpoint.py | 16 +- python/tests/test_lifecycle.py | 4 +- 3 files changed, 138 insertions(+), 218 deletions(-) diff --git a/python/src/sandlock/sandbox.py b/python/src/sandlock/sandbox.py index 0b2db28af..1e9b3bdcd 100644 --- a/python/src/sandlock/sandbox.py +++ b/python/src/sandlock/sandbox.py @@ -3,7 +3,7 @@ A Sandbox holds both the configuration (policy fields) and the runtime state for executing commands. Configuration fields are set at construction -time; runtime state (``_native``, ``_handle``) is initialized lazily. +time; runtime state (``_native``, ``_session``) is initialized lazily. """ from __future__ import annotations @@ -530,16 +530,12 @@ def __post_init__(self): self.on_error = BranchAction(self.on_error) # 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._session = _Session(None) # the create()/start() lifecycle handle 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 self._restore_skipped = [] # SkippedFd entries from the last restore - self._lock = threading.RLock() - self._idle = threading.Condition(self._lock) - self._waiting = False - self._status = None @staticmethod def _next_name() -> str: @@ -588,15 +584,13 @@ def _popen_process(self): def _live_handle(self): """The handle owning the currently running child, from either source: - the ``create``/``start``/``run``/``spawn`` lifecycle (``self._handle``) or + the ``create``/``start``/``spawn`` lifecycle (``self._session``) or a :meth:`popen` :class:`Process` (which owns its handle). Returns ``None`` when nothing is running. This is the single source of truth for "busy".""" - if self._handle is not None: - return self._handle + if self._session.handle is not None: + return self._session.handle proc = self._popen_process() - if proc is not None and proc._handle is not None: - return proc._handle - return None + return proc._session.handle if proc is not None else None def _check_not_running(self) -> None: """Raise if any child is live, so a second lifecycle call can't leak the @@ -612,7 +606,7 @@ def _reject_if_popen(self) -> None: must not touch a handle the ``Process`` owns — freeing or signalling it behind the ``Process``'s back is exactly the aliasing this split avoids.""" proc = self._popen_process() - if proc is not None and proc._handle is not None: + if proc is not None and proc._session.handle is not None: raise RuntimeError( "this sandbox is running a process started with popen(); manage it " "through the returned Process (proc.wait() / proc.kill()), not the " @@ -632,16 +626,7 @@ def __exit__(self, exc_type, exc_val, exc_tb): proc = self._popen_process() if proc is not None: proc.__exit__(exc_type, exc_val, exc_tb) - with self._lock: - if self._handle is not None: - from ._sdk import _lib - if self._waiting: - _lib.sandlock_handle_kill(self._handle) - while self._waiting: - self._idle.wait() - if self._handle is not None: - _lib.sandlock_handle_free(self._handle) - self._handle = None + self._session.close(self._session.free) # Freeing an undecided branch preserves it, same as KEEP. self._release_pending() return False @@ -660,10 +645,7 @@ def pid(self) -> int | None: # reading it can't race a concurrent shutdown() freeing the handle. return proc.pid from ._sdk import _lib - with self._lock: - if self._handle is None: - return None - return _lib.sandlock_handle_pid(self._handle) or None + return self._session.peek(lambda h: _lib.sandlock_handle_pid(h) or None) @property def is_running(self) -> bool: @@ -755,10 +737,7 @@ def run(self, cmd: Sequence[str], timeout: float | None = None): None means no timeout. """ import ctypes - from ._sdk import ( - _c_result_p, _lib, _make_argv, _read_result_bytes, _read_result_changes, - Result, ExitReason, - ) + from ._sdk import _c_result_p, _lib, _make_argv, Result, ExitReason self._check_not_running() @@ -770,17 +749,15 @@ def run(self, cmd: Sequence[str], timeout: float | None = None): # The one-shot run path immediately drives wait on this same Python # thread, so it can use the FFI current-thread runtime and avoid # eager Tokio worker-thread creation. - self._handle = _lib.sandlock_create_for_run( + handle = _lib.sandlock_create_for_run( native.ptr, _encode(resolved_name), argv, argc, ) - if not self._handle: + if not handle: return Result(success=False, exit_code=-1, error="sandlock_create failed") - if _lib.sandlock_start(self._handle) != 0: - _lib.sandlock_handle_free(self._handle) - self._handle = None + if _lib.sandlock_start(handle) != 0: + _lib.sandlock_handle_free(handle) return Result(success=False, exit_code=-1, error="sandlock_start failed") - handle, self._handle = self._handle, None try: # This handle's runtime advances only inside native calls, so # wait natively; no other thread shares it. @@ -790,7 +767,6 @@ def run(self, cmd: Sequence[str], timeout: float | None = None): ctypes.byref(status_p)) if rc < 0: raise RuntimeError("sandlock_handle_wait failed") - timed_out = rc == 1 _lib.sandlock_result_free(status_p) result_p = _lib.sandlock_handle_shutdown(handle) finally: @@ -798,27 +774,11 @@ def run(self, cmd: Sequence[str], timeout: float | None = None): if not result_p: return Result(success=False, exit_code=-1, error="sandlock_handle_wait failed") - - exit_code = _lib.sandlock_result_exit_code(result_p) - success = _lib.sandlock_result_success(result_p) - reason = ExitReason(_lib.sandlock_result_reason(result_p)) - signal = _lib.sandlock_result_signal(result_p) - if timed_out: - exit_code, success, reason, signal = -1, False, ExitReason.TIMEOUT, -1 - 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( - success=bool(success), - exit_code=exit_code, - reason=reason, - signal=signal, - stdout=stdout, - stderr=stderr, - changes=changes, - ) + result = _take_result(result_p) + if rc == 1: + result.exit_code, result.success, result.reason, result.signal = ( + -1, False, ExitReason.TIMEOUT, -1) + return result def run_with_handlers( self, @@ -1028,12 +988,12 @@ def create(self, cmd: Sequence[str]) -> None: argv, argc = _make_argv(list(cmd)) resolved_name = self._resolve_name() - self._status = None - self._handle = _lib.sandlock_create( + handle = _lib.sandlock_create( native.ptr, _encode(resolved_name), argv, argc, ) - if not self._handle: + if not handle: raise RuntimeError("sandlock_create failed") + self._session = _Session(handle) def start(self) -> None: """Release a previously ``create()``d child to ``execve`` the @@ -1044,14 +1004,11 @@ def start(self) -> None: """ from ._sdk import _lib - with self._lock: - if self._waiting: - raise RuntimeError("lifecycle operation already in progress") - if self._handle is None: - raise RuntimeError("sandbox has not been created") - if _lib.sandlock_start(self._handle) != 0: - _lib.sandlock_handle_free(self._handle) - self._handle = None + session = self._session + with session.lock: + if session.call(_lib.sandlock_start, exclusive=True, + missing="sandbox has not been created") != 0: + session.free() raise RuntimeError("sandlock_start failed") def spawn(self, cmd: Sequence[str]) -> None: @@ -1074,49 +1031,17 @@ def wait(self, timeout: float | None = None) -> "ExitStatus": shutdown() to terminate remaining jobs and collect output and changes. """ self._reject_if_popen() - with self._lock: - if self._handle is None: - raise RuntimeError("sandbox is not running") - if self._waiting: - raise RuntimeError("lifecycle operation already in progress") - if self._status is not None: - return self._status - handle = self._handle - self._waiting = True - try: - self._status = _wait_status(handle, timeout, self._lock) - return self._status - finally: - with self._lock: - self._waiting = False - self._idle.notify_all() + return self._session.wait(timeout) def process_groups(self) -> list[int]: """Return a sorted snapshot of tracked live process group IDs.""" self._reject_if_popen() - with self._lock: - if self._handle is None: - raise RuntimeError("session is unavailable") - return _read_process_groups(self._handle) + return self._session.call(_read_process_groups) def shutdown(self) -> "Result": """SIGKILL remaining jobs, reap them, and collect output and changes.""" - from ._sdk import _lib self._reject_if_popen() - with self._lock: - if self._handle is None or self._waiting: - raise RuntimeError("session is unavailable") - handle = self._handle - self._waiting = True - try: - with self._lock: - return _take_result(_lib.sandlock_handle_shutdown(handle)) - finally: - with self._lock: - self._park_or_free(handle) - self._handle = None - self._waiting = False - self._idle.notify_all() + return self._session.shutdown(self._park_or_free) def popen( self, @@ -1172,7 +1097,7 @@ def popen( fd_out = ctypes.c_int(-1) fd_err = ctypes.c_int(-1) # The handle is owned by the returned Process, not the Sandbox: it is - # never stored in self._handle. self._process (set below) is the busy + # never stored in self._session. self._process (set below) is the busy # marker (`_live_handle`), so a stale Process can never alias a later # child through the sandbox. handle = _lib.sandlock_popen( @@ -1323,11 +1248,7 @@ def reduce(self, cmd: list[str], fork_result: "ForkResult") -> "Result": def _signal(self, name: str) -> None: from ._sdk import _lib self._reject_if_popen() - with self._lock: - if self._handle is None: - raise RuntimeError("sandbox is not running") - if getattr(_lib, name)(self._handle) != 0: - raise RuntimeError("sandbox signal failed") + self._session.signal(getattr(_lib, name)) def pause(self) -> None: """Send SIGSTOP to the tracked live process groups.""" @@ -1354,10 +1275,7 @@ def ports(self) -> dict[int, int]: # The Process owns the handle; its ports() reads it under the lock so # this can't race a concurrent shutdown() freeing it. return proc.ports() - with self._lock: - if self._handle is None: - return {} - return _read_port_mappings(self._handle) + return self._session.peek(_read_port_mappings, {}) def checkpoint( self, @@ -1386,12 +1304,9 @@ def checkpoint( # would fight the streaming Process for it, so reject rather than reach # past it — consistent with wait/pause/resume/kill. self._reject_if_popen() - with self._lock: - if self._waiting: - raise RuntimeError("lifecycle operation already in progress") - if self._handle is None: - raise RuntimeError("sandbox is not running (use start() or run() first)") - ptr = _lib.sandlock_handle_checkpoint(self._handle) + ptr = self._session.call( + _lib.sandlock_handle_checkpoint, exclusive=True, + missing="sandbox is not running (use start() or run() first)") if not ptr: raise RuntimeError("checkpoint capture failed") cp = Checkpoint(ptr) @@ -1439,8 +1354,7 @@ def restore_interactive(self, cp: "Checkpoint") -> None: ) if not handle: raise RuntimeError("checkpoint restore failed") - self._status = None - self._handle = handle + self._session = _Session(handle) # Copy the skipped-fd diagnostics out eagerly: the native entries live # on the handle, which shutdown() frees. skipped = [] @@ -1566,23 +1480,14 @@ class Process: def __init__(self, sandbox: "Sandbox", handle, stdin_fd: int, stdout_fd: int, stderr_fd: int): import os - import threading from ._sdk import _lib self._sandbox = sandbox - # _lock guards _handle (and the _waiting reservation) so kill()/pid on - # one thread can't observe or act on a handle shutdown() is freeing on - # another — the "kill from another thread while wait() blocks" pattern. - self._lock = threading.RLock() - self._idle = threading.Condition(self._lock) - self._waiting = False # The handle and pid are set only AFTER the fds are wrapped below: if an # fdopen raises, this Process never takes ownership, so popen()'s caller # frees the handle exactly once (no double free via __del__). - self._handle = None + self._session = _Session(None) self._pid = -1 - self._result = None - self._status = None self.stdin = self.stdout = self.stderr = None # os.fdopen takes ownership of each fd: closing the stream closes the fd, # so a Process that is closed/exited never leaks the pipe ends. Wrap the @@ -1611,30 +1516,22 @@ def __init__(self, sandbox: "Sandbox", handle, stdin_fd: int, stdout_fd: int, st self.stdin, self.stdout, self.stderr = opened # Take ownership only after every stream has been wrapped successfully. self._pid = _lib.sandlock_handle_pid(handle) or -1 - self._handle = handle + self._session = _Session(handle) @property def pid(self) -> int | None: """The child PID while running, else ``None`` (after :meth:`shutdown`).""" - with self._lock: - return self._pid if self._handle is not None and self._pid > 0 else None + return self._session.peek(lambda _h: self._pid if self._pid > 0 else None) def ports(self) -> dict[int, int]: """Current virtual→real port mappings while running, else ``{}`` (needs - ``port_remap``). Reads the handle under the lock.""" - with self._lock: - if self._handle is None: - return {} - return _read_port_mappings(self._handle) + ``port_remap``).""" + return self._session.peek(_read_port_mappings, {}) def kill(self) -> None: """SIGKILL tracked live groups; usable from another thread during wait.""" from ._sdk import _lib - with self._lock: - if self._handle is None: - return - if _lib.sandlock_handle_kill(self._handle) != 0: - raise RuntimeError("sandbox kill failed") + self._session.signal(_lib.sandlock_handle_kill, missing_ok=True) def wait(self, timeout: float | None = None) -> "ExitStatus": """Wait for the main process while retaining the session. @@ -1643,34 +1540,16 @@ def wait(self, timeout: float | None = None) -> "ExitStatus": A timeout raises TimeoutError without terminating jobs. Repeated waits return the cached main status. """ - with self._lock: - if self._waiting: - raise RuntimeError("lifecycle operation already in progress") - if self._status is not None: - return self._status - if self._handle is None: - raise RuntimeError("process is not running") - handle = self._handle - self._waiting = True - try: - if self.stdin is not None and not self.stdin.closed: - try: - self.stdin.close() - except OSError: - pass - self._status = _wait_status(handle, timeout, self._lock) - return self._status - finally: - with self._lock: - self._waiting = False - self._idle.notify_all() + if self.stdin is not None and not self.stdin.closed: + try: + self.stdin.close() + except OSError: + pass + return self._session.wait(timeout) def process_groups(self) -> list[int]: """Return a sorted snapshot of tracked live process group IDs.""" - with self._lock: - if self._handle is None: - raise RuntimeError("session is unavailable") - return _read_process_groups(self._handle) + return self._session.call(_read_process_groups) def shutdown(self) -> "Result": """Terminate and reap remaining jobs, returning the complete result. @@ -1678,43 +1557,17 @@ def shutdown(self) -> "Result": Caller-owned output streams remain open. A repeated call returns the cached result; a deferred branch remains available on the Sandbox. """ - from ._sdk import _lib, ExitStatus - with self._lock: - if self._waiting: - raise RuntimeError("lifecycle operation already in progress") - if self._result is not None: - return self._result - if self._handle is None: - raise RuntimeError("process is not running") - handle = self._handle - self._waiting = True try: - with self._lock: - self._result = _take_result(_lib.sandlock_handle_shutdown(handle)) - r = self._result - self._status = ExitStatus(r.success, r.exit_code, r.reason, r.signal) - return r + return self._session.shutdown(self._sandbox._park_or_free) finally: - with self._lock: - self._sandbox._park_or_free(handle) - self._handle = None - self._waiting = False - self._idle.notify_all() - if self._sandbox._popen_process() is self: + if self._session.handle is None and self._sandbox._popen_process() is self: self._sandbox._process = None def __enter__(self) -> "Process": return self def __exit__(self, exc_type, exc_val, exc_tb) -> None: - with self._lock: - if self._handle is not None: - if self._waiting: - self.kill() - while self._waiting: - self._idle.wait() - if self._handle is not None: - self.shutdown() + self._session.close(self.shutdown) for stream in (self.stdin, self.stdout, self.stderr): if stream is not None and not stream.closed: try: @@ -1730,7 +1583,8 @@ def __del__(self): # run during interpreter shutdown when imports/globals are gone — so this # is strictly best-effort inside a broad guard. try: - if getattr(self, "_handle", None) is None: + session = getattr(self, "_session", None) + if session is None or session.handle is None: return import warnings @@ -1746,6 +1600,86 @@ def __del__(self): pass +class _Session: + """A native handle shared between threads. The lock serializes handle + calls, which the FFI requires, but wait() blocks outside it so another + thread can still kill the session.""" + + def __init__(self, handle): + self.handle = handle + self.lock = threading.RLock() + self._idle = threading.Condition(self.lock) + self._waiting = False + self.status = None + self.result = None + + def call(self, fn, *args, exclusive=False, missing="sandbox is not running"): + with self.lock: + if exclusive and self._waiting: + raise RuntimeError("lifecycle operation already in progress") + if self.handle is None: + raise RuntimeError(missing) + return fn(self.handle, *args) + + def peek(self, fn, default=None): + with self.lock: + return default if self.handle is None else fn(self.handle) + + def signal(self, fn, missing_ok=False): + with self.lock: + if self.handle is None and missing_ok: + return + if self.call(fn) != 0: + raise RuntimeError("sandbox signal failed") + + def wait(self, timeout): + with self.lock: + if self.status is not None: + return self.status + handle = self.call(lambda h: h, exclusive=True) + self._waiting = True + try: + self.status = _wait_status(handle, timeout, self.lock) + return self.status + finally: + with self.lock: + self._waiting = False + self._idle.notify_all() + + def shutdown(self, release): + """Shut down under the lock, then hand the handle to ``release``.""" + from ._sdk import _lib, ExitStatus + with self.lock: + if self.result is None: + handle = self.call(lambda h: h, exclusive=True) + try: + self.result = _take_result(_lib.sandlock_handle_shutdown(handle)) + finally: + self.handle = None + release(handle) + r = self.result + self.status = ExitStatus(r.success, r.exit_code, r.reason, r.signal) + return self.result + + def close(self, finish): + """Kill an in-flight wait so it returns, then ``finish`` the session.""" + from ._sdk import _lib + with self.lock: + if self.handle is not None and self._waiting: + _lib.sandlock_handle_kill(self.handle) + while self._waiting: + self._idle.wait() + if self.handle is not None: + finish() + + def free(self): + from ._sdk import _lib + with self.lock: + if self.handle is not None: + _lib.sandlock_handle_free(self.handle) + self.handle = None + + def _wait_status(handle, timeout, lock): import math import os diff --git a/python/tests/test_checkpoint.py b/python/tests/test_checkpoint.py index ba6bf3d99..cee2a8c46 100644 --- a/python/tests/test_checkpoint.py +++ b/python/tests/test_checkpoint.py @@ -13,7 +13,6 @@ import pytest from sandlock import Sandbox, Checkpoint, SkippedFd -from sandlock._sdk import _encode, _lib, _make_argv _PYTHON_READABLE = list(dict.fromkeys([ @@ -32,20 +31,9 @@ def _policy(**overrides): def running_sandbox(): """A sandbox with a long-running process for checkpoint tests.""" sb = _policy() - argv, argc = _make_argv(["sleep", "60"]) - native = sb._ensure_native() - sb._handle = _lib.sandlock_create( - native.ptr, - _encode(sb._resolve_name()), - argv, - argc, - ) - assert sb._handle, "create failed" - assert _lib.sandlock_start(sb._handle) == 0, "start failed" + sb.spawn(["sleep", "60"]) yield sb - if sb._handle: - _lib.sandlock_handle_free(sb._handle) - sb._handle = None + sb._session.free() class TestCheckpointCapture: diff --git a/python/tests/test_lifecycle.py b/python/tests/test_lifecycle.py index 713a78309..71c7bf1ee 100644 --- a/python/tests/test_lifecycle.py +++ b/python/tests/test_lifecycle.py @@ -9,7 +9,6 @@ import pytest from sandlock import Sandbox, ExitReason -from sandlock._sdk import _lib _BIN_READABLE = ["/usr", "/lib", "/lib64", "/bin", "/etc", "/proc"] @@ -125,8 +124,7 @@ def test_create_then_discard_reaps_child(self): assert _proc_state(pid) == "S" # Free the handle without start() — Rust Drop should SIGKILL + waitpid. - _lib.sandlock_handle_free(sb._handle) - sb._handle = None + sb._session.free() # After Drop, /proc/ must be gone. If it's still there as 'Z', # the reap is missing. From be74611151962de36b5f1805af192ba4600801cc Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Thu, 15 Oct 2026 09:11:26 -0700 Subject: [PATCH 20/23] python: free an abandoned sandbox session without blocking Sandbox.__del__ ran the whole of __exit__, which can block on an idle condition for a wait in progress and also shuts down a popen Process that has its own finalizer. Neither applies when the sandbox is collected: a running wait() keeps it reachable. Free the session handle and any pending branch directly; the native drop kills what is left. Cover the case the finalizer exists for: a sandbox dropped after wait() must not leave its background jobs running. Signed-off-by: Cong Wang --- python/src/sandlock/sandbox.py | 4 +++- python/tests/test_session.py | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/python/src/sandlock/sandbox.py b/python/src/sandlock/sandbox.py index 1e9b3bdcd..578d06463 100644 --- a/python/src/sandlock/sandbox.py +++ b/python/src/sandlock/sandbox.py @@ -614,8 +614,10 @@ def _reject_if_popen(self) -> None: ) def __del__(self): + # A running wait() keeps self reachable, so there is none to interrupt. try: - self.__exit__(None, None, None) + self._session.free() + self._release_pending() except Exception: pass diff --git a/python/tests/test_session.py b/python/tests/test_session.py index 7008e41a0..1e604b4c1 100644 --- a/python/tests/test_session.py +++ b/python/tests/test_session.py @@ -65,6 +65,20 @@ def test_popen_context_cleans_background_jobs_after_wait(): os.kill(child, 0) +def test_abandoned_sandbox_ends_background_jobs(tmp_path): + import gc + pidfile = tmp_path / "job" + sb = policy(fs_writable=[str(tmp_path)]) + sb.spawn(["sh", "-c", f"sleep 60 & echo $! > {pidfile}"]) + assert sb.wait(timeout=5).success + child = int(pidfile.read_text()) + os.kill(child, 0) + del sb + gc.collect() + with pytest.raises(ProcessLookupError): + os.kill(child, 0) + + def test_shutdown_defers_cow_until_jobs_finish(tmp_path): with policy(fs_writable=[str(tmp_path)], workdir=str(tmp_path), on_exit=BranchAction.DEFER) as sb: sb.spawn(["sh", "-c", f"echo data > {tmp_path}/out"]) From 3d18f3ff4c8d352b501801a6d88e13341c794954 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Thu, 15 Oct 2026 13:51:38 -0700 Subject: [PATCH 21/23] pgroup: signal groups by number on kernels without pidfd group signals Kernels before 6.9 reject PIDFD_SIGNAL_PROCESS_GROUP with EINVAL, so every group sweep failed there. shutdown() could then end no session that had a job left, and owns() refused job-control moves into the sandbox's own groups. Fall back to kill(-pgid) while the group's leader has not exited: its pid is still held, so no other group can have the number. Once the leader is gone the number may be reused, but a probe that finds no group at all is still conclusive, because an empty group can never regain members. Only a group whose leader is gone and that still has members stays out of reach, and shutdown reports it. Signed-off-by: Cong Wang --- crates/sandlock-core/src/pgroup.rs | 22 +++++++-- .../tests/integration/test_protection.rs | 46 +++++++++++++------ 2 files changed, 52 insertions(+), 16 deletions(-) diff --git a/crates/sandlock-core/src/pgroup.rs b/crates/sandlock-core/src/pgroup.rs index dbf8d26a7..721b2c987 100644 --- a/crates/sandlock-core/src/pgroup.rs +++ b/crates/sandlock-core/src/pgroup.rs @@ -47,7 +47,7 @@ impl ProcessGroups { .map(|leaders| { leaders .get(&pgid) - .is_some_and(|leader| pidfd_signal_group(leader, 0).is_ok()) + .is_some_and(|leader| signal_group(pgid, leader, 0).is_ok()) }) .unwrap_or(false) } @@ -57,7 +57,7 @@ impl ProcessGroups { .map_err(|_| io::Error::other("process group table poisoned"))?; let mut groups = Vec::new(); for (&pgid, leader) in leaders.iter() { - match pidfd_signal_group(leader, 0) { + match signal_group(pgid, leader, 0) { Ok(()) => groups.push(pgid), Err(e) if e.raw_os_error() == Some(libc::ESRCH) => {} Err(e) => return Err(e), @@ -77,7 +77,7 @@ impl ProcessGroups { .map_err(|_| io::Error::other("process group table poisoned"))?; let mut reached = 0; let mut failure = None; - leaders.retain(|_, leader| match pidfd_signal_group(leader, sig) { + leaders.retain(|&pgid, leader| match signal_group(pgid, leader, sig) { Ok(()) => { reached += 1; true @@ -103,6 +103,22 @@ impl Default for ProcessGroups { } } +/// Kernels before 6.9 refuse group signals by pidfd. A leader that has not +/// exited still holds its pid, so no other group can have taken the number. +/// Once it has, the number is reusable: only a probe finding no group at all +/// is conclusive, since an empty group can never regain members. +fn signal_group(pgid: i32, leader: &OwnedFd, sig: i32) -> io::Result<()> { + let refused = match pidfd_signal_group(leader, sig) { + Err(e) if e.raw_os_error() == Some(libc::EINVAL) => e, + result => return result, + }; + let reachable = !has_exited(leader.as_raw_fd()); + if unsafe { libc::kill(-pgid, if reachable { sig } else { 0 }) } != 0 { + return Err(io::Error::last_os_error()); + } + if reachable { Ok(()) } else { Err(refused) } +} + pub(crate) fn has_exited(pidfd: RawFd) -> bool { let mut pfd = libc::pollfd { fd: pidfd, diff --git a/crates/sandlock-core/tests/integration/test_protection.rs b/crates/sandlock-core/tests/integration/test_protection.rs index fb2a8226a..da0c48122 100644 --- a/crates/sandlock-core/tests/integration/test_protection.rs +++ b/crates/sandlock-core/tests/integration/test_protection.rs @@ -614,7 +614,6 @@ fn shutdown_without_pidfd_group_signals() { ); } let dir = tempfile::tempdir().unwrap(); - let ready = dir.path().join("ready"); let mut builder = sandlock_core::Sandbox::builder(); for protection in Protection::all() { builder = builder.allow_degraded(protection); @@ -638,29 +637,50 @@ fn shutdown_without_pidfd_group_signals() { assert!(result.success()); assert_eq!(result.stdout.as_deref(), Some(b"ok\n".as_slice())); assert!(sb.shutdown().await.unwrap().success()); - let mut sb = policy; - let script = format!( - r#" + let spawn = |name: &str, job: &str| { + let ready = dir.path().join(name); + let tmp = ready.with_extension("tmp"); + let script = format!( + r#" import os, signal, time +def publish(): + open({tmp:?}, "w").write(str(os.getpid())) + os.rename({tmp:?}, {ready:?}) + signal.pause() if os.fork() == 0: os.setsid() - open({ready:?}, "w").write(str(os.getpid())) - signal.pause() + {job} while not os.path.exists({ready:?}): time.sleep(0.01) os._exit(7) "# - ); - sb.spawn(&["python3", "-c", &script]).await.unwrap(); - assert_eq!(sb.wait().await.unwrap(), sandlock_core::ExitStatus::Code(7)); - let child: i32 = std::fs::read_to_string(&ready).unwrap().parse().unwrap(); - // Only group signals reach a job in another session, so shutdown - // reports that it cannot end the session instead of hanging. + ); + let mut sb = policy.clone(); + async move { + sb.spawn(&["python3", "-c", &script]).await.unwrap(); + assert_eq!(sb.wait().await.unwrap(), sandlock_core::ExitStatus::Code(7)); + let job: i32 = std::fs::read_to_string(&ready).unwrap().parse().unwrap(); + (sb, job) + } + }; + + // A job whose group leader is still alive is reached by number. + let (mut sb, leader) = spawn("leader", "publish()").await; + let result = tokio::time::timeout(std::time::Duration::from_secs(5), sb.shutdown()) + .await + .unwrap(); + assert_eq!(result.unwrap().exit_status, sandlock_core::ExitStatus::Code(7)); + assert_eq!(unsafe { libc::kill(leader, 0) }, -1); + + // Once the leader is gone its number may be reused, so shutdown + // reports the job as unreachable instead of hanging. + let (mut sb, orphan) = + spawn("orphan", "publish() if os.fork() == 0 else os._exit(0)").await; assert!(tokio::time::timeout(std::time::Duration::from_secs(5), sb.shutdown()) .await .unwrap() .is_err()); drop(sb); - assert_eq!(unsafe { libc::kill(child, libc::SIGKILL) }, 0); + assert_eq!(unsafe { libc::kill(orphan, libc::SIGKILL) }, 0); }); } From b28e017fe5c84a249f06250f70451e6ca6e471e5 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Thu, 15 Oct 2026 14:02:32 -0700 Subject: [PATCH 22/23] cli: kill sandboxes only through the supervisor's group table sandlock kill sent the control request, then also called killpg(child_pid) and SIGKILLed the supervisor. The child pid was recorded at startup. Now that a session outlives its main process, that pid can be reaped and reused long before the name is released, so the killpg could hit an unrelated group. Killing the supervisor unconditionally also took down an embedding host on success, and on a kernel where some groups were out of reach it released the remaining jobs while the CLI still reported success. Kill only through the tracked groups, which the control loop now holds with or without a seccomp supervisor. Report a refusal instead of claiming success. Only a supervisor that accepts the connection but never answers is killed, through a pidfd opened while SO_PEERCRED vouches for it, and the session reaper then kills the main process. Signed-off-by: Cong Wang --- crates/sandlock-cli/src/main.rs | 29 ++---------- crates/sandlock-cli/tests/cli_test.rs | 55 ++++++++++++++++++++++ crates/sandlock-core/src/control.rs | 61 ++++++++++++++++++------- crates/sandlock-core/src/sandbox.rs | 2 + crates/sandlock-core/src/sys/syscall.rs | 8 ++++ docs/cli.md | 6 +-- 6 files changed, 117 insertions(+), 44 deletions(-) diff --git a/crates/sandlock-cli/src/main.rs b/crates/sandlock-cli/src/main.rs index 08b4c0833..6ef4aa0e4 100644 --- a/crates/sandlock-cli/src/main.rs +++ b/crates/sandlock-cli/src/main.rs @@ -358,30 +358,11 @@ async fn main() -> Result<()> { eprintln!("sandlock: {e}"); std::process::exit(1); } - // Both pids come from the kernel, so this works on a supervisor - // that is stopped or wedged. - let pids = match sandlock_core::control::sandbox_pids(&name) { - Ok(p) => p, - Err(e) => { - eprintln!("sandlock: {}", e); - std::process::exit(1); - } - }; - // Groups the sandbox created are known only to the supervisor, - // and a wedged one cannot answer, so the child's own group is - // signalled here regardless. The supervisor may sit in a - // different group, so signal it directly too. - let _ = sandlock_core::control::send_control_request( - &name, - "kill", - serde_json::Value::Object(Default::default()), - ); - unsafe { libc::killpg(pids.child, libc::SIGKILL) }; - unsafe { libc::kill(pids.supervisor, libc::SIGKILL) }; - println!( - "Killed sandbox '{}' (child PID {}, supervisor PID {})", - name, pids.child, pids.supervisor - ); + if let Err(e) = sandlock_core::control::kill_sandbox(&name) { + eprintln!("sandlock: kill '{}': {}", name, e); + std::process::exit(1); + } + println!("Killed sandbox '{}'", name); } Command::Check => { diff --git a/crates/sandlock-cli/tests/cli_test.rs b/crates/sandlock-cli/tests/cli_test.rs index 06411b0a0..24ce62959 100644 --- a/crates/sandlock-cli/tests/cli_test.rs +++ b/crates/sandlock-cli/tests/cli_test.rs @@ -671,6 +671,61 @@ fn test_kill_stops_sandbox() { } } +#[test] +fn test_kill_stops_sandbox_without_supervisor() { + let name = format!("test-kill-nosup-{}", std::process::id()); + let mut args = sandlock_sleep_args(&name); + args.insert(1, "--no-supervisor".into()); + let mut child = std::process::Command::new(env!("CARGO_BIN_EXE_sandlock")) + .args(&args) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::piped()) + .spawn() + .expect("spawn sandlock"); + if let Err(e) = wait_for_sandbox(&mut child, &name) { + panic!("{}", e); + } + let out = sandlock_bin().args(["kill", &name]).output().expect("sandlock kill"); + assert!( + out.status.success(), + "kill should succeed: stderr={}", + String::from_utf8_lossy(&out.stderr) + ); + assert!(!child.wait().expect("wait for killed child").success()); +} + +#[test] +fn test_kill_wedged_supervisor_kills_main() { + use std::os::unix::process::ExitStatusExt; + let name = format!("test-kill-wedged-{}", std::process::id()); + let mut child = spawn_sandbox(&name); + if let Err(e) = wait_for_sandbox(&mut child, &name) { + panic!("{}", e); + } + let ps = sandlock_bin().args(["ps"]).output().expect("sandlock ps"); + let main: i32 = String::from_utf8_lossy(&ps.stdout) + .lines() + .find_map(|line| { + let mut fields = line.split_whitespace(); + (fields.next() == Some(name.as_str())).then(|| fields.next()?.parse().ok())? + }) + .expect("main pid in ps output"); + + unsafe { libc::kill(child.id() as i32, libc::SIGSTOP) }; + let out = sandlock_bin().args(["kill", &name]).output().expect("sandlock kill"); + assert!( + out.status.success(), + "kill should succeed: stderr={}", + String::from_utf8_lossy(&out.stderr) + ); + assert_eq!(child.wait().expect("wait for supervisor").signal(), Some(libc::SIGKILL)); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while unsafe { libc::kill(main, 0) } == 0 { + assert!(std::time::Instant::now() < deadline, "main {main} outlived its supervisor"); + std::thread::sleep(std::time::Duration::from_millis(20)); + } +} + #[test] fn test_kill_nonexistent_sandbox() { let out = sandlock_bin() diff --git a/crates/sandlock-core/src/control.rs b/crates/sandlock-core/src/control.rs index 61ce3d854..4ca5e0a96 100644 --- a/crates/sandlock-core/src/control.rs +++ b/crates/sandlock-core/src/control.rs @@ -177,6 +177,7 @@ pub struct SandboxInfo { pub(crate) fn spawn_control_loop( sockets: ControlSocket, ctx: Option>, + groups: Arc, sandbox: Sandbox, info: SandboxInfo, ) -> tokio::task::JoinHandle<()> { @@ -185,7 +186,7 @@ pub(crate) fn spawn_control_loop( let sandbox = Arc::new(tokio::sync::Mutex::new(sandbox)); tokio::spawn(async move { let ControlSocket { control, claim: _claim } = sockets; - control_loop(control, ctx, sandbox, info, unsafe { libc::getuid() }).await; + control_loop(control, ctx, groups, sandbox, info, unsafe { libc::getuid() }).await; }) } @@ -271,6 +272,7 @@ impl tokio::io::AsyncWrite for ControlStream { async fn control_loop( listener: ControlFd, ctx: Option>, + groups: Arc, sandbox: Arc>, info: SandboxInfo, my_uid: u32, @@ -289,7 +291,7 @@ async fn control_loop( let Ok(stream) = AsyncFd::new(stream).map(ControlStream) else { continue }; let _ = tokio::time::timeout( std::time::Duration::from_secs(5), - serve_one(stream, ctx.as_ref(), &sandbox, &info), + serve_one(stream, ctx.as_ref(), &groups, &sandbox, &info), ) .await; } @@ -321,6 +323,7 @@ pub struct ControlResponse { async fn serve_one( stream: ControlStream, ctx: Option<&Arc>, + groups: &crate::pgroup::ProcessGroups, sandbox: &Arc>, info: &SandboxInfo, ) { @@ -370,7 +373,7 @@ async fn serve_one( "info" => handle_info(&mut stream, info).await, "config" => handle_config(&mut stream, ctx, sandbox).await, "ports" => handle_ports(&mut stream, ctx).await, - "kill" => handle_kill(&mut stream, ctx).await, + "kill" => handle_kill(&mut stream, ctx, groups).await, _ => { let resp = ControlResponse { v: 1, @@ -437,29 +440,27 @@ async fn handle_config( /// Only the supervisor knows the groups a process has moved into, so /// `sandlock kill` asks here before it signals the child's own group. -async fn handle_kill(stream: &mut ControlStream, ctx: Option<&Arc>) { +async fn handle_kill( + stream: &mut ControlStream, + ctx: Option<&Arc>, + groups: &crate::pgroup::ProcessGroups, +) { if let Some(ctx) = ctx { ctx.policy_fn.lock().await.cancellation.cancel(); } - let resp = match ctx.map(|ctx| ctx.groups.signal(libc::SIGKILL)) { - Some(Ok(groups)) => ControlResponse { + let resp = match groups.signal(libc::SIGKILL) { + Ok(groups) => ControlResponse { v: 1, ok: true, data: Some(serde_json::json!({ "groups": groups })), err: None, }, - Some(Err(e)) => ControlResponse { + Err(e) => ControlResponse { v: 1, ok: false, data: None, err: Some(format!("signal process groups: {}", e)), }, - None => ControlResponse { - v: 1, - ok: false, - data: None, - err: Some("no supervisor".into()), - }, }; let _ = write_response(stream, &resp).await; } @@ -616,7 +617,7 @@ fn connect_entry(name: &str, entry: &ownership::Entry, my_uid: u32) -> Result<(U Ok((stream, cred)) } -/// The two PIDs `kill` needs, stamped by the kernel at semaphore publication. +/// The sandbox's PIDs, stamped by the kernel at semaphore publication. #[derive(Debug, Clone, Copy)] pub struct SandboxPids { /// The child, which leads its own process group. @@ -654,9 +655,34 @@ fn send_control_request_as( args: serde_json::Value, my_uid: u32, ) -> Result { - use std::io::{Read, Write}; + let (stream, _) = connect_control(name, my_uid)?; + exchange(name, stream, verb, args) +} - let (mut stream, _) = connect_control(name, my_uid)?; +/// Kill every process group the supervisor tracks. A supervisor that +/// accepts the connection but never answers is stopped or wedged; it is +/// killed instead, and the session reaper then kills the main process. +pub fn kill_sandbox(name: &str) -> Result<(), String> { + let (stream, cred) = connect_control(name, unsafe { libc::getuid() })?; + // Opened while connected, so it names the peer SO_PEERCRED vouched for. + let supervisor = crate::sys::syscall::pidfd_open(cred.pid as u32, 0) + .map_err(|e| format!("pidfd_open supervisor: {e}"))?; + let args = serde_json::Value::Object(Default::default()); + match exchange(name, stream, "kill", args) { + Ok(resp) if resp.ok => Ok(()), + Ok(resp) => Err(resp.err.unwrap_or_else(|| "kill failed".into())), + Err(_) => crate::sys::syscall::pidfd_send_signal(&supervisor, libc::SIGKILL) + .map_err(|e| format!("kill unresponsive supervisor: {e}")), + } +} + +fn exchange( + name: &str, + mut stream: UnixStream, + verb: &str, + args: serde_json::Value, +) -> Result { + use std::io::{Read, Write}; // Set a 2-second timeout on reads so a wedged supervisor does not // block the CLI forever. @@ -816,7 +842,8 @@ mod tests { let sandbox = Arc::new(tokio::sync::Mutex::new(test_sandbox())); tokio::spawn(async move { let _claim = sockets.claim; - control_loop(sockets.control, None, sandbox, info(), expected_uid).await; + let groups = Arc::new(crate::pgroup::ProcessGroups::new()); + control_loop(sockets.control, None, groups, sandbox, info(), expected_uid).await; }) } diff --git a/crates/sandlock-core/src/sandbox.rs b/crates/sandlock-core/src/sandbox.rs index 55400a3ac..8f81b6841 100644 --- a/crates/sandlock-core/src/sandbox.rs +++ b/crates/sandlock-core/src/sandbox.rs @@ -2513,6 +2513,7 @@ impl Sandbox { self.rt_mut().control_handle = Some(crate::control::spawn_control_loop( sockets, Some(control_ctx), + Arc::clone(&self.rt().groups), sandbox_snapshot, control_info.clone(), )); @@ -2538,6 +2539,7 @@ impl Sandbox { self.rt_mut().control_handle = Some(crate::control::spawn_control_loop( sockets, None, + Arc::clone(&self.rt().groups), self.clone(), control_info, )); diff --git a/crates/sandlock-core/src/sys/syscall.rs b/crates/sandlock-core/src/sys/syscall.rs index a043e5b8b..95bba1599 100644 --- a/crates/sandlock-core/src/sys/syscall.rs +++ b/crates/sandlock-core/src/sys/syscall.rs @@ -194,6 +194,14 @@ pub fn pidfd_getfd(pidfd: &OwnedFd, targetfd: i32, flags: u32) -> io::Result io::Result<()> { + use std::os::unix::io::AsRawFd; + unsafe { + syscall4(crate::arch::SYS_PIDFD_SEND_SIGNAL, pidfd.as_raw_fd() as u64, sig as u64, 0, 0)? + }; + Ok(()) +} + /// Signal the process group led by the process behind `pidfd`. The group /// is named by the pidfd rather than by number, so a recycled pgid cannot /// redirect the signal, and it stays reachable after its leader is reaped. diff --git a/docs/cli.md b/docs/cli.md index 02532519a..5a7ff53c3 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -226,8 +226,8 @@ sandlock run -p build -- make test # trailing command overrides [program] `sandlock ps` lists every running sandbox with its uptime, port mappings, and command. `sandlock inspect NAME` prints the effective policy as JSON -(`--toml` prints it in profile syntax). `sandlock kill NAME` stops a -sandbox and its supervisor. +(`--toml` prints it in profile syntax). `sandlock kill NAME` asks the +supervisor to kill every process group the sandbox created. ``` $ sandlock ps @@ -243,7 +243,7 @@ http_inject_ca = [] ... $ sandlock kill web.local -Killed sandbox 'web.local' (child PID 12346, supervisor PID 12340) +Killed sandbox 'web.local' ``` Ports show as `virtual→real` when `--port-remap` is active; see From cba0f7f413211cdc96306ae6e2b0da8cd7f5c3e3 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Thu, 15 Oct 2026 14:21:16 -0700 Subject: [PATCH 23/23] ps: stop reporting the pid of an exited main process ps showed the main pid from the registry and read its uptime and command line from /proc. Background jobs now keep a session alive after main exits, and its pid is then free for reuse, so ps could describe an unrelated process. The info verb now reports whether main has exited, from the main pidfd the control loop holds. ps reads /proc for the pid only while the supervisor vouches that main is alive, and otherwise shows the session as running with "(main exited)" in place of a pid. Signed-off-by: Cong Wang --- crates/sandlock-cli/src/main.rs | 35 ++++++++++------ crates/sandlock-core/src/control.rs | 41 +++++++++++++------ crates/sandlock-core/src/sandbox.rs | 14 +++++-- .../tests/integration/test_control.rs | 30 ++++++++++++++ 4 files changed, 92 insertions(+), 28 deletions(-) diff --git a/crates/sandlock-cli/src/main.rs b/crates/sandlock-cli/src/main.rs index 6ef4aa0e4..5c2e89569 100644 --- a/crates/sandlock-cli/src/main.rs +++ b/crates/sandlock-cli/src/main.rs @@ -285,23 +285,32 @@ async fn main() -> Result<()> { } else { println!("NAME PID UPTIME STATUS PORTS CMD"); for name in &names { - // The pids need no answer from the supervisor; the rest - // does, and a supervisor that does not answer is still - // alive because its socket is. - let (status, ports) = match sandlock_core::control::sandbox_info(name) { - Ok(info) => ( + // The registry's main pid outlives main itself, so it is + // looked up only while the supervisor vouches for it. One + // that cannot answer is still alive because its socket is. + let pid = sandlock_core::control::sandbox_pids(name).map(|p| p.child); + let (status, ports, pid, uptime, cmd) = match (sandlock_core::control::sandbox_info(name), pid) { + (Ok(info), _) if info.main_exited => ( info.mode.unwrap_or_else(|| "running".to_string()), query_ports(name), + "-".to_string(), + "-".to_string(), + "(main exited)".to_string(), ), - Err(_) => ("unresponsive".to_string(), "?".to_string()), - }; - let (pid, uptime, cmd) = match sandlock_core::control::sandbox_pids(name) { - Ok(pids) => ( - pids.child.to_string(), - proc_uptime(pids.child).unwrap_or_else(|| "?".to_string()), - proc_cmdline(pids.child).unwrap_or_else(|| "?".to_string()), + (Ok(info), Ok(pid)) => ( + info.mode.unwrap_or_else(|| "running".to_string()), + query_ports(name), + pid.to_string(), + proc_uptime(pid).unwrap_or_else(|| "?".to_string()), + proc_cmdline(pid).unwrap_or_else(|| "?".to_string()), + ), + (info, pid) => ( + if info.is_ok() { "running" } else { "unresponsive" }.to_string(), + "?".to_string(), + pid.map_or_else(|_| "?".to_string(), |p| p.to_string()), + "?".to_string(), + "?".to_string(), ), - Err(_) => ("?".to_string(), "?".to_string(), "?".to_string()), }; println!( "{:<32} {:>8} {:>12} {:<10} {:<24} {}", diff --git a/crates/sandlock-core/src/control.rs b/crates/sandlock-core/src/control.rs index 4ca5e0a96..23b4ba51a 100644 --- a/crates/sandlock-core/src/control.rs +++ b/crates/sandlock-core/src/control.rs @@ -169,6 +169,16 @@ pub(crate) fn bind_control_socket(name: &str) -> std::io::Result #[derive(serde::Serialize, serde::Deserialize, Clone, Debug)] pub struct SandboxInfo { pub mode: Option, + /// Background jobs can keep a session alive after its main process + /// exits, and the main pid in the registry is then free for reuse. + #[serde(default)] + pub main_exited: bool, +} + +/// The live session the control loop answers for. +pub(crate) struct Session { + pub groups: Arc, + pub main: OwnedFd, } /// Spawn the control-loop task. `ctx` is `None` for sandboxes without a @@ -177,7 +187,7 @@ pub struct SandboxInfo { pub(crate) fn spawn_control_loop( sockets: ControlSocket, ctx: Option>, - groups: Arc, + session: Session, sandbox: Sandbox, info: SandboxInfo, ) -> tokio::task::JoinHandle<()> { @@ -186,7 +196,7 @@ pub(crate) fn spawn_control_loop( let sandbox = Arc::new(tokio::sync::Mutex::new(sandbox)); tokio::spawn(async move { let ControlSocket { control, claim: _claim } = sockets; - control_loop(control, ctx, groups, sandbox, info, unsafe { libc::getuid() }).await; + control_loop(control, ctx, session, sandbox, info, unsafe { libc::getuid() }).await; }) } @@ -272,7 +282,7 @@ impl tokio::io::AsyncWrite for ControlStream { async fn control_loop( listener: ControlFd, ctx: Option>, - groups: Arc, + session: Session, sandbox: Arc>, info: SandboxInfo, my_uid: u32, @@ -291,7 +301,7 @@ async fn control_loop( let Ok(stream) = AsyncFd::new(stream).map(ControlStream) else { continue }; let _ = tokio::time::timeout( std::time::Duration::from_secs(5), - serve_one(stream, ctx.as_ref(), &groups, &sandbox, &info), + serve_one(stream, ctx.as_ref(), &session, &sandbox, &info), ) .await; } @@ -323,7 +333,7 @@ pub struct ControlResponse { async fn serve_one( stream: ControlStream, ctx: Option<&Arc>, - groups: &crate::pgroup::ProcessGroups, + session: &Session, sandbox: &Arc>, info: &SandboxInfo, ) { @@ -370,10 +380,10 @@ async fn serve_one( } match req.verb.as_str() { - "info" => handle_info(&mut stream, info).await, + "info" => handle_info(&mut stream, info, session).await, "config" => handle_config(&mut stream, ctx, sandbox).await, "ports" => handle_ports(&mut stream, ctx).await, - "kill" => handle_kill(&mut stream, ctx, groups).await, + "kill" => handle_kill(&mut stream, ctx, &session.groups).await, _ => { let resp = ControlResponse { v: 1, @@ -386,8 +396,12 @@ async fn serve_one( } } -async fn handle_info(stream: &mut ControlStream, info: &SandboxInfo) { - let resp = match serde_json::to_value(info) { +async fn handle_info(stream: &mut ControlStream, info: &SandboxInfo, session: &Session) { + let info = SandboxInfo { + main_exited: crate::pgroup::has_exited(session.main.as_raw_fd()), + ..info.clone() + }; + let resp = match serde_json::to_value(&info) { Ok(data) => ControlResponse { v: 1, ok: true, data: Some(data), err: None }, Err(e) => ControlResponse { v: 1, @@ -832,7 +846,7 @@ mod tests { } fn info() -> SandboxInfo { - SandboxInfo { mode: Some("test".into()) } + SandboxInfo { mode: Some("test".into()), main_exited: false } } /// Bind a listener for `name`, run the control loop on it with @@ -842,8 +856,11 @@ mod tests { let sandbox = Arc::new(tokio::sync::Mutex::new(test_sandbox())); tokio::spawn(async move { let _claim = sockets.claim; - let groups = Arc::new(crate::pgroup::ProcessGroups::new()); - control_loop(sockets.control, None, groups, sandbox, info(), expected_uid).await; + let session = Session { + groups: Arc::new(crate::pgroup::ProcessGroups::new()), + main: crate::sys::syscall::pidfd_open(std::process::id(), 0).unwrap(), + }; + control_loop(sockets.control, None, session, sandbox, info(), expected_uid).await; }) } diff --git a/crates/sandlock-core/src/sandbox.rs b/crates/sandlock-core/src/sandbox.rs index 8f81b6841..60fd44503 100644 --- a/crates/sandlock-core/src/sandbox.rs +++ b/crates/sandlock-core/src/sandbox.rs @@ -2261,7 +2261,15 @@ impl Sandbox { let notif_fd_num = read_u32_fd(pipes.notif_r.as_raw_fd()) .map_err(|e| SandboxRuntimeError::Child(format!("read notif fd from child: {}", e)))?; - let control_info = crate::control::SandboxInfo { mode: self.mode.clone() }; + let control_info = crate::control::SandboxInfo { mode: self.mode.clone(), main_exited: false }; + let control_groups = Arc::clone(&self.rt().groups); + let control_session = || -> Result { + Ok(crate::control::Session { + groups: Arc::clone(&control_groups), + main: pidfd.as_ref().expect("reaper pins the child") + .try_clone().map_err(SandboxRuntimeError::Io)?, + }) + }; let is_nested_mode = notif_fd_num == 0; @@ -2513,7 +2521,7 @@ impl Sandbox { self.rt_mut().control_handle = Some(crate::control::spawn_control_loop( sockets, Some(control_ctx), - Arc::clone(&self.rt().groups), + control_session()?, sandbox_snapshot, control_info.clone(), )); @@ -2539,7 +2547,7 @@ impl Sandbox { self.rt_mut().control_handle = Some(crate::control::spawn_control_loop( sockets, None, - Arc::clone(&self.rt().groups), + control_session()?, self.clone(), control_info, )); diff --git a/crates/sandlock-core/tests/integration/test_control.rs b/crates/sandlock-core/tests/integration/test_control.rs index 2aff35eb9..2a82c2992 100644 --- a/crates/sandlock-core/tests/integration/test_control.rs +++ b/crates/sandlock-core/tests/integration/test_control.rs @@ -875,3 +875,33 @@ async fn test_control_cancelled_creation_releases_its_name() { assert!(!sandlock_core::control::list_sandboxes().unwrap().contains(&name)); drop(cancelled); } + +/// A session can outlive its main process, whose pid is then free for +/// reuse; ps must stop reporting it instead of reading a recycled /proc entry. +#[tokio::test(flavor = "multi_thread")] +async fn test_control_ps_hides_the_pid_of_an_exited_main() { + let name = format!("test-ctrl-main-exited-{}", std::process::id()); + let mut sb = sandlock_core::Sandbox::builder() + .fs_read("/usr") + .fs_read("/bin") + .fs_read("/lib") + .fs_read_if_exists("/lib64") + .fs_read("/proc") + .fs_read("/dev") + .build() + .unwrap() + .with_name(&name); + sb.spawn(&["sh", "-c", "sleep 60 & exit 0"]).await.unwrap(); + sb.wait().await.unwrap(); + assert!(!sb.process_groups().unwrap().is_empty()); + + let info = sandlock_core::control::sandbox_info(&name).expect("info"); + assert!(info.main_exited); + let out = sandlock_bin().args(["ps"]).output().expect("sandlock ps"); + let stdout = String::from_utf8_lossy(&out.stdout); + let row = stdout.lines().find(|l| l.starts_with(&name)).expect("ps lists the session"); + let fields: Vec<&str> = row.split_whitespace().collect(); + assert_eq!(fields[1], "-", "exited main has no pid: {row}"); + assert!(row.ends_with("(main exited)"), "{row}"); + sb.shutdown().await.unwrap(); +}