summaryrefslogtreecommitdiffstats
path: root/debian/patches/upstream/0007-kill-Support-mandating-the-presence-of-a-userspace-s.patch
diff options
context:
space:
mode:
Diffstat (limited to '')
-rw-r--r--debian/patches/upstream/0007-kill-Support-mandating-the-presence-of-a-userspace-s.patch147
1 files changed, 147 insertions, 0 deletions
diff --git a/debian/patches/upstream/0007-kill-Support-mandating-the-presence-of-a-userspace-s.patch b/debian/patches/upstream/0007-kill-Support-mandating-the-presence-of-a-userspace-s.patch
new file mode 100644
index 0000000..7fcd490
--- /dev/null
+++ b/debian/patches/upstream/0007-kill-Support-mandating-the-presence-of-a-userspace-s.patch
@@ -0,0 +1,147 @@
+From: Chris Down <chris@chrisdown.name>
+Date: Wed, 26 Oct 2022 15:47:36 +0100
+Subject: [PATCH 07/24] kill: Support mandating the presence of a userspace
+ signal handler
+
+In production we've had several incidents over the years where a process
+has a signal handler registered for SIGHUP or one of the SIGUSR signals
+which can be used to signal a request to reload configs, rotate log
+files, and the like. While this may seem harmless enough, what we've
+seen happen repeatedly is something like the following:
+
+1. A process is using SIGHUP/SIGUSR[12] to request some
+ application-handled state change -- reloading configs, rotating a log
+ file, etc;
+2. This kind of request is deprecated and removed, so the signal handler
+ is removed. However, a site where the signal might be sent from is
+ missed (often logrotate or a service manager);
+3. Because the default disposition of these signals is terminal, sooner
+ or later these applications are going to be sent SIGHUP or similar
+ and end up unexpectedly killed.
+
+I know for a fact that we're not the only organistion experiencing this:
+in general, signal use is pretty tricky to reason about and safely
+remove because of the fairly aggressive SIG_DFL behaviour for some
+common signals, especially for SIGHUP which has a particularly ambiguous
+meaning. Especially in a large, highly interconnected codebase,
+reasoning about signal interactions between system configuration and
+applications can be highly complex, and it's inevitable that on occasion
+a callsite will be missed.
+
+In some cases the right call to avoid this will be to migrate services
+towards other forms of IPC for this purpose, but inevitably there will
+be some services which must continue using signals, so we need a safe
+way to support them.
+
+This patch adds support for the -r/--require-handler flag, which checks
+if a userspace handler is present for the signal being sent. If it is
+not, the process will be skipped.
+
+With this flag we can enforce that all SIGHUP reload cases and SIGUSR
+equivalents use --require-handler. This effectively mitigates the case
+we've seen time and time again where SIGHUP is used to rotate log files
+or reload configs, but the sending site is mistakenly left present after
+the removal of signal handler, resulting in unintended termination of
+the process.
+
+Signed-off-by: Chris Down <chris@chrisdown.name>
+---
+ misc-utils/kill.1.adoc | 2 ++
+ misc-utils/kill.c | 36 ++++++++++++++++++++++++++++++++++++
+ 2 files changed, 38 insertions(+)
+
+diff --git a/misc-utils/kill.1.adoc b/misc-utils/kill.1.adoc
+index 4a6996a..40ab024 100644
+--- a/misc-utils/kill.1.adoc
++++ b/misc-utils/kill.1.adoc
+@@ -62,6 +62,8 @@ Similar to *-l*, but it will print signal names and their corresponding numbers.
+ Do not restrict the command-name-to-PID conversion to processes with the same UID as the present process.
+ *-p*, *--pid*::
+ Only print the process ID (PID) of the named processes, do not send any signals.
++*-r*, *--require-handler*::
++Do not send the signal if it is not caught in userspace by the signalled process.
+ *--verbose*::
+ Print PID(s) that will be signaled with *kill* along with the signal.
+ *-q*, *--queue* _value_::
+diff --git a/misc-utils/kill.c b/misc-utils/kill.c
+index f557aac..c469074 100644
+--- a/misc-utils/kill.c
++++ b/misc-utils/kill.c
+@@ -95,6 +95,7 @@ struct kill_control {
+ check_all:1,
+ do_kill:1,
+ do_pid:1,
++ require_handler:1,
+ use_sigval:1,
+ #ifdef UL_HAVE_PIDFD
+ timeout:1,
+@@ -212,6 +213,7 @@ static void __attribute__((__noreturn__)) usage(void)
+ fputs(_(" -p, --pid print pids without signaling them\n"), out);
+ fputs(_(" -l, --list[=<signal>] list signal names, or convert a signal number to a name\n"), out);
+ fputs(_(" -L, --table list signal names and numbers\n"), out);
++ fputs(_(" -r, --require-handler do not send signal if signal handler is not present\n"), out);
+ fputs(_(" --verbose print pids that will be signaled\n"), out);
+
+ fputs(USAGE_SEPARATOR, out);
+@@ -302,6 +304,10 @@ static char **parse_arguments(int argc, char **argv, struct kill_control *ctl)
+ print_all_signals(stdout, 1);
+ exit(EXIT_SUCCESS);
+ }
++ if (!strcmp(arg, "-r") || !strcmp(arg, "--require-handler")) {
++ ctl->require_handler = 1;
++ continue;
++ }
+ if (!strcmp(arg, "-p") || !strcmp(arg, "--pid")) {
+ ctl->do_pid = 1;
+ if (ctl->do_kill)
+@@ -448,6 +454,32 @@ static int kill_verbose(const struct kill_control *ctl)
+ return rc;
+ }
+
++static int check_signal_handler(const struct kill_control *ctl)
++{
++ uintmax_t sigcgt = 0;
++ int rc = 0, has_hnd = 0;
++ struct path_cxt *pc;
++
++ if (!ctl->require_handler)
++ return 1;
++
++ pc = ul_new_procfs_path(ctl->pid, NULL);
++ if (!pc)
++ return -ENOMEM;
++
++ rc = procfs_process_get_stat_nth(pc, 34, &sigcgt);
++ if (rc)
++ return -EINVAL;
++
++ ul_unref_path(pc);
++
++ has_hnd = ((1UL << (ctl->numsig - 1)) & sigcgt) != 0;
++ if (ctl->verbose && !has_hnd)
++ printf(_("not signalling pid %d, it has no userspace handler for signal %d\n"), ctl->pid, ctl->numsig);
++
++ return has_hnd;
++}
++
+ int main(int argc, char **argv)
+ {
+ struct kill_control ctl = { .numsig = SIGTERM };
+@@ -470,6 +502,8 @@ int main(int argc, char **argv)
+ errno = 0;
+ ctl.pid = strtol(ctl.arg, &ep, 10);
+ if (errno == 0 && ep && *ep == '\0' && ctl.arg < ep) {
++ if (check_signal_handler(&ctl) <= 0)
++ continue;
+ if (kill_verbose(&ctl) != 0)
+ nerrs++;
+ ct++;
+@@ -491,6 +525,8 @@ int main(int argc, char **argv)
+ continue;
+ if (procfs_dirent_get_pid(d, &ctl.pid) != 0)
+ continue;
++ if (check_signal_handler(&ctl) <= 0)
++ continue;
+
+ if (kill_verbose(&ctl) != 0)
+ nerrs++;