From cb25615849484caf6c7e64b6b8b4578afa0e090a Mon Sep 17 00:00:00 2001 From: Daniel JB Clark Date: Sun, 16 Aug 2026 19:23:06 -0400 Subject: [PATCH 1/9] Signal the whole process group when a command times out exec_timeout fires, GracefulTerminate() signals the process we started, and anything that process spawned survives it. The survivor still holds the write end of the pipe, so the parent stays blocked in its read loop on a command it has already given up on, and exec_timeout does not bound the promise's wall clock at all. Reproduced with a shell payload, which is the ordinary case since any script that runs another program has this shape: body contain c { useshell => "noshell"; exec_timeout => "2"; } bundle agent t { commands: "/bin/sh" arglist => { "-c", "sleep 30; exit 0" }, contain => c; } Before: 30.3s. The sh is terminated on schedule but the sleep is orphaned and runs to completion holding the pipe. After: 5.2s, and no orphan is left behind. With a payload that also traps INT and TERM (`trap '' INT TERM; sleep 30`), 30.3s becomes 4.4s. cf_popen()'s child now calls setpgid(0, 0) so that a timed-out command and its descendants can be signalled as a unit. setpgid() is async-signal-safe, so it is legal in the post-fork child alongside the existing sigprocmask() call. TimeOut() reads the process group *before* calling GracefulTerminate() and sweeps it after. Reading it first is not cosmetic: once the process has been killed, getpgid() fails with ESRCH and there is no safe way left to tell whether it led a group of its own. The sweep is guarded on pgid == pid precisely so that a child which somehow is not a group leader cannot cause a negative kill() against our own group. Two things a reviewer should weigh, neither of which I am certain about: 1. setpgid() applies to every cf_popen() child, not just those with an exec_timeout. That detaches children from the agent's process group, so a terminal SIGINT (Ctrl-C) sent to the foreground group no longer reaches a running child. For non-interactive agent runs this is invisible, but it is a real behaviour change for interactive use, and the alternative -- setting the process group only when an exec_timeout is present -- would make the timeout path structurally different from the normal one. 2. The sweep is an unconditional SIGKILL to the group, after the leader has already been through the graceful ladder. Escalating over the group instead would be gentler but would change GracefulTerminate(), which is shared with the stale-lock path where group semantics are wrong. Tests: tests/unit/process_terminate_unix_test 6/6. Full unit suite 64 PASS, exit 0, and the FAIL/ERROR/XFAIL set is identical to before this change -- the pre-existing process_test XFAILs are the Darwin process stub, unrelated. Verified separately that argv fidelity through commands: arglist is unaffected, and that a command with no timeout still runs normally. Changelog: Title --- libpromises/pipes_unix.c | 7 +++++++ libpromises/timeout.c | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/libpromises/pipes_unix.c b/libpromises/pipes_unix.c index a9aece3778..eb1ad57796 100644 --- a/libpromises/pipes_unix.c +++ b/libpromises/pipes_unix.c @@ -236,6 +236,13 @@ static pid_t GenericCreatePipeAndFork(IOPipe *pipes) sigset_t sigmask; sigemptyset(&sigmask); sigprocmask(SIG_SETMASK, &sigmask, NULL); + + /* Lead a new process group, so that anything the command spawns can be + * signalled as a unit. Without this only the direct child is reachable, + * and a grandchild outlives exec_timeout still holding the pipe open, + * which leaves the parent blocked reading it. setpgid() is + * async-signal-safe, so it is legal here. */ + setpgid(0, 0); } ALARM_PID = (pid != 0 ? pid : -1); diff --git a/libpromises/timeout.c b/libpromises/timeout.c index 5ad63baf85..28bdded07e 100644 --- a/libpromises/timeout.c +++ b/libpromises/timeout.c @@ -42,7 +42,27 @@ void TimeOut() if (ALARM_PID != -1) { Log(LOG_LEVEL_VERBOSE, "Time out of process %jd", (intmax_t)ALARM_PID); + + /* Read the process group while the process is still alive to be read: + * once GracefulTerminate() has killed it, getpgid() fails with ESRCH and + * we would have no safe way to tell whether it led a group of its own. */ + const pid_t pgid = getpgid(ALARM_PID); + GracefulTerminate(ALARM_PID, PROCESS_START_TIME_UNKNOWN); + + /* GracefulTerminate() only reaches the process we started. Anything that + * process spawned survives it, and keeps the pipe open, so the caller + * stays blocked reading a command it has already given up on. + * + * Guarded on the timed-out process leading its own group, which + * cf_popen()'s child arranges with setpgid(). If that did not take + * effect the process is still in our group, its pgid is not its pid, and + * a negative kill() here would signal an unrelated group -- possibly our + * own. */ + if (pgid == ALARM_PID) + { + kill(-ALARM_PID, SIGKILL); + } } else { From 847373cf6223846dc2a4f938da1fd61f0ccdea4f Mon Sep 17 00:00:00 2001 From: Daniel JB Clark Date: Sun, 16 Aug 2026 20:09:50 -0400 Subject: [PATCH 2/9] Only give a timed-out command its own process group Follow-up to the parent commit, from an independent review of it. The parent called setpgid(0, 0) in every cf_popen() child. That is wrong, and the reason given for it -- keeping the timeout path structurally identical to the normal path -- had it backwards. The timeout path is different: it is the only one that has to kill a tree. The normal path has to stay reachable by the things that already kill trees. GenericCreatePipeAndFork() is the single fork behind cf_popen, cf_popen_select, cf_popensetuid, cf_popen_sh, cf_popen_sh_select, cf_popen_shsetuid and cf_popen_full_duplex, so the parent commit moved every child of every one of those out of the agent's process group. Three consequences, none of them wanted: - A child in its own process group is no longer in the terminal's foreground group, so on an interactive run it is stopped by SIGTTIN the first time it reads the terminal. Type-'r' pipes do not redirect stdin, so the child inherits the agent's. The agent then blocks forever reading a pipe held by a stopped child. Measured under a pty with `sh -c 'read x; echo GOT-$x'` and no exec_timeout: before this commit the agent hung indefinitely with the child in state T and its pgid equal to its own pid, where stock finishes in 0.1s. That is the same unbounded hang the parent commit exists to remove, reintroduced on a path that has no timeout to end it. - A terminal SIGINT reaches only the foreground group. cf-agent's handler exits immediately, so Ctrl-C used to take its children with it and now orphans them. - cf-execd's agent_expireafter kills by process group (cf-execd-runner.c). Children detached from that group survive the watchdog -- which exists for precisely the hung command this series is about. So the process group is now created only when a timeout is armed, which is exactly when TimeOut() may need to sweep it. SetTimeOut() records that, ClearTimeOut() and TimeOut() clear it. The four callers that open-coded `alarm(0); signal(SIGALRM, SIG_DFL);` call ClearTimeOut() instead, so that a command completing in time cannot leave the flag set for the next, unrelated, child; those are the only SetTimeOut() users in the tree (verify_exec.c, nfs.c, cf-monitord/history.c). Tests: with a timeout, the documented repro is unchanged -- `sh -c 'sleep 30; exit 0'` under exec_timeout => "2" bounded at 10.8s against 30.3s before the series, no orphaned sleep. Without a timeout, the pty case above now completes normally instead of hanging. Changelog: Title --- cf-agent/nfs.c | 6 ++---- cf-agent/verify_exec.c | 3 +-- cf-monitord/history.c | 3 +-- libpromises/pipes_unix.c | 26 ++++++++++++++++++++------ libpromises/timeout.c | 19 +++++++++++++++++++ libpromises/timeout.h | 10 ++++++++++ 6 files changed, 53 insertions(+), 14 deletions(-) diff --git a/cf-agent/nfs.c b/cf-agent/nfs.c index da8b5cd78a..6d2810d343 100644 --- a/cf-agent/nfs.c +++ b/cf-agent/nfs.c @@ -578,8 +578,7 @@ bool LoadMountInfo(Seq *list) } free(vbuff); - alarm(0); - signal(SIGALRM, SIG_DFL); + ClearTimeOut(); cf_pclose(pp); return true; } @@ -1175,8 +1174,7 @@ void MountAll() } free(line); - alarm(0); - signal(SIGALRM, SIG_DFL); + ClearTimeOut(); cf_pclose(pp); } diff --git a/cf-agent/verify_exec.c b/cf-agent/verify_exec.c index 15c74d5fee..6ad6c2b0b4 100644 --- a/cf-agent/verify_exec.c +++ b/cf-agent/verify_exec.c @@ -472,8 +472,7 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a, if (a->contain.timeout != CF_NOINT) { - alarm(0); - signal(SIGALRM, SIG_DFL); + ClearTimeOut(); } Log(info_or_verbose, "Completed execution of '%s'", cmdline); diff --git a/cf-monitord/history.c b/cf-monitord/history.c index e0375b2a1b..d012eeb368 100644 --- a/cf-monitord/history.c +++ b/cf-monitord/history.c @@ -374,8 +374,7 @@ static Item *NovaReSample(EvalContext *ctx, int slot, const Attributes *attr, co if (a.contain.timeout != 0) { - alarm(0); - signal(SIGALRM, SIG_DFL); + ClearTimeOut(); } Log(LOG_LEVEL_INFO, "Collected sample of %s", pp->promiser); diff --git a/libpromises/pipes_unix.c b/libpromises/pipes_unix.c index eb1ad57796..7142b250dd 100644 --- a/libpromises/pipes_unix.c +++ b/libpromises/pipes_unix.c @@ -33,6 +33,7 @@ #include #include #include +#include static bool CfSetuid(uid_t uid, gid_t gid); @@ -237,12 +238,25 @@ static pid_t GenericCreatePipeAndFork(IOPipe *pipes) sigemptyset(&sigmask); sigprocmask(SIG_SETMASK, &sigmask, NULL); - /* Lead a new process group, so that anything the command spawns can be - * signalled as a unit. Without this only the direct child is reachable, - * and a grandchild outlives exec_timeout still holding the pipe open, - * which leaves the parent blocked reading it. setpgid() is - * async-signal-safe, so it is legal here. */ - setpgid(0, 0); + /* When a timeout is armed, lead a new process group, so that anything + * the command spawns can be signalled as a unit. Without this only the + * direct child is reachable, and a grandchild outlives exec_timeout + * still holding the pipe open, which leaves the parent blocked reading + * it. setpgid() is async-signal-safe, so it is legal here. + * + * Only when a timeout is armed. A child in a process group of its own + * is no longer in the terminal's foreground group, so on an interactive + * run it is stopped by SIGTTIN the moment it reads the terminal, and + * the agent then blocks forever on the pipe -- the very hang this is + * meant to bound, reintroduced on a path with no timeout to end it. It + * also leaves the child out of reach of a terminal SIGINT and of + * cf-execd's agent_expireafter, both of which kill by process group. + * Children with no timeout have nothing to bound their wait, so they + * must stay in ours. */ + if (TimeOutIsArmed()) + { + setpgid(0, 0); + } } ALARM_PID = (pid != 0 ? pid : -1); diff --git a/libpromises/timeout.c b/libpromises/timeout.c index 28bdded07e..cbfe0df8b7 100644 --- a/libpromises/timeout.c +++ b/libpromises/timeout.c @@ -26,18 +26,37 @@ #include #include +/* Set while a timeout alarm is pending. cf_popen()'s child consults it to + * decide whether to lead a process group of its own; only a child that may + * have to be killed as a tree needs one. */ +static bool TIMEOUT_ARMED = false; /* GLOBAL_X */ + void SetTimeOut(int timeout) { ALARM_PID = -1; + TIMEOUT_ARMED = true; signal(SIGALRM, (void *) TimeOut); alarm(timeout); } +void ClearTimeOut(void) +{ + alarm(0); + signal(SIGALRM, SIG_DFL); + TIMEOUT_ARMED = false; +} + +bool TimeOutIsArmed(void) +{ + return TIMEOUT_ARMED; +} + /*************************************************************************/ void TimeOut() { alarm(0); + TIMEOUT_ARMED = false; if (ALARM_PID != -1) { diff --git a/libpromises/timeout.h b/libpromises/timeout.h index 33af0f7f05..332b9c8ce3 100644 --- a/libpromises/timeout.h +++ b/libpromises/timeout.h @@ -26,6 +26,16 @@ #define CFENGINE_TIMEOUT_H void SetTimeOut(int timeout); + +/* True between SetTimeOut() arming the alarm and the alarm being disarmed. + * Consulted by code that forks a child which the timeout may have to + * terminate, to decide whether that child needs a process group of its own. */ +bool TimeOutIsArmed(void); + +/* Cancel a pending alarm and restore the default handler. Callers used to + * open-code this; it also has to clear the armed flag, so that a command which + * completes in time does not leave it set for the next, unrelated, child. */ +void ClearTimeOut(void); void TimeOut(void); time_t SetReferenceTime(void); From 6e522a7306bd39cc2d219672e37393c2bdbe948b Mon Sep 17 00:00:00 2001 From: Daniel JB Clark Date: Mon, 17 Aug 2026 15:34:27 -0400 Subject: [PATCH 3/9] Fixed a commands: promise being reported as compliant after exec_timeout fired A commands: promise whose exec_timeout fired was still judged solely on the child's wait status. RepairExec() handed that status to VerifyCommandRetcode(), and with the default kept_returncodes an exit status of 0 was reported as repaired: aggregate compliance 100%, promise_repaired set, repair_timeout not set, and PromiseResultIsOK() true. Nothing consulted whether the alarm had fired. ACTION_RESULT_TIMEOUT was declared in cf-agent/verify_exec.c and VerifyExecPromise() had a case for it, but no path in the file ever returned it, so PROMISE_RESULT_TIMEOUT was unreachable for this promise type. This is a fail-open rather than a reporting nit. A check run under exec_timeout cannot be distinguished from one that passed, so a policy keying a later promise off if_ok, if_repaired or depends_on treats "the check never finished" as "the check succeeded". The exit status of a signalled command does not say whether it ran to completion: the shell is signalled, its own child keeps running, the shell reaps it and exits 0, and that 0 is what the promise was judged on. Shortening the termination ladder narrows the window but cannot close it. TimeOut() now records that it fired, in a volatile sig_atomic_t written from the SIGALRM handler, and SetTimeOut() clears it so the flag describes only the command just run. RepairExec() classifies on that flag in preference to the exit status, and returns ACTION_RESULT_TIMEOUT. The flag is sampled immediately after cf_pclose() returns, and never earlier. The output read loop ends as soon as the command closes its output, which it can do long before it exits, so the alarm may not fire until cf_pclose() is already waiting for the child. Sampling before that wait misses exactly the shape this is meant to catch. TimeOut() also records whether it actually had a process to signal. cf_pclose() clears ALARM_PID before waiting, so a command that closes its output and then outlives its timeout reaches TimeOut() with nothing to signal: the alarm fires, the ALARM_PID == -1 branch runs, and the command runs to completion untouched. Reporting that as a termination would be a false statement in an error message about a fail-open, so the two cases are worded differently: Command '...' exceeded exec_timeout of 2 seconds and was terminated Command '...' exceeded exec_timeout of 2 seconds; it was NOT terminated and ran to completion Both are PROMISE_RESULT_TIMEOUT at 0% compliance; only the wording differs. That a timed-out command is not always terminated is a separate defect, not addressed here; the second wording makes it visible rather than mislabelled. PROMISE_RESULT_TIMEOUT already flowed correctly through PromiseResultUpdate() (CHANGE + TIMEOUT -> TIMEOUT), so only the missing classification and return had to be supplied. Two limits are known and unchanged. A promise with background => "true" is still reported by the parent as kept, so this does not make exec_timeout visible on that path. And there is a residual window between cf_pclose() returning and the alarm being disarmed in which a just-on-time completion could be labelled a timeout; closing it means blocking SIGALRM, then alarm(0), then sampling. Commands finishing within their timeout are unaffected: exit 0 still repairs the promise and a non-zero exit still fails it. Ticket: CFE-4726 Changelog: Title --- cf-agent/verify_exec.c | 31 +++++++++++++++++++++++++++++-- libpromises/timeout.c | 25 +++++++++++++++++++++++++ libpromises/timeout.h | 10 ++++++++++ 3 files changed, 64 insertions(+), 2 deletions(-) diff --git a/cf-agent/verify_exec.c b/cf-agent/verify_exec.c index 15c74d5fee..53ec985a58 100644 --- a/cf-agent/verify_exec.c +++ b/cf-agent/verify_exec.c @@ -288,6 +288,9 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a, CommandPrefix(cmdline, comm); + /* Set once the command has been reaped, if its exec_timeout fired. */ + bool timed_out = false; + bool do_work_here = true; #ifndef __MINGW32__ @@ -448,7 +451,31 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a, { int ret = cf_pclose(pfp); - if (ret == -1) + /* Sample only now, and never earlier. The read loop above ends as + * soon as the command closes its output, which it can do long + * before it exits -- so the alarm may not fire until cf_pclose() + * is already waiting for the child. Reading the flag before that + * wait misses exactly the case this is here to catch. It is still + * read before the alarm is disarmed further down. */ + timed_out = (a->contain.timeout != CF_NOINT) && TimeOutHasFired(); + + if (timed_out) + { + /* The command exceeded exec_timeout and was signalled, so its + * exit status cannot be trusted to say so. A command killed + * after it has written its last output, or one that exits + * normally while only its children are killed, is reaped with a + * status VerifyCommandRetcode() reads as success -- and the + * promise is then reported kept or repaired even though the + * command never completed. Classify on the timeout instead. */ + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_TIMEOUT, pp, a, + TimeOutSignalledProcess() + ? "Command '%s' exceeded exec_timeout of %d seconds and was terminated" + : "Command '%s' exceeded exec_timeout of %d seconds; it was NOT terminated and ran to completion", + pp->promiser, a->contain.timeout); + *result = PromiseResultUpdate(*result, PROMISE_RESULT_TIMEOUT); + } + else if (ret == -1) { cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, "Finished script '%s' - failed (abnormal termination)", pp->promiser); *result = PromiseResultUpdate(*result, PROMISE_RESULT_FAIL); @@ -492,7 +519,7 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a, } #endif /* !__MINGW32__ */ - return ACTION_RESULT_OK; + return timed_out ? ACTION_RESULT_TIMEOUT : ACTION_RESULT_OK; } /*************************************************************/ diff --git a/libpromises/timeout.c b/libpromises/timeout.c index 5ad63baf85..5033ceada1 100644 --- a/libpromises/timeout.c +++ b/libpromises/timeout.c @@ -26,21 +26,46 @@ #include #include +/* Set by TimeOut() when the alarm fires, so that the caller can tell "the + * command timed out" from "the command finished". Written from a signal + * handler, hence volatile sig_atomic_t. */ +static volatile sig_atomic_t TIMEOUT_FIRED = 0; /* GLOBAL_X */ + +/* Set only when TimeOut() actually had a process to signal. The alarm can fire + * with ALARM_PID already cleared -- cf_pclose() clears it before waiting -- in + * which case the command timed out but was never terminated, and saying + * otherwise would be a false statement in an error message. */ +static volatile sig_atomic_t TIMEOUT_SIGNALLED = 0; /* GLOBAL_X */ + void SetTimeOut(int timeout) { ALARM_PID = -1; + TIMEOUT_FIRED = 0; + TIMEOUT_SIGNALLED = 0; signal(SIGALRM, (void *) TimeOut); alarm(timeout); } +bool TimeOutHasFired(void) +{ + return TIMEOUT_FIRED != 0; +} + +bool TimeOutSignalledProcess(void) +{ + return TIMEOUT_SIGNALLED != 0; +} + /*************************************************************************/ void TimeOut() { alarm(0); + TIMEOUT_FIRED = 1; if (ALARM_PID != -1) { + TIMEOUT_SIGNALLED = 1; Log(LOG_LEVEL_VERBOSE, "Time out of process %jd", (intmax_t)ALARM_PID); GracefulTerminate(ALARM_PID, PROCESS_START_TIME_UNKNOWN); } diff --git a/libpromises/timeout.h b/libpromises/timeout.h index 33af0f7f05..7fee008f4a 100644 --- a/libpromises/timeout.h +++ b/libpromises/timeout.h @@ -26,6 +26,16 @@ #define CFENGINE_TIMEOUT_H void SetTimeOut(int timeout); + +/* True if the alarm armed by the last SetTimeOut() actually fired. Lets a + * caller report that a command was timed out even when the command's own exit + * status would otherwise read as success. Cleared by SetTimeOut(). */ +bool TimeOutHasFired(void); + +/* True if that alarm also had a process to signal. False means the command + * exceeded its timeout but was never terminated, which callers must not + * describe as a termination. */ +bool TimeOutSignalledProcess(void); void TimeOut(void); time_t SetReferenceTime(void); From 0ab083c4d1c5f34e71626c58c886c1974bb26500 Mon Sep 17 00:00:00 2001 From: Daniel JB Clark Date: Mon, 17 Aug 2026 15:34:27 -0400 Subject: [PATCH 4/9] Added acceptance tests for commands: promises that exceed exec_timeout Five tests in the new tests/acceptance/08_commands/04_exec_timeout/, covering the outcome classification of a commands: promise run under exec_timeout: - timeout_overrides_exit_zero.cf: a fired timeout is reported as repair_timeout even though the command exits 0. Without the fix the exit status wins and the promise is reported repaired at 100% compliance. - within_timeout_normal_outcomes.cf: an armed timeout that does not fire leaves the outcome to the exit status -- 0 repairs the promise, non-zero fails it. Guards the normal path. - timeout_overrides_kept_returncodes.cf: kept_returncodes => { "0" } does not resurrect "kept" when the timeout fired. - timeout_after_output_closed.cf: the timeout is detected even when the command closes its output long before it exits, so the alarm fires only once the agent is already waiting for the child. Deliberately slow (about 12 seconds): with output closed there is no process registered for the alarm to signal, so the child runs to completion. - timeout_does_not_leak_to_next_promise.cf: a fired timeout is charged to the promise whose command timed out; the next commands promise, armed with its own timeout, still comes out repaired. All five report through dcs_all_classes() from dcs.sub.cf, except the kept_returncodes test: classes bodies cannot compose, so it carries a local copy of that body with kept_returncodes added. Ticket: CFE-4726 Changelog: none --- .../timeout_after_output_closed.cf | 61 +++++++++++++++++ .../timeout_does_not_leak_to_next_promise.cf | 66 +++++++++++++++++++ .../timeout_overrides_exit_zero.cf | 58 ++++++++++++++++ .../timeout_overrides_kept_returncodes.cf | 66 +++++++++++++++++++ .../within_timeout_normal_outcomes.cf | 59 +++++++++++++++++ 5 files changed, 310 insertions(+) create mode 100644 tests/acceptance/08_commands/04_exec_timeout/timeout_after_output_closed.cf create mode 100644 tests/acceptance/08_commands/04_exec_timeout/timeout_does_not_leak_to_next_promise.cf create mode 100644 tests/acceptance/08_commands/04_exec_timeout/timeout_overrides_exit_zero.cf create mode 100644 tests/acceptance/08_commands/04_exec_timeout/timeout_overrides_kept_returncodes.cf create mode 100644 tests/acceptance/08_commands/04_exec_timeout/within_timeout_normal_outcomes.cf diff --git a/tests/acceptance/08_commands/04_exec_timeout/timeout_after_output_closed.cf b/tests/acceptance/08_commands/04_exec_timeout/timeout_after_output_closed.cf new file mode 100644 index 0000000000..bf679fbd5e --- /dev/null +++ b/tests/acceptance/08_commands/04_exec_timeout/timeout_after_output_closed.cf @@ -0,0 +1,61 @@ +####################################################### +# +# Test that exec_timeout is detected even when the command +# closes its output long before it exits. The agent's read +# loop ends at EOF, so it is already waiting for the child +# when the alarm fires; a timeout sampled while output was +# still open misses exactly this shape. The command then runs +# to completion and exits 0, and must still be reported as +# timed out. +# +# Deliberately slow: with its output already closed there is +# no process left registered for the alarm to signal, so the +# child's full 10 second sleep runs out before it is reaped. +# Expect around 12 seconds of wall clock. +# +####################################################### +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +####################################################### +bundle agent test +{ + meta: + "description" + string => "exec_timeout is detected when the command closes its output before it exits"; + + "test_skip_unsupported" + string => "windows", + comment => "Drives /bin/sh with a POSIX shell payload"; + + commands: + "/bin/sh" + arglist => { "-c", "exec 1>&- 2>&-; sleep 10; exit 0" }, + contain => exec_timeout_2, + classes => dcs_all_classes("output_closed"); +} + +body contain exec_timeout_2 +{ + exec_timeout => "2"; +} + +####################################################### +bundle agent check +{ + methods: + "any" + usebundle => dcs_passif_expected( + "output_closed_repair_timeout", + "output_closed_promise_kept,output_closed_promise_repaired", + $(this.promise_filename) + ), + inherit => "true"; +} + +### PROJECT_ID: core +### CATEGORY_ID: 26 diff --git a/tests/acceptance/08_commands/04_exec_timeout/timeout_does_not_leak_to_next_promise.cf b/tests/acceptance/08_commands/04_exec_timeout/timeout_does_not_leak_to_next_promise.cf new file mode 100644 index 0000000000..c38a4e0330 --- /dev/null +++ b/tests/acceptance/08_commands/04_exec_timeout/timeout_does_not_leak_to_next_promise.cf @@ -0,0 +1,66 @@ +####################################################### +# +# Test that a fired exec_timeout is charged to the promise +# whose command timed out, and only that promise: a +# subsequent commands: promise that finishes inside its own +# timeout must come out repaired, not timed out. +# +####################################################### +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +####################################################### +bundle agent test +{ + meta: + "description" + string => "A fired exec_timeout does not leak into the next commands promise"; + + "test_skip_unsupported" + string => "windows", + comment => "Drives /bin/sh with POSIX shell payloads"; + + commands: + "/bin/sh" + arglist => { "-c", "sleep 2.4; exit 0" }, + contain => exec_timeout_2, + classes => dcs_all_classes("leak_first"); + + # Runs after the first command has already timed out, armed with + # its own timeout so that a stale flag surviving from the first + # promise would be sampled here if it leaked. + "/bin/sh" + arglist => { "-c", "exit 0" }, + contain => exec_timeout_10, + classes => dcs_all_classes("leak_second"); +} + +body contain exec_timeout_2 +{ + exec_timeout => "2"; +} + +body contain exec_timeout_10 +{ + exec_timeout => "10"; +} + +####################################################### +bundle agent check +{ + methods: + "any" + usebundle => dcs_passif_expected( + "leak_first_repair_timeout,leak_second_promise_repaired", + "leak_first_promise_kept,leak_first_promise_repaired,leak_second_repair_timeout", + $(this.promise_filename) + ), + inherit => "true"; +} + +### PROJECT_ID: core +### CATEGORY_ID: 26 diff --git a/tests/acceptance/08_commands/04_exec_timeout/timeout_overrides_exit_zero.cf b/tests/acceptance/08_commands/04_exec_timeout/timeout_overrides_exit_zero.cf new file mode 100644 index 0000000000..c5022af938 --- /dev/null +++ b/tests/acceptance/08_commands/04_exec_timeout/timeout_overrides_exit_zero.cf @@ -0,0 +1,58 @@ +####################################################### +# +# Test that a commands: promise whose exec_timeout fires is +# reported as timed out even though the command exits 0. The +# exit status of a command that was signalled does not say +# whether it ran to completion, so a fired timeout must take +# precedence over a successful exit status. +# +####################################################### +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +####################################################### +bundle agent test +{ + meta: + "description" + string => "A fired exec_timeout takes precedence over a successful exit status"; + + "test_skip_unsupported" + string => "windows", + comment => "Drives /bin/sh with a POSIX shell payload"; + + commands: + # The sleep outlives the 2 second timeout by less than the + # termination ladder's grace period, so the signalled shell + # still reaps its child and exits 0 -- the shape in which the + # exit status used to win over the fired timeout. + "/bin/sh" + arglist => { "-c", "sleep 2.4; exit 0" }, + contain => exec_timeout_2, + classes => dcs_all_classes("timeout_exit0"); +} + +body contain exec_timeout_2 +{ + exec_timeout => "2"; +} + +####################################################### +bundle agent check +{ + methods: + "any" + usebundle => dcs_passif_expected( + "timeout_exit0_repair_timeout", + "timeout_exit0_promise_kept,timeout_exit0_promise_repaired", + $(this.promise_filename) + ), + inherit => "true"; +} + +### PROJECT_ID: core +### CATEGORY_ID: 26 diff --git a/tests/acceptance/08_commands/04_exec_timeout/timeout_overrides_kept_returncodes.cf b/tests/acceptance/08_commands/04_exec_timeout/timeout_overrides_kept_returncodes.cf new file mode 100644 index 0000000000..4f293fe0bb --- /dev/null +++ b/tests/acceptance/08_commands/04_exec_timeout/timeout_overrides_kept_returncodes.cf @@ -0,0 +1,66 @@ +####################################################### +# +# Test that kept_returncodes does not resurrect "kept" when +# exec_timeout fired: the timed-out command exits 0, and 0 is +# listed in kept_returncodes, but the promise must still be +# reported as timed out, not kept. +# +####################################################### +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +####################################################### +bundle agent test +{ + meta: + "description" + string => "kept_returncodes does not resurrect kept when exec_timeout fired"; + + "test_skip_unsupported" + string => "windows", + comment => "Drives /bin/sh with a POSIX shell payload"; + + commands: + "/bin/sh" + arglist => { "-c", "sleep 2.4; exit 0" }, + contain => exec_timeout_2, + classes => all_classes_kept_0("timeout_kept0"); +} + +body contain exec_timeout_2 +{ + exec_timeout => "2"; +} + +# dcs_all_classes (dcs.sub.cf) carries no returncode attributes and +# classes bodies do not compose, so this is dcs_all_classes plus +# kept_returncodes. +body classes all_classes_kept_0(prefix) +{ + promise_kept => { "$(prefix)_promise_kept" }; + promise_repaired => { "$(prefix)_promise_repaired" }; + repair_failed => { "$(prefix)_repair_failed" }; + repair_denied => { "$(prefix)_repair_denied" }; + repair_timeout => { "$(prefix)_repair_timeout" }; + kept_returncodes => { "0" }; +} + +####################################################### +bundle agent check +{ + methods: + "any" + usebundle => dcs_passif_expected( + "timeout_kept0_repair_timeout", + "timeout_kept0_promise_kept,timeout_kept0_promise_repaired", + $(this.promise_filename) + ), + inherit => "true"; +} + +### PROJECT_ID: core +### CATEGORY_ID: 26 diff --git a/tests/acceptance/08_commands/04_exec_timeout/within_timeout_normal_outcomes.cf b/tests/acceptance/08_commands/04_exec_timeout/within_timeout_normal_outcomes.cf new file mode 100644 index 0000000000..6ea7e32995 --- /dev/null +++ b/tests/acceptance/08_commands/04_exec_timeout/within_timeout_normal_outcomes.cf @@ -0,0 +1,59 @@ +####################################################### +# +# Test that an armed exec_timeout that does not fire leaves +# the outcome of a commands: promise to the exit status: 0 +# repairs the promise and non-zero fails it, exactly as if no +# timeout were set. Guards the normal path against +# regressions from the timed-out path. +# +####################################################### +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +####################################################### +bundle agent test +{ + meta: + "description" + string => "An exec_timeout that does not fire leaves the outcome to the exit status"; + + "test_skip_unsupported" + string => "windows", + comment => "Drives /bin/sh with POSIX shell payloads"; + + commands: + "/bin/sh" + arglist => { "-c", "sleep 1; exit 0" }, + contain => exec_timeout_10, + classes => dcs_all_classes("within_exit0"); + + "/bin/sh" + arglist => { "-c", "sleep 1; exit 3" }, + contain => exec_timeout_10, + classes => dcs_all_classes("within_exit3"); +} + +body contain exec_timeout_10 +{ + exec_timeout => "10"; +} + +####################################################### +bundle agent check +{ + methods: + "any" + usebundle => dcs_passif_expected( + "within_exit0_promise_repaired,within_exit3_repair_failed", + "within_exit0_repair_timeout,within_exit0_repair_failed,within_exit3_repair_timeout,within_exit3_promise_repaired", + $(this.promise_filename) + ), + inherit => "true"; +} + +### PROJECT_ID: core +### CATEGORY_ID: 26 From ade76f61600d2393e1e1debe628d234ae3932cdd Mon Sep 17 00:00:00 2001 From: Daniel JB Clark Date: Mon, 17 Aug 2026 16:45:14 -0400 Subject: [PATCH 5/9] Log when a timed command's process group cannot be created or read The group kill added by the merged branch degrades safely when setpgid() or getpgid() fails -- the pgid guard in TimeOut() sees the command never led a group of its own and skips the negative kill -- but it degraded silently, and what it degrades to is the original bug: the command's descendants survive the timeout and hold the pipe open. An operator debugging that hang should not have to discover it by reading the source, so both failure paths now say what happened and what it costs. Neither failure has an expected occurrence: setpgid(0, 0) on a fresh fork()ed child that is not a session leader has no documented error left to fail with, and getpgid() runs while the child is still unreaped. The logs exist for the case the next platform disagrees. Log() is not async-signal-safe, so the child-side call is confined to the setpgid() failure branch, where the alternative is losing the descendants silently; the handler-side call joins Log() calls TimeOut() already makes. Changelog: None --- libpromises/pipes_unix.c | 12 +++++++++++- libpromises/timeout.c | 6 ++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/libpromises/pipes_unix.c b/libpromises/pipes_unix.c index 7142b250dd..71dc902660 100644 --- a/libpromises/pipes_unix.c +++ b/libpromises/pipes_unix.c @@ -255,7 +255,17 @@ static pid_t GenericCreatePipeAndFork(IOPipe *pipes) * must stay in ours. */ if (TimeOutIsArmed()) { - setpgid(0, 0); + if (setpgid(0, 0) != 0) + { + /* The command stays in our group, so TimeOut()'s check of its + * group will skip the group kill; only its descendants are + * then out of the timeout's reach. Log() is not + * async-signal-safe, so it is confined to this failure branch, + * where the alternative is losing the descendants silently. */ + Log(LOG_LEVEL_WARNING, + "Could not give the timed command its own process group (setpgid: %s), its descendants will survive a timeout", + GetErrorStr()); + } } } diff --git a/libpromises/timeout.c b/libpromises/timeout.c index 394361b4cf..1b8a1a3e4c 100644 --- a/libpromises/timeout.c +++ b/libpromises/timeout.c @@ -94,6 +94,12 @@ void TimeOut() * once GracefulTerminate() has killed it, getpgid() fails with ESRCH and * we would have no safe way to tell whether it led a group of its own. */ const pid_t pgid = getpgid(ALARM_PID); + if (pgid == -1) + { + Log(LOG_LEVEL_WARNING, + "Could not read the process group of timed-out process %jd (getpgid: %s), not signalling its process group", + (intmax_t)ALARM_PID, GetErrorStr()); + } GracefulTerminate(ALARM_PID, PROCESS_START_TIME_UNKNOWN); From 3d8e90d6876c465c7b672afbfa20e2154735d52a Mon Sep 17 00:00:00 2001 From: Daniel JB Clark Date: Mon, 17 Aug 2026 16:54:02 -0400 Subject: [PATCH 6/9] Add an acceptance test for descendants of a timed-out command The five existing exec_timeout tests all pin down how a timed-out promise is classified; none of them notices whether the timeout actually ended the command's process tree. This one does: the shell waits on a 30 second sleep under exec_timeout 2, so at the timeout the sleep is alive holding the write end of the output pipe, and only killing the command's process group releases the pipe before the sleep's natural end. The test touches a marker file on either side of the timed promise and fails if their mtimes are more than 20 seconds apart -- well past the roughly 10 seconds the timeout plus termination ladder takes, well short of the 30 the surviving sleep would hold the agent for. Verified to discriminate: with the setpgid() hunk in GenericCreatePipeAndFork() removed and the tree rebuilt, the test fails, with the single-test suite run taking 32 seconds; with the hunk restored byte-identically it passes in a 20 second run. Changelog: none --- .../timeout_kills_descendants.cf | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 tests/acceptance/08_commands/04_exec_timeout/timeout_kills_descendants.cf diff --git a/tests/acceptance/08_commands/04_exec_timeout/timeout_kills_descendants.cf b/tests/acceptance/08_commands/04_exec_timeout/timeout_kills_descendants.cf new file mode 100644 index 0000000000..aef80dc338 --- /dev/null +++ b/tests/acceptance/08_commands/04_exec_timeout/timeout_kills_descendants.cf @@ -0,0 +1,98 @@ +####################################################### +# +# Test that a fired exec_timeout also ends the command's +# descendants. The termination ladder only reaches the direct +# child; the sleep the shell spawned inherits the write end of +# the output pipe, and if it survives the timeout it holds that +# pipe open for its full 30 seconds while the agent sits in its +# read loop -- exec_timeout then does not bound the promise's +# wall clock at all. Killing the command's process group ends +# the sleep at the timeout, so the promise completes in well +# under 20 seconds. +# +# The elapsed time is measured between two marker files touched +# immediately before and after the timed promise; commands in a +# bundle run in order of declaration. +# +####################################################### +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +####################################################### +bundle agent init +{ + files: + # The markers must come from this run: a stale pair could carry a + # bounded-looking mtime difference from an earlier one. + "$(G.testdir)/desc_start" delete => tidy; + "$(G.testdir)/desc_end" delete => tidy; +} + +####################################################### +bundle agent test +{ + meta: + "description" + string => "A fired exec_timeout terminates the command's descendants"; + + "test_skip_unsupported" + string => "windows", + comment => "Drives /bin/sh with POSIX shell payloads"; + + commands: + "/bin/sh" + arglist => { "-c", "touch $(G.testdir)/desc_start" }; + + # The shell waits for its sleep, so at the 2 second timeout the + # sleep is alive holding the pipe. Only ending it too releases + # the pipe before its natural end at 30 seconds. + "/bin/sh" + arglist => { "-c", "sleep 30; exit 0" }, + contain => exec_timeout_2, + classes => dcs_all_classes("desc_timed"); + + "/bin/sh" + arglist => { "-c", "touch $(G.testdir)/desc_end" }; +} + +body contain exec_timeout_2 +{ + exec_timeout => "2"; +} + +####################################################### +bundle agent check +{ + vars: + "start" string => filestat("$(G.testdir)/desc_start", "mtime"); + "end" string => filestat("$(G.testdir)/desc_end", "mtime"); + "elapsed" string => eval("$(end) - $(start)", "math", "infix"); + + classes: + # 20 is well past the ~10 seconds the timeout plus termination + # ladder takes, and well short of the sleep's 30. The fileexists() + # guards keep a missing marker from passing as boundedness. + "bounded" + and => { + fileexists("$(G.testdir)/desc_start"), + fileexists("$(G.testdir)/desc_end"), + islessthan("$(elapsed)", "20") + }, + scope => "namespace"; + + methods: + "any" + usebundle => dcs_passif_expected( + "desc_timed_repair_timeout,bounded", + "desc_timed_promise_kept,desc_timed_promise_repaired", + $(this.promise_filename) + ), + inherit => "true"; +} + +### PROJECT_ID: core +### CATEGORY_ID: 26 From d004c19abaa800f75df118cdda676c832e0a1876 Mon Sep 17 00:00:00 2001 From: Daniel JB Clark Date: Mon, 17 Aug 2026 21:40:03 -0400 Subject: [PATCH 7/9] Guard TimeOut()'s process-group kill against MinGW; TIMEOUT_ARMED -> sig_atomic_t timeout.c builds unconditionally on Windows, but getpgid() and kill(-pid, ...) are POSIX-only and neither is declared for MinGW in this tree. The child-side half of this series -- the setpgid() call that gives a timed command its own process group -- already lives in pipes_unix.c, which the Makefile compiles only `if !NT`. So on Windows no timed command is ever put in a process group of its own, and there is nothing for the parent-side group kill to safely widen its signal to: guard it out rather than leave an undeclared-symbol build break. GracefulTerminate() itself is untouched and keeps reaching the direct child on every platform; only the "kill its whole tree" widening is POSIX-only. Also fixes TIMEOUT_ARMED: it was a plain bool written from TimeOut(), a signal handler, same as TIMEOUT_FIRED and TIMEOUT_SIGNALLED already are but this one was not -- an oversight in the original series. Made volatile sig_atomic_t to match, and its two boolean literals became 0/1 since sig_atomic_t is not required to be bool-compatible. Changelog: None --- libpromises/timeout.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/libpromises/timeout.c b/libpromises/timeout.c index 1b8a1a3e4c..dccdb466ad 100644 --- a/libpromises/timeout.c +++ b/libpromises/timeout.c @@ -39,15 +39,16 @@ static volatile sig_atomic_t TIMEOUT_SIGNALLED = 0; /* GLOBAL_X */ /* Set while a timeout alarm is pending. cf_popen()'s child consults it to * decide whether to lead a process group of its own; only a child that may - * have to be killed as a tree needs one. */ -static bool TIMEOUT_ARMED = false; /* GLOBAL_X */ + * have to be killed as a tree needs one. Cleared from the signal handler as + * well as from ClearTimeOut(), hence volatile sig_atomic_t. */ +static volatile sig_atomic_t TIMEOUT_ARMED = 0; /* GLOBAL_X */ void SetTimeOut(int timeout) { ALARM_PID = -1; TIMEOUT_FIRED = 0; TIMEOUT_SIGNALLED = 0; - TIMEOUT_ARMED = true; + TIMEOUT_ARMED = 1; signal(SIGALRM, (void *) TimeOut); alarm(timeout); } @@ -59,12 +60,12 @@ void ClearTimeOut(void) * after the disarm. Only the next SetTimeOut() resets them. */ alarm(0); signal(SIGALRM, SIG_DFL); - TIMEOUT_ARMED = false; + TIMEOUT_ARMED = 0; } bool TimeOutIsArmed(void) { - return TIMEOUT_ARMED; + return TIMEOUT_ARMED != 0; } bool TimeOutHasFired(void) @@ -83,13 +84,14 @@ void TimeOut() { alarm(0); TIMEOUT_FIRED = 1; - TIMEOUT_ARMED = false; + TIMEOUT_ARMED = 0; if (ALARM_PID != -1) { TIMEOUT_SIGNALLED = 1; Log(LOG_LEVEL_VERBOSE, "Time out of process %jd", (intmax_t)ALARM_PID); +#ifndef __MINGW32__ /* Read the process group while the process is still alive to be read: * once GracefulTerminate() has killed it, getpgid() fails with ESRCH and * we would have no safe way to tell whether it led a group of its own. */ @@ -100,9 +102,11 @@ void TimeOut() "Could not read the process group of timed-out process %jd (getpgid: %s), not signalling its process group", (intmax_t)ALARM_PID, GetErrorStr()); } +#endif GracefulTerminate(ALARM_PID, PROCESS_START_TIME_UNKNOWN); +#ifndef __MINGW32__ /* GracefulTerminate() only reaches the process we started. Anything that * process spawned survives it, and keeps the pipe open, so the caller * stays blocked reading a command it has already given up on. @@ -111,11 +115,16 @@ void TimeOut() * cf_popen()'s child arranges with setpgid(). If that did not take * effect the process is still in our group, its pgid is not its pid, and * a negative kill() here would signal an unrelated group -- possibly our - * own. */ + * own. + * + * Windows has no POSIX process groups and no setpgid() in the child + * (cf_popen() lives in pipes_unix.c), so there is nothing to widen the + * signal to there. */ if (pgid == ALARM_PID) { kill(-ALARM_PID, SIGKILL); } +#endif } else { From dbf759d16ae5d085fdafc7176eccc07c77b7a48a Mon Sep 17 00:00:00 2001 From: Daniel JB Clark Date: Mon, 17 Aug 2026 21:40:30 -0400 Subject: [PATCH 8/9] Add a unit test pinning ClearTimeOut()'s contract The parent commit's own review noted this contract had no direct test: that a fired timeout stays visible through TimeOutHasFired() and TimeOutSignalledProcess() after ClearTimeOut() runs, and that ClearTimeOut() disarms TimeOutIsArmed() unconditionally regardless. The four SetTimeOut() callers only exercise it indirectly through exec_timeout's acceptance tests, which check agent-visible outcomes (promise compliance, log lines) rather than the flags themselves. Six cases: SetTimeOut() arms and resets the record; ClearTimeOut() disarms; a fired alarm with no process to signal sets HasFired but not SignalledProcess; ClearTimeOut() after firing preserves HasFired/ SignalledProcess while still disarming; the same preservation with a TRUE SignalledProcess (needs a real forked child -- the no-process case can't tell "preserved" from "wiped" when the flag started false); and a subsequent SetTimeOut() resets the record for the next command. Confirmed every assertion is load-bearing by individually breaking the implementation four ways (ClearTimeOut() clearing FIRED/ SIGNALLED, ClearTimeOut() forgetting to disarm, the handler forgetting to disarm, ClearTimeOut() clearing a true SIGNALLED) and checking exactly one test fails each time. The fourth of those came from review: the first five cases alone could not distinguish ClearTimeOut() preserving a true SIGNALLED from wiping it, since every case up to that point left the flag false either way. Unix-only (`if !NT` in tests/unit/Makefile.am): it waits on a real SIGALRM, and platform.h declares alarm() for Windows but no implementation of it ships in this tree. Changelog: None --- tests/unit/Makefile.am | 7 +++ tests/unit/timeout_test.c | 124 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+) create mode 100644 tests/unit/timeout_test.c diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am index 3864b67876..403bee2687 100644 --- a/tests/unit/Makefile.am +++ b/tests/unit/Makefile.am @@ -170,6 +170,13 @@ noinst_PROGRAMS = redirection_test_stub redirection_test_stub_SOURCES = redirection_test_stub.c endif +# Waits for a real SIGALRM. platform.h declares alarm() for Windows but no +# implementation of it ships in this tree, so the test would have nothing to +# wait for there. +if !NT +check_PROGRAMS += timeout_test +endif + check_SCRIPTS = dynamic_dependency_test.sh \ tar_portability_test.sh diff --git a/tests/unit/timeout_test.c b/tests/unit/timeout_test.c new file mode 100644 index 0000000000..e9cf95c5c3 --- /dev/null +++ b/tests/unit/timeout_test.c @@ -0,0 +1,124 @@ +#include + +#include +#include +#include + +/* The alarm handler interrupts the sleep, so this normally returns as soon as + * it fires. Loop anyway: the alarm may already have fired before we get here, + * and a sleep interrupted by anything else must not end the wait early. */ +static void WaitForAlarm(void) +{ + for (int i = 0; (i < 30) && !TimeOutHasFired(); i++) + { + struct timespec ts = { .tv_sec = 0, .tv_nsec = 100 * 1000 * 1000 }; + nanosleep(&ts, NULL); + } +} + +static void test_set_arms_and_resets_the_record(void) +{ + SetTimeOut(3600); + assert_true(TimeOutIsArmed()); + assert_false(TimeOutHasFired()); + assert_false(TimeOutSignalledProcess()); + ClearTimeOut(); +} + +/* A command that finished in time must not leave the flag set for the next, + * unrelated, child: it is what decides whether cf_popen()'s child puts itself + * in a process group of its own. */ +static void test_clear_disarms(void) +{ + SetTimeOut(3600); + ClearTimeOut(); + assert_false(TimeOutIsArmed()); +} + +static void test_fired_alarm_without_a_process(void) +{ + SetTimeOut(1); + WaitForAlarm(); + + assert_true(TimeOutHasFired()); + /* Nothing else disarms on this path -- the handler does it itself. */ + assert_false(TimeOutIsArmed()); + /* SetTimeOut() cleared ALARM_PID, so there was no process to signal and + * the caller must not describe the command as terminated. */ + assert_false(TimeOutSignalledProcess()); +} + +/* ClearTimeOut() runs after the command is reaped, before the caller reports + * on it. Clearing the record there would make a timed-out command whose exit + * status reads as success indistinguishable from one that finished. */ +static void test_clear_preserves_the_record(void) +{ + SetTimeOut(1); + WaitForAlarm(); + assert_true(TimeOutHasFired()); + + ClearTimeOut(); + assert_false(TimeOutIsArmed()); + assert_true(TimeOutHasFired()); + assert_false(TimeOutSignalledProcess()); +} + +/* The other tests never give TimeOut() a process to signal, so they cannot + * tell a ClearTimeOut() that wipes a TRUE TimeOutSignalledProcess() from one + * that leaves it alone -- both look identical when the flag started false. + * Fork a real child and let the alarm reach it. */ +static void test_clear_preserves_a_true_signalled_flag(void) +{ + pid_t child = fork(); + if (child == 0) + { + /* Long enough to still be alive when TimeOut() runs GracefulTerminate() + * on it; short enough that a leaked child does not linger. */ + sleep(5); + _exit(0); + } + assert_true(child > 0); + + SetTimeOut(1); + ALARM_PID = child; + WaitForAlarm(); + + assert_true(TimeOutHasFired()); + assert_true(TimeOutSignalledProcess()); + + ClearTimeOut(); + assert_false(TimeOutIsArmed()); + assert_true(TimeOutSignalledProcess()); + + int status; + waitpid(child, &status, 0); +} + +static void test_next_set_resets_the_record(void) +{ + SetTimeOut(1); + WaitForAlarm(); + assert_true(TimeOutHasFired()); + + SetTimeOut(3600); + assert_true(TimeOutIsArmed()); + assert_false(TimeOutHasFired()); + assert_false(TimeOutSignalledProcess()); + ClearTimeOut(); +} + +int main() +{ + const UnitTest tests[] = + { + unit_test(test_set_arms_and_resets_the_record), + unit_test(test_clear_disarms), + unit_test(test_fired_alarm_without_a_process), + unit_test(test_clear_preserves_the_record), + unit_test(test_clear_preserves_a_true_signalled_flag), + unit_test(test_next_set_resets_the_record) + }; + + PRINT_TEST_BANNER(); + return run_tests(tests); +} From 0e06ad3d731508eb255c22c599da383648978b3d Mon Sep 17 00:00:00 2001 From: Daniel JB Clark Date: Tue, 18 Aug 2026 10:34:28 -0400 Subject: [PATCH 9/9] Terser comments Review feedback on #6305: the added comments were heavier than they needed to be. Halves their volume, keeping only the non-obvious constraints (ordering, the pgid guard, the SIGTTIN hazard). No code change: the non-comment token stream is byte-identical. Ticket: CFE-4729 Changelog: none --- cf-agent/verify_exec.c | 20 +++++++----------- libpromises/pipes_unix.c | 29 +++++++++----------------- libpromises/timeout.c | 44 +++++++++++++--------------------------- libpromises/timeout.h | 22 ++++++++------------ 4 files changed, 39 insertions(+), 76 deletions(-) diff --git a/cf-agent/verify_exec.c b/cf-agent/verify_exec.c index 091e887b3a..89ca3a77c8 100644 --- a/cf-agent/verify_exec.c +++ b/cf-agent/verify_exec.c @@ -451,23 +451,17 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a, { int ret = cf_pclose(pfp); - /* Sample only now, and never earlier. The read loop above ends as - * soon as the command closes its output, which it can do long - * before it exits -- so the alarm may not fire until cf_pclose() - * is already waiting for the child. Reading the flag before that - * wait misses exactly the case this is here to catch. It is still - * read before the alarm is disarmed further down. */ + /* Only after cf_pclose(): the read loop ends when the command + * closes its output, which can be long before it exits, so the + * alarm may not fire until cf_pclose() is already waiting. */ timed_out = (a->contain.timeout != CF_NOINT) && TimeOutHasFired(); if (timed_out) { - /* The command exceeded exec_timeout and was signalled, so its - * exit status cannot be trusted to say so. A command killed - * after it has written its last output, or one that exits - * normally while only its children are killed, is reaped with a - * status VerifyCommandRetcode() reads as success -- and the - * promise is then reported kept or repaired even though the - * command never completed. Classify on the timeout instead. */ + /* Classify on the timeout, not the exit status: a command + * killed after its last output, or one whose children alone + * are killed, is reaped with a status VerifyCommandRetcode() + * reads as success. */ cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_TIMEOUT, pp, a, TimeOutSignalledProcess() ? "Command '%s' exceeded exec_timeout of %d seconds and was terminated" diff --git a/libpromises/pipes_unix.c b/libpromises/pipes_unix.c index 71dc902660..a5718bd1ad 100644 --- a/libpromises/pipes_unix.c +++ b/libpromises/pipes_unix.c @@ -238,30 +238,21 @@ static pid_t GenericCreatePipeAndFork(IOPipe *pipes) sigemptyset(&sigmask); sigprocmask(SIG_SETMASK, &sigmask, NULL); - /* When a timeout is armed, lead a new process group, so that anything - * the command spawns can be signalled as a unit. Without this only the - * direct child is reachable, and a grandchild outlives exec_timeout - * still holding the pipe open, which leaves the parent blocked reading - * it. setpgid() is async-signal-safe, so it is legal here. + /* Lead a new process group so a timeout can signal the command and + * everything it spawns as a unit; setpgid() is async-signal-safe. * - * Only when a timeout is armed. A child in a process group of its own - * is no longer in the terminal's foreground group, so on an interactive - * run it is stopped by SIGTTIN the moment it reads the terminal, and - * the agent then blocks forever on the pipe -- the very hang this is - * meant to bound, reintroduced on a path with no timeout to end it. It - * also leaves the child out of reach of a terminal SIGINT and of - * cf-execd's agent_expireafter, both of which kill by process group. - * Children with no timeout have nothing to bound their wait, so they - * must stay in ours. */ + * Only when a timeout is armed. A child outside the terminal's + * foreground group is stopped by SIGTTIN as soon as it reads the + * terminal, and is out of reach of a terminal SIGINT and of cf-execd's + * agent_expireafter, which kill by group. Without a timeout to bound + * the wait, that is a hang with nothing to end it. */ if (TimeOutIsArmed()) { if (setpgid(0, 0) != 0) { - /* The command stays in our group, so TimeOut()'s check of its - * group will skip the group kill; only its descendants are - * then out of the timeout's reach. Log() is not - * async-signal-safe, so it is confined to this failure branch, - * where the alternative is losing the descendants silently. */ + /* Only descendants are lost: TimeOut()'s pgid check skips the + * group kill. Log() is not async-signal-safe, so it is + * confined to this branch. */ Log(LOG_LEVEL_WARNING, "Could not give the timed command its own process group (setpgid: %s), its descendants will survive a timeout", GetErrorStr()); diff --git a/libpromises/timeout.c b/libpromises/timeout.c index dccdb466ad..f18361df51 100644 --- a/libpromises/timeout.c +++ b/libpromises/timeout.c @@ -26,21 +26,16 @@ #include #include -/* Set by TimeOut() when the alarm fires, so that the caller can tell "the - * command timed out" from "the command finished". Written from a signal - * handler, hence volatile sig_atomic_t. */ +/* All three are written from the signal handler, hence sig_atomic_t. */ + +/* The alarm fired. */ static volatile sig_atomic_t TIMEOUT_FIRED = 0; /* GLOBAL_X */ -/* Set only when TimeOut() actually had a process to signal. The alarm can fire - * with ALARM_PID already cleared -- cf_pclose() clears it before waiting -- in - * which case the command timed out but was never terminated, and saying - * otherwise would be a false statement in an error message. */ +/* ...and had a process to signal. The alarm can fire with ALARM_PID already + * cleared, i.e. timed out but never terminated. */ static volatile sig_atomic_t TIMEOUT_SIGNALLED = 0; /* GLOBAL_X */ -/* Set while a timeout alarm is pending. cf_popen()'s child consults it to - * decide whether to lead a process group of its own; only a child that may - * have to be killed as a tree needs one. Cleared from the signal handler as - * well as from ClearTimeOut(), hence volatile sig_atomic_t. */ +/* An alarm is pending. */ static volatile sig_atomic_t TIMEOUT_ARMED = 0; /* GLOBAL_X */ void SetTimeOut(int timeout) @@ -55,9 +50,8 @@ void SetTimeOut(int timeout) void ClearTimeOut(void) { - /* Deliberately leaves TIMEOUT_FIRED and TIMEOUT_SIGNALLED alone: they - * record what happened to the last armed timeout, and remain readable - * after the disarm. Only the next SetTimeOut() resets them. */ + /* Leaves TIMEOUT_FIRED/TIMEOUT_SIGNALLED readable after the disarm; only + * SetTimeOut() resets them. */ alarm(0); signal(SIGALRM, SIG_DFL); TIMEOUT_ARMED = 0; @@ -92,9 +86,8 @@ void TimeOut() Log(LOG_LEVEL_VERBOSE, "Time out of process %jd", (intmax_t)ALARM_PID); #ifndef __MINGW32__ - /* Read the process group while the process is still alive to be read: - * once GracefulTerminate() has killed it, getpgid() fails with ESRCH and - * we would have no safe way to tell whether it led a group of its own. */ + /* Must be read before GracefulTerminate(): afterwards getpgid() fails + * with ESRCH. */ const pid_t pgid = getpgid(ALARM_PID); if (pgid == -1) { @@ -107,19 +100,10 @@ void TimeOut() GracefulTerminate(ALARM_PID, PROCESS_START_TIME_UNKNOWN); #ifndef __MINGW32__ - /* GracefulTerminate() only reaches the process we started. Anything that - * process spawned survives it, and keeps the pipe open, so the caller - * stays blocked reading a command it has already given up on. - * - * Guarded on the timed-out process leading its own group, which - * cf_popen()'s child arranges with setpgid(). If that did not take - * effect the process is still in our group, its pgid is not its pid, and - * a negative kill() here would signal an unrelated group -- possibly our - * own. - * - * Windows has no POSIX process groups and no setpgid() in the child - * (cf_popen() lives in pipes_unix.c), so there is nothing to widen the - * signal to there. */ + /* GracefulTerminate() reaches only the process we started; its + * descendants keep the pipe open. The pgid check matters: if setpgid() + * in cf_popen()'s child did not take effect, the process is still in + * our group and a negative kill() would signal us. */ if (pgid == ALARM_PID) { kill(-ALARM_PID, SIGKILL); diff --git a/libpromises/timeout.h b/libpromises/timeout.h index a0738df710..d31675bc08 100644 --- a/libpromises/timeout.h +++ b/libpromises/timeout.h @@ -27,26 +27,20 @@ void SetTimeOut(int timeout); -/* Cancel a pending alarm and restore the default handler. Callers used to - * open-code this; it also has to clear the armed flag, so that a command which - * completes in time does not leave it set for the next, unrelated, child. It - * does not clear what TimeOutHasFired() and TimeOutSignalledProcess() report: - * that record stays readable after the disarm, until the next SetTimeOut(). */ +/* Cancel a pending alarm and restore the default handler. Does not clear what + * TimeOutHasFired()/TimeOutSignalledProcess() report; SetTimeOut() does. */ void ClearTimeOut(void); -/* True between SetTimeOut() arming the alarm and the alarm being disarmed. - * Consulted by code that forks a child which the timeout may have to - * terminate, to decide whether that child needs a process group of its own. */ +/* True between SetTimeOut() and the disarm. Tells a forking caller whether the + * child needs a process group of its own. */ bool TimeOutIsArmed(void); -/* True if the alarm armed by the last SetTimeOut() actually fired. Lets a - * caller report that a command was timed out even when the command's own exit - * status would otherwise read as success. Cleared by SetTimeOut(). */ +/* True if the last armed alarm fired, even if the command's exit status reads + * as success. Cleared by SetTimeOut(). */ bool TimeOutHasFired(void); -/* True if that alarm also had a process to signal. False means the command - * exceeded its timeout but was never terminated, which callers must not - * describe as a termination. */ +/* True if that alarm also had a process to signal. False means timed out but + * not terminated. */ bool TimeOutSignalledProcess(void); void TimeOut(void); time_t SetReferenceTime(void);