From 2bc769d0eb19f3a60d94bc09954ce785a67a041c Mon Sep 17 00:00:00 2001 From: "Mark A. Grondona" Date: Wed, 6 Nov 2024 14:35:34 +0000 Subject: [PATCH 1/6] imp: allow negative PID to be passed to imp_set_signal_child() Problem: The current IMP signal interface does not allow a negative PID to be passed to imp_set_signal_child(), and therefore signals can only be forwarded to the direct child of the IMP. This works well with `flux-imp exec`, where the child is a job shell which itself forwards signals, but doesn't work as well in other situations where the IMP is managing processes not running as a job. Allow a negative PID less than -1 to be passed to imp_set_signal_child(). This will allow the caller to request an entire process group be signaled, instead of just a single child PID. --- src/imp/signals.c | 2 +- src/imp/signals.h | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/imp/signals.c b/src/imp/signals.c index 670398c..4c55042 100644 --- a/src/imp/signals.c +++ b/src/imp/signals.c @@ -67,7 +67,7 @@ static void fwd_signal (int signum) if (count < 0) imp_warn ("Failed to forward SIGKILL: %s", strerror (errno)); } - else if (imp_child > 0) + else if (imp_child != -1) kill (imp_child, signum); } diff --git a/src/imp/signals.h b/src/imp/signals.h index 0bf0208..d9b1896 100644 --- a/src/imp/signals.h +++ b/src/imp/signals.h @@ -14,9 +14,10 @@ #include #include "imp_state.h" -/* Set the target of IMP signal forwarding +/* Set the target of IMP signal forwarding. `pid` may be less than -1, + * in which case the entire process group `-pid` will be signaled. */ -void imp_set_signal_child (pid_t child); +void imp_set_signal_child (pid_t pid); /* Setup RFC 15 standard IMP signal forwarding */ From 1bf67ee23204e242cd92c90033bdba29b99b2eea Mon Sep 17 00:00:00 2001 From: "Mark A. Grondona" Date: Wed, 6 Nov 2024 14:43:42 +0000 Subject: [PATCH 2/6] imp: run: put child in new process group Problem: Before the removal of `flux-imp kill`, commands invoked by `flux-imp run` were signaled by process group instead of an individual process ID. This worked well because `flux-imp run` replaced itself with the target command, and thus the command and its children were in a process group (set up by flux-core's libsubprocess). Now, the IMP itself will be in that process group, and if it tries to forward signals to the entire process group it will end up re-signaling itself. Call `setpgrp(2)` in the child to place it in its own process group. The IMP now has a process group to which it can forward signals. --- src/imp/run.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/imp/run.c b/src/imp/run.c index 06211dd..4b6725a 100644 --- a/src/imp/run.c +++ b/src/imp/run.c @@ -199,6 +199,12 @@ imp_run (struct imp_state *imp, /* unblock all signals */ imp_sigunblock_all (); + /* Place child in its own process group, so that parent IMP + * can signal the pgrp as a whole + */ + if (setpgrp () < 0) + imp_die (1, "setpgrp: %s", strerror (errno)); + if (setuid (geteuid()) < 0 || setgid (getegid()) < 0) imp_die (1, "setuid: %s", strerror (errno)); From 67023196c29efed389faab75845ca1f9fa8e7873 Mon Sep 17 00:00:00 2001 From: "Mark A. Grondona" Date: Wed, 6 Nov 2024 14:50:25 +0000 Subject: [PATCH 3/6] imp: forward signals to child's process group in `flux-imp run` Problem: `flux-imp run` only forwards signals to its direct child. When that child is a shell script running other processes, common signals like SIGINT and SIGTERM are blocked. Even if the direct child is not a shell, terminating only the IMP's child may leave stray procesess which do not immediately exit as desired. Deliver forwarded signals to the child's process group. Fixes #194 --- src/imp/run.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/imp/run.c b/src/imp/run.c index 4b6725a..32e1059 100644 --- a/src/imp/run.c +++ b/src/imp/run.c @@ -193,7 +193,7 @@ imp_run (struct imp_state *imp, if ((child = fork ()) < 0) imp_die (1, "run: fork: %s", strerror (errno)); - imp_set_signal_child (child); + imp_set_signal_child (-child); if (child == 0) { /* unblock all signals */ From 147bba5917391bf14cc98d2182f2a90a817a9e5c Mon Sep 17 00:00:00 2001 From: "Mark A. Grondona" Date: Wed, 6 Nov 2024 14:58:16 +0000 Subject: [PATCH 4/6] testsuite: fix cleanup in imp run tests Problem: A test_when_finished() call in t2002-imp-run.t references an unset variable. Fix the test_when_finished() call. --- t/t2002-imp-run.t | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t2002-imp-run.t b/t/t2002-imp-run.t index 60f7783..e25e67b 100755 --- a/t/t2002-imp-run.t +++ b/t/t2002-imp-run.t @@ -186,7 +186,7 @@ test_expect_success SUDO,NO_CHAIN_LINT 'flux-imp run: setuid IMP lingers' ' $SUDO $flux_imp run sleep & imp_pid=$! && wait_for_file $TESTDIR/sleep.pid && - test_when_finished "$SUDO rm -f $pidfile" && + test_when_finished "$SUDO rm -f $TESTDIR/sleep.pid" && pid=$(cat $TESTDIR/sleep.pid) && test $(ps --no-header -o comm -p ${pid}) = "flux-imp" && kill -TERM $pid && From e4de116cca7140dd26a50ea2c495d36ce8ebd100 Mon Sep 17 00:00:00 2001 From: "Mark A. Grondona" Date: Wed, 6 Nov 2024 17:33:51 +0000 Subject: [PATCH 5/6] imp: reset all ignored signals in imp_sigunblock_all() Problem: The IMP does not reset signals that are ignored before executing child processes, but SIG_IGN is inherited across exec(2), so this may leave the IMP unable to forward certain signals like SIGINT to its child processes. Reset all signals from 1 (SIGHUP) to SIGRTMIN to SIG_DFL in imp_sigunblock_all() so that no signals are ignored before calling exec(2). --- src/imp/signals.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/imp/signals.c b/src/imp/signals.c index 4c55042..c4314df 100644 --- a/src/imp/signals.c +++ b/src/imp/signals.c @@ -37,12 +37,34 @@ void imp_sigblock_all (void) imp_die (1, "failed to block signals: %s", strerror (errno)); } +static void reset_ignored_signals (void) +{ + struct sigaction sa; + int i; + + memset (&sa, 0, sizeof(sa)); + sa.sa_handler = SIG_DFL; + sa.sa_flags = 0; + sigemptyset (&sa.sa_mask); + + for (i = 1; i < SIGRTMIN; i++) { + /* Note: it is expected that some signals will fail sigaction(2) + * here (e.g. SIGKILL). Just ignore these errors. + */ + (void) sigaction (i, &sa, NULL); + } +} + void imp_sigunblock_all (void) { sigset_t mask; sigemptyset (&mask); if (sigprocmask (SIG_SETMASK, &mask, NULL) < 0) imp_die (1, "failed to unblock signals: %s", strerror (errno)); + + /* Also need to reset any signals that may be currently ignored + */ + reset_ignored_signals (); } static void fwd_signal (int signum) From f1f6b936c880a2b15dadeaeaf66b8e98dd09f7a1 Mon Sep 17 00:00:00 2001 From: "Mark A. Grondona" Date: Wed, 6 Nov 2024 17:36:57 +0000 Subject: [PATCH 6/6] testsuite: test SIGINT handling in t2002-imp-run.t Problem: t2002-imp-run.t tests that `flux-imp run` handles SIGTERM as a side effect of the "linger" test, but SIGINT isn't tested and this signal has been shown to specifically have problems (when run under sudo(1) SIGINT was ignored). Add a test that ensures SIGINT can be forwarded by the IMP with `flux-imp run`. --- t/t2002-imp-run.t | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/t/t2002-imp-run.t b/t/t2002-imp-run.t index e25e67b..b3c416d 100755 --- a/t/t2002-imp-run.t +++ b/t/t2002-imp-run.t @@ -192,6 +192,15 @@ test_expect_success SUDO,NO_CHAIN_LINT 'flux-imp run: setuid IMP lingers' ' kill -TERM $pid && test_expect_code 143 wait $imp_pid ' +test_expect_success SUDO,NO_CHAIN_LINT 'flux-imp run: setuid IMP can successfully forward SIGINT' ' + $SUDO $flux_imp run sleep & + imp_pid=$! && + wait_for_file $TESTDIR/sleep.pid && + test_when_finished "$SUDO rm -f $TESTDIR/sleep.pid" && + pid=$(cat $TESTDIR/sleep.pid) && + kill -INT $pid && + test_expect_code 130 wait $imp_pid +' test_expect_success SUDO 'flux-imp run will not run file with bad ownership' ' $SUDO chown $USER $TESTDIR/test.sh && test_must_fail $SUDO $flux_imp run test &&