CFE-4729: Kill a timed-out command's whole process group - #6305
Open
djbclark wants to merge 9 commits into
Open
Conversation
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
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
…out 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
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
Combines the exec_timeout outcome reporting of the previous two commits with the process-group kill for descendants of a timed-out command. The two changes edit SetTimeOut() and TimeOut() on adjacent lines but are complementary: one records what the timeout did so the promise can be classified on it, the other widens what the timeout kills. Resolution decisions, beyond keeping both sides' lines: - SetTimeOut() now sets all three flags: it clears the fired/signalled record of the previous timeout and arms the new one. - TimeOut() sets TIMEOUT_FIRED and clears TIMEOUT_ARMED on entry; inside the ALARM_PID != -1 branch it sets TIMEOUT_SIGNALLED and performs the group kill, in that branch only, since both describe having had a process to signal. - ClearTimeOut() clears only TIMEOUT_ARMED. It must not clear TIMEOUT_FIRED or TIMEOUT_SIGNALLED: RepairExec() samples the fired flag right after cf_pclose() and evaluates the signalled flag while reporting, both before its ClearTimeOut() call, and the record is meant to stay readable until the next SetTimeOut() re-arms. Clearing it here would make the disarm order load-bearing for every future caller and silently revert timed-out promises to being judged on their exit status if a caller ever disarmed first. Comments in timeout.h and ClearTimeOut() now state that contract.
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
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
…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
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A
commands:promise's descendants surviveexec_timeout.GracefulTerminate()reaches only the direct child; if that child is a shell runningsleep 30; exit 0, killing the shell does not stop thesleep, which keeps the pipe open, so the agent stays blocked reading a command it already gave up on.exec_timeoutbounds nothing in that shape.The change
The timed command's child now leads a process group of its own, but only while a timeout is armed (
TimeOutIsArmed(), checked incf_popen()'s child viasetpgid(0, 0)) — not unconditionally. Unconditional grouping was tried and reverted: it detaches every piped child from the terminal's foreground group, so an interactivetype => "r"command that reads stdin is stopped bySIGTTINthe moment it does, with nothing to end the resulting hang.TimeOut()now signals that group (kill(-ALARM_PID, SIGKILL)) afterGracefulTerminate(), guarded ongetpgid(ALARM_PID) == ALARM_PID— only when the child actually led its own group, so asetpgid()failure degrades to the pre-existing single-process kill rather than signalling something unrelated.SetTimeOut()/ClearTimeOut()/TimeOut()track the armed state inTIMEOUT_ARMED(volatile sig_atomic_t, written from the signal handler). The four open-codedalarm(0); signal(SIGALRM, SIG_DFL);call sites now go throughClearTimeOut()instead, so a command finishing in time cannot leave the flag set for the next, unrelated, child.Windows has no POSIX process groups;
getpgid()/kill(-pid, ...)are guarded#ifndef __MINGW32__, matchingpipes_unix.c(thesetpgid()half) already beingif !NTonly.Supersedes #6299
This branch is #6299 (
fix/exec-timeout-promise-outcome,TIMEOUT_FIRED/TIMEOUT_SIGNALLEDoutcome reporting) merged with this process-group fix — both touchSetTimeOut()/TimeOut()/cf-agent/verify_exec.c, and #6299'sClearTimeOut()had to survive the merge without disturbing what it samples. If #6299 lands separately, this rebases down to the process-group commits alone; if not, this supersedes it and #6299 can close in favor of it.Tests
Unit:
tests/unit/timeout_test.c, 6 cases pinningSetTimeOut()/ClearTimeOut()/TimeOut()'s armed/fired/signalled contract, including one that forks a real child so aClearTimeOut()that wipes a trueTimeOutSignalledProcess()(not just a false one) is caught.if !NT— waits on a realSIGALRM.Acceptance: the six tests already in
tests/acceptance/08_commands/04_exec_timeout/(from #6299) pass unchanged, includingtimeout_kills_descendants.cf.Discrimination on the two new commits, individually:
ClearTimeOut()/TimeOut()each failing to disarm,ClearTimeOut()clearingTIMEOUT_FIRED/TIMEOUT_SIGNALLED, andClearTimeOut()clearing a trueTIMEOUT_SIGNALLED— each breaks exactly one unit test. Full rebuild clean (0 warnings), source restores byte-identical by sha256 throughout.Cut from master
17eb78e6d. Tracked as CFE-4729.