CFE-4729: Kill a timed-out command's whole process group

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);
 }
 

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);
@@ -472,8 +499,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);
@@ -492,7 +518,7 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a,
     }
 #endif /* !__MINGW32__ */
 
-    return ACTION_RESULT_OK;
+    return timed_out ? ACTION_RESULT_TIMEOUT : ACTION_RESULT_OK;
 }
 
 /*************************************************************/

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);

libpromises/pipes_unix.c

@@ -33,6 +33,7 @@
 #include <file_lib.h>
 #include <signals.h>
 #include <string_lib.h>
+#include <timeout.h>
 
 static bool CfSetuid(uid_t uid, gid_t gid);
 
@@ -236,6 +237,36 @@ static pid_t GenericCreatePipeAndFork(IOPipe *pipes)
         sigset_t sigmask;
         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.
+         *
+         * 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())
+        {
+            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());
+            }
+        }
     }
 
     ALARM_PID = (pid != 0 ? pid : -1);

libpromises/timeout.c

@@ -26,23 +26,105 @@
 #include <timeout.h>
 #include <process_lib.h>
 
+/* 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 */
+
+/* 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. */
+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 = 1;
     signal(SIGALRM, (void *) TimeOut);
     alarm(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. */
+    alarm(0);
+    signal(SIGALRM, SIG_DFL);
+    TIMEOUT_ARMED = 0;
+}
+
+bool TimeOutIsArmed(void)
+{
+    return TIMEOUT_ARMED != 0;
+}
+
+bool TimeOutHasFired(void)
+{
+    return TIMEOUT_FIRED != 0;
+}
+
+bool TimeOutSignalledProcess(void)
+{
+    return TIMEOUT_SIGNALLED != 0;
+}
+
 /*************************************************************************/
 
 void TimeOut()
 {
     alarm(0);
+    TIMEOUT_FIRED = 1;
+    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. */
+        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());
+        }
+#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.
+         *
+         * 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. */
+        if (pgid == ALARM_PID)
+        {
+            kill(-ALARM_PID, SIGKILL);
+        }
+#endif
     }
     else
     {

libpromises/timeout.h

@@ -26,6 +26,28 @@
 #define CFENGINE_TIMEOUT_H
 
 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(). */
+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. */
+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(). */
+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);
 

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