From f1d44cc2a0ebd0d3cf8919a00104af37291d5042 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sun, 26 Jul 2026 11:42:04 +0200 Subject: [PATCH] fix(sandbox): kill in-container process group on command timeout When a sandboxed shell command timed out (or the turn was cancelled), only the host-side docker exec client was killed - Docker does not propagate the signal, so the in-container process kept running (CPU/memory, half-written files in /workspace) until the container was torn down at session end. Repeated timeouts accumulated runaway processes inside the container. shellTool and parallelShellTool now run sandboxed commands under a pid-marker wrapper (wrapSandboxCommand): the wrapper records the container-side pid of its group-leading shell in a per-invocation pidfile under /tmp, with the command passed as $1 (never interpolated, so quoting cannot break out). docker exec processes are their own process-group leaders (pgid == pid, verified empirically on alpine), so on timeout/cancel odek follows up with docker exec ... kill -KILL -, tearing down the command and every child it forked. Children that call setsid/setpgid escape the group - the follow-up is best-effort, not a hard guarantee. The "container restart after N stale kills" escalation from the report is deliberately not implemented: the group-kill follow-up addresses the leak directly without mid-session container churn. E2E tests (gated on ODEK_E2E + docker) prove no in-container survivors after shell and parallel_shell timeouts while the container init stays healthy; both fail against the pre-fix code. Unit tests updated for the new buildCmd signature and argv shape. --- AGENTS.md | 1 + cmd/odek/perf_tools.go | 11 +++++- cmd/odek/sandbox_e2e_test.go | 77 ++++++++++++++++++++++++++++++++++++ cmd/odek/shell.go | 70 ++++++++++++++++++++++++++------ cmd/odek/shell_test.go | 31 ++++++++++++--- 5 files changed, 172 insertions(+), 18 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index baec75b1..43d6882f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -174,6 +174,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU - **`~/.odek` trust-anchor protection** (`internal/danger/classifier.go`, `cmd/odek/file_tool.go`) — generic file tools reject writes to `~/.odek/config.json`, `secrets.env`, `IDENTITY.md`, `skills/`, `sessions/`, `audit/`, `plans/`, `schedules.json`, `schedule-state.json`, `mcp_approvals.json`, `mcp_tool_approvals.json`, `project_sandbox_approvals.json`, `restart.json`, `telegram.lock`, and related state files. These paths classify as `system_write` and must be modified through their dedicated subsystems. Matching is case-insensitive so variants such as `CONFIG.JSON` or `SECRETS.ENV` are also blocked on case-insensitive filesystems (e.g. macOS APFS). Shell reads of these trust anchors are also escalated to `system_write`. - **Nonce'd tool-result delimiter** (`internal/loop/loop.go`) — the static `┌── TOOL RESULT: ... └── END TOOL RESULT: ...` delimiter is now unique per tool call via a random hex nonce embedded in both the opening and closing lines. A tool or MCP server can no longer forge the closing delimiter to break out of the data framing and inject instructions. - **`parallel_shell` context + process-group kill** (`cmd/odek/perf_tools.go`) — commands now run via `exec.CommandContext` bound to the agent context, in their own process group. Cancellation or timeout kills the whole group (negative PID), so `sh -c 'sleep 3600 &'` cannot leave orphaned children. Per-command timeouts are also capped at 30 minutes. +- **In-container timeout kill** (`cmd/odek/shell.go::wrapSandboxCommand`) — killing the host-side `docker exec` client on timeout/cancel does not terminate the in-container process (Docker does not propagate the signal). Sandboxed `shell`/`parallel_shell` commands therefore run under a pid-marker wrapper that records the container-side group leader's pid in `/tmp/.odek-cmd-*.pid`; on cancellation odek follows up with `docker exec … kill -KILL -`, so a timed-out command cannot keep burning CPU/memory or leave half-written files until the container is torn down. Children that call setsid/setpgid escape the group — best-effort, not a guarantee. - **`batch_patch` trusted-class propagation** (`cmd/odek/perf_tools.go`) — `batch_patch` now passes its cached `trustedClasses` to `CheckOperation`, matching `write_file` and `patch`. A trusted `local_write` class is honored across all patches in the batch instead of re-prompting per patch. - **Browser link URL wrapping** (`cmd/odek/browser_tool.go`) — interactive element text was already wrapped as untrusted, but link URLs in `clickableRef.URL` were returned raw. They are now wrapped too, while an unexported `rawURL` is kept for internal click resolution. - **Browser post-redirect URL attribution** (`cmd/odek/browser_tool.go`) — `browser_navigate` now uses `resp.Request.URL` (the final post-redirect URL) for the snapshot URL, the untrusted-content source, and relative-link click resolution, instead of attributing content to the original requested URL. diff --git a/cmd/odek/perf_tools.go b/cmd/odek/perf_tools.go index a81e2956..60ec67d5 100644 --- a/cmd/odek/perf_tools.go +++ b/cmd/odek/perf_tools.go @@ -498,8 +498,11 @@ func (t *parallelShellTool) runOne(cmd parallelShellCmd) parallelShellEntry { defer cancel() var shCmd *exec.Cmd + var killInContainer func() if t.containerName != "" { - shCmd = exec.CommandContext(ctx, "docker", "exec", "-w", "/workspace", t.containerName, "sh", "-c", cmd.Command) + argv, followUp := wrapSandboxCommand(t.containerName, cmd.Command) + shCmd = exec.CommandContext(ctx, "docker", argv...) + killInContainer = followUp } else { shCmd = exec.CommandContext(ctx, "sh", "-c", cmd.Command) } @@ -521,6 +524,12 @@ func (t *parallelShellTool) runOne(cmd parallelShellCmd) parallelShellEntry { shCmd.WaitDelay = 3 * time.Second err := shCmd.Run() + // Killing the host-side `docker exec` client does not terminate the + // in-container process (Docker does not propagate the signal); kill its + // process group explicitly so a timed-out command cannot linger. + if ctx.Err() != nil && killInContainer != nil { + killInContainer() + } entry.Stdout = strings.TrimSpace(stdout.String()) entry.Stderr = strings.TrimSpace(stderr.String()) entry.DurationMs = time.Since(start).Milliseconds() diff --git a/cmd/odek/sandbox_e2e_test.go b/cmd/odek/sandbox_e2e_test.go index 86a82f4c..af4d28f8 100644 --- a/cmd/odek/sandbox_e2e_test.go +++ b/cmd/odek/sandbox_e2e_test.go @@ -218,3 +218,80 @@ func TestE2E_SandboxFileInjection_MultipleFiles(t *testing.T) { } } } + +// TestE2E_SandboxTimeoutKillsInContainerProcesses verifies finding 7's fix: +// when a sandboxed shell command times out, the in-container process group +// is killed — not just the host-side `docker exec` client. Before the fix, +// the in-container processes kept running until the container was destroyed. +func TestE2E_SandboxTimeoutKillsInContainerProcesses(t *testing.T) { + skipIfNoE2E(t) + + workDir := t.TempDir() + containerName := fmt.Sprintf("odek-test-timeout-%d", time.Now().UnixNano()) + args := sandbox.BuildRunArgs(sandboxConfig{ + Image: "alpine:latest", + Network: "none", + }, containerName, workDir, "alpine:latest") + createCmd := exec.Command("docker", args...) + createCmd.Stderr = os.Stderr + if err := createCmd.Run(); err != nil { + t.Fatalf("create container: %v", err) + } + defer exec.Command("docker", "rm", "-f", containerName).Run() + time.Sleep(500 * time.Millisecond) + + st := &shellTool{containerName: containerName, timeout: 1 * time.Second} + // Fork a background child AND block in the foreground: both must die. + _, err := st.Call(`{"command": "sleep 300 & sleep 300"}`) + if err == nil || !strings.Contains(err.Error(), "timed out") { + t.Fatalf("expected timeout error, got: %v", err) + } + + // The follow-up runs synchronously before Call returns. PID 1 is + // "sleep infinity" — assert no leftover "sleep 300" processes remain. + out, err := exec.Command("docker", "exec", containerName, "ps").CombinedOutput() + if err != nil { + t.Fatalf("docker exec ps: %v\n%s", err, out) + } + if strings.Contains(string(out), "sleep 300") { + t.Errorf("in-container processes survived the timeout:\n%s", out) + } + if !strings.Contains(string(out), "sleep infinity") { + t.Errorf("container init process missing — unexpected container state:\n%s", out) + } +} + +// TestE2E_SandboxParallelTimeoutKillsInContainerProcesses is the +// parallel_shell analogue: a per-command timeout must kill the in-container +// process group, not just the host-side client. +func TestE2E_SandboxParallelTimeoutKillsInContainerProcesses(t *testing.T) { + skipIfNoE2E(t) + + workDir := t.TempDir() + containerName := fmt.Sprintf("odek-test-ptimeout-%d", time.Now().UnixNano()) + args := sandbox.BuildRunArgs(sandboxConfig{ + Image: "alpine:latest", + Network: "none", + }, containerName, workDir, "alpine:latest") + createCmd := exec.Command("docker", args...) + createCmd.Stderr = os.Stderr + if err := createCmd.Run(); err != nil { + t.Fatalf("create container: %v", err) + } + defer exec.Command("docker", "rm", "-f", containerName).Run() + time.Sleep(500 * time.Millisecond) + + pt := ¶llelShellTool{containerName: containerName} + _, err := pt.Call(`{"commands": [{"command": "sleep 300 & sleep 300", "timeout": 1}]}`) + if err != nil { + t.Fatalf("parallel_shell Call: %v", err) + } + + out, err := exec.Command("docker", "exec", containerName, "ps").CombinedOutput() + if err != nil { + t.Fatalf("docker exec ps: %v\n%s", err, out) + } + if strings.Contains(string(out), "sleep 300") { + t.Errorf("in-container processes survived the parallel_shell timeout:\n%s", out) + } +} diff --git a/cmd/odek/shell.go b/cmd/odek/shell.go index 930c3f53..bacf12c4 100644 --- a/cmd/odek/shell.go +++ b/cmd/odek/shell.go @@ -5,9 +5,11 @@ import ( "context" "encoding/json" "fmt" + "os" "os/exec" "strings" "sync" + "sync/atomic" "syscall" "time" @@ -161,10 +163,10 @@ func (t *shellTool) Call(args string) (string, error) { // Bound execution: cancel with the agent context (Ctrl-C / turn timeout) // and a generous backstop timeout so a stuck command can never wedge the - // agent forever. Note: in sandbox mode this kills the host-side - // `docker exec` client, which unblocks the agent, but Docker does not - // propagate the signal to the in-container process — that lingers until the - // container is torn down at session end. + // agent forever. In sandbox mode this kills the host-side `docker exec` + // client, which unblocks the agent — but Docker does not propagate the + // signal to the in-container process, so buildCmd also returns a + // follow-up that kills the in-container process group explicitly. base := t.toolCtx() timeout := t.timeout if timeout <= 0 { @@ -173,7 +175,7 @@ func (t *shellTool) Call(args string) (string, error) { ctx, cancel := context.WithTimeout(base, timeout) defer cancel() - cmd := t.buildCmd(ctx, input.Command) + cmd, killInContainer := t.buildCmd(ctx, input.Command) // Run the command in its own process group and, on cancel/timeout, kill the // WHOLE group — not just the `sh` leader. `sh -c ""` may fork children // (e.g. `sleep`); killing only the leader leaves them alive holding the @@ -198,6 +200,15 @@ func (t *shellTool) Call(args string) (string, error) { err := cmd.Run() + // On timeout/cancel the host-side `docker exec` client was killed by the + // group kill above, but the in-container process survives it (Docker does + // not propagate the signal). Kill its process group explicitly so a + // timed-out command cannot keep burning CPU/memory or leave half-written + // files in /workspace until the container is torn down. + if ctx.Err() != nil && killInContainer != nil { + killInContainer() + } + // Surface cancellation/timeout as a clear, actionable error rather than an // opaque "signal: killed". if ctxErr := ctx.Err(); ctxErr != nil { @@ -283,15 +294,52 @@ func (t *shellTool) promptUser(cmd, description string) error { // buildCmd constructs the exec.Cmd for the given shell command. // // When sandbox mode is active (containerName is non-empty), the command -// is wrapped in "docker exec -w /workspace sh -c ". -// The -w /workspace flag ensures the command runs in the working directory -// that was mounted into the container during setupSandbox(). +// is wrapped in "docker exec -w /workspace " with a pid-marker +// wrapper (see wrapSandboxCommand), and the returned follow-up kills the +// in-container process group — call it when the command's context is +// cancelled or times out. The -w /workspace flag ensures the command runs +// in the working directory that was mounted into the container during +// setupSandbox(). The follow-up is nil in host mode. // // When running on the host (default), the command executes via "sh -c" // in odek's current working directory. -func (t *shellTool) buildCmd(ctx context.Context, command string) *exec.Cmd { +func (t *shellTool) buildCmd(ctx context.Context, command string) (*exec.Cmd, func()) { if t.containerName != "" { - return exec.CommandContext(ctx, "docker", "exec", "-w", "/workspace", t.containerName, "sh", "-c", command) + argv, followUp := wrapSandboxCommand(t.containerName, command) + return exec.CommandContext(ctx, "docker", argv...), followUp + } + return exec.CommandContext(ctx, "sh", "-c", command), nil +} + +// sandboxCmdSeq numbers sandboxed command invocations so each gets a unique +// pid-marker file inside the container. +var sandboxCmdSeq atomic.Uint64 + +// wrapSandboxCommand builds the "docker exec" argv that runs command inside +// the container with a pid-marker wrapper, plus a follow-up function that +// kills the in-container process group and removes the marker. +// +// Killing the host-side `docker exec` client on timeout/cancel does NOT +// terminate the in-container process — Docker does not propagate the signal +// — so without a follow-up, timed-out commands keep running (CPU/memory, +// half-written files in /workspace) until the container is destroyed. +// +// The wrapper records the container-side pid of the shell in a +// per-invocation pidfile. docker exec processes are their own process-group +// leaders (pgid == pid; verified on alpine and debian images), so +// signalling the negative pid tears down the command and every child it +// forked. Children that call setsid/setpgid themselves escape the group — +// this is best-effort, not a hard guarantee. The command string travels as +// a positional argument ($1), never interpolated into the wrapper, so +// quoting cannot break out of it. +func wrapSandboxCommand(containerName, command string) (argv []string, followUp func()) { + pidFile := fmt.Sprintf("/tmp/.odek-cmd-%d-%d.pid", os.Getpid(), sandboxCmdSeq.Add(1)) + wrapper := "echo $$ > " + pidFile + "; sh -c \"$1\"; rc=$?; rm -f " + pidFile + "; exit $rc" + argv = []string{"exec", "-w", "/workspace", containerName, "sh", "-c", wrapper, "odek-cmd", command} + followUp = func() { + // Best-effort: the container may already be gone (session cleanup). + _ = exec.Command("docker", "exec", containerName, "sh", "-c", + "kill -KILL -$(cat "+pidFile+") 2>/dev/null; rm -f "+pidFile).Run() } - return exec.CommandContext(ctx, "sh", "-c", command) + return argv, followUp } diff --git a/cmd/odek/shell_test.go b/cmd/odek/shell_test.go index 04e81cb9..6fea156b 100644 --- a/cmd/odek/shell_test.go +++ b/cmd/odek/shell_test.go @@ -189,20 +189,39 @@ func TestShellTool_Call_StdoutAndStderr(t *testing.T) { func TestShellTool_BuildCmd_Local(t *testing.T) { st := &shellTool{} - cmd := st.buildCmd(context.Background(), "echo test") + cmd, followUp := st.buildCmd(context.Background(), "echo test") args := cmd.Args if args[0] != "sh" || args[1] != "-c" || args[2] != "echo test" { t.Errorf("local cmd args = %v, want [sh -c 'echo test']", args) } + if followUp != nil { + t.Error("host mode must not return an in-container follow-up") + } } func TestShellTool_BuildCmd_Docker(t *testing.T) { st := &shellTool{containerName: "odek-12345"} - cmd := st.buildCmd(context.Background(), "echo test") + cmd, followUp := st.buildCmd(context.Background(), "echo test") args := cmd.Args - expected := []string{"docker", "exec", "-w", "/workspace", "odek-12345", "sh", "-c", "echo test"} - if !stringSlicesEqual(args, expected) { - t.Errorf("docker cmd args = %v, want %v", args, expected) + // Shape: docker exec -w /workspace sh -c odek-cmd + if len(args) != 10 { + t.Fatalf("docker cmd args = %v, want 10 elements", args) + } + if args[0] != "docker" || args[1] != "exec" || args[2] != "-w" || args[3] != "/workspace" || + args[4] != "odek-12345" || args[5] != "sh" || args[6] != "-c" || args[8] != "odek-cmd" || args[9] != "echo test" { + t.Errorf("docker cmd args = %v, unexpected shape", args) + } + // The wrapper records the group-leader pid in a marker file and the + // command travels as $1, never interpolated. + wrapper := args[7] + if !strings.Contains(wrapper, "echo $$ > /tmp/.odek-cmd-") || !strings.Contains(wrapper, `sh -c "$1"`) { + t.Errorf("wrapper = %q, want pid-marker + $1 dispatch", wrapper) + } + if strings.Contains(wrapper, "echo test") { + t.Errorf("wrapper = %q, command must not be interpolated into it", wrapper) + } + if followUp == nil { + t.Error("sandbox mode must return an in-container kill follow-up") } } @@ -362,7 +381,7 @@ func TestShellTool_CheckApproval(t *testing.T) { func TestShellTool_BuildCmd_Default(t *testing.T) { st := &shellTool{} - cmd := st.buildCmd(context.Background(), "echo hello") + cmd, _ := st.buildCmd(context.Background(), "echo hello") if cmd.Args[0] != "sh" { t.Errorf("expected sh, got %s", cmd.Args[0]) }