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 unexpectedly skipped. Let's introduce
an intermediate state, so that during unit start we enter
UNIT_ACTIVATING at least once.

Fixes #35635

diff --git a/src/basic/unit-def.c b/src/basic/unit-def.c
index 03dc663b9a6db9d6eec2f1c6f65e6ff0e2cd63eb..129d61b99cbc9e57549083d26d34e2c7f42a4457 100644 (file)
--- 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 d022983bdcabc2e4a38123ce316545c47212546c..bead8b47ac1773b2bee5da6f34e4c4920efa0265 100644 (file)
--- 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 ba99c4781c58e0f2f9c1baf999a704ffc3773d3b..7e369e34afad24ac851c0a6b9652c0d1518c36a2 100644 (file)
--- a/src/core/socket.c
+++ b/src/core/socket.c
@@ -60,6 +60,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,
@@ -1822,6 +1823,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,
@@ -1860,6 +1862,7 @@ static int socket_coldplug(Unit *u) {
         }
 
         if (IN_SET(s->deserialized_state,
+                   SOCKET_START_OPEN,
                    SOCKET_START_CHOWN,
                    SOCKET_START_POST,
                    SOCKET_LISTENING,
@@ -1868,7 +1871,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)
@@ -2226,12 +2233,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)) {
 
@@ -2242,17 +2244,36 @@ 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);
+        assert(IN_SET(s->state, SOCKET_DEAD, SOCKET_FAILED, SOCKET_START_PRE));
+
+        /* 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 unexpectedly 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) {
@@ -2279,7 +2300,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 socket_enter_running(Socket *s, int cfd_in) {
@@ -2467,6 +2488,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;
@@ -2518,6 +2540,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);
@@ -3154,7 +3177,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;