From c0438456f738c39de3b3f3504d86ec0b86b94dde Mon Sep 17 00:00:00 2001 From: dzerik Date: Mon, 29 Jun 2026 01:06:37 +0300 Subject: [PATCH 1/2] =?UTF-8?q?feat(ffi):=20C=20ABI=20for=20streaming-stdi?= =?UTF-8?q?o=20popen=20=E2=80=94=20sandlock=5Fpopen=20+=20handle=5Fkill?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second piece of the streaming-stdio process API (RFC #67): expose the core Sandbox::popen over the C ABI so the Python/Go bindings can drive a confined process's stdio while it runs. The Python/Go wrapper follows as the next PR in the series; this lands the C surface it builds on. - sandlock_popen(policy, name, argv, argc, stdin_mode, stdout_mode, stderr_mode, out_stdin_fd, out_stdout_fd, out_stderr_fd): create+start a live handle with per-stream StdioMode (0=inherit, 1=piped, 2=null). Each piped stream's owned fd is returned through its out pointer; inherit/null yield -1. Uses the multi-threaded live runtime so the supervisor keeps pumping while the caller blocks on the fds. - sandlock_handle_kill: SIGKILL the handle's process group without freeing the handle, so the caller can still collect the exit status via sandlock_handle_wait. - Existing sandlock_handle_wait/_pid/_free already cover popen handles; their safety docs now name sandlock_popen as a second handle source. Fail-loud / no-leak (the binding boundary): an unknown StdioMode returns a null handle with every out fd left -1 and logs the rejected discriminant to stderr (not swallowed into an indistinguishable null); a piped fd whose out pointer is null is closed rather than leaked. The owned fds are converted to raw ints only on the infallible side of the result match, so an error return drops and closes them instead of leaking a dangling fd. Because the FFI always takes each piped stream, the core wait()-time stdin EOF safety net cannot fire here, so the deadlock warning (close a piped stdin and drain piped stdout/stderr before wait) is carried explicitly on the sandlock_popen and sandlock_handle_wait docs. Header regenerated with cbindgen (additive only, doc-only delta). Tests drive the symbols directly: stdout streaming + exit, stdin/stdout roundtrip through cat, stderr piped, Null yields no fd, unknown mode rejected with out fds reset, the kill lifecycle (kill->wait->free, kill->free without wait, idempotent kill), and a destructive fd-leak test (PIPED stream with a null out pointer must close its fd — verified to fail if the close branch regresses). Refs #67 --- crates/sandlock-ffi/include/sandlock.h | 55 +++- crates/sandlock-ffi/src/lib.rs | 164 +++++++++- crates/sandlock-ffi/tests/popen.rs | 410 +++++++++++++++++++++++++ 3 files changed, 624 insertions(+), 5 deletions(-) create mode 100644 crates/sandlock-ffi/tests/popen.rs diff --git a/crates/sandlock-ffi/include/sandlock.h b/crates/sandlock-ffi/include/sandlock.h index 217b642d..33b0ee90 100644 --- a/crates/sandlock-ffi/include/sandlock.h +++ b/crates/sandlock-ffi/include/sandlock.h @@ -684,6 +684,42 @@ sandlock_handle_t *sandlock_create_for_run(const sandlock_sandbox_t *policy, const char *const *argv, unsigned int argc); +/** + * Spawn a confined process with per-stream stdio wiring and return a live + * handle (the streaming counterpart of `sandlock_create` + `sandlock_start`). + * + * `stdin_mode`/`stdout_mode`/`stderr_mode` are `StdioMode` discriminants + * (0=inherit, 1=piped, 2=null). For each stream set to *piped*, the matching + * `out_*_fd` receives an owned fd the caller must `close()`; for inherit/null + * it receives -1. Pass null for an `out_*_fd` to discard that fd (it is closed + * rather than leaked). Returns null on error (unknown mode, build/fork + * failure) with every `out_*_fd` left -1. + * + * The handle uses a multi-threaded runtime so the seccomp supervisor keeps + * pumping while the caller does blocking IO on the returned fds. Wait for the + * process with `sandlock_handle_wait`, terminate with `sandlock_handle_kill`, + * and release with `sandlock_handle_free`. + * + * Deadlock warning: a piped fd is yours once returned — `close()` a piped + * `out_stdin_fd` *before* `sandlock_handle_wait`, or a child that reads to EOF + * (e.g. `cat`) never exits and the wait blocks forever. Likewise drain a piped + * `out_stdout_fd`/`out_stderr_fd` before waiting: a child that fills the pipe + * buffer blocks on write and never reaches exit. + * + * # Safety + * As `sandlock_create`; `out_*_fd` must each be null or a valid `*mut c_int`. + */ +sandlock_handle_t *sandlock_popen(const sandlock_sandbox_t *policy, + const char *name, + const char *const *argv, + unsigned int argc, + uint32_t stdin_mode, + uint32_t stdout_mode, + uint32_t stderr_mode, + int *out_stdin_fd, + int *out_stdout_fd, + int *out_stderr_fd); + /** * Release a previously `sandlock_create`d child to execve. Returns 0 on * success, -1 on error. @@ -701,11 +737,26 @@ int sandlock_start(sandlock_handle_t *h); */ 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. + * + * # Safety + * `h` must be a valid handle from `sandlock_create` / `sandlock_popen`. + */ +int sandlock_handle_kill(sandlock_handle_t *h); + /** * Wait for the sandbox to exit. Returns a result handle with stdout/stderr. * + * 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 from `sandlock_create`. + * `h` must be a valid handle from `sandlock_create` / `sandlock_popen`. */ sandlock_result_t *sandlock_handle_wait(sandlock_handle_t *h); @@ -733,7 +784,7 @@ char *sandlock_handle_port_mappings(const sandlock_handle_t *h); * Free a sandbox handle. Kills the process if still running. * * # Safety - * `h` must be null or a valid handle from `sandlock_create`. + * `h` must be null or a valid handle from `sandlock_create` / `sandlock_popen`. */ void sandlock_handle_free(sandlock_handle_t *h); diff --git a/crates/sandlock-ffi/src/lib.rs b/crates/sandlock-ffi/src/lib.rs index 57b0c5ee..60f8fa92 100644 --- a/crates/sandlock-ffi/src/lib.rs +++ b/crates/sandlock-ffi/src/lib.rs @@ -10,7 +10,7 @@ use std::time::Duration; use sandlock_core::pipeline::Stage; use sandlock_core::sandbox::{BranchAction, ByteSize, SandboxBuilder}; -use sandlock_core::{Protection, RunResult, Sandbox}; +use sandlock_core::{Protection, RunResult, Sandbox, StdioMode}; pub mod handler; pub mod notif_repr; @@ -1098,6 +1098,141 @@ pub unsafe extern "C" fn sandlock_create_for_run( sandlock_create_with_runtime(policy, name, argv, argc, build_runtime) } +/// Map a raw `StdioMode` discriminant (0=inherit, 1=piped, 2=null) to the enum. +/// Returns `None` for any other value so callers can reject it loudly. +fn stdio_mode_from_raw(v: u32) -> Option { + match v { + 0 => Some(StdioMode::Inherit), + 1 => Some(StdioMode::Piped), + 2 => Some(StdioMode::Null), + _ => None, + } +} + +/// Write `fd` through `out` if non-null; otherwise close it (a piped stream +/// whose fd the caller did not ask for must not leak). +unsafe fn write_or_close_fd(out: *mut c_int, fd: c_int) { + if out.is_null() { + if fd >= 0 { + libc::close(fd); + } + } else { + *out = fd; + } +} + +/// Spawn a confined process with per-stream stdio wiring and return a live +/// handle (the streaming counterpart of `sandlock_create` + `sandlock_start`). +/// +/// `stdin_mode`/`stdout_mode`/`stderr_mode` are `StdioMode` discriminants +/// (0=inherit, 1=piped, 2=null). For each stream set to *piped*, the matching +/// `out_*_fd` receives an owned fd the caller must `close()`; for inherit/null +/// it receives -1. Pass null for an `out_*_fd` to discard that fd (it is closed +/// rather than leaked). Returns null on error (unknown mode, build/fork +/// failure) with every `out_*_fd` left -1. +/// +/// The handle uses a multi-threaded runtime so the seccomp supervisor keeps +/// pumping while the caller does blocking IO on the returned fds. Wait for the +/// process with `sandlock_handle_wait`, terminate with `sandlock_handle_kill`, +/// and release with `sandlock_handle_free`. +/// +/// Deadlock warning: a piped fd is yours once returned — `close()` a piped +/// `out_stdin_fd` *before* `sandlock_handle_wait`, or a child that reads to EOF +/// (e.g. `cat`) never exits and the wait blocks forever. Likewise drain a piped +/// `out_stdout_fd`/`out_stderr_fd` before waiting: a child that fills the pipe +/// buffer blocks on write and never reaches exit. +/// +/// # Safety +/// As `sandlock_create`; `out_*_fd` must each be null or a valid `*mut c_int`. +#[no_mangle] +pub unsafe extern "C" fn sandlock_popen( + policy: *const sandlock_sandbox_t, + name: *const c_char, + argv: *const *const c_char, + argc: c_uint, + stdin_mode: u32, + stdout_mode: u32, + stderr_mode: u32, + out_stdin_fd: *mut c_int, + out_stdout_fd: *mut c_int, + out_stderr_fd: *mut c_int, +) -> *mut sandlock_handle_t { + use std::os::fd::IntoRawFd; + + if !out_stdin_fd.is_null() { + *out_stdin_fd = -1; + } + if !out_stdout_fd.is_null() { + *out_stdout_fd = -1; + } + if !out_stderr_fd.is_null() { + *out_stderr_fd = -1; + } + + if policy.is_null() || argv.is_null() { + return ptr::null_mut(); + } + let (stdin, stdout, stderr) = match ( + stdio_mode_from_raw(stdin_mode), + stdio_mode_from_raw(stdout_mode), + stdio_mode_from_raw(stderr_mode), + ) { + (Some(a), Some(b), Some(c)) => (a, b, c), + _ => { + // Fail loud, not into an indistinguishable NULL: name the offending + // discriminant(s) so the binding author sees why the handle is null. + eprintln!( + "sandlock: sandlock_popen rejected an unknown StdioMode \ + (stdin={stdin_mode}, stdout={stdout_mode}, stderr={stderr_mode}); \ + valid values are 0=inherit, 1=piped, 2=null" + ); + return ptr::null_mut(); + } + }; + let policy = &(*policy)._private; + let name = match optional_name(name) { + Ok(name) => name, + Err(_) => return ptr::null_mut(), + }; + let args = read_argv(argv, argc); + let arg_refs: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); + + let rt = match build_live_runtime() { + Some(rt) => rt, + None => return ptr::null_mut(), + }; + let mut sb = match name { + Some(ref n) => policy.clone().with_name(n.clone()), + None => policy.clone(), + }; + + // popen returns a Process borrowing `sb`; take the owned fds and let it drop + // so `sb` can move into the handle. The process keeps running (owned by `sb`). + // Return OwnedFds (not raw ints) from the future: on the Err/None branch + // below they drop-close themselves instead of leaking a dangling raw fd. + let owned = block_on_runtime(&rt, async { + let mut proc = sb.popen(&arg_refs, stdin, stdout, stderr).await?; + Ok::<_, sandlock_core::SandlockError>(( + proc.take_stdin(), + proc.take_stdout(), + proc.take_stderr(), + )) + }); + let (in_fd, out_fd, err_fd) = match owned { + Some(Ok(t)) => t, + _ => return ptr::null_mut(), + }; + // Convert to raw only on the infallible side of the match. + write_or_close_fd(out_stdin_fd, in_fd.map(|f| f.into_raw_fd()).unwrap_or(-1)); + write_or_close_fd(out_stdout_fd, out_fd.map(|f| f.into_raw_fd()).unwrap_or(-1)); + write_or_close_fd(out_stderr_fd, err_fd.map(|f| f.into_raw_fd()).unwrap_or(-1)); + + Box::into_raw(Box::new(sandlock_handle_t { + sandbox: sb, + runtime: rt, + })) +} + /// Release a previously `sandlock_create`d child to execve. Returns 0 on /// success, -1 on error. /// @@ -1127,10 +1262,33 @@ pub unsafe extern "C" fn sandlock_handle_pid(h: *const sandlock_handle_t) -> i32 (*h).sandbox.pid().unwrap_or(0) } +/// 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. +/// +/// # Safety +/// `h` must be a valid handle from `sandlock_create` / `sandlock_popen`. +#[no_mangle] +pub unsafe extern "C" fn sandlock_handle_kill(h: *mut sandlock_handle_t) -> c_int { + if h.is_null() { + return -1; + } + let h = &mut *h; + if h.sandbox.kill().is_err() { + return -1; + } + 0 +} + /// Wait for the sandbox to exit. Returns a result handle with stdout/stderr. /// +/// 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 from `sandlock_create`. +/// `h` must be a valid handle from `sandlock_create` / `sandlock_popen`. #[no_mangle] pub unsafe extern "C" fn sandlock_handle_wait(h: *mut sandlock_handle_t) -> *mut sandlock_result_t { if h.is_null() { @@ -1211,7 +1369,7 @@ pub unsafe extern "C" fn sandlock_handle_port_mappings(h: *const sandlock_handle /// Free a sandbox handle. Kills the process if still running. /// /// # Safety -/// `h` must be null or a valid handle from `sandlock_create`. +/// `h` must be null or a valid handle from `sandlock_create` / `sandlock_popen`. #[no_mangle] pub unsafe extern "C" fn sandlock_handle_free(h: *mut sandlock_handle_t) { if !h.is_null() { diff --git a/crates/sandlock-ffi/tests/popen.rs b/crates/sandlock-ffi/tests/popen.rs new file mode 100644 index 00000000..e6882fd9 --- /dev/null +++ b/crates/sandlock-ffi/tests/popen.rs @@ -0,0 +1,410 @@ +//! Integration tests for the C ABI streaming-stdio `sandlock_popen` (RFC #67). +//! +//! These drive the FFI symbols directly (no C compilation step) and read the +//! returned pipe fds. The handle uses a multi-threaded runtime, so blocking +//! reads on the test thread do not starve the supervisor. + +use std::ffi::CString; +use std::io::{Read, Write}; +use std::os::fd::FromRawFd; +use std::os::raw::{c_char, c_int, c_uint}; +use std::ptr; + +use sandlock_ffi::{ + sandlock_handle_free, sandlock_handle_kill, sandlock_handle_wait, sandlock_popen, + sandlock_result_exit_code, sandlock_result_free, sandlock_result_success, + sandlock_sandbox_build, sandlock_sandbox_builder_fs_read, sandlock_sandbox_builder_new, + sandlock_sandbox_free, sandlock_sandbox_t, +}; + +const INHERIT: u32 = 0; +const PIPED: u32 = 1; +const NULL: u32 = 2; + +/// Build a policy that can exec the usual coreutils in a minimal rootfs. +fn build_policy() -> *mut sandlock_sandbox_t { + let mut b = sandlock_sandbox_builder_new(); + for p in ["/usr", "/lib", "/lib64", "/bin", "/etc", "/proc", "/dev"] { + let c = CString::new(p).unwrap(); + b = unsafe { sandlock_sandbox_builder_fs_read(b, c.as_ptr()) }; + } + let mut err: c_int = 0; + let policy = unsafe { sandlock_sandbox_build(b, &mut err, ptr::null_mut()) }; + assert_eq!(err, 0, "policy build failed"); + assert!(!policy.is_null()); + policy +} + +fn argv(cmd: &[&str]) -> (Vec, Vec<*const c_char>) { + let owned: Vec = cmd.iter().map(|s| CString::new(*s).unwrap()).collect(); + let ptrs: Vec<*const c_char> = owned.iter().map(|c| c.as_ptr()).collect(); + (owned, ptrs) +} + +#[test] +fn popen_streams_stdout_and_collects_exit() { + let policy = build_policy(); + let (_owned, av) = argv(&["echo", "ffi-hi"]); + + let mut fd_in: c_int = -1; + let mut fd_out: c_int = -1; + let mut fd_err: c_int = -1; + let h = unsafe { + sandlock_popen( + policy, + ptr::null(), + av.as_ptr(), + av.len() as c_uint, + INHERIT, + PIPED, + INHERIT, + &mut fd_in, + &mut fd_out, + &mut fd_err, + ) + }; + assert!(!h.is_null(), "sandlock_popen returned null"); + assert_eq!(fd_in, -1, "stdin was inherit → no fd"); + assert!(fd_out >= 0, "stdout was piped → expected an fd"); + assert_eq!(fd_err, -1, "stderr was inherit → no fd"); + + let mut out = String::new(); + unsafe { std::fs::File::from_raw_fd(fd_out) } + .read_to_string(&mut out) + .unwrap(); + assert_eq!(out, "ffi-hi\n"); + + let res = unsafe { sandlock_handle_wait(h) }; + assert!(!res.is_null()); + assert_eq!(unsafe { sandlock_result_exit_code(res) }, 0); + unsafe { + sandlock_result_free(res); + sandlock_handle_free(h); + sandlock_sandbox_free(policy); + } +} + +#[test] +fn popen_stdin_stdout_roundtrip() { + let policy = build_policy(); + let (_owned, av) = argv(&["cat"]); + + let mut fd_in: c_int = -1; + let mut fd_out: c_int = -1; + let mut fd_err: c_int = -1; + let h = unsafe { + sandlock_popen( + policy, + ptr::null(), + av.as_ptr(), + av.len() as c_uint, + PIPED, + PIPED, + INHERIT, + &mut fd_in, + &mut fd_out, + &mut fd_err, + ) + }; + assert!(!h.is_null()); + assert!(fd_in >= 0 && fd_out >= 0); + + let mut stdin = unsafe { std::fs::File::from_raw_fd(fd_in) }; + let mut stdout = unsafe { std::fs::File::from_raw_fd(fd_out) }; + stdin.write_all(b"ping\n").unwrap(); + drop(stdin); // EOF → cat exits + + let mut out = String::new(); + stdout.read_to_string(&mut out).unwrap(); + assert_eq!(out, "ping\n"); + + let res = unsafe { sandlock_handle_wait(h) }; + assert_eq!(unsafe { sandlock_result_exit_code(res) }, 0); + unsafe { + sandlock_result_free(res); + sandlock_handle_free(h); + sandlock_sandbox_free(policy); + } +} + +#[test] +fn popen_null_mode_yields_no_fd() { + let policy = build_policy(); + let (_owned, av) = argv(&["echo", "discarded"]); + + let mut fd_in: c_int = -1; + let mut fd_out: c_int = -1; + let mut fd_err: c_int = -1; + let h = unsafe { + sandlock_popen( + policy, + ptr::null(), + av.as_ptr(), + av.len() as c_uint, + INHERIT, + NULL, + INHERIT, + &mut fd_in, + &mut fd_out, + &mut fd_err, + ) + }; + assert!(!h.is_null()); + assert_eq!(fd_out, -1, "Null stdout yields no caller fd"); + let res = unsafe { sandlock_handle_wait(h) }; + assert_eq!(unsafe { sandlock_result_exit_code(res) }, 0); + unsafe { + sandlock_result_free(res); + sandlock_handle_free(h); + sandlock_sandbox_free(policy); + } +} + +#[test] +fn popen_stderr_piped_through_ffi() { + let policy = build_policy(); + let (_owned, av) = argv(&["sh", "-c", "echo err 1>&2"]); + + let mut fd_in: c_int = -1; + let mut fd_out: c_int = -1; + let mut fd_err: c_int = -1; + let h = unsafe { + sandlock_popen( + policy, + ptr::null(), + av.as_ptr(), + av.len() as c_uint, + INHERIT, + INHERIT, + PIPED, + &mut fd_in, + &mut fd_out, + &mut fd_err, + ) + }; + assert!(!h.is_null()); + assert_eq!(fd_out, -1, "stdout was inherit"); + assert!(fd_err >= 0, "stderr was piped → expected an fd"); + + let mut out = String::new(); + unsafe { std::fs::File::from_raw_fd(fd_err) } + .read_to_string(&mut out) + .unwrap(); + assert_eq!(out, "err\n"); + + let res = unsafe { sandlock_handle_wait(h) }; + assert_eq!(unsafe { sandlock_result_exit_code(res) }, 0); + unsafe { + sandlock_result_free(res); + sandlock_handle_free(h); + sandlock_sandbox_free(policy); + } +} + +/// kill → wait → free: kill terminates the group, wait still collects the +/// (non-success) exit status, free releases cleanly. +#[test] +fn popen_kill_then_wait_then_free() { + let policy = build_policy(); + let (_owned, av) = argv(&["sleep", "100"]); + + let mut a: c_int = -1; + let mut b: c_int = -1; + let mut c: c_int = -1; + let h = unsafe { + sandlock_popen( + policy, + ptr::null(), + av.as_ptr(), + av.len() as c_uint, + INHERIT, + INHERIT, + INHERIT, + &mut a, + &mut b, + &mut c, + ) + }; + assert!(!h.is_null()); + + assert_eq!(unsafe { sandlock_handle_kill(h) }, 0, "kill should succeed"); + let res = unsafe { sandlock_handle_wait(h) }; + assert!(!res.is_null(), "wait must still return after kill"); + assert!( + !unsafe { sandlock_result_success(res) }, + "a killed process is not success" + ); + unsafe { + sandlock_result_free(res); + sandlock_handle_free(h); + sandlock_sandbox_free(policy); + } +} + +/// kill → free without wait must not hang: free's Sandbox::drop reaps the +/// still-Running process. +#[test] +fn popen_kill_then_free_without_wait() { + let policy = build_policy(); + let (_owned, av) = argv(&["sleep", "100"]); + + let mut a: c_int = -1; + let mut b: c_int = -1; + let mut c: c_int = -1; + let h = unsafe { + sandlock_popen( + policy, + ptr::null(), + av.as_ptr(), + av.len() as c_uint, + INHERIT, + INHERIT, + INHERIT, + &mut a, + &mut b, + &mut c, + ) + }; + assert!(!h.is_null()); + assert_eq!(unsafe { sandlock_handle_kill(h) }, 0); + // No wait — free must reap, not hang. + unsafe { + sandlock_handle_free(h); + sandlock_sandbox_free(policy); + } +} + +/// kill is idempotent: a second kill (process already dying/exited) still +/// returns 0 (ESRCH is swallowed). +#[test] +fn popen_kill_is_idempotent() { + let policy = build_policy(); + let (_owned, av) = argv(&["sleep", "100"]); + + let mut a: c_int = -1; + let mut b: c_int = -1; + let mut c: c_int = -1; + let h = unsafe { + sandlock_popen( + policy, + ptr::null(), + av.as_ptr(), + av.len() as c_uint, + INHERIT, + INHERIT, + INHERIT, + &mut a, + &mut b, + &mut c, + ) + }; + assert!(!h.is_null()); + assert_eq!(unsafe { sandlock_handle_kill(h) }, 0); + assert_eq!( + unsafe { sandlock_handle_kill(h) }, + 0, + "second kill must be idempotent" + ); + let res = unsafe { sandlock_handle_wait(h) }; + unsafe { + sandlock_result_free(res); + sandlock_handle_free(h); + sandlock_sandbox_free(policy); + } +} + +#[test] +fn popen_rejects_unknown_stdio_mode() { + let policy = build_policy(); + let (_owned, av) = argv(&["echo", "x"]); + + let mut fd_in: c_int = 7; // pre-set non-sentinel to prove it is reset + let mut fd_out: c_int = 7; + let mut fd_err: c_int = 7; + let h = unsafe { + sandlock_popen( + policy, + ptr::null(), + av.as_ptr(), + av.len() as c_uint, + INHERIT, + 99, + INHERIT, // 99 is not a valid StdioMode + &mut fd_in, + &mut fd_out, + &mut fd_err, + ) + }; + assert!( + h.is_null(), + "unknown stdio mode must fail loudly (null handle)" + ); + assert_eq!( + (fd_in, fd_out, fd_err), + (-1, -1, -1), + "out fds reset to -1 on error" + ); + unsafe { sandlock_sandbox_free(policy) }; +} + +/// A PIPED stream whose out-pointer is null must have its pipe fd *closed*, not +/// leaked (the advertised no-leak invariant of `write_or_close_fd`). Run enough +/// iterations that a per-call fd leak would blow the process fd count, and +/// assert it stays bounded. If the null-out-ptr close branch regressed into a +/// leak, this fails; the other tests (all passing real `&mut fd`) would not. +#[test] +fn popen_piped_stream_with_null_out_ptr_is_closed_not_leaked() { + fn fd_count() -> usize { + std::fs::read_dir("/proc/self/fd").unwrap().count() + } + + let policy = build_policy(); + let (_owned, av) = argv(&["echo", "leak-check"]); + + let mut samples = Vec::new(); + for i in 0..40 { + let mut fd_in: c_int = -1; + let mut fd_err: c_int = -1; + // stdout is PIPED but its out pointer is null → the pipe read-end must be + // closed inside sandlock_popen, never handed out and never leaked. + let h = unsafe { + sandlock_popen( + policy, + ptr::null(), + av.as_ptr(), + av.len() as c_uint, + INHERIT, + PIPED, + INHERIT, + &mut fd_in, + ptr::null_mut(), // discard the piped stdout fd + &mut fd_err, + ) + }; + assert!( + !h.is_null(), + "popen with a null out-pointer should still succeed" + ); + assert_eq!(fd_in, -1, "stdin inherited → no fd"); + assert_eq!(fd_err, -1, "stderr inherited → no fd"); + let res = unsafe { sandlock_handle_wait(h) }; + unsafe { + sandlock_result_free(res); + sandlock_handle_free(h); + } + // Skip the first few iterations: the runtime lazily opens fds that stay + // open and would skew a raw before/after diff. + if i >= 5 { + samples.push(fd_count()); + } + } + unsafe { sandlock_sandbox_free(policy) }; + + let min = *samples.iter().min().unwrap(); + let max = *samples.iter().max().unwrap(); + assert!( + max - min <= 2, + "process fd count grew across iterations (min={min}, max={max}) — a PIPED \ + stream with a null out-pointer is leaking its fd instead of being closed" + ); +} From 35c06aadcd58515e8105d278c7efbfda6cdb3495 Mon Sep 17 00:00:00 2001 From: dzerik Date: Thu, 2 Jul 2026 09:03:50 +0300 Subject: [PATCH 2/2] refactor(ffi): extract shared prepare() prologue for create/popen sandlock_popen re-inlined almost the entire prologue of sandlock_create_with_runtime (null check, optional_name, read_argv, runtime build, with_name), risking drift: a future change to name validation or argv parsing had to be applied in two places. Extract prepare(policy, name, argv, argc, build_rt) -> Option<(Sandbox, Runtime, Vec)> and have both entry points call it. Each keeps its own tail: create* drive sb.create(), popen drives sb.popen() + fd handoff. Behavior-preserving: out-fd reset and the fail-loud unknown-StdioMode rejection stay ahead of the shared prologue in popen. No ABI/header change (prepare is private). All sandlock-ffi tests green. --- crates/sandlock-ffi/src/lib.rs | 79 +++++++++++++++++++--------------- 1 file changed, 44 insertions(+), 35 deletions(-) diff --git a/crates/sandlock-ffi/src/lib.rs b/crates/sandlock-ffi/src/lib.rs index 60f8fa92..af955aee 100644 --- a/crates/sandlock-ffi/src/lib.rs +++ b/crates/sandlock-ffi/src/lib.rs @@ -1021,41 +1021,61 @@ pub struct sandlock_handle_t { runtime: tokio::runtime::Runtime, } -/// Fork the child and install policy; the child is parked between policy -/// install and execve. Returns a live handle. Call `sandlock_start` to -/// release the child to execve. +/// Shared entry-point prologue for the create/popen family: validate the +/// pointers, parse the optional name and argv, build the requested runtime, +/// and apply the name to a cloned policy. Returns the owned +/// `(Sandbox, Runtime, args)` triple, or `None` on any invalid input or +/// runtime-build failure (callers map `None` to a null handle). +/// +/// The tail stays with each caller: `sandlock_create*` drive `sb.create()`, +/// `sandlock_popen` drives `sb.popen()` + fd hand-off. Factoring the prologue +/// here keeps null/name/argv/runtime handling from drifting between them. /// /// # Safety /// `policy` must be a valid policy pointer. `name` may be NULL to /// auto-generate a sandbox name, or a valid NUL-terminated string. /// `argv` must point to `argc` C strings. -unsafe fn sandlock_create_with_runtime( +unsafe fn prepare( policy: *const sandlock_sandbox_t, name: *const c_char, argv: *const *const c_char, argc: c_uint, build_rt: fn() -> Option, -) -> *mut sandlock_handle_t { +) -> Option<(Sandbox, tokio::runtime::Runtime, Vec)> { if policy.is_null() || argv.is_null() { - return ptr::null_mut(); + return None; } let policy = &(*policy)._private; - let name = match optional_name(name) { - Ok(name) => name, - Err(_) => return ptr::null_mut(), - }; + let name = optional_name(name).ok()?; let args = read_argv(argv, argc); - let arg_refs: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); - - let rt = match build_rt() { - Some(rt) => rt, - None => return ptr::null_mut(), - }; - - let mut sb = match name { + let rt = build_rt()?; + let sb = match name { Some(ref n) => policy.clone().with_name(n.clone()), None => policy.clone(), }; + Some((sb, rt, args)) +} + +/// Fork the child and install policy; the child is parked between policy +/// install and execve. Returns a live handle. Call `sandlock_start` to +/// release the child to execve. +/// +/// # Safety +/// `policy` must be a valid policy pointer. `name` may be NULL to +/// auto-generate a sandbox name, or a valid NUL-terminated string. +/// `argv` must point to `argc` C strings. +unsafe fn sandlock_create_with_runtime( + policy: *const sandlock_sandbox_t, + name: *const c_char, + argv: *const *const c_char, + argc: c_uint, + build_rt: fn() -> Option, +) -> *mut sandlock_handle_t { + let (mut sb, rt, args) = match prepare(policy, name, argv, argc, build_rt) { + Some(t) => t, + None => return ptr::null_mut(), + }; + let arg_refs: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); if !matches!(block_on_runtime(&rt, sb.create(&arg_refs)), Some(Ok(()))) { return ptr::null_mut(); @@ -1169,9 +1189,6 @@ pub unsafe extern "C" fn sandlock_popen( *out_stderr_fd = -1; } - if policy.is_null() || argv.is_null() { - return ptr::null_mut(); - } let (stdin, stdout, stderr) = match ( stdio_mode_from_raw(stdin_mode), stdio_mode_from_raw(stdout_mode), @@ -1189,22 +1206,14 @@ pub unsafe extern "C" fn sandlock_popen( return ptr::null_mut(); } }; - let policy = &(*policy)._private; - let name = match optional_name(name) { - Ok(name) => name, - Err(_) => return ptr::null_mut(), - }; - let args = read_argv(argv, argc); - let arg_refs: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); - - let rt = match build_live_runtime() { - Some(rt) => rt, + // Shared prologue (null/name/argv/runtime) via `prepare`; popen keeps its own + // `sb.popen()` + fd-handoff tail. build_live_runtime: the multi-threaded + // runtime keeps the seccomp supervisor pumping during the caller's blocking IO. + let (mut sb, rt, args) = match prepare(policy, name, argv, argc, build_live_runtime) { + Some(t) => t, None => return ptr::null_mut(), }; - let mut sb = match name { - Some(ref n) => policy.clone().with_name(n.clone()), - None => policy.clone(), - }; + let arg_refs: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); // popen returns a Process borrowing `sb`; take the owned fds and let it drop // so `sb` can move into the handle. The process keeps running (owned by `sb`).