diff --git a/NEWS b/NEWS index c810f22c93..1614fc4940 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,12 @@ CHANGES WITH 262: Feature Removals and Incompatible Changes: + * Services of Type=notify-reload are now required to catch or block + ReloadSignal= when they send READY=1. If they do neither at that + point, the service fails to start with a protocol error. Previously, + such services were allowed to start even though a later reload could + invoke the signal's default action and terminate the service. + * systemd-repart's MakeSymlinks= option now expands "%" specifiers in the symlink target. Previously, these were only expanded in the source. diff --git a/man/systemd.service.xml b/man/systemd.service.xml index 69600b61e2..57bb224c24 100644 --- a/man/systemd.service.xml +++ b/man/systemd.service.xml @@ -256,6 +256,26 @@ command for reloading of the service's configuration. The signal to send can be tweaked via ReloadSignal=, see below. + + For services, systemd verifies that the service's main process + has actually installed a handler for the configured reload signal (by default + SIGHUP). This verification checks for both traditional signal handlers (via + sigaction2) + and blocked signals that are typically handled via + signalfd2. + This verification is performed at two points: + + + When the service first sends READY=1 during initial startup. If no + signal handler is detected at this point, the service startup is aborted. + + Immediately before sending the reload signal. If the handler is no longer present, + systemd logs a warning but the reload signal is still sent. + + + This prevents accidentally starting services that lack the required signal handler. Services + using must install their reload signal handler before sending the + initial READY=1 notification. Behavior of is very similar to ; however, diff --git a/src/basic/process-util.c b/src/basic/process-util.c index ab563c5105..ececbf97ac 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -21,6 +21,7 @@ #include "alloc-util.h" #include "architecture.h" #include "argv-util.h" +#include "bitfield.h" #include "capability-util.h" #include "cgroup-util.h" #include "dirent-util.h" @@ -804,6 +805,53 @@ int get_process_umask(pid_t pid, mode_t *ret) { return parse_mode(m, ret); } +static int pid_get_sigmask(pid_t pid, const char *field_name, uint64_t *ret) { + _cleanup_free_ char *field = NULL; + int r; + + assert(pid >= 0); + assert(field_name); + assert(ret); + + r = procfs_file_get_field(pid, "status", field_name, &field); + if (r == -ENOENT) + return -ESRCH; + if (r < 0) + return r; + + return safe_atou64_full(field, 16, ret); +} + +static int pidref_has_sigmask(const PidRef *pidref, int sig, const char *field_name) { + uint64_t mask; + int r; + + if (!pidref_is_set(pidref)) + return -ESRCH; + if (pidref_is_remote(pidref)) + return -EREMOTE; + if (!SIGNAL_VALID(sig)) + return -EINVAL; + + r = pid_get_sigmask(pidref->pid, field_name, &mask); + if (r < 0) + return r; + + r = pidref_verify(pidref); + if (r < 0) + return r; + + return BIT_SET(mask, sig - 1); +} + +int pidref_has_sigcgt(const PidRef *pidref, int sig) { + return pidref_has_sigmask(pidref, sig, "SigCgt"); +} + +int pidref_has_sigblk(const PidRef *pidref, int sig) { + return pidref_has_sigmask(pidref, sig, "SigBlk"); +} + /* * Return values: * < 0 : pidref_wait_for_terminate() failed to get the state of the diff --git a/src/basic/process-util.h b/src/basic/process-util.h index 007397704e..56514c8ed8 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -53,6 +53,8 @@ int pidref_get_ppid_as_pidref(const PidRef *pidref, PidRef *ret); int pid_get_start_time(pid_t pid, usec_t *ret); int pidref_get_start_time(const PidRef *pid, usec_t *ret); int get_process_umask(pid_t pid, mode_t *ret); +int pidref_has_sigcgt(const PidRef *pidref, int sig); +int pidref_has_sigblk(const PidRef *pidref, int sig); static inline bool SIGINFO_CODE_IS_DEAD(int code) { return IN_SET(code, CLD_EXITED, CLD_KILLED, CLD_DUMPED); diff --git a/src/core/service.c b/src/core/service.c index 1c8460b136..1918147525 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -3137,6 +3137,53 @@ static void service_enter_reload_post(Service *s) { service_reload_finish(s, SERVICE_SUCCESS); } +static int service_check_reload_signal_handler(Service *s, const char *missing_suffix, const char *error_suffix) { + int r; + + assert(s); + + if (!pidref_is_set(&s->main_pid) || IN_SET(s->reload_signal, SIGKILL, SIGSTOP)) + return 0; + + /* Check if the process has a traditional signal handler (SigCgt) */ + r = pidref_has_sigcgt(&s->main_pid, s->reload_signal); + if (r < 0) { + if (r != -ESRCH) + log_unit_warning_errno( + UNIT(s), r, + "Failed to check for reload signal handler%s: %m", + strempty(error_suffix)); + return r; + } + + if (r == 0) { + /* No traditional handler, check if the signal is blocked (SigBlk). A blocked signal is + * typically handled via signalfd, which is a valid way to handle signals (e.g., via + * sd_event_add_signal() with SD_EVENT_SIGNAL_PROCMASK). */ + r = pidref_has_sigblk(&s->main_pid, s->reload_signal); + if (r < 0) { + if (r != -ESRCH) + log_unit_warning_errno( + UNIT(s), r, + "Failed to check for blocked reload signal%s: %m", + strempty(error_suffix)); + return r; + } + } + + if (r == 0) { + log_unit_warning( + UNIT(s), + "Main process " PID_FMT " lacks handler for reload signal %s%s.", + s->main_pid.pid, + signal_to_string(s->reload_signal), + strempty(missing_suffix)); + return -EOPNOTSUPP; + } + + return 0; +} + static void service_enter_reload_signal(Service *s) { int r; @@ -3161,6 +3208,14 @@ static void service_enter_reload_signal(Service *s) { goto fail; } + /* This is naturally racy, but that's fine. The issue we're looking for is almost always a + * static programming error (handler not yet installed), but if a user wants to shoot + * themself in the foot intentionally by racing with us, who are we to stop them :-) */ + (void) service_check_reload_signal_handler( + s, + ", sending reload signal anyway", + ", sending reload signal anyway"); + r = pidref_kill_and_sigcont(&s->main_pid, s->reload_signal); if (r < 0) { log_unit_warning_errno(UNIT(s), r, "Failed to send reload signal: %m"); @@ -5399,6 +5454,14 @@ static void service_notify_message_process_state(Service *s, char * const *tags) if (strv_contains(tags, "READY=1")) { + if (s->type == SERVICE_NOTIFY_RELOAD && s->state == SERVICE_START) { + r = service_check_reload_signal_handler(s, ", refusing service startup", ", ignoring"); + if (r == -EOPNOTSUPP) { + service_enter_signal(s, SERVICE_STOP_SIGTERM, SERVICE_FAILURE_PROTOCOL); + return; + } + } + if (s->notify_state == NOTIFY_RELOADING) s->notify_state = NOTIFY_RELOAD_READY; else diff --git a/test/integration-tests/TEST-80-NOTIFYACCESS/TEST-80-NOTIFYACCESS.units/notify-reload-harness.sh b/test/integration-tests/TEST-80-NOTIFYACCESS/TEST-80-NOTIFYACCESS.units/notify-reload-harness.sh new file mode 100755 index 0000000000..c6a291199d --- /dev/null +++ b/test/integration-tests/TEST-80-NOTIFYACCESS/TEST-80-NOTIFYACCESS.units/notify-reload-harness.sh @@ -0,0 +1,44 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: LGPL-2.1-or-later +set -eux +set -o pipefail + +MODE="${1:-well-behaved}" +FIFO_IN="${2:-/tmp/reload-test-fifo-in}" +FIFO_OUT="${3:-/tmp/reload-test-fifo-out}" + +handle_signal() { + echo "reload" >"$FIFO_OUT" + systemd-notify --reloading + systemd-notify --ready --status="Reloaded" +} + +case "$MODE" in + no-handler) + ;; + toggle-handler|well-behaved) + trap 'handle_signal' HUP + ;; + *) + echo "Unknown mode: $MODE" >&2 + exit 1 + ;; +esac + +systemd-notify --ready --status="Started with mode=$MODE" + +while read -r cmd < "$FIFO_IN"; do + case "$cmd" in + remove-handler) + trap - HUP + echo "handler-removed" >"$FIFO_OUT" + ;; + exit) + break + ;; + *) + echo "Unknown command: $cmd" >&2 + exit 1 + ;; + esac +done diff --git a/test/integration-tests/TEST-80-NOTIFYACCESS/TEST-80-NOTIFYACCESS.units/notify-reload-no-handler.service b/test/integration-tests/TEST-80-NOTIFYACCESS/TEST-80-NOTIFYACCESS.units/notify-reload-no-handler.service new file mode 100644 index 0000000000..3e0152c332 --- /dev/null +++ b/test/integration-tests/TEST-80-NOTIFYACCESS/TEST-80-NOTIFYACCESS.units/notify-reload-no-handler.service @@ -0,0 +1,6 @@ +[Unit] +Description=Test notify-reload without signal handler + +[Service] +Type=notify-reload +ExecStart=/usr/lib/systemd/tests/testdata/TEST-80-NOTIFYACCESS.units/notify-reload-harness.sh no-handler /tmp/reload-nohandler-in /tmp/reload-nohandler-out diff --git a/test/integration-tests/TEST-80-NOTIFYACCESS/TEST-80-NOTIFYACCESS.units/notify-reload-sigstop.service b/test/integration-tests/TEST-80-NOTIFYACCESS/TEST-80-NOTIFYACCESS.units/notify-reload-sigstop.service new file mode 100644 index 0000000000..83c4314ae0 --- /dev/null +++ b/test/integration-tests/TEST-80-NOTIFYACCESS/TEST-80-NOTIFYACCESS.units/notify-reload-sigstop.service @@ -0,0 +1,7 @@ +[Unit] +Description=Test notify-reload with SIGSTOP + +[Service] +Type=notify-reload +ReloadSignal=SIGSTOP +ExecStart=/usr/lib/systemd/tests/testdata/TEST-80-NOTIFYACCESS.units/notify-reload-harness.sh no-handler /tmp/reload-sigstop-in /tmp/reload-sigstop-out diff --git a/test/integration-tests/TEST-80-NOTIFYACCESS/TEST-80-NOTIFYACCESS.units/notify-reload-toggle-handler.service b/test/integration-tests/TEST-80-NOTIFYACCESS/TEST-80-NOTIFYACCESS.units/notify-reload-toggle-handler.service new file mode 100644 index 0000000000..3f609c99e1 --- /dev/null +++ b/test/integration-tests/TEST-80-NOTIFYACCESS/TEST-80-NOTIFYACCESS.units/notify-reload-toggle-handler.service @@ -0,0 +1,6 @@ +[Unit] +Description=Test notify-reload with handler that can be removed + +[Service] +Type=notify-reload +ExecStart=/usr/lib/systemd/tests/testdata/TEST-80-NOTIFYACCESS.units/notify-reload-harness.sh toggle-handler /tmp/reload-toggle-in /tmp/reload-toggle-out diff --git a/test/integration-tests/TEST-80-NOTIFYACCESS/TEST-80-NOTIFYACCESS.units/notify-reload-well-behaved.service b/test/integration-tests/TEST-80-NOTIFYACCESS/TEST-80-NOTIFYACCESS.units/notify-reload-well-behaved.service new file mode 100644 index 0000000000..f81e2b8904 --- /dev/null +++ b/test/integration-tests/TEST-80-NOTIFYACCESS/TEST-80-NOTIFYACCESS.units/notify-reload-well-behaved.service @@ -0,0 +1,6 @@ +[Unit] +Description=Test notify-reload with proper signal handler + +[Service] +Type=notify-reload +ExecStart=/usr/lib/systemd/tests/testdata/TEST-80-NOTIFYACCESS.units/notify-reload-harness.sh well-behaved /tmp/reload-wellbehaved-in /tmp/reload-wellbehaved-out diff --git a/test/units/TEST-80-NOTIFYACCESS.sh b/test/units/TEST-80-NOTIFYACCESS.sh index 4439e46362..41e65c37c6 100755 --- a/test/units/TEST-80-NOTIFYACCESS.sh +++ b/test/units/TEST-80-NOTIFYACCESS.sh @@ -172,4 +172,85 @@ assert_eq "$(systemctl show fdstore-nopin.service -P NFileDescriptorStore)" 0 assert_eq "$(systemctl show fdstore-pin.service -P SubState)" dead assert_eq "$(systemctl show fdstore-nopin.service -P SubState)" dead +# Test notify-reload signal handler validation + +cleanup_notify_reload() { + systemctl stop notify-reload-no-handler.service \ + notify-reload-sigstop.service \ + notify-reload-toggle-handler.service \ + notify-reload-well-behaved.service || : + systemctl reset-failed notify-reload-no-handler.service \ + notify-reload-sigstop.service \ + notify-reload-toggle-handler.service \ + notify-reload-well-behaved.service || : + rm -f /tmp/reload-nohandler-in /tmp/reload-nohandler-out \ + /tmp/reload-nohandler-cursor \ + /tmp/reload-sigstop-in /tmp/reload-sigstop-out \ + /tmp/reload-toggle-in /tmp/reload-toggle-out \ + /tmp/reload-toggle-cursor \ + /tmp/reload-wellbehaved-in /tmp/reload-wellbehaved-out +} + +trap cleanup_notify_reload EXIT +cleanup_notify_reload + +# Test 1: Service without signal handler should fail to start +mkfifo /tmp/reload-nohandler-in /tmp/reload-nohandler-out +journalctl -q -n 0 --cursor-file=/tmp/reload-nohandler-cursor +(! systemctl start notify-reload-no-handler.service) +assert_eq "$(systemctl show notify-reload-no-handler.service -P SubState)" "failed" +assert_eq "$(systemctl show notify-reload-no-handler.service -P Result)" "protocol" +# Verify error was logged about missing signal handler +journalctl --sync +journalctl -u notify-reload-no-handler.service --cursor-file=/tmp/reload-nohandler-cursor \ + --grep "lacks handler for reload signal" >/dev/null +systemctl reset-failed notify-reload-no-handler.service +rm -f /tmp/reload-nohandler-in /tmp/reload-nohandler-out + +# Test 2: SIGSTOP cannot be caught or blocked, so it is exempt from handler validation +mkfifo /tmp/reload-sigstop-in /tmp/reload-sigstop-out +systemctl start notify-reload-sigstop.service +assert_eq "$(systemctl show notify-reload-sigstop.service -P SubState)" "running" +systemctl stop notify-reload-sigstop.service +rm -f /tmp/reload-sigstop-in /tmp/reload-sigstop-out + +# Test 3: Service that removes handler should warn on reload but still reload +mkfifo /tmp/reload-toggle-in /tmp/reload-toggle-out +systemctl start notify-reload-toggle-handler.service +assert_eq "$(systemctl show notify-reload-toggle-handler.service -P SubState)" "running" + +# Command service to remove its signal handler +timeout 10 bash -c 'echo "remove-handler" >/tmp/reload-toggle-in' +response="$(timeout 10 cat /tmp/reload-toggle-out)" +assert_eq "$response" "handler-removed" + +# Reload should succeed (with warning) even though handler is gone - service will be killed by SIGHUP +# but that's intentional after the first start succeeds +journalctl -q -n 0 --cursor-file=/tmp/reload-toggle-cursor +systemctl reload --no-block notify-reload-toggle-handler.service +# The service will die from SIGHUP since the handler is removed +timeout 10 bash -c 'while systemctl is-active --quiet notify-reload-toggle-handler.service; do sleep .5; done' +# Verify warning was logged +journalctl --sync +journalctl -u notify-reload-toggle-handler.service --cursor-file=/tmp/reload-toggle-cursor \ + --grep "lacks handler for reload signal" >/dev/null +rm -f /tmp/reload-toggle-in /tmp/reload-toggle-out + +# Test 4: Well-behaved service with handler should work correctly +mkfifo /tmp/reload-wellbehaved-in /tmp/reload-wellbehaved-out +systemctl start notify-reload-well-behaved.service +assert_eq "$(systemctl show notify-reload-well-behaved.service -P SubState)" "running" + +# Reload should succeed +systemctl reload --no-block notify-reload-well-behaved.service +timeout 10 bash -c 'until [[ $(systemctl show notify-reload-well-behaved.service -P SubState) == "reload-signal" ]]; do sleep .5; done' +response="$(timeout 10 cat /tmp/reload-wellbehaved-out)" +assert_eq "$response" "reload" +timeout 10 bash -c 'until [[ $(systemctl show notify-reload-well-behaved.service -P SubState) == "running" ]]; do sleep .5; done' +assert_eq "$(systemctl show notify-reload-well-behaved.service -P ReloadResult)" "success" + +timeout 10 bash -c 'echo "exit" >/tmp/reload-wellbehaved-in' +systemctl stop notify-reload-well-behaved.service +rm -f /tmp/reload-wellbehaved-in /tmp/reload-wellbehaved-out + touch /testok