From b5d543cdb5d48475ead130de44a24661792cdcf7 Mon Sep 17 00:00:00 2001 From: Adrianno Esnarriaga Sereno Date: Mon, 21 Sep 2026 12:49:46 -0300 Subject: [PATCH 1/3] fix(tools): stop unsandboxed shell hang after output cap When sandbox is off, filling the result buffer left the drain loop and blocked forever in waitpid, freezing the agent. Truncated copies also skipped the NUL terminator. Reap leftover children like sandbox_exec, kill the process group, and always NUL-terminate the capture buffer. Refs: #69 --- src/tools/shell.c | 67 +++++++++++++++++++++++++++++++++++++--------- tests/test_shell.c | 42 +++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 12 deletions(-) diff --git a/src/tools/shell.c b/src/tools/shell.c index 89f125d..46aac41 100644 --- a/src/tools/shell.c +++ b/src/tools/shell.c @@ -19,12 +19,14 @@ #include "sandbox/sandbox.h" #include "sandbox/allowlist.h" #include "cJSON.h" +#include #include #include #include #include #include #include +#include #include #define DEFAULT_TIMEOUT_SEC 60 @@ -56,6 +58,47 @@ static int fallback_is_blocked(const char *cmd) static const config_t *g_shell_cfg; +static void kill_command_tree(pid_t pid) +{ + if (kill(-pid, SIGKILL) != 0) + (void)kill(pid, SIGKILL); +} + +/* Kill leftover children after drain (timeout or output cap), matching sandbox_exec. */ +static int reap_running_child(pid_t pid) +{ + int status = 0; + int wr = waitpid(pid, &status, WNOHANG); + int retries; + struct timespec ts; + if (wr != 0) + return 0; + kill_command_tree(pid); + for (retries = 0; retries < 40; retries++) { + wr = waitpid(pid, &status, WNOHANG); + if (wr != 0) + return 1; + ts.tv_sec = 0; + ts.tv_nsec = 50 * 1000 * 1000; + (void)nanosleep(&ts, NULL); + } + (void)waitpid(pid, &status, 0); + return 1; +} + +static void append_unsandboxed_output(char *result_buf, size_t max_len, size_t *total, + const char *chunk, size_t n) +{ + size_t add = n; + if (*total + add >= max_len - 1) + add = max_len - 1 - *total; + if (add == 0) + return; + memcpy(result_buf + *total, chunk, add); + *total += add; + result_buf[*total] = '\0'; +} + /* ------------------------------------------------------------------ */ /* Unsandboxed execution (fork + poll + waitpid) */ /* ------------------------------------------------------------------ */ @@ -68,7 +111,6 @@ static int run_unsandboxed(const char *command, int timeout_sec, size_t total = 0; int timed_out = 0; int elapsed_ms = 0; - int status; char buf[256]; if (pipe(pipefd) != 0) { snprintf(result_buf, max_len, "{\"error\":\"pipe failed\"}"); @@ -86,9 +128,11 @@ static int run_unsandboxed(const char *command, int timeout_sec, dup2(pipefd[1], STDOUT_FILENO); dup2(pipefd[1], STDERR_FILENO); close(pipefd[1]); + (void)setpgid(0, 0); execl("/bin/sh", "sh", "-c", command, (char *)NULL); _exit(127); } + (void)setpgid(pid, pid); close(pipefd[1]); result_buf[0] = '\0'; while (total < max_len - 1 && elapsed_ms < timeout_sec * 1000) { @@ -102,28 +146,27 @@ static int run_unsandboxed(const char *command, int timeout_sec, rem = timeout_sec * 1000 - elapsed_ms; if (rem > 5000) rem = 5000; r = poll(&pfd, 1, rem); - if (r < 0) break; + if (r < 0) { + if (errno == EINTR) continue; + break; + } if (r == 0) { elapsed_ms += rem; if (elapsed_ms >= timeout_sec * 1000) { timed_out = 1; - kill(pid, SIGKILL); + kill_command_tree(pid); break; } continue; } - n = read(pipefd[0], buf, sizeof(buf) - 1); + n = read(pipefd[0], buf, sizeof(buf)); if (n <= 0) break; - buf[n] = '\0'; - { - size_t add = (size_t)n; - if (total + add >= max_len - 1) add = max_len - 1 - total; - memcpy(result_buf + total, buf, add + 1); - total += add; - } + append_unsandboxed_output(result_buf, max_len, &total, buf, (size_t)n); } + result_buf[total] = '\0'; close(pipefd[0]); - waitpid(pid, &status, 0); + if (reap_running_child(pid)) + timed_out = 1; if (timed_out && total < max_len - 32) snprintf(result_buf + total, max_len - total, "\n[Command timed out]"); return 0; diff --git a/tests/test_shell.c b/tests/test_shell.c index 8b43d94..0687664 100644 --- a/tests/test_shell.c +++ b/tests/test_shell.c @@ -6,8 +6,11 @@ #include "tools/tool.h" #include "tools/shell.h" #include "core/config.h" +#include #include +#include #include +#include static int tests_run = 0; static int tests_failed = 0; @@ -68,6 +71,44 @@ static void test_shell_missing_command(void) MU_ASSERT(r == -1, "missing command returns -1"); } +static void output_cap_hang_watchdog(int sig) +{ + (void)sig; + fprintf(stderr, "FAIL: unsandboxed shell hung after filling the output cap\n"); + _exit(2); +} + +static int buf_has_nul(const char *buf, size_t n) +{ + size_t i; + for (i = 0; i < n; i++) { + if (buf[i] == '\0') + return 1; + } + return 0; +} + +static void test_shell_caps_output_without_hanging(void) +{ + const tool_t *t = tool_shell_get(); + char buf[64]; + int r; + tool_shell_set_config(NULL); + memset(buf, 'B', sizeof(buf)); + signal(SIGALRM, output_cap_hang_watchdog); + alarm(5); + /* Fill the 64-byte cap, then sleep so the child stays alive without + * writing (SIGPIPE will not reap it). Unsandboxed waitpid used to block + * forever on this path (#69). + */ + r = t->execute("{\"command\":\"printf '%080d' 0; sleep 9999\"}", buf, + sizeof(buf)); + alarm(0); + signal(SIGALRM, SIG_DFL); + MU_ASSERT(r == 0, "capped shell command returns"); + MU_ASSERT(buf_has_nul(buf, sizeof(buf)), "capped output is NUL-terminated"); +} + int main(void) { MU_RUN(test_shell_blocked_rm_rf); @@ -75,6 +116,7 @@ int main(void) MU_RUN(test_shell_ls_succeeds); MU_RUN(test_shell_invalid_json); MU_RUN(test_shell_missing_command); + MU_RUN(test_shell_caps_output_without_hanging); printf("%d tests run, %d failed\n", tests_run, tests_failed); return tests_failed ? 1 : 0; } From 76a763a3ce49aa8f69090bf555fb36b1ca612e22 Mon Sep 17 00:00:00 2001 From: Adrianno Esnarriaga Sereno Date: Mon, 21 Sep 2026 13:32:50 -0300 Subject: [PATCH 2/3] fix(tools): kill unsandboxed shell process group before wait waitpid(WNOHANG) skipping kill_command_tree leaked background grandchildren after the shell exited. Signal the group first, assert no leftover sleep argv, and record the hang/NUL fix in CHANGELOG. Refs: #96 --- CHANGELOG.md | 1 + src/tools/shell.c | 9 ++- tests/test_shell.c | 156 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 163 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cc2e71b..c2a9eb4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to ShellClaw are documented here. Format follows [Keep a Cha ## [Unreleased] ### Fixed +- Unsandboxed `shell` no longer blocks forever in `waitpid` after the output cap fills; leftover children (including background grandchildren) are SIGKILL'd via the command process group, and truncated capture is NUL-terminated (#69). - `write_file` maps to the intended path instead of the first existing ancestor, so a nested path cannot truncate a workspace file treated as a directory or overwrite a same-named file in a parent (#67). Leaf workspace symlinks (dangling or an in-workspace alias) are rejected (`lstat` + `O_NOFOLLOW`) instead of creating host files outside the workspace (#90). - `write_file` persists via unique temp (`mkstemp`)+fsync+rename so a failed write cannot wipe an existing workspace file and a sibling `path.tmp` is not truncated (#78). - Camera capture fails closed when `workspace_only` is on with an empty `workspace_path`, and rejects leaf symlink outputs (#91, #90). diff --git a/src/tools/shell.c b/src/tools/shell.c index 46aac41..5638e59 100644 --- a/src/tools/shell.c +++ b/src/tools/shell.c @@ -64,16 +64,19 @@ static void kill_command_tree(pid_t pid) (void)kill(pid, SIGKILL); } -/* Kill leftover children after drain (timeout or output cap), matching sandbox_exec. */ +/* 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. */ static int reap_running_child(pid_t pid) { int status = 0; - int wr = waitpid(pid, &status, WNOHANG); + int wr; int retries; struct timespec ts; + kill_command_tree(pid); + wr = waitpid(pid, &status, WNOHANG); if (wr != 0) return 0; - kill_command_tree(pid); for (retries = 0; retries < 40; retries++) { wr = waitpid(pid, &status, WNOHANG); if (wr != 0) diff --git a/tests/test_shell.c b/tests/test_shell.c index 0687664..97c1fd4 100644 --- a/tests/test_shell.c +++ b/tests/test_shell.c @@ -6,10 +6,14 @@ #include "tools/tool.h" #include "tools/shell.h" #include "core/config.h" +#include +#include +#include #include #include #include #include +#include #include static int tests_run = 0; @@ -88,13 +92,134 @@ static int buf_has_nul(const char *buf, size_t n) return 0; } +static int argv0_is_sleep(const char *arg0) +{ + const char *base; + if (!arg0 || !arg0[0]) + return 0; + base = strrchr(arg0, '/'); + base = base ? base + 1 : arg0; + return strcmp(base, "sleep") == 0; +} + +static int cmdline_is_sleep_marker(const char *buf, size_t n, const char *marker) +{ + size_t i = 0; + const char *arg1; + if (n == 0 || !marker) + return 0; + while (i < n && buf[i] != '\0') + i++; + if (i >= n || i + 1 >= n) + return 0; + if (!argv0_is_sleep(buf)) + return 0; + arg1 = buf + i + 1; + return strcmp(arg1, marker) == 0; +} + +static int count_sleep_argv_proc(const char *marker) +{ + DIR *dir; + struct dirent *ent; + int count = 0; + dir = opendir("/proc"); + if (!dir) + return -1; + while ((ent = readdir(dir)) != NULL) { + char path[64]; + char buf[256]; + ssize_t n; + int fd; + if (!isdigit((unsigned char)ent->d_name[0])) + continue; + if (snprintf(path, sizeof(path), "/proc/%s/cmdline", ent->d_name) + >= (int)sizeof(path)) + continue; + fd = open(path, O_RDONLY); + if (fd < 0) + continue; + n = read(fd, buf, sizeof(buf) - 1); + close(fd); + if (n <= 0) + continue; + buf[n] = '\0'; + if (cmdline_is_sleep_marker(buf, (size_t)n + 1, marker)) + count++; + } + closedir(dir); + return count; +} + +static int ps_line_is_sleep_marker(char *line, const char *marker) +{ + char *s = line; + char *nl; + char *base; + size_t marker_len; + nl = strchr(line, '\n'); + if (nl) + *nl = '\0'; + while (*s == ' ' || *s == '\t') + s++; + base = strrchr(s, '/'); + base = base ? base + 1 : s; + marker_len = strlen(marker); + if (strncmp(base, "sleep ", 6) != 0) + return 0; + return strcmp(base + 6, marker) == 0 && marker_len > 0; +} + +static int count_sleep_argv_ps(const char *marker) +{ + FILE *fp; + char line[256]; + int count = 0; + fp = popen("ps -axo args=", "r"); + if (!fp) + return -1; + while (fgets(line, sizeof(line), fp) != NULL) { + if (ps_line_is_sleep_marker(line, marker)) + count++; + } + (void)pclose(fp); + return count; +} + +/* Linux CI: /proc cmdline. Darwin (no /proc): ps args. argv0 must be sleep. */ +static int count_sleep_argv(const char *marker) +{ + int n = count_sleep_argv_proc(marker); + if (n >= 0) + return n; + return count_sleep_argv_ps(marker); +} + +static int sleep_argv_did_not_grow(const char *marker, int before) +{ + struct timespec ts; + int i; + for (i = 0; i < 20; i++) { + int now = count_sleep_argv(marker); + if (now >= 0 && now <= before) + return 1; + ts.tv_sec = 0; + ts.tv_nsec = 50 * 1000 * 1000; + (void)nanosleep(&ts, NULL); + } + return 0; +} + static void test_shell_caps_output_without_hanging(void) { const tool_t *t = tool_shell_get(); char buf[64]; int r; + int before; tool_shell_set_config(NULL); memset(buf, 'B', sizeof(buf)); + before = count_sleep_argv("9999"); + MU_ASSERT(before >= 0, "can count sleep 9999 processes"); signal(SIGALRM, output_cap_hang_watchdog); alarm(5); /* Fill the 64-byte cap, then sleep so the child stays alive without @@ -106,7 +231,37 @@ static void test_shell_caps_output_without_hanging(void) alarm(0); signal(SIGALRM, SIG_DFL); MU_ASSERT(r == 0, "capped shell command returns"); + MU_ASSERT(buf[0] == '0', "capped output starts with truncated zeros"); MU_ASSERT(buf_has_nul(buf, sizeof(buf)), "capped output is NUL-terminated"); + MU_ASSERT(sleep_argv_did_not_grow("9999", before), + "sequential sleep 9999 did not leak"); +} + +static void test_shell_caps_output_kills_background_sleep(void) +{ + const tool_t *t = tool_shell_get(); + char buf[64]; + int r; + int before; + tool_shell_set_config(NULL); + memset(buf, 'B', sizeof(buf)); + before = count_sleep_argv("9998"); + MU_ASSERT(before >= 0, "can count sleep 9998 processes"); + signal(SIGALRM, output_cap_hang_watchdog); + alarm(5); + /* Shell can exit after printf while the background sleep stays in the + * process group. Skipping kill_command_tree when waitpid already reaped + * the shell leaked that grandchild (#96). + */ + r = t->execute("{\"command\":\"trap '' HUP; sleep 9998 & printf '%080d' 0\"}", + buf, sizeof(buf)); + alarm(0); + signal(SIGALRM, SIG_DFL); + MU_ASSERT(r == 0, "background capped shell command returns"); + MU_ASSERT(buf[0] == '0', "background capped output is truncated zeros"); + MU_ASSERT(buf_has_nul(buf, sizeof(buf)), "background capped output is NUL-terminated"); + MU_ASSERT(sleep_argv_did_not_grow("9998", before), + "background sleep 9998 did not leak"); } int main(void) @@ -117,6 +272,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_caps_output_kills_background_sleep); printf("%d tests run, %d failed\n", tests_run, tests_failed); return tests_failed ? 1 : 0; } From 7db8f6b68ee2aa960c7adf129325d00a13870502 Mon Sep 17 00:00:00 2001 From: Adrianno Esnarriaga Sereno Date: Mon, 21 Sep 2026 13:32:50 -0300 Subject: [PATCH 3/3] test(tools): expose POSIX popen and nanosleep in test_shell Linux glibc + -Werror hid popen/pclose/nanosleep without a feature test macro, so CI failed to compile the leftover-sleep scanner. Refs: #96 --- tests/test_shell.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_shell.c b/tests/test_shell.c index 97c1fd4..ceb9551 100644 --- a/tests/test_shell.c +++ b/tests/test_shell.c @@ -2,6 +2,7 @@ * @file test_shell.c * @brief Unit tests for shell tool: safe commands, blocklist, timeout. */ +#define _POSIX_C_SOURCE 200809L #include "tools/tool.h" #include "tools/shell.h"