From 125c0744ea5c00bf72f8cb37daa98f8de97480b3 Mon Sep 17 00:00:00 2001 From: dzerik Date: Thu, 2 Jul 2026 10:05:48 +0300 Subject: [PATCH 1/2] =?UTF-8?q?feat(go):=20streaming-stdio=20Popen=20?= =?UTF-8?q?=E2=80=94=20Process=20with=20per-stream=20StdioMode=20(RFC=20#6?= =?UTF-8?q?7)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Go ergonomic layer over the C ABI sandlock_popen, mirroring the FFI and Python bindings. Sandbox.Popen(stdio, cmd...) forks the confined child with per-stream stdio and returns a live *Process — the streaming counterpart of Spawn. Each stream set to StdioPiped is handed back on the Process as an *os.File (Stdin/Stdout/Stderr) the caller reads/writes while the child runs; inherit/null streams leave it nil. The zero Stdio inherits all three (identical to Spawn). - StdioMode (Inherit=0/Piped=1/Null=2) shares the stable ABI discriminants; an out-of-range mode is rejected before the child is spawned. - os.NewFile takes ownership of each piped fd, so closing the file closes the fd — the fds never leak (via Close, the SetFinalizer reap, or the *os.File's own finalizer). On a null handle the FFI leaves the out-fds at -1, so no stream is wrapped on the error path. - Wait closes a still-open piped Stdin first to deliver EOF (so a reader child like cat exits), then waits and frees the handle. Close closes all piped streams regardless of handle state and reaps the child. - No wait-timeout is added: unlike the Python binding, Kill (process-group signal by PID) already interrupts a blocked Wait from another goroutine, so the undrained-pipe hang has a built-in escape hatch. Tests mirror the FFI/Python popen suites: stdout streaming, stdin/stdout roundtrip through cat, wait-closes-unclosed-stdin (watchdog), both stdout+stderr piped, null yields no stream, invalid mode rejected, Kill interrupts a blocked Wait, Close closes streams + reaps. README documents Popen/Stdio. Refs #67 --- go/README.md | 24 +++- go/sandbox.go | 23 ++++ go/sandlock_linux.go | 95 ++++++++++++- go/sandlock_linux_test.go | 272 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 412 insertions(+), 2 deletions(-) diff --git a/go/README.md b/go/README.md index 5e7795f..aa066f3 100644 --- a/go/README.md +++ b/go/README.md @@ -119,6 +119,7 @@ func (s *Sandbox) Run(ctx context.Context, cmd ...string) (*Result, error) func (s *Sandbox) RunInteractive(ctx context.Context, cmd ...string) (int, error) func (s *Sandbox) DryRun(ctx context.Context, cmd ...string) (*DryRunResult, error) func (s *Sandbox) Spawn(cmd ...string) (*Process, error) +func (s *Sandbox) Popen(stdio Stdio, cmd ...string) (*Process, error) ``` - **Run** captures stdout/stderr and waits. A `ctx` deadline kills the process @@ -129,6 +130,22 @@ func (s *Sandbox) Spawn(cmd ...string) (*Process, error) filesystem `Changes` it would have made, and discards them. Requires `Workdir`. - **Spawn** starts a process without waiting, returning a `*Process`. +- **Popen** is the streaming counterpart of Spawn: each stream set to + `StdioPiped` is handed back on the `*Process` as an `*os.File` + (`Stdin`/`Stdout`/`Stderr`) you read/write while the child runs. The zero + `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`. + +```go +p, _ := sb.Popen(sandlock.Stdio{Stdin: sandlock.StdioPiped, Stdout: sandlock.StdioPiped}, "cat") +defer p.Close() +p.Stdin.Write([]byte("hi\n")) +p.Stdin.Close() // EOF so cat exits +out, _ := io.ReadAll(p.Stdout) // "hi\n" +res, _ := p.Wait() +``` ### Dynamic policy callbacks @@ -177,7 +194,12 @@ 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) +func (p *Process) Close() error // release the handle (kills if running), close piped streams + +// Popen only: caller-owned pipe ends, non-nil per stream wired StdioPiped. +p.Stdin // *os.File +p.Stdout // *os.File +p.Stderr // *os.File ``` ### Confine the current process diff --git a/go/sandbox.go b/go/sandbox.go index 3e81efc..45bb99c 100644 --- a/go/sandbox.go +++ b/go/sandbox.go @@ -285,6 +285,29 @@ type Result struct { Stderr []byte // captured standard error } +// StdioMode selects how one of a Popen'd process's standard streams is wired. +// The values are the stable ABI discriminants shared with the C/Rust core. +type StdioMode uint32 + +const ( + // StdioInherit shares the parent's fd (the child writes to the same + // terminal/file). It is the zero value, so an unset stream inherits. + StdioInherit StdioMode = 0 + // StdioPiped connects the stream to a pipe; Popen hands the caller the + // owning end as an *os.File on the returned Process. + StdioPiped StdioMode = 1 + // StdioNull connects the stream to /dev/null. + StdioNull StdioMode = 2 +) + +// Stdio selects the wiring of a Popen'd process's three standard streams. The +// zero value wires all three as StdioInherit, matching Spawn. +type Stdio struct { + Stdin StdioMode + Stdout StdioMode + Stderr StdioMode +} + // ChangeKind classifies a filesystem change observed during a dry run. type ChangeKind byte diff --git a/go/sandlock_linux.go b/go/sandlock_linux.go index 7bee31b..9190a11 100644 --- a/go/sandlock_linux.go +++ b/go/sandlock_linux.go @@ -22,6 +22,7 @@ import ( "context" "encoding/json" "fmt" + "os" "runtime" "strings" "sync" @@ -799,6 +800,13 @@ type Process struct { h *C.sandlock_handle_t pid int waiting bool // a Wait owns the handle; other handle ops must defer to it + + // 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 + // closes the fd). Nil for Spawn'd processes and for inherit/null streams. + Stdin *os.File + Stdout *os.File + Stderr *os.File } // Spawn forks the sandboxed child, installs the policy, and releases it to @@ -838,10 +846,78 @@ func (s *Sandbox) Spawn(cmd ...string) (*Process, error) { return p, nil } +// fdFile wraps an fd owned by the caller (handed back by sandlock_popen) as an +// *os.File that closes the fd when closed or finalized, or nil for the -1 +// sentinel of an inherit/null stream. +func fdFile(fd C.int, name string) *os.File { + if fd < 0 { + return nil + } + return os.NewFile(uintptr(fd), name) +} + +// Popen forks the sandboxed child with per-stream stdio and returns a live +// Process without waiting — the streaming counterpart of Spawn. For each stream +// set to StdioPiped, the matching Process field (Stdin/Stdout/Stderr) is an +// *os.File the caller reads/writes while the child runs; inherit/null streams +// leave it nil. The zero Stdio inherits all three (identical to Spawn). +// +// Deadlock warning (as with os/exec pipes): a piped Stdin is yours — close it +// before Wait, or a child that reads to EOF (e.g. cat) never exits and Wait +// blocks. Likewise drain a piped Stdout/Stderr before Wait, or a child that +// fills the pipe buffer blocks on write. As an escape hatch, Kill from another +// goroutine interrupts a blocked Wait; Close reaps the child and closes the +// streams. Wait closes a still-open piped Stdin for you to deliver EOF. +func (s *Sandbox) Popen(stdio Stdio, cmd ...string) (*Process, error) { + if len(cmd) == 0 { + return nil, fmt.Errorf("sandlock: empty command") + } + for _, m := range []StdioMode{stdio.Stdin, stdio.Stdout, stdio.Stderr} { + if m > StdioNull { + return nil, fmt.Errorf("sandlock: invalid StdioMode %d", uint32(m)) + } + } + policyPtr, err := s.buildPolicy() + if err != nil { + return nil, err + } + defer C.sandlock_sandbox_free(policyPtr) + + argv, err := cArgv(cmd) + if err != nil { + return nil, err + } + defer freeArgv(argv) + ap, ac := argvPtr(argv) + name := s.cName() + defer freeName(name) + + fdIn, fdOut, fdErr := C.int(-1), C.int(-1), C.int(-1) + h := C.sandlock_popen( + policyPtr, name, ap, ac, + C.uint32_t(stdio.Stdin), C.uint32_t(stdio.Stdout), C.uint32_t(stdio.Stderr), + &fdIn, &fdOut, &fdErr, + ) + if h == nil { + return nil, fmt.Errorf("sandlock: popen failed") + } + p := &Process{ + h: h, + pid: int(C.sandlock_handle_pid(h)), + Stdin: fdFile(fdIn, "sandlock-stdin"), + Stdout: fdFile(fdOut, "sandlock-stdout"), + Stderr: fdFile(fdErr, "sandlock-stderr"), + } + runtime.SetFinalizer(p, (*Process).finalize) + return p, nil +} + // finalize is the SetFinalizer cleanup for a Process abandoned without // Wait/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. +// 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 +// they, and each *os.File's own finalizer closes its fd — no fd leaks. func (p *Process) finalize() { p.mu.Lock() defer p.mu.Unlock() @@ -869,6 +945,9 @@ func (p *Process) Pid() int { // 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) { p.mu.Lock() if p.h == nil || p.waiting { @@ -876,9 +955,16 @@ func (p *Process) Wait() (*Result, error) { return nil, ErrNotRunning } h := p.h + stdin := p.Stdin p.waiting = true 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() + } + r := C.sandlock_handle_wait(h) p.mu.Lock() @@ -957,6 +1043,13 @@ func (p *Process) Ports() (map[int]int, error) { func (p *Process) Close() error { p.mu.Lock() defer p.mu.Unlock() + // Release caller-owned pipe fds regardless of handle state: a Close after + // Wait still needs to close the piped stdout/stderr the caller drained. + for _, f := range []*os.File{p.Stdin, p.Stdout, p.Stderr} { + if f != nil { + _ = f.Close() + } + } if p.h == nil { return nil } diff --git a/go/sandlock_linux_test.go b/go/sandlock_linux_test.go index 50f0de8..8ea3adc 100644 --- a/go/sandlock_linux_test.go +++ b/go/sandlock_linux_test.go @@ -4,7 +4,10 @@ package sandlock_test import ( "context" + "fmt" + "io" "os" + "runtime" "strings" "testing" "time" @@ -223,6 +226,275 @@ func TestProcessKillInterruptsWait(t *testing.T) { } } +// --- streaming-stdio popen (RFC #67), mirrors the FFI/Python popen suites --- + +func TestPopenStreamsStdout(t *testing.T) { + requireLandlock(t) + sb := &sandlock.Sandbox{FSReadable: rootfs} + p, err := sb.Popen(sandlock.Stdio{Stdout: sandlock.StdioPiped}, "echo", "go-hi") + if err != nil { + t.Fatalf("Popen: %v", err) + } + defer p.Close() + if p.Stdin != nil || p.Stderr != nil { + t.Fatalf("inherited streams must be nil (stdin=%v stderr=%v)", p.Stdin, p.Stderr) + } + if p.Stdout == nil { + t.Fatal("piped stdout must be non-nil") + } + out, err := io.ReadAll(p.Stdout) + if err != nil { + t.Fatalf("read stdout: %v", err) + } + if string(out) != "go-hi\n" { + t.Fatalf("stdout = %q, want %q", out, "go-hi\n") + } + res, err := p.Wait() + if err != nil { + t.Fatalf("Wait: %v", err) + } + if !res.Success || res.ExitCode != 0 { + t.Fatalf("want success exit 0, got success=%v exit=%d", res.Success, res.ExitCode) + } +} + +func TestPopenStdinStdoutRoundtrip(t *testing.T) { + requireLandlock(t) + sb := &sandlock.Sandbox{FSReadable: rootfs} + p, err := sb.Popen(sandlock.Stdio{Stdin: sandlock.StdioPiped, Stdout: sandlock.StdioPiped}, "cat") + if err != nil { + t.Fatalf("Popen: %v", err) + } + defer p.Close() + if _, err := p.Stdin.Write([]byte("ping\n")); err != nil { + t.Fatalf("write stdin: %v", err) + } + p.Stdin.Close() // EOF so cat exits + out, err := io.ReadAll(p.Stdout) + if err != nil { + t.Fatalf("read stdout: %v", err) + } + if string(out) != "ping\n" { + t.Fatalf("roundtrip = %q, want %q", out, "ping\n") + } + if res, _ := p.Wait(); !res.Success { + t.Fatalf("cat should succeed, got %+v", res) + } +} + +func TestPopenWaitClosesUnclosedStdin(t *testing.T) { + // cat reads stdin to EOF. If the caller pipes stdin but never closes it, + // Wait must close it to deliver EOF — else cat blocks forever and Wait hangs. + // Run under a watchdog so a regression surfaces as a failure, not a hung job. + requireLandlock(t) + sb := &sandlock.Sandbox{FSReadable: rootfs} + p, err := sb.Popen(sandlock.Stdio{Stdin: sandlock.StdioPiped, Stdout: sandlock.StdioPiped}, "cat") + if err != nil { + t.Fatalf("Popen: %v", err) + } + defer p.Close() + if _, err := p.Stdin.Write([]byte("data\n")); err != nil { + t.Fatalf("write stdin: %v", err) + } + // Deliberately do NOT close stdin — Wait is responsible for the EOF. Drain + // stdout concurrently so a full pipe can never be the reason cat blocks. + go io.ReadAll(p.Stdout) + + done := make(chan *sandlock.Result, 1) + go func() { + res, _ := p.Wait() + done <- res + }() + select { + case res := <-done: + if res == nil || !res.Success { + t.Fatalf("wait result not success: %+v", res) + } + case <-time.After(10 * time.Second): + p.Kill() + t.Fatal("Wait did not close unclosed piped stdin → cat blocked forever") + } +} + +func TestPopenStdoutAndStderrBothPiped(t *testing.T) { + requireLandlock(t) + sb := &sandlock.Sandbox{FSReadable: rootfs} + p, err := sb.Popen( + sandlock.Stdio{Stdout: sandlock.StdioPiped, Stderr: sandlock.StdioPiped}, + "sh", "-c", "echo out; echo err 1>&2", + ) + if err != nil { + t.Fatalf("Popen: %v", err) + } + defer p.Close() + out, _ := io.ReadAll(p.Stdout) + errb, _ := io.ReadAll(p.Stderr) + if string(out) != "out\n" { + t.Fatalf("stdout = %q, want %q", out, "out\n") + } + if string(errb) != "err\n" { + t.Fatalf("stderr = %q, want %q", errb, "err\n") + } + if res, _ := p.Wait(); !res.Success { + t.Fatalf("want success, got %+v", res) + } +} + +func TestPopenNullStdoutNoStream(t *testing.T) { + requireLandlock(t) + sb := &sandlock.Sandbox{FSReadable: rootfs} + p, err := sb.Popen(sandlock.Stdio{Stdout: sandlock.StdioNull}, "echo", "discarded") + if err != nil { + t.Fatalf("Popen: %v", err) + } + defer p.Close() + if p.Stdout != nil { + t.Fatal("null stdout must yield no caller stream") + } + if res, _ := p.Wait(); res.ExitCode != 0 { + t.Fatalf("want exit 0, got %d", res.ExitCode) + } +} + +func TestPopenInvalidStdioMode(t *testing.T) { + // An out-of-range discriminant is rejected before the child is spawned, so + // this needs no Landlock. + sb := &sandlock.Sandbox{FSReadable: rootfs} + if _, err := sb.Popen(sandlock.Stdio{Stdout: sandlock.StdioMode(99)}, "echo", "x"); err == nil { + t.Fatal("an out-of-range StdioMode must be rejected") + } +} + +func TestPopenKillInterruptsWait(t *testing.T) { + // A piped stdout we never drain would block Wait on a child that never exits; + // Kill from another goroutine must interrupt it (the Go escape hatch that + // makes a wait-timeout unnecessary here). + requireLandlock(t) + sb := &sandlock.Sandbox{FSReadable: rootfs} + p, err := sb.Popen(sandlock.Stdio{Stdout: sandlock.StdioPiped}, "sleep", "60") + if err != nil { + t.Fatalf("Popen: %v", err) + } + defer p.Close() + done := make(chan error, 1) + go func() { + _, werr := p.Wait() + done <- werr + }() + if err := p.Kill(); err != nil { + t.Fatalf("Kill: %v", err) + } + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("Kill did not interrupt a blocked Wait within 5s") + } +} + +func TestPopenCloseClosesStreamsAndReaps(t *testing.T) { + requireLandlock(t) + sb := &sandlock.Sandbox{FSReadable: rootfs} + p, err := sb.Popen(sandlock.Stdio{Stdout: sandlock.StdioPiped}, "sleep", "60") + if err != nil { + t.Fatalf("Popen: %v", err) + } + stdout := p.Stdout + if err := p.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + // Close must close the piped stream: a further read fails on the closed fd. + if _, err := stdout.Read(make([]byte, 1)); err == nil { + t.Fatal("Close must close the piped stdout stream") + } + // Close is idempotent. + if err := p.Close(); err != nil { + t.Fatalf("second Close: %v", err) + } +} + +// procDead reports whether pid names no live process: its /proc entry is gone +// (reaped) or its state is zombie/dead. Robust to the reap-vs-zombie race a +// plain kill(pid, 0) would be sensitive to. +func procDead(pid int) bool { + data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pid)) + if err != nil { + return true // no /proc entry → reaped + } + // "pid (comm) STATE ..." — comm may contain spaces/parens, so the state is + // the character two positions after the final ')'. + s := string(data) + j := strings.LastIndexByte(s, ')') + if j < 0 || j+2 >= len(s) { + return true + } + switch s[j+2] { + case 'Z', 'X', 'x': // zombie / dead + return true + } + return false +} + +func TestPopenFinalizerReapsAbandonedProcess(t *testing.T) { + // A Process dropped without Wait/Close must not leak: the finalizer kills the + // child's process group and frees the handle. Capture the pid in a scope that + // drops the Process, then force GC and confirm the child is gone. + requireLandlock(t) + pid := func() int { + sb := &sandlock.Sandbox{FSReadable: rootfs} + p, err := sb.Popen(sandlock.Stdio{Stdout: sandlock.StdioPiped}, "sleep", "60") + if err != nil { + t.Fatalf("Popen: %v", err) + } + return p.Pid() // p becomes unreachable when this closure returns + }() + if pid <= 0 { + t.Fatalf("no pid, got %d", pid) + } + + deadline := time.Now().Add(10 * time.Second) + for { + runtime.GC() + if procDead(pid) { + return // finalizer reaped the abandoned child + } + if time.Now().After(deadline) { + t.Fatalf("abandoned Popen child (pid %d) was not reaped by the finalizer", pid) + } + time.Sleep(50 * time.Millisecond) + } +} + +func TestPopenCloseAfterWaitClosesLeftoverStream(t *testing.T) { + requireLandlock(t) + sb := &sandlock.Sandbox{FSReadable: rootfs} + p, err := sb.Popen(sandlock.Stdio{Stdout: sandlock.StdioPiped}, "echo", "leftover") + if err != nil { + t.Fatalf("Popen: %v", err) + } + stdout := p.Stdout + + // Wait without draining first (small output fits the pipe buffer); the stream + // stays open so the caller can still read the buffered output afterward. + if res, _ := p.Wait(); !res.Success { + t.Fatalf("want success, got %+v", res) + } + got, err := io.ReadAll(stdout) + if err != nil { + t.Fatalf("read after wait: %v", err) + } + if string(got) != "leftover\n" { + t.Fatalf("post-wait read = %q, want %q", got, "leftover\n") + } + + // Close after Wait must still release the leftover piped stream. + if err := p.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + if _, err := stdout.Read(make([]byte, 1)); err == nil { + t.Fatal("Close after Wait must close the leftover piped stream") + } +} + func TestSyscallNr(t *testing.T) { nr, err := sandlock.SyscallNr("openat") if err != nil { From 187bf510f5c1a62383901dc0799102c0050a6595 Mon Sep 17 00:00:00 2001 From: dzerik Date: Sat, 4 Jul 2026 12:21:27 +0300 Subject: [PATCH 2/2] Address review: align Kill idempotency, document Wait's Result, test zero Stdio MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Kill: a process already reaped by Wait now returns nil (like ESRCH), not ErrNotRunning — matching the FFI (sandlock_handle_kill returns 0) and the Python binding (no-op after wait()). Keeps the "Kill from another goroutine" escape hatch race-free when a Kill fires just as Wait reaps the child. Only the reaped case (h == nil) collapses to nil; a live handle with no pid stays an error, so a genuine no-op kill is never reported as success (and killpg(-0) is never reachable). - Wait doc + README: state that a Popen'd process's Result carries the exit status only; piped output was streamed to Stdout/Stderr and inherited/null output went to the parent fd or /dev/null, so (unlike Run) Result.Stdout/Result.Stderr are always empty. - Tests: zero-Stdio-equals-Spawn (all three streams inherited, no caller stream, child succeeds) pins the StdioInherit==0 zero-value contract; Kill-after-Wait-is-nil covers the idempotency alignment above. --- go/README.md | 5 ++++- go/sandlock_linux.go | 32 +++++++++++++++++++++++++---- go/sandlock_linux_test.go | 43 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 5 deletions(-) diff --git a/go/README.md b/go/README.md index aa066f3..1db6663 100644 --- a/go/README.md +++ b/go/README.md @@ -136,7 +136,10 @@ 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`. + 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 + (inherited/null streams go to the parent fd or `/dev/null`), so (unlike `Run`) + `Result.Stdout`/`Result.Stderr` are always empty. ```go p, _ := sb.Popen(sandlock.Stdio{Stdin: sandlock.StdioPiped, Stdout: sandlock.StdioPiped}, "cat") diff --git a/go/sandlock_linux.go b/go/sandlock_linux.go index 9190a11..b0b9a71 100644 --- a/go/sandlock_linux.go +++ b/go/sandlock_linux.go @@ -938,8 +938,14 @@ func (p *Process) Pid() int { return p.pid } -// Wait blocks until the process exits, returns its captured Result, and -// releases the handle. After Wait the Process is no longer running. +// 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 @@ -997,11 +1003,29 @@ func (p *Process) Pause() error { return p.signal(syscall.SIGSTOP) } // Resume sends SIGCONT to the sandbox process group. func (p *Process) Resume() error { return p.signal(syscall.SIGCONT) } -// Kill sends SIGKILL to the sandbox process group. +// 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 + 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 } diff --git a/go/sandlock_linux_test.go b/go/sandlock_linux_test.go index 8ea3adc..9529a19 100644 --- a/go/sandlock_linux_test.go +++ b/go/sandlock_linux_test.go @@ -356,6 +356,49 @@ func TestPopenNullStdoutNoStream(t *testing.T) { } } +func TestPopenZeroStdioInheritsAllLikeSpawn(t *testing.T) { + // The zero Stdio value must wire all three streams as StdioInherit (the ABI + // default StdioInherit == 0), identical to Spawn: no caller stream is handed + // back and the child runs to success. Pins the zero-value contract the type + // doc, the Popen doc, and the README all state but nothing exercised. + requireLandlock(t) + sb := &sandlock.Sandbox{FSReadable: rootfs} + p, err := sb.Popen(sandlock.Stdio{}, "true") + if err != nil { + t.Fatalf("Popen: %v", err) + } + defer p.Close() + if p.Stdin != nil || p.Stdout != nil || p.Stderr != nil { + t.Fatalf("zero Stdio must inherit all three; got streams stdin=%v stdout=%v stderr=%v", + p.Stdin != nil, p.Stdout != nil, p.Stderr != nil) + } + res, err := p.Wait() + if err != nil { + t.Fatalf("Wait: %v", err) + } + if !res.Success || res.ExitCode != 0 { + t.Fatalf("want success exit 0, got success=%v exit=%d", res.Success, res.ExitCode) + } +} + +func TestPopenKillAfterWaitIsNil(t *testing.T) { + // Kill on an already-reaped Process must be nil (not ErrNotRunning), matching + // the FFI (returns 0) and Python (no-op): a "Kill from another goroutine" + // firing just as Wait reaps the child must not surface a spurious error. + requireLandlock(t) + sb := &sandlock.Sandbox{FSReadable: rootfs} + p, err := sb.Popen(sandlock.Stdio{}, "true") + if err != nil { + t.Fatalf("Popen: %v", err) + } + if _, err := p.Wait(); err != nil { + t.Fatalf("Wait: %v", err) + } + if err := p.Kill(); err != nil { + t.Fatalf("Kill after Wait must return nil, got %v", err) + } +} + func TestPopenInvalidStdioMode(t *testing.T) { // An out-of-range discriminant is rejected before the child is spawned, so // this needs no Landlock.