From ad637eebc172f97a81787998993cb1bdde8d2c86 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 12 Oct 2026 11:20:34 +0000 Subject: [PATCH] fix(shell): stop trickle output from bypassing the command timeout The shell and sandbox wait loops only advanced elapsed time when poll returned idle. A command that wrote faster than the poll slice never hit shell_timeout_sec or sandbox_exec's timeout, so the agent lock stayed held until the output buffer filled. Count monotonic wall time in both loops and still report the deadline when the child exits because the pipe was closed. Co-authored-by: esadrianno --- src/sandbox/sandbox.c | 85 ++++++++++++++++++++++++++++++++++++------- src/tools/shell.c | 53 ++++++++++++++++++++++----- tests/test_sandbox.c | 35 ++++++++++++++++++ tests/test_shell.c | 50 +++++++++++++++++++++++++ 4 files changed, 200 insertions(+), 23 deletions(-) diff --git a/src/sandbox/sandbox.c b/src/sandbox/sandbox.c index 4aac1a7..78bbdd4 100644 --- a/src/sandbox/sandbox.c +++ b/src/sandbox/sandbox.c @@ -107,36 +107,90 @@ static void cgroup_remove(const char *base, const char *name) #endif /* __linux__ */ -static size_t drain_pipe(int fd, char *buf, size_t cap, int timeout_ms) +/* Wall time since start, or -1 if the monotonic clock cannot be read. */ +static long monotonic_elapsed_ms(const struct timespec *start) { + struct timespec now; + long ms; + + if (clock_gettime(CLOCK_MONOTONIC, &now) != 0) + return -1; + ms = (now.tv_sec - start->tv_sec) * 1000L + + (now.tv_nsec - start->tv_nsec) / 1000000L; + return ms < 0 ? 0 : ms; +} + +static int poll_remain_ms(int timeout_ms, long elapsed_ms, int slice_cap) +{ + long remain; + + remain = (long)timeout_ms - elapsed_ms; + if (remain < 1) + return 1; + if (remain > slice_cap) + return slice_cap; + return (int)remain; +} + +/* + * Stop at timeout_ms of wall time, not idle poll time. A writer that keeps + * the pipe readable otherwise never advances the old counter, so the agent + * stays blocked until cap fills. + */ +static size_t drain_pipe(int fd, char *buf, size_t cap, int timeout_ms, int *hit_deadline) +{ + struct timespec start; size_t total = 0; - int elapsed_ms = 0; + int have_clock; + long elapsed_ms = 0; char chunk[512]; - if (cap == 0) return 0; - while (total < cap - 1 && elapsed_ms < timeout_ms) { + + if (hit_deadline) + *hit_deadline = 0; + if (cap == 0) + return 0; + have_clock = clock_gettime(CLOCK_MONOTONIC, &start) == 0; + while (total < cap - 1) { struct pollfd pfd; int slice; int r; ssize_t n; + long wall; + + if (have_clock) { + wall = monotonic_elapsed_ms(&start); + if (wall < 0) + have_clock = 0; + else + elapsed_ms = wall; + } + if (elapsed_ms >= (long)timeout_ms) { + if (hit_deadline) + *hit_deadline = 1; + break; + } + slice = poll_remain_ms(timeout_ms, elapsed_ms, PIPE_POLL_SLICE_MS); pfd.fd = fd; pfd.events = POLLIN; pfd.revents = 0; - slice = timeout_ms - elapsed_ms; - if (slice > PIPE_POLL_SLICE_MS) slice = PIPE_POLL_SLICE_MS; r = poll(&pfd, 1, slice); if (r < 0) { - if (errno == EINTR) continue; + if (errno == EINTR) + continue; break; } if (r == 0) { - elapsed_ms += slice; + if (!have_clock) + elapsed_ms += slice; continue; } n = read(fd, chunk, sizeof(chunk)); - if (n <= 0) break; + if (n <= 0) + break; { size_t add = (size_t)n; - if (total + add >= cap - 1) add = cap - 1 - total; + if (total + add >= cap - 1) + add = cap - 1 - total; memcpy(buf + total, chunk, add); total += add; } @@ -467,9 +521,14 @@ int sandbox_exec(const char *cmd, char *out, size_t out_cap, return report_isolation_failure(out, out_cap, iso); } } - total = drain_pipe(pipefd[0], out, out_cap, timeout_ms); - close(pipefd[0]); - timed_out = reap_child(pid, NULL); + { + int drain_deadline = 0; + total = drain_pipe(pipefd[0], out, out_cap, timeout_ms, &drain_deadline); + close(pipefd[0]); + timed_out = reap_child(pid, NULL); + if (drain_deadline) + timed_out = 1; + } if (timed_out && total < out_cap - 40) snprintf(out + total, out_cap - total, "\n[Sandbox: command timed out after %d ms]", timeout_ms); diff --git a/src/tools/shell.c b/src/tools/shell.c index 65a2d97..65b8c78 100644 --- a/src/tools/shell.c +++ b/src/tools/shell.c @@ -66,6 +66,19 @@ static void kill_command_tree(pid_t pid) (void)kill(pid, SIGKILL); } +/* Wall time since start, or -1 if the monotonic clock cannot be read. */ +static long monotonic_elapsed_ms(const struct timespec *start) +{ + struct timespec now; + long ms; + + if (clock_gettime(CLOCK_MONOTONIC, &now) != 0) + return -1; + ms = (now.tv_sec - start->tv_sec) * 1000L + + (now.tv_nsec - start->tv_nsec) / 1000000L; + return ms < 0 ? 0 : ms; +} + /* Always SIGKILL the process group first so background grandchildren cannot * leak after the shell has already exited (SIGPIPE, or `cmd &`). Then reap * the tracked pid. Matching the hang fix for #69 / #96. */ @@ -115,7 +128,9 @@ static int run_unsandboxed(const char *command, int timeout_sec, pid_t pid; size_t total = 0; int timed_out = 0; - int elapsed_ms = 0; + long elapsed_ms = 0; + struct timespec start_ts; + int have_clock; char buf[256]; if (pipe(pipefd) != 0) { snprintf(result_buf, max_len, "{\"error\":\"pipe failed\"}"); @@ -140,28 +155,46 @@ static int run_unsandboxed(const char *command, int timeout_sec, (void)setpgid(pid, pid); close(pipefd[1]); result_buf[0] = '\0'; - while (total < max_len - 1 && elapsed_ms < timeout_sec * 1000) { + have_clock = clock_gettime(CLOCK_MONOTONIC, &start_ts) == 0; + /* Wall time, not idle poll time: a steady writer never used to trip + * the timeout, so the agent lock stayed held until the buffer filled. */ + while (total < max_len - 1) { struct pollfd pfd; + long remain; int rem; int r; ssize_t n; + long wall; + if (have_clock) { + wall = monotonic_elapsed_ms(&start_ts); + if (wall < 0) + have_clock = 0; + else + elapsed_ms = wall; + } + if (elapsed_ms >= (long)timeout_sec * 1000L) { + timed_out = 1; + kill_command_tree(pid); + break; + } pfd.fd = pipefd[0]; pfd.events = POLLIN; pfd.revents = 0; - rem = timeout_sec * 1000 - elapsed_ms; - if (rem > 5000) rem = 5000; + remain = (long)timeout_sec * 1000L - elapsed_ms; + if (remain > 5000) + rem = 5000; + else if (remain < 1) + rem = 1; + else + rem = (int)remain; r = poll(&pfd, 1, rem); if (r < 0) { if (errno == EINTR) continue; break; } if (r == 0) { - elapsed_ms += rem; - if (elapsed_ms >= timeout_sec * 1000) { - timed_out = 1; - kill_command_tree(pid); - break; - } + if (!have_clock) + elapsed_ms += rem; continue; } n = read(pipefd[0], buf, sizeof(buf)); diff --git a/tests/test_sandbox.c b/tests/test_sandbox.c index 45fa2d0..daa7e9c 100644 --- a/tests/test_sandbox.c +++ b/tests/test_sandbox.c @@ -19,6 +19,7 @@ #include "sandbox/sandbox_landlock.h" #include #include +#include #include #include #include @@ -277,6 +278,39 @@ static int test_missing_workspace_fail_closed(void) return 0; } +static void trickle_timeout_watchdog(int sig) +{ + (void)sig; + fprintf(stderr, "FAIL: sandbox timeout did not fire while stdout trickled\n"); + _exit(2); +} + +static int test_timeout_kills_trickle_output(void) +{ + char out[8192]; + struct timespec t0; + struct timespec t1; + long elapsed_ms; + int rc; + + ASSERT(clock_gettime(CLOCK_MONOTONIC, &t0) == 0); + signal(SIGALRM, trickle_timeout_watchdog); + alarm(4); + rc = sandbox_exec("while true; do echo x; sleep 0.05; done", + out, sizeof(out), 400, NULL); + alarm(0); + signal(SIGALRM, SIG_DFL); + if (skip_if_isolation_denied(rc, out, "test_timeout_kills_trickle_output")) + return 0; + ASSERT(clock_gettime(CLOCK_MONOTONIC, &t1) == 0); + elapsed_ms = (t1.tv_sec - t0.tv_sec) * 1000L + + (t1.tv_nsec - t0.tv_nsec) / 1000000L; + ASSERT(rc == 0); + ASSERT(strstr(out, "timed out") != NULL); + ASSERT(elapsed_ms < 2500); + return 0; +} + static int test_timeout_kills_process(void) { char out[4096]; @@ -832,6 +866,7 @@ int main(void) RUN(test_dev_null_is_writable()); RUN(test_workspace_chdir()); RUN(test_missing_workspace_fail_closed()); + RUN(test_timeout_kills_trickle_output()); RUN(test_timeout_kills_process()); #ifdef __linux__ RUN(test_landlock_probe_without_restrict()); diff --git a/tests/test_shell.c b/tests/test_shell.c index a37341b..a9bafc9 100644 --- a/tests/test_shell.c +++ b/tests/test_shell.c @@ -281,6 +281,55 @@ static void test_shell_caps_output_without_hanging(void) "sequential sleep 9999 did not leak"); } +static void trickle_timeout_watchdog(int sig) +{ + (void)sig; + fprintf(stderr, "FAIL: shell timeout did not fire while stdout trickled\n"); + _exit(2); +} + +/* Idle-only poll accounting used to ignore shell_timeout_sec while the + * child kept the pipe readable. The agent lock stayed held until the + * output buffer filled. */ +static void test_shell_trickle_output_hits_timeout(void) +{ + const tool_t *t = tool_shell_get(); + config_t *cfg = NULL; + char errbuf[256]; + char cfg_path[64]; + char buf[8192]; + FILE *f; + struct timespec t0; + struct timespec t1; + long elapsed_ms; + int r; + + snprintf(cfg_path, sizeof(cfg_path), "/tmp/sc_shell_to_%d.toml", (int)getpid()); + f = fopen(cfg_path, "w"); + MU_ASSERT(f != NULL, "write timeout config"); + fputs("[agent]\nmodel = \"test\"\n[sandbox]\nenabled = false\nshell_timeout_sec = 1\n", f); + fclose(f); + MU_ASSERT(config_load(cfg_path, &cfg, errbuf, sizeof(errbuf)) == 0, "load timeout config"); + tool_shell_set_config(cfg); + memset(buf, 0, sizeof(buf)); + signal(SIGALRM, trickle_timeout_watchdog); + alarm(4); + MU_ASSERT(clock_gettime(CLOCK_MONOTONIC, &t0) == 0, "clock start"); + r = t->execute("{\"command\":\"while true; do echo x; sleep 0.05; done\"}", + buf, sizeof(buf)); + alarm(0); + signal(SIGALRM, SIG_DFL); + MU_ASSERT(clock_gettime(CLOCK_MONOTONIC, &t1) == 0, "clock end"); + elapsed_ms = (t1.tv_sec - t0.tv_sec) * 1000L + + (t1.tv_nsec - t0.tv_nsec) / 1000000L; + tool_shell_set_config(NULL); + config_free(cfg); + unlink(cfg_path); + MU_ASSERT(r == 0, "trickle command returns"); + MU_ASSERT(strstr(buf, "timed out") != NULL, "trickle command reports timeout"); + MU_ASSERT(elapsed_ms < 3000, "trickle command stopped near the 1s timeout"); +} + static void test_shell_caps_output_kills_background_sleep(void) { const tool_t *t = tool_shell_get(); @@ -317,6 +366,7 @@ int main(void) MU_RUN(test_shell_invalid_json); MU_RUN(test_shell_missing_command); MU_RUN(test_shell_caps_output_without_hanging); + MU_RUN(test_shell_trickle_output_hits_timeout); MU_RUN(test_shell_caps_output_kills_background_sleep); printf("%d tests run, %d failed\n", tests_run, tests_failed); return tests_failed ? 1 : 0;