From e203524b06206221d4a0e466bd10eb8a6f2bff03 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 16 Dec 2024 01:54:11 +0100 Subject: [PATCH] core/socket: introduce intermediate SOCKET_START_OPEN state Prior to this commit, if no Exec*= is defined for socket, and the unit was in SOCKET_FAILED state, failure of socket_open_fds() would induce state transition SOCKET_FAILED -> SOCKET_FAILED, and OnFailure= deps get unexpected skipped. Let's introduce an intermediate state, so that during unit start we enter UNIT_ACTIVATING at least once. Fixes #35635 --- src/basic/unit-def.c | 1 + src/basic/unit-def.h | 1 + src/core/socket.c | 48 ++++++++++++++++++++++++++++++++------------ 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/src/basic/unit-def.c b/src/basic/unit-def.c index 03dc663b9a6..129d61b99cb 100644 --- a/src/basic/unit-def.c +++ b/src/basic/unit-def.c @@ -251,6 +251,7 @@ DEFINE_STRING_TABLE_LOOKUP(slice_state, SliceState); static const char* const socket_state_table[_SOCKET_STATE_MAX] = { [SOCKET_DEAD] = "dead", [SOCKET_START_PRE] = "start-pre", + [SOCKET_START_OPEN] = "start-open", [SOCKET_START_CHOWN] = "start-chown", [SOCKET_START_POST] = "start-post", [SOCKET_LISTENING] = "listening", diff --git a/src/basic/unit-def.h b/src/basic/unit-def.h index d022983bdca..bead8b47ac1 100644 --- a/src/basic/unit-def.h +++ b/src/basic/unit-def.h @@ -168,6 +168,7 @@ typedef enum SliceState { typedef enum SocketState { SOCKET_DEAD, SOCKET_START_PRE, + SOCKET_START_OPEN, SOCKET_START_CHOWN, SOCKET_START_POST, SOCKET_LISTENING, diff --git a/src/core/socket.c b/src/core/socket.c index a402b341074..1d4ceaedc8c 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -59,6 +59,7 @@ struct SocketPeer { static const UnitActiveState state_translation_table[_SOCKET_STATE_MAX] = { [SOCKET_DEAD] = UNIT_INACTIVE, [SOCKET_START_PRE] = UNIT_ACTIVATING, + [SOCKET_START_OPEN] = UNIT_ACTIVATING, [SOCKET_START_CHOWN] = UNIT_ACTIVATING, [SOCKET_START_POST] = UNIT_ACTIVATING, [SOCKET_LISTENING] = UNIT_ACTIVE, @@ -1841,6 +1842,7 @@ static void socket_set_state(Socket *s, SocketState state) { socket_unwatch_fds(s); if (!IN_SET(state, + SOCKET_START_OPEN, SOCKET_START_CHOWN, SOCKET_START_POST, SOCKET_LISTENING, @@ -1879,6 +1881,7 @@ static int socket_coldplug(Unit *u) { } if (IN_SET(s->deserialized_state, + SOCKET_START_OPEN, SOCKET_START_CHOWN, SOCKET_START_POST, SOCKET_LISTENING, @@ -1887,7 +1890,11 @@ static int socket_coldplug(Unit *u) { /* Originally, we used to simply reopen all sockets here that we didn't have file descriptors * for. However, this is problematic, as we won't traverse through the SOCKET_START_CHOWN state for * them, and thus the UID/GID wouldn't be right. Hence, instead simply check if we have all fds open, - * and if there's a mismatch, warn loudly. */ + * and if there's a mismatch, warn loudly. + * + * Note that SOCKET_START_OPEN requires no special treatment, as it's only intermediate + * between SOCKET_START_PRE and SOCKET_START_CHOWN and shall otherwise not be observed. + * It's listed only for consistency. */ r = socket_check_open(s); if (r == SOCKET_OPEN_NONE) @@ -2228,12 +2235,7 @@ static void socket_enter_start_chown(Socket *s) { int r; assert(s); - - r = socket_open_fds(s); - if (r < 0) { - log_unit_warning_errno(UNIT(s), r, "Failed to listen on sockets: %m"); - goto fail; - } + assert(s->state == SOCKET_START_OPEN); if (!isempty(s->user) || !isempty(s->group)) { @@ -2244,17 +2246,35 @@ static void socket_enter_start_chown(Socket *s) { r = socket_chown(s, &s->control_pid); if (r < 0) { log_unit_warning_errno(UNIT(s), r, "Failed to spawn 'start-chown' task: %m"); - goto fail; + socket_enter_stop_pre(s, SOCKET_FAILURE_RESOURCES); + return; } socket_set_state(s, SOCKET_START_CHOWN); } else socket_enter_start_post(s); +} - return; +static void socket_enter_start_open(Socket *s) { + int r; -fail: - socket_enter_stop_pre(s, SOCKET_FAILURE_RESOURCES); + assert(s); + + /* We force a state transition here even though we're not spawning any process (i.e. the state is purely + * intermediate), so that failure of socket_open_fds() always causes a state change in unit_notify(). + * Otherwise, if no Exec*= is defined, we might go from previous SOCKET_FAILED to SOCKET_FAILED, + * meaning the OnFailure= deps are unexpected skipped (#35635). */ + + socket_set_state(s, SOCKET_START_OPEN); + + r = socket_open_fds(s); + if (r < 0) { + log_unit_error_errno(UNIT(s), r, "Failed to listen on sockets: %m"); + socket_enter_stop_pre(s, SOCKET_FAILURE_RESOURCES); + return; + } + + socket_enter_start_chown(s); } static void socket_enter_start_pre(Socket *s) { @@ -2281,7 +2301,7 @@ static void socket_enter_start_pre(Socket *s) { socket_set_state(s, SOCKET_START_PRE); } else - socket_enter_start_chown(s); + socket_enter_start_open(s); } static void flush_ports(Socket *s) { @@ -2484,6 +2504,7 @@ static int socket_start(Unit *u) { /* Already on it! */ if (IN_SET(s->state, SOCKET_START_PRE, + SOCKET_START_OPEN, SOCKET_START_CHOWN, SOCKET_START_POST)) return 0; @@ -2535,6 +2556,7 @@ static int socket_stop(Unit *u) { * kill mode. */ if (IN_SET(s->state, SOCKET_START_PRE, + SOCKET_START_OPEN, SOCKET_START_CHOWN, SOCKET_START_POST)) { socket_enter_signal(s, SOCKET_STOP_PRE_SIGTERM, SOCKET_SUCCESS); @@ -3171,7 +3193,7 @@ static void socket_sigchld_event(Unit *u, pid_t pid, int code, int status) { case SOCKET_START_PRE: if (f == SOCKET_SUCCESS) - socket_enter_start_chown(s); + socket_enter_start_open(s); else socket_enter_signal(s, SOCKET_FINAL_SIGTERM, f); break;