From 6767b7b3af8124fd76dae5f0ca3e7494b5b4aeb8 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 16 Dec 2024 15:26:02 +0100 Subject: [PATCH 1/9] sysusers.d/basic: fix alignment Follow-up for a4c0528f14257e23a8fad8855f178ef71d99978b --- sysusers.d/basic.conf.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sysusers.d/basic.conf.in b/sysusers.d/basic.conf.in index 992af346ca..84bbe3854f 100644 --- a/sysusers.d/basic.conf.in +++ b/sysusers.d/basic.conf.in @@ -11,7 +11,7 @@ u root 0:0 "Super User" /root # The nobody user/group for NFS file systems g {{NOBODY_GROUP_NAME}} 65534 - - -u! {{NOBODY_USER_NAME }} 65534:65534 "Kernel Overflow User" - +u! {{NOBODY_USER_NAME}} 65534:65534 "Kernel Overflow User" - # Administrator group: can *see* more than normal users g adm {{ADM_GID }} - - From 2fbdfe0b0d355d0bd6ea8b93a000f92bc7f5fe5f Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 16 Dec 2024 01:29:35 +0100 Subject: [PATCH 2/9] core/socket: trivial coding style cleanups --- src/core/socket.c | 62 +++++++++++++++++++++++------------------------ src/core/socket.h | 2 +- 2 files changed, 31 insertions(+), 33 deletions(-) diff --git a/src/core/socket.c b/src/core/socket.c index 6599527515..446d3bf9a9 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -75,7 +75,6 @@ static const UnitActiveState state_translation_table[_SOCKET_STATE_MAX] = { static int socket_dispatch_io(sd_event_source *source, int fd, uint32_t revents, void *userdata); static int socket_dispatch_timer(sd_event_source *source, usec_t usec, void *userdata); -static void flush_ports(Socket *s); static bool SOCKET_STATE_WITH_PROCESS(SocketState state) { return IN_SET(state, @@ -372,11 +371,13 @@ static int socket_add_extras(Socket *s) { return 0; } -static const char *socket_find_symlink_target(Socket *s) { +static const char* socket_find_symlink_target(Socket *s) { const char *found = NULL; + assert(s); + LIST_FOREACH(port, p, s->ports) { - const char *f = NULL; + const char *f; switch (p->type) { @@ -389,7 +390,7 @@ static const char *socket_find_symlink_target(Socket *s) { break; default: - break; + f = NULL; } if (f) { @@ -483,7 +484,7 @@ static int socket_load(Unit *u) { return socket_verify(s); } -static SocketPeer *socket_peer_dup(const SocketPeer *q) { +static SocketPeer* socket_peer_dup(const SocketPeer *q) { SocketPeer *p; assert(q); @@ -502,7 +503,7 @@ static SocketPeer *socket_peer_dup(const SocketPeer *q) { return p; } -static SocketPeer *socket_peer_free(SocketPeer *p) { +static SocketPeer* socket_peer_free(SocketPeer *p) { assert(p); if (p->socket) @@ -578,7 +579,6 @@ static const char* listen_lookup(int family, int type) { return "ListenSequentialPacket"; assert_not_reached(); - return NULL; } static void socket_dump(Unit *u, FILE *f, const char *prefix) { @@ -973,7 +973,7 @@ static void socket_close_fds(Socket *s) { break; default: - break; + ; } } @@ -984,6 +984,8 @@ static void socket_close_fds(Socket *s) { /* Note that we don't return NULL here, since s has not been freed. */ } +DEFINE_TRIVIAL_CLEANUP_FUNC_FULL(Socket*, socket_close_fds, NULL); + static void socket_apply_socket_options(Socket *s, SocketPort *p, int fd) { int r; @@ -1295,7 +1297,6 @@ static int socket_symlink(Socket *s) { (void) mkdir_parents_label(*i, s->directory_mode); r = symlink_idempotent(p, *i, false); - if (r == -EEXIST && s->remove_on_stop) { /* If there's already something where we want to create the symlink, and the destructive * RemoveOnStop= mode is set, then we might as well try to remove what already exists and try @@ -1304,7 +1305,6 @@ static int socket_symlink(Socket *s) { if (unlink(*i) >= 0) r = symlink_idempotent(p, *i, false); } - if (r < 0) log_unit_warning_errno(UNIT(s), r, "Failed to create symlink %s %s %s, ignoring: %m", p, special_glyph(SPECIAL_GLYPH_ARROW_RIGHT), *i); @@ -1624,8 +1624,6 @@ static int socket_address_listen_in_cgroup( return fd; } -DEFINE_TRIVIAL_CLEANUP_FUNC_FULL(Socket *, socket_close_fds, NULL); - static int socket_open_fds(Socket *orig_s) { _cleanup_(socket_close_fdsp) Socket *s = orig_s; _cleanup_freecon_ char *label = NULL; @@ -1685,7 +1683,7 @@ static int socket_open_fds(Socket *orig_s) { p->fd = special_address_create(p->path, s->writable); if (p->fd < 0) - return log_unit_error_errno(UNIT(s), p->fd, "Failed to open special file %s: %m", p->path); + return log_unit_error_errno(UNIT(s), p->fd, "Failed to open special file '%s': %m", p->path); break; case SOCKET_FIFO: @@ -1695,7 +1693,7 @@ static int socket_open_fds(Socket *orig_s) { s->directory_mode, s->socket_mode); if (p->fd < 0) - return log_unit_error_errno(UNIT(s), p->fd, "Failed to open FIFO %s: %m", p->path); + return log_unit_error_errno(UNIT(s), p->fd, "Failed to open FIFO '%s': %m", p->path); socket_apply_fifo_options(s, p->fd); socket_symlink(s); @@ -1709,7 +1707,7 @@ static int socket_open_fds(Socket *orig_s) { s->mq_maxmsg, s->mq_msgsize); if (p->fd < 0) - return log_unit_error_errno(UNIT(s), p->fd, "Failed to open message queue %s: %m", p->path); + return log_unit_error_errno(UNIT(s), p->fd, "Failed to open message queue '%s': %m", p->path); break; case SOCKET_USB_FUNCTION: { @@ -1733,12 +1731,13 @@ static int socket_open_fds(Socket *orig_s) { break; } + default: assert_not_reached(); } } - s = NULL; + TAKE_PTR(s); return 0; } @@ -2127,7 +2126,6 @@ static void socket_enter_signal(Socket *s, SocketState state, SocketResult f) { log_unit_warning_errno(UNIT(s), r, "Failed to kill processes: %m"); goto fail; } - if (r > 0) { r = socket_arm_timer(s, /* relative= */ true, s->timeout_usec); if (r < 0) { @@ -2181,6 +2179,21 @@ static void socket_enter_stop_pre(Socket *s, SocketResult f) { socket_enter_stop_post(s, SOCKET_SUCCESS); } +static void flush_ports(Socket *s) { + assert(s); + + /* Flush all incoming traffic, regardless if actual bytes or new connections, so that this socket isn't busy + * anymore */ + + LIST_FOREACH(port, p, s->ports) { + if (p->fd < 0) + continue; + + (void) flush_accept(p->fd); + (void) flush_fd(p->fd); + } +} + static void socket_enter_listening(Socket *s) { int r; @@ -2285,21 +2298,6 @@ static void socket_enter_start_pre(Socket *s) { socket_enter_start_chown(s); } -static void flush_ports(Socket *s) { - assert(s); - - /* Flush all incoming traffic, regardless if actual bytes or new connections, so that this socket isn't busy - * anymore */ - - LIST_FOREACH(port, p, s->ports) { - if (p->fd < 0) - continue; - - (void) flush_accept(p->fd); - (void) flush_fd(p->fd); - } -} - static void socket_enter_running(Socket *s, int cfd_in) { /* Note that this call takes possession of the connection fd passed. It either has to assign it * somewhere or close it. */ diff --git a/src/core/socket.h b/src/core/socket.h index f21fc7f544..51fc96280b 100644 --- a/src/core/socket.h +++ b/src/core/socket.h @@ -176,7 +176,7 @@ int socket_collect_fds(Socket *s, int **ret); /* Called from the service code when a per-connection service ended */ void socket_connection_unref(Socket *s); -SocketPort *socket_port_free(SocketPort *p); +SocketPort* socket_port_free(SocketPort *p); DEFINE_TRIVIAL_CLEANUP_FUNC(SocketPort*, socket_port_free); void socket_free_ports(Socket *s); From 240457204bc7f49e5da24a66ae4e10714ba2076b Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 17 Dec 2024 13:51:24 +0100 Subject: [PATCH 3/9] core/socket: use correct kill operation for SOCKET_STOP_PRE_SIGTERM Follow-up for a232ebcc2cd3d5e16b0d0fb0be61549f25fb8239 --- src/core/socket.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core/socket.c b/src/core/socket.c index 446d3bf9a9..9234a91876 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -2104,8 +2104,10 @@ static void socket_enter_stop_post(Socket *s, SocketResult f) { } static int state_to_kill_operation(Socket *s, SocketState state) { - if (state == SOCKET_STOP_PRE_SIGTERM && unit_has_job_type(UNIT(s), JOB_RESTART)) - return KILL_RESTART; + assert(s); + + if (state == SOCKET_STOP_PRE_SIGTERM) + return unit_has_job_type(UNIT(s), JOB_RESTART) ? KILL_RESTART : KILL_TERMINATE; if (state == SOCKET_FINAL_SIGTERM) return KILL_TERMINATE; From d1e3536d6b6550df5df4a87f1c8e4bf9d80a6678 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 16 Dec 2024 01:28:02 +0100 Subject: [PATCH 4/9] core/socket: close fds also on SOCKET_CLEAN state Follow-up for c968d76a382f905b419cacd23a6b20aa31aca580 We'd only transition to SOCKET_CLEAN state if previously inactive (see socket_clean()), hence no fds shall be persisted in this state. --- src/core/socket.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/core/socket.c b/src/core/socket.c index 9234a91876..d37bdea926 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -1845,8 +1845,7 @@ static void socket_set_state(Socket *s, SocketState state) { SOCKET_RUNNING, SOCKET_STOP_PRE, SOCKET_STOP_PRE_SIGTERM, - SOCKET_STOP_PRE_SIGKILL, - SOCKET_CLEANING)) + SOCKET_STOP_PRE_SIGKILL)) socket_close_fds(s); if (state != old_state) From 90761f7f68f6fce3f4d8673ad6c7d435a90052c7 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 16 Dec 2024 16:41:58 +0100 Subject: [PATCH 5/9] basic/recurse-dir: modernize readdir_all() --- src/basic/recurse-dir.c | 18 +++++++---------- src/basic/recurse-dir.h | 44 ++++++++++++++++++++--------------------- 2 files changed, 29 insertions(+), 33 deletions(-) diff --git a/src/basic/recurse-dir.c b/src/basic/recurse-dir.c index 378fd92b06..389a784c40 100644 --- a/src/basic/recurse-dir.c +++ b/src/basic/recurse-dir.c @@ -26,14 +26,9 @@ static bool ignore_dirent(const struct dirent *de, RecurseDirFlags flags) { dot_or_dot_dot(de->d_name); } -int readdir_all(int dir_fd, - RecurseDirFlags flags, - DirectoryEntries **ret) { - +int readdir_all(int dir_fd, RecurseDirFlags flags, DirectoryEntries **ret) { _cleanup_free_ DirectoryEntries *de = NULL; - struct dirent *entry; DirectoryEntries *nde; - size_t add, sz, j; int r; assert(dir_fd >= 0); @@ -77,11 +72,11 @@ int readdir_all(int dir_fd, nde = realloc(de, bs); if (!nde) return -ENOMEM; - de = nde; } de->n_entries = 0; + struct dirent *entry; FOREACH_DIRENT_IN_BUFFER(entry, de->buffer, de->buffer_size) { if (ignore_dirent(entry, flags)) continue; @@ -98,16 +93,17 @@ int readdir_all(int dir_fd, de->n_entries++; } + size_t sz, j; + sz = ALIGN(offsetof(DirectoryEntries, buffer) + de->buffer_size); - add = sizeof(struct dirent*) * de->n_entries; - if (add > SIZE_MAX - add) + if (!INC_SAFE(&sz, sizeof(struct dirent*) * de->n_entries)) return -ENOMEM; - nde = realloc(de, sz + add); + nde = realloc(de, sz); if (!nde) return -ENOMEM; - de = nde; + de->entries = (struct dirent**) ((uint8_t*) de + ALIGN(offsetof(DirectoryEntries, buffer) + de->buffer_size)); j = 0; diff --git a/src/basic/recurse-dir.h b/src/basic/recurse-dir.h index aaeae950fe..f1b740ff83 100644 --- a/src/basic/recurse-dir.h +++ b/src/basic/recurse-dir.h @@ -8,6 +8,28 @@ #include "stat-util.h" #include "macro.h" +typedef enum RecurseDirFlags { + /* Interpreted by readdir_all() */ + RECURSE_DIR_SORT = 1 << 0, /* sort file directory entries before processing them */ + RECURSE_DIR_IGNORE_DOT = 1 << 1, /* ignore all dot files ("." and ".." are always ignored) */ + RECURSE_DIR_ENSURE_TYPE = 1 << 2, /* guarantees that 'd_type' field of 'de' is not DT_UNKNOWN */ + + /* Interpreted by recurse_dir() */ + RECURSE_DIR_SAME_MOUNT = 1 << 3, /* skips over subdirectories that are submounts */ + RECURSE_DIR_INODE_FD = 1 << 4, /* passes an opened inode fd (O_DIRECTORY fd in case of dirs, O_PATH otherwise) */ + RECURSE_DIR_TOPLEVEL = 1 << 5, /* call RECURSE_DIR_ENTER/RECURSE_DIR_LEAVE once for top-level dir, too, with dir_fd=-1 and NULL dirent */ +} RecurseDirFlags; + +typedef struct DirectoryEntries { + size_t n_entries; + struct dirent **entries; + size_t buffer_size; + struct dirent buffer[]; +} DirectoryEntries; + +int readdir_all(int dir_fd, RecurseDirFlags flags, DirectoryEntries **ret); +int readdir_all_at(int fd, const char *path, RecurseDirFlags flags, DirectoryEntries **ret); + typedef enum RecurseDirEvent { RECURSE_DIR_ENTER, /* only for dir inodes */ RECURSE_DIR_LEAVE, /* only for dir inodes */ @@ -56,27 +78,5 @@ typedef int (*recurse_dir_func_t)( const struct statx *sx, /* statx data (only if statx_mask was non-zero) */ void *userdata); -typedef enum RecurseDirFlags { - /* Interpreted by readdir_all() */ - RECURSE_DIR_SORT = 1 << 0, /* sort file directory entries before processing them */ - RECURSE_DIR_IGNORE_DOT = 1 << 1, /* ignore all dot files ("." and ".." are always ignored) */ - RECURSE_DIR_ENSURE_TYPE = 1 << 2, /* guarantees that 'd_type' field of 'de' is not DT_UNKNOWN */ - - /* Interpreted by recurse_dir() */ - RECURSE_DIR_SAME_MOUNT = 1 << 3, /* skips over subdirectories that are submounts */ - RECURSE_DIR_INODE_FD = 1 << 4, /* passes an opened inode fd (O_DIRECTORY fd in case of dirs, O_PATH otherwise) */ - RECURSE_DIR_TOPLEVEL = 1 << 5, /* call RECURSE_DIR_ENTER/RECURSE_DIR_LEAVE once for top-level dir, too, with dir_fd=-1 and NULL dirent */ -} RecurseDirFlags; - -typedef struct DirectoryEntries { - size_t n_entries; - struct dirent** entries; - size_t buffer_size; - struct dirent buffer[]; -} DirectoryEntries; - -int readdir_all(int dir_fd, RecurseDirFlags flags, DirectoryEntries **ret); -int readdir_all_at(int fd, const char *path, RecurseDirFlags flags, DirectoryEntries **ret); - int recurse_dir(int dir_fd, const char *path, unsigned statx_mask, unsigned n_depth_max, RecurseDirFlags flags, recurse_dir_func_t func, void *userdata); int recurse_dir_at(int atfd, const char *path, unsigned statx_mask, unsigned n_depth_max, RecurseDirFlags flags, recurse_dir_func_t func, void *userdata); From 80706c32699882ac12b5a0953bc6b500e924a1a3 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 16 Dec 2024 19:59:17 +0100 Subject: [PATCH 6/9] core/socket: clean up usbffs handling - Log about errors in socket_open_fds() - Open parent dir fd and operate under it, instead of constructing paths everywhere - Replace scandir() with readdir_all() --- src/core/socket.c | 99 ++++++++++++++++++++--------------------------- 1 file changed, 41 insertions(+), 58 deletions(-) diff --git a/src/core/socket.c b/src/core/socket.c index d37bdea926..ba99c4781c 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -32,6 +32,7 @@ #include "parse-util.h" #include "path-util.h" #include "process-util.h" +#include "recurse-dir.h" #include "selinux-util.h" #include "serialize.h" #include "service.h" @@ -111,9 +112,8 @@ static bool SOCKET_SERVICE_IS_ACTIVE(Service *s, bool allow_finalize) { } static void socket_init(Unit *u) { - Socket *s = SOCKET(u); + Socket *s = ASSERT_PTR(SOCKET(u)); - assert(u); assert(u->load_state == UNIT_STUB); s->backlog = SOMAXCONN_DELUXE; @@ -144,7 +144,7 @@ static void socket_unwatch_control_pid(Socket *s) { unit_unwatch_pidref_done(UNIT(s), &s->control_pid); } -static void socket_cleanup_fd_list(SocketPort *p) { +static void socket_port_close_auxiliary_fds(SocketPort *p) { assert(p); close_many(p->auxiliary_fds, p->n_auxiliary_fds); @@ -152,13 +152,13 @@ static void socket_cleanup_fd_list(SocketPort *p) { p->n_auxiliary_fds = 0; } -SocketPort *socket_port_free(SocketPort *p) { +SocketPort* socket_port_free(SocketPort *p) { if (!p) return NULL; sd_event_source_unref(p->event_source); - socket_cleanup_fd_list(p); + socket_port_close_auxiliary_fds(p); safe_close(p->fd); free(p->path); @@ -941,13 +941,11 @@ static void socket_close_fds(Socket *s) { assert(s); LIST_FOREACH(port, p, s->ports) { - bool was_open; - - was_open = p->fd >= 0; + bool was_open = p->fd >= 0; p->event_source = sd_event_source_disable_unref(p->event_source); p->fd = safe_close(p->fd); - socket_cleanup_fd_list(p); + socket_port_close_auxiliary_fds(p); /* One little note: we should normally not delete any sockets in the file system here! After all some * other process we spawned might still have a reference of this fd and wants to continue to use @@ -1219,13 +1217,14 @@ static int special_address_create(const char *path, bool writable) { return TAKE_FD(fd); } -static int usbffs_address_create(const char *path) { +static int usbffs_address_create_at(int dfd, const char *name) { _cleanup_close_ int fd = -EBADF; struct stat st; - assert(path); + assert(dfd >= 0); + assert(name); - fd = open(path, O_RDWR|O_CLOEXEC|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW); + fd = openat(dfd, name, O_RDWR|O_CLOEXEC|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW); if (fd < 0) return -errno; @@ -1329,57 +1328,40 @@ static int usbffs_write_descs(int fd, Service *s) { return copy_file_fd(s->usb_function_strings, fd, 0); } -static int usbffs_select_ep(const struct dirent *d) { - return d->d_name[0] != '.' && !streq(d->d_name, "ep0"); -} - -static int usbffs_dispatch_eps(SocketPort *p) { - _cleanup_free_ struct dirent **ent = NULL; - size_t n, k; +static int usbffs_dispatch_eps(SocketPort *p, int dfd) { + _cleanup_free_ DirectoryEntries *des = NULL; int r; - r = scandir(p->path, &ent, usbffs_select_ep, alphasort); + assert(p); + assert(dfd >= 0); + + r = readdir_all(dfd, RECURSE_DIR_SORT|RECURSE_DIR_IGNORE_DOT, &des); if (r < 0) - return -errno; + return r; - n = (size_t) r; - p->auxiliary_fds = new(int, n); - if (!p->auxiliary_fds) { - r = -ENOMEM; - goto clear; - } + p->auxiliary_fds = new(int, des->n_entries); + if (!p->auxiliary_fds) + return -ENOMEM; - p->n_auxiliary_fds = n; + FOREACH_ARRAY(i, des->entries, des->n_entries) { + const struct dirent *de = *i; - k = 0; - for (size_t i = 0; i < n; ++i) { - _cleanup_free_ char *ep = NULL; + if (streq(de->d_name, "ep0")) + continue; - ep = path_make_absolute(ent[i]->d_name, p->path); - if (!ep) { - r = -ENOMEM; - goto fail; - } - - path_simplify(ep); - - r = usbffs_address_create(ep); + r = usbffs_address_create_at(dfd, de->d_name); if (r < 0) goto fail; - p->auxiliary_fds[k++] = r; + p->auxiliary_fds[p->n_auxiliary_fds++] = r; } - r = 0; - goto clear; + assert(p->n_auxiliary_fds < des->n_entries); + + return 0; fail: - close_many(p->auxiliary_fds, k); - p->auxiliary_fds = mfree(p->auxiliary_fds); - p->n_auxiliary_fds = 0; - -clear: - free_many((void**) ent, n); + socket_port_close_auxiliary_fds(p); return r; } @@ -1711,23 +1693,24 @@ static int socket_open_fds(Socket *orig_s) { break; case SOCKET_USB_FUNCTION: { - _cleanup_free_ char *ep = NULL; + _cleanup_close_ int dfd = -EBADF; - ep = path_make_absolute("ep0", p->path); - if (!ep) - return -ENOMEM; + dfd = open(p->path, O_DIRECTORY|O_CLOEXEC); + if (dfd < 0) + return log_unit_error_errno(UNIT(s), errno, + "Failed to open USB FunctionFS dir '%s': %m", p->path); - p->fd = usbffs_address_create(ep); + p->fd = usbffs_address_create_at(dfd, "ep0"); if (p->fd < 0) - return p->fd; + return log_unit_error_errno(UNIT(s), p->fd, "Failed to open USB FunctionFS ep0: %m"); r = usbffs_write_descs(p->fd, SERVICE(UNIT_DEREF(s->service))); if (r < 0) - return r; + return log_unit_error_errno(UNIT(s), r, "Failed to write to USB FunctionFS ep0: %m"); - r = usbffs_dispatch_eps(p); + r = usbffs_dispatch_eps(p, dfd); if (r < 0) - return r; + return log_unit_error_errno(UNIT(s), r, "Failed to dispatch USB FunctionFS eps: %m"); break; } From 58a86a15047389909bc9c50b9940883dd231d56a Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 16 Dec 2024 01:54:11 +0100 Subject: [PATCH 7/9] 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 --- src/basic/unit-def.c | 1 + src/basic/unit-def.h | 1 + src/core/socket.c | 49 ++++++++++++++++++++++++++++++++------------ 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/src/basic/unit-def.c b/src/basic/unit-def.c index 03dc663b9a..129d61b99c 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 d022983bdc..bead8b47ac 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 ba99c4781c..7e369e34af 100644 --- 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; From d8a2ed33fdc2a683a4746918b879137177943b1c Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 30 Dec 2024 23:04:21 +0100 Subject: [PATCH 8/9] test: move test for MaxConnectionsPerSource= to TEST-07-PID1 Follow-up for 3456c89ac26872f4befa2bdf7c260529932e3909 --- ...-AUX-UTILS.socket.sh => TEST-07-PID1.socket-max-connection.sh} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/units/{TEST-74-AUX-UTILS.socket.sh => TEST-07-PID1.socket-max-connection.sh} (100%) diff --git a/test/units/TEST-74-AUX-UTILS.socket.sh b/test/units/TEST-07-PID1.socket-max-connection.sh similarity index 100% rename from test/units/TEST-74-AUX-UTILS.socket.sh rename to test/units/TEST-07-PID1.socket-max-connection.sh From 7c194bb93ce3740b6c1db2d76112bc61fc2474c3 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 30 Dec 2024 23:35:32 +0100 Subject: [PATCH 9/9] TEST-07-PID1: add test case for #35635 --- test/units/TEST-07-PID1.socket-on-failure.sh | 73 ++++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100755 test/units/TEST-07-PID1.socket-on-failure.sh diff --git a/test/units/TEST-07-PID1.socket-on-failure.sh b/test/units/TEST-07-PID1.socket-on-failure.sh new file mode 100755 index 0000000000..affcec44ff --- /dev/null +++ b/test/units/TEST-07-PID1.socket-on-failure.sh @@ -0,0 +1,73 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: LGPL-2.1-or-later +# shellcheck disable=SC2016 +set -eux +set -o pipefail + +# shellcheck source=test/units/util.sh +. "$(dirname "$0")"/util.sh + +UNIT_NAME="TEST-07-PID1-socket-$RANDOM" + +at_exit() { + systemctl stop "$UNIT_NAME.socket" + rm -f /run/systemd/system/"$UNIT_NAME".{socket,service} \ + /run/systemd/system/TEST-07-PID1-socket-OnFailure.service +} + +trap at_exit EXIT + +# Test triggering OnFailure= when fails to listen on socket (#35635) + +mkdir -p "/tmp/$UNIT_NAME" + +cat >/run/systemd/system/"$UNIT_NAME.socket" </run/systemd/system/"$UNIT_NAME.service" </run/systemd/system/TEST-07-PID1-socket-OnFailure.service <