From 866abacf8805a74fb7c59cae1f64963e0a540b14 Mon Sep 17 00:00:00 2001 From: Chris Down Date: Tue, 1 Nov 2022 00:17:21 +0000 Subject: [PATCH] pgrep: Support matching on 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 organisation 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 -H/--require-handler flag, which matches on processes with a userspace handler present for the signal being sent. 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 --- man/pgrep.1 | 17 +++++++++--- src/pgrep.c | 49 +++++++++++++++++++++++++++------- testsuite/pgrep.test/pgrep.exp | 3 ++- testsuite/pwait.test/pwait.exp | 6 ++++- 4 files changed, 59 insertions(+), 16 deletions(-) diff --git a/man/pgrep.1 b/man/pgrep.1 index 57ddc4e3..64e610dd 100644 --- a/man/pgrep.1 +++ b/man/pgrep.1 @@ -7,7 +7,7 @@ .\" the Free Software Foundation; either version 2 of the License, or .\" (at your option) any later version. .\" -.TH PGREP "1" "2022-08-31" "procps-ng" "User Commands" +.TH PGREP "1" "2022-11-01" "procps-ng" "User Commands" .SH NAME pgrep, pkill, pidwait \- look up, signal, or wait for processes based on name and other attributes .SH SYNOPSIS @@ -53,9 +53,14 @@ will wait for each process instead of listing them on stdout. .TQ \fB\-\-signal\fR \fIsignal\fR Defines the signal to send to each matched process. Either the numeric or -the symbolic signal name can be used. -.RB ( pkill -only.) +the symbolic signal name can be used. In +.B pgrep +or +.B pidwait +mode this has no effect unless used in conjunction with +\fB\-\-require\-handler\fR to filter to processes with a userspace signal +handler present for a particular signal. + .TP \fB\-c\fR, \fB\-\-count\fR Suppress normal output; instead print a count of matching processes. When @@ -187,6 +192,10 @@ For example, this can be useful when elevating with .BR sudo or similar tools. .TP +\fB\-H\fR, \fB\-\-require\-handler\fR\fR +Only match processes with a userspace signal handler present for the signal to +be sent. +.TP \fB\-\-cgroup \fIname\fP,... Match on provided control group (cgroup) v2 name. See .BR cgroups (8) diff --git a/src/pgrep.c b/src/pgrep.c index eeebf3e8..7d0cc932 100644 --- a/src/pgrep.c +++ b/src/pgrep.c @@ -75,12 +75,13 @@ enum pids_item Items[] = { PIDS_CMDLINE, PIDS_STATE, PIDS_TIME_ELAPSED, - PIDS_CGROUP_V + PIDS_CGROUP_V, + PIDS_SIGCATCH }; enum rel_items { EU_PID, EU_PPID, EU_PGRP, EU_EUID, EU_RUID, EU_RGID, EU_SESSION, EU_TGID, EU_STARTTIME, EU_TTYNAME, EU_CMD, EU_CMDLINE, EU_STA, EU_ELAPSED, - EU_CGROUP + EU_CGROUP, EU_SIGCATCH }; #define grow_size(x) do { \ if ((x) < 0 || (size_t)(x) >= INT_MAX / 5 / sizeof(struct el)) \ @@ -119,6 +120,7 @@ static int opt_echo = 0; static int opt_threads = 0; static pid_t opt_ns_pid = 0; static bool use_sigqueue = false; +static bool require_handler = false; static union sigval sigval = {0}; static const char *opt_delim = "\n"; @@ -157,7 +159,7 @@ static int __attribute__ ((__noreturn__)) usage(int opt) fputs(_(" -w, --lightweight list all TID\n"), fp); break; case PKILL: - fputs(_(" -, --signal signal to send (either number or name)\n"), fp); + fputs(_(" -H, --require-handler match only if signal handler is present\n"), fp); fputs(_(" -q, --queue integer value to be sent with the signal\n"), fp); fputs(_(" -e, --echo display what is killed\n"), fp); break; @@ -167,6 +169,7 @@ static int __attribute__ ((__noreturn__)) usage(int opt) break; #endif } + fputs(_(" -, --signal signal to send (either number or name)\n"), fp); fputs(_(" -c, --count count of matching processes\n"), fp); fputs(_(" -f, --full use full process name to match\n"), fp); fputs(_(" -g, --pgroup match listed process group IDs\n"), fp); @@ -212,7 +215,7 @@ static struct el *get_our_ancestors(void) while (!done) { struct pids_info *info = NULL; - if (procps_pids_new(&info, Items, 15) < 0) + if (procps_pids_new(&info, Items, 16) < 0) xerrx(EXIT_FATAL, _("Unable to create pid info structure")); if (i == size) { @@ -472,6 +475,24 @@ static int match_numlist (long value, const struct el *restrict list) return found; } +static unsigned long long unhex (const char *restrict in) +{ + unsigned long long ret; + char *rem; + errno = 0; + ret = strtoull(in, &rem, 16); + if (errno || *rem != '\0') { + xwarnx(_("not a hex string: %s"), in); + return 0; + } + return ret; +} + +static int match_signal_handler (const char *restrict sigcgt, const int signal) +{ + return sigcgt && (((1UL << (signal - 1)) & unhex(sigcgt)) != 0); +} + static int match_strlist (const char *restrict value, const struct el *restrict list) { int found = 0; @@ -648,7 +669,7 @@ static struct el * select_procs (int *num) _("Error reading reference namespace information\n")); } - if (procps_pids_new(&info, Items, 15) < 0) + if (procps_pids_new(&info, Items, 16) < 0) xerrx(EXIT_FATAL, _("Unable to create pid info structure")); which = PIDS_FETCH_TASKS_ONLY; @@ -691,6 +712,8 @@ static struct el * select_procs (int *num) match = 0; else if (opt_cgroup && ! match_cgroup_list (PIDS_GETSTV(CGROUP), opt_cgroup)) match = 0; + else if (require_handler && ! match_signal_handler (PIDS_GETSTR(SIGCATCH), opt_signal)) + match = 0; task_cmdline = PIDS_GETSTR(CMDLINE); @@ -796,6 +819,7 @@ static int pidfd_open (pid_t pid, unsigned int flags) static void parse_opts (int argc, char **argv) { char opts[64] = ""; + int sig; int opt; int criteria_count = 0; @@ -808,6 +832,7 @@ static void parse_opts (int argc, char **argv) static const struct option longopts[] = { {"signal", required_argument, NULL, SIGNAL_OPTION}, {"ignore-ancestors", no_argument, NULL, 'A'}, + {"require-handler", no_argument, NULL, 'H'}, {"count", no_argument, NULL, 'c'}, {"cgroup", required_argument, NULL, CGROUP_OPTION}, {"delimiter", required_argument, NULL, 'd'}, @@ -840,6 +865,10 @@ static void parse_opts (int argc, char **argv) {NULL, 0, NULL, 0} }; + sig = signal_option(&argc, argv); + if (-1 < sig) + opt_signal = sig; + #ifdef ENABLE_PIDWAIT if (strcmp (program_invocation_short_name, "pidwait") == 0 || strcmp (program_invocation_short_name, "lt-pidwait") == 0) { @@ -849,18 +878,14 @@ static void parse_opts (int argc, char **argv) #endif if (strcmp (program_invocation_short_name, "pkill") == 0 || strcmp (program_invocation_short_name, "lt-pkill") == 0) { - int sig; prog_mode = PKILL; - sig = signal_option(&argc, argv); - if (-1 < sig) - opt_signal = sig; strcat (opts, "eq:"); } else { strcat (opts, "lad:vw"); prog_mode = PGREP; } - strcat (opts, "LF:cfinoxP:O:Ag:s:u:U:G:t:r:?Vh"); + strcat (opts, "LF:cfinoxP:O:AHg:s:u:U:G:t:r:?Vh"); while ((opt = getopt_long (argc, argv, opts, longopts, NULL)) != -1) { switch (opt) { @@ -1017,6 +1042,10 @@ static void parse_opts (int argc, char **argv) usage ('?'); ++criteria_count; break; + case 'H': + require_handler = true; + ++criteria_count; + break; case 'h': case '?': usage (opt); diff --git a/testsuite/pgrep.test/pgrep.exp b/testsuite/pgrep.test/pgrep.exp index e57e044d..1110e70f 100644 --- a/testsuite/pgrep.test/pgrep.exp +++ b/testsuite/pgrep.test/pgrep.exp @@ -20,6 +20,7 @@ make_testproc set testproc_len [ string length $testproc_comm ] set testproc_trim [ string range $testproc_comm 0 [ expr { $testproc_len - 2 } ] ] set testproc1_sid [ string trim [ exec $ps --no-headers -o sid $testproc1_pid ] ] +set not_testproc1_sid [ expr { $testproc1_sid + 1 } ] set test "pgrep find both test pids" spawn $pgrep $testproc_comm @@ -75,7 +76,7 @@ spawn $pgrep -s $testproc1_sid $testproc_comm expect_pass "$test" "^$testproc1_pid\\s+$testproc2_pid\\s*$" set test "pgrep doesn't match with bogus sid" -spawn $pgrep -s -1 $testproc_comm +spawn $pgrep -s $not_testproc1_sid $testproc_comm expect_blank "$test" set test "pgrep matches on tty" diff --git a/testsuite/pwait.test/pwait.exp b/testsuite/pwait.test/pwait.exp index 5ec6bed6..060024f0 100644 --- a/testsuite/pwait.test/pwait.exp +++ b/testsuite/pwait.test/pwait.exp @@ -20,6 +20,10 @@ expect_pass "$test" "^\(lt-\)\?pwait: no matching criteria specified\\s*" make_testproc +set ps "${topdir}src/ps/pscommand" +set testproc1_sid [ string trim [ exec $ps --no-headers -o sid $testproc1_pid ] ] +set not_testproc1_sid [ expr { $testproc1_sid + 1 } ] + set test "pwait with not matching gid" spawn $pwait -G $not_gid $testproc_comm expect_blank $test @@ -29,7 +33,7 @@ spawn $pwait -P $not_ppid $testproc_comm expect_blank "$test" set test "pwait doesn't match with bogus sid" -spawn $pwait -s -1 $testproc_comm +spawn $pwait -s $not_testproc1_sid $testproc_comm expect_blank "$test" set test "pwait doesn't match with bogus tty"