From dff9808a628c31b7ecb1f1aba8fdc3be06ce8372 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 6 Oct 2023 17:50:48 +0200 Subject: [PATCH 1/7] serialize: add new helper deserialize_fd() Currently, when we deserialize an fd we do a lot of manual work. Add a common helper that makes this more robust and uniform. Note that this sometimes changes behaviour slightly, but in ways that shouldn't really matter: if we fail to deserialize an fd correctly we'll unset (i.e. set to -EBADF) the fd in the deserialized data structure. Previously, we'd leave the old value in place. This should not change effective result (as in either case we'll be in a bad state afterwards, just once we mix old/invalidated state with new state, while now we'll reset the state explicitly to invalidated state on failure). In particular as deserialization starts from an empty structure generally, hence the old value should be unset anyway. Another slight change is that if we fail to deserialize some object half way, and we already have taken out one fd from the serialized fdset we'll now just close it instead of returning it to/leaving it in the fdset. Given that such "orphaned" fds are blanket closed after deserialization finishes this also shouldn't change behaviour IRL. Also, the idle_pipe was previously incorrectly serialized: we'd serialize invalidated fds, which would fail, but because parsing errors on this were ignored on the deserializatin noone noticed. This is fixed. --- src/core/automount.c | 10 +- src/core/dynamic-user.c | 17 ++-- src/core/execute-serialize.c | 188 ++++++++++++----------------------- src/core/execute.c | 53 +++------- src/core/manager-serialize.c | 14 ++- src/core/service.c | 85 +++++++--------- src/core/unit-serialize.c | 23 +---- src/shared/serialize.c | 31 ++++-- src/shared/serialize.h | 1 + 9 files changed, 157 insertions(+), 265 deletions(-) diff --git a/src/core/automount.c b/src/core/automount.c index a34bfae3ac8..8df715b1afd 100644 --- a/src/core/automount.c +++ b/src/core/automount.c @@ -925,14 +925,8 @@ static int automount_deserialize_item(Unit *u, const char *key, const char *valu log_unit_error_errno(u, r, "Failed to add expire token to set: %m"); } } else if (streq(key, "pipe-fd")) { - int fd; - - if ((fd = parse_fd(value)) < 0 || !fdset_contains(fds, fd)) - log_unit_debug(u, "Failed to parse pipe-fd value: %s", value); - else { - safe_close(a->pipe_fd); - a->pipe_fd = fdset_remove(fds, fd); - } + safe_close(a->pipe_fd); + a->pipe_fd = deserialize_fd(fds, value); } else log_unit_debug(u, "Unknown serialization key: %s", key); diff --git a/src/core/dynamic-user.c b/src/core/dynamic-user.c index dc349afecfa..a70f64f7ff6 100644 --- a/src/core/dynamic-user.c +++ b/src/core/dynamic-user.c @@ -648,7 +648,8 @@ int dynamic_user_serialize(Manager *m, FILE *f, FDSet *fds) { void dynamic_user_deserialize_one(Manager *m, const char *value, FDSet *fds, DynamicUser **ret) { _cleanup_free_ char *name = NULL, *s0 = NULL, *s1 = NULL; - int r, fd0, fd1; + _cleanup_close_ int fd0 = -EBADF, fd1 = -EBADF; + int r; assert(value); assert(fds); @@ -661,15 +662,13 @@ void dynamic_user_deserialize_one(Manager *m, const char *value, FDSet *fds, Dyn return; } - if ((fd0 = parse_fd(s0)) < 0 || !fdset_contains(fds, fd0)) { - log_debug("Unable to process dynamic user fd specification."); + fd0 = deserialize_fd(fds, s0); + if (fd0 < 0) return; - } - if ((fd1 = parse_fd(s1)) < 0 || !fdset_contains(fds, fd1)) { - log_debug("Unable to process dynamic user fd specification."); + fd1 = deserialize_fd(fds, s1); + if (fd1 < 0) return; - } r = dynamic_user_add(m, name, (int[]) { fd0, fd1 }, ret); if (r < 0) { @@ -677,8 +676,8 @@ void dynamic_user_deserialize_one(Manager *m, const char *value, FDSet *fds, Dyn return; } - (void) fdset_remove(fds, fd0); - (void) fdset_remove(fds, fd1); + TAKE_FD(fd0); + TAKE_FD(fd1); if (ret) /* If the caller uses it directly, increment the refcount */ (*ret)->n_ref++; diff --git a/src/core/execute-serialize.c b/src/core/execute-serialize.c index 147bbd60edd..1886a59b84c 100644 --- a/src/core/execute-serialize.c +++ b/src/core/execute-serialize.c @@ -1232,17 +1232,11 @@ static int exec_runtime_deserialize(ExecRuntime *rt, FILE *f, FDSet *fds) { if (r == 0) break; - if ((fd = parse_fd(w)) < 0 || !fdset_contains(fds, fd)) - log_debug("Failed to parse %s value: %s, ignoring.", l, w); - else { - r = fdset_remove(fds, fd); - if (r < 0) { - log_debug_errno(r, "Failed to remove %s value=%d from fdset, ignoring: %m", l, fd); - continue; - } + fd = deserialize_fd(fds, w); + if (fd < 0) + continue; - rt->shared->netns_storage_socket[i] = fd; - } + rt->shared->netns_storage_socket[i] = fd; } } else if ((val = startswith(l, "exec-runtime-ipcns-storage-socket="))) { for (size_t i = 0; i < 2; ++i) { @@ -1255,17 +1249,11 @@ static int exec_runtime_deserialize(ExecRuntime *rt, FILE *f, FDSet *fds) { if (r == 0) break; - if ((fd = parse_fd(w)) < 0 || !fdset_contains(fds, fd)) - log_debug("Failed to parse %s value: %s, ignoring.", l, w); - else { - r = fdset_remove(fds, fd); - if (r < 0) { - log_debug_errno(r, "Failed to remove %s value=%d from fdset, ignoring: %m", l, fd); - continue; - } + fd = deserialize_fd(fds, w); + if (fd < 0) + continue; - rt->shared->ipcns_storage_socket[i] = fd; - } + rt->shared->ipcns_storage_socket[i] = fd; } } else if ((val = startswith(l, "exec-runtime-dynamic-creds-user="))) dynamic_user_deserialize_one(/* m= */ NULL, val, fds, &rt->dynamic_creds->user); @@ -1297,17 +1285,11 @@ static int exec_runtime_deserialize(ExecRuntime *rt, FILE *f, FDSet *fds) { if (r == 0) break; - if ((fd = parse_fd(w)) < 0 || !fdset_contains(fds, fd)) - log_debug("Failed to parse %s value: %s, ignoring.", l, w); - else { - r = fdset_remove(fds, fd); - if (r < 0) { - log_debug_errno(r, "Failed to remove %s value=%d from fdset, ignoring: %m", l, fd); - continue; - } + fd = deserialize_fd(fds, w); + if (fd < 0) + continue; - rt->ephemeral_storage_socket[i] = fd; - } + rt->ephemeral_storage_socket[i] = fd; } } else log_warning("Failed to parse serialized line, ignoring: %s", l); @@ -1316,6 +1298,16 @@ static int exec_runtime_deserialize(ExecRuntime *rt, FILE *f, FDSet *fds) { return 0; } +static bool exec_parameters_is_idle_pipe_set(const ExecParameters *p) { + assert(p); + + return p->idle_pipe && + p->idle_pipe[0] >= 0 && + p->idle_pipe[1] >= 0 && + p->idle_pipe[2] >= 0 && + p->idle_pipe[3] >= 0; +} + static int exec_parameters_serialize(const ExecParameters *p, FILE *f, FDSet *fds) { int r; @@ -1434,17 +1426,13 @@ static int exec_parameters_serialize(const ExecParameters *p, FILE *f, FDSet *fd return r; } - if (p->idle_pipe) { + if (exec_parameters_is_idle_pipe_set(p)) { _cleanup_free_ char *serialized_fds = NULL; for (size_t i = 0; i < 4; ++i) { - int copy = -EBADF; - - if (p->idle_pipe[i] >= 0) { - copy = fdset_put_dup(fds, p->idle_pipe[i]); - if (copy < 0) - return copy; - } + int copy = fdset_put_dup(fds, p->idle_pipe[i]); + if (copy < 0) + return copy; r = strextendf(&serialized_fds, "%d ", copy); if (r < 0) @@ -1609,17 +1597,11 @@ static int exec_parameters_deserialize(ExecParameters *p, FILE *f, FDSet *fds) { if (r == 0) break; - if ((fd = parse_fd(w)) < 0 || !fdset_contains(fds, fd)) - log_debug("Failed to parse %s value: %s, ignoring.", l, w); - else { - r = fdset_remove(fds, fd); - if (r < 0) { - log_debug_errno(r, "Failed to remove %s value=%d from fdset, ignoring: %m", l, fd); - continue; - } + fd = deserialize_fd(fds, w); + if (fd < 0) + continue; - p->fds[i] = fd; - } + p->fds[i] = fd; } } else if ((val = startswith(l, "exec-parameters-fd-names="))) { r = deserialize_strv(&p->fd_names, val); @@ -1719,94 +1701,60 @@ static int exec_parameters_deserialize(ExecParameters *p, FILE *f, FDSet *fds) { if (r == 0) break; - if ((fd = parse_fd(w)) < 0 || !fdset_contains(fds, fd)) - log_debug("Failed to parse %s value: %s, ignoring.", l, w); - else { - r = fdset_remove(fds, fd); - if (r < 0) { - log_debug_errno(r, "Failed to remove %s value=%d from fdset, ignoring: %m", l, fd); - continue; - } + fd = deserialize_fd(fds, w); + if (fd < 0) + continue; - p->idle_pipe[i] = fd; - } + p->idle_pipe[i] = fd; } } else if ((val = startswith(l, "exec-parameters-stdin-fd="))) { int fd; - if ((fd = parse_fd(val)) < 0 || !fdset_contains(fds, fd)) - log_debug("Failed to parse %s value: %s, ignoring.", l, val); - else { - r = fdset_remove(fds, fd); - if (r < 0) { - log_debug_errno(r, "Failed to remove %s value=%d from fdset, ignoring: %m", l, fd); - continue; - } + fd = deserialize_fd(fds, val); + if (fd < 0) + continue; + + p->stdin_fd = fd; - p->stdin_fd = fd; - } } else if ((val = startswith(l, "exec-parameters-stdout-fd="))) { int fd; - if ((fd = parse_fd(val)) < 0 || !fdset_contains(fds, fd)) - log_debug("Failed to parse %s value: %s, ignoring.", l, val); - else { - r = fdset_remove(fds, fd); - if (r < 0) { - log_debug_errno(r, "Failed to remove %s value=%d from fdset, ignoring: %m", l, fd); - continue; - } + fd = deserialize_fd(fds, val); + if (fd < 0) + continue; + + p->stdout_fd = fd; - p->stdout_fd = fd; - } } else if ((val = startswith(l, "exec-parameters-stderr-fd="))) { int fd; - if ((fd = parse_fd(val)) < 0 || !fdset_contains(fds, fd)) - log_debug("Failed to parse %s value: %s, ignoring.", l, val); - else { - r = fdset_remove(fds, fd); - if (r < 0) { - log_debug_errno(r, "Failed to remove %s value=%d from fdset, ignoring: %m", l, fd); - continue; - } + fd = deserialize_fd(fds, val); + if (fd < 0) + continue; - p->stderr_fd = fd; - } + p->stderr_fd = fd; } else if ((val = startswith(l, "exec-parameters-exec-fd="))) { int fd; - if ((fd = parse_fd(val)) < 0 || !fdset_contains(fds, fd)) - log_debug("Failed to parse %s value: %s, ignoring.", l, val); - else { - r = fdset_remove(fds, fd); - if (r < 0) { - log_debug_errno(r, "Failed to remove %s value=%d from fdset, ignoring: %m", l, fd); - continue; - } + fd = deserialize_fd(fds, val); + if (fd < 0) + continue; - /* This is special and relies on close-on-exec semantics, make sure it's - * there */ - r = fd_cloexec(fd, true); - if (r < 0) - return r; + /* This is special and relies on close-on-exec semantics, make sure it's + * there */ + r = fd_cloexec(fd, true); + if (r < 0) + return r; - p->exec_fd = fd; - } + p->exec_fd = fd; } else if ((val = startswith(l, "exec-parameters-bpf-outer-map-fd="))) { int fd; - if ((fd = parse_fd(val)) < 0 || !fdset_contains(fds, fd)) - log_debug("Failed to parse %s value: %s, ignoring.", l, val); - else { - r = fdset_remove(fds, fd); - if (r < 0) { - log_debug_errno(r, "Failed to remove %s value=%d from fdset, ignoring: %m", l, fd); - continue; - } + fd = deserialize_fd(fds, val); + if (fd < 0) + continue; - p->bpf_outer_map_fd = fd; - } + p->bpf_outer_map_fd = fd; } else if ((val = startswith(l, "exec-parameters-notify-socket="))) { r = free_and_strdup(&p->notify_socket, val); if (r < 0) @@ -1826,17 +1774,11 @@ static int exec_parameters_deserialize(ExecParameters *p, FILE *f, FDSet *fds) { } else if ((val = startswith(l, "exec-parameters-user-lookup-fd="))) { int fd; - if ((fd = parse_fd(val)) < 0 || !fdset_contains(fds, fd)) - log_debug("Failed to parse %s value: %s, ignoring.", l, val); - else { - r = fdset_remove(fds, fd); - if (r < 0) { - log_debug_errno(r, "Failed to remove %s value=%d from fdset, ignoring: %m", l, fd); - continue; - } + fd = deserialize_fd(fds, val); + if (fd < 0) + continue; - p->user_lookup_fd = fd; - } + p->user_lookup_fd = fd; } else if ((val = startswith(l, "exec-parameters-files-env="))) { r = deserialize_strv(&p->files_env, val); if (r < 0) diff --git a/src/core/execute.c b/src/core/execute.c index 06fcc741f87..d9c8a19c380 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2002,27 +2002,18 @@ int exec_shared_runtime_deserialize_compat(Unit *u, const char *key, const char return -ENOMEM; } else if (streq(key, "netns-socket-0")) { - int fd; - - if ((fd = parse_fd(value)) < 0 || !fdset_contains(fds, fd)) { - log_unit_debug(u, "Failed to parse netns socket value: %s", value); - return 0; - } safe_close(rt->netns_storage_socket[0]); - rt->netns_storage_socket[0] = fdset_remove(fds, fd); + rt->netns_storage_socket[0] = deserialize_fd(fds, value); + if (rt->netns_storage_socket[0] < 0) + return 0; } else if (streq(key, "netns-socket-1")) { - int fd; - - if ((fd = parse_fd(value)) < 0 || !fdset_contains(fds, fd)) { - log_unit_debug(u, "Failed to parse netns socket value: %s", value); - return 0; - } safe_close(rt->netns_storage_socket[1]); - rt->netns_storage_socket[1] = fdset_remove(fds, fd); - + rt->netns_storage_socket[1] = deserialize_fd(fds, value); + if (rt->netns_storage_socket[1] < 0) + return 0; } else return 0; @@ -2088,13 +2079,9 @@ int exec_shared_runtime_deserialize_one(Manager *m, const char *value, FDSet *fd n = strcspn(v, " "); buf = strndupa_safe(v, n); - netns_fdpair[0] = parse_fd(buf); + netns_fdpair[0] = deserialize_fd(fds, buf); if (netns_fdpair[0] < 0) - return log_debug_errno(netns_fdpair[0], "Unable to parse exec-runtime specification netns-socket-0=%s: %m", buf); - if (!fdset_contains(fds, netns_fdpair[0])) - return log_debug_errno(SYNTHETIC_ERRNO(EBADF), - "exec-runtime specification netns-socket-0= refers to unknown fd %d: %m", netns_fdpair[0]); - netns_fdpair[0] = fdset_remove(fds, netns_fdpair[0]); + return netns_fdpair[0]; if (v[n] != ' ') goto finalize; p = v + n + 1; @@ -2107,13 +2094,9 @@ int exec_shared_runtime_deserialize_one(Manager *m, const char *value, FDSet *fd n = strcspn(v, " "); buf = strndupa_safe(v, n); - netns_fdpair[1] = parse_fd(buf); + netns_fdpair[1] = deserialize_fd(fds, buf); if (netns_fdpair[1] < 0) - return log_debug_errno(netns_fdpair[1], "Unable to parse exec-runtime specification netns-socket-1=%s: %m", buf); - if (!fdset_contains(fds, netns_fdpair[1])) - return log_debug_errno(SYNTHETIC_ERRNO(EBADF), - "exec-runtime specification netns-socket-1= refers to unknown fd %d: %m", netns_fdpair[1]); - netns_fdpair[1] = fdset_remove(fds, netns_fdpair[1]); + return netns_fdpair[1]; if (v[n] != ' ') goto finalize; p = v + n + 1; @@ -2126,13 +2109,9 @@ int exec_shared_runtime_deserialize_one(Manager *m, const char *value, FDSet *fd n = strcspn(v, " "); buf = strndupa_safe(v, n); - ipcns_fdpair[0] = parse_fd(buf); + ipcns_fdpair[0] = deserialize_fd(fds, buf); if (ipcns_fdpair[0] < 0) - return log_debug_errno(ipcns_fdpair[0], "Unable to parse exec-runtime specification ipcns-socket-0=%s: %m", buf); - if (!fdset_contains(fds, ipcns_fdpair[0])) - return log_debug_errno(SYNTHETIC_ERRNO(EBADF), - "exec-runtime specification ipcns-socket-0= refers to unknown fd %d: %m", ipcns_fdpair[0]); - ipcns_fdpair[0] = fdset_remove(fds, ipcns_fdpair[0]); + return ipcns_fdpair[0]; if (v[n] != ' ') goto finalize; p = v + n + 1; @@ -2145,13 +2124,9 @@ int exec_shared_runtime_deserialize_one(Manager *m, const char *value, FDSet *fd n = strcspn(v, " "); buf = strndupa_safe(v, n); - ipcns_fdpair[1] = parse_fd(buf); + ipcns_fdpair[1] = deserialize_fd(fds, buf); if (ipcns_fdpair[1] < 0) - return log_debug_errno(ipcns_fdpair[1], "Unable to parse exec-runtime specification ipcns-socket-1=%s: %m", buf); - if (!fdset_contains(fds, ipcns_fdpair[1])) - return log_debug_errno(SYNTHETIC_ERRNO(EBADF), - "exec-runtime specification ipcns-socket-1= refers to unknown fd %d: %m", ipcns_fdpair[1]); - ipcns_fdpair[1] = fdset_remove(fds, ipcns_fdpair[1]); + return ipcns_fdpair[1]; } finalize: diff --git a/src/core/manager-serialize.c b/src/core/manager-serialize.c index 09c49b6d6ca..1d7a1bed359 100644 --- a/src/core/manager-serialize.c +++ b/src/core/manager-serialize.c @@ -456,12 +456,11 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { } else if ((val = startswith(l, "notify-fd="))) { int fd; - if ((fd = parse_fd(val)) < 0 || !fdset_contains(fds, fd)) - log_notice("Failed to parse notify fd, ignoring: \"%s\"", val); - else { + fd = deserialize_fd(fds, val); + if (fd >= 0) { m->notify_event_source = sd_event_source_disable_unref(m->notify_event_source); safe_close(m->notify_fd); - m->notify_fd = fdset_remove(fds, fd); + m->notify_fd = fd; } } else if ((val = startswith(l, "notify-socket="))) { @@ -472,12 +471,11 @@ int manager_deserialize(Manager *m, FILE *f, FDSet *fds) { } else if ((val = startswith(l, "cgroups-agent-fd="))) { int fd; - if ((fd = parse_fd(val)) < 0 || !fdset_contains(fds, fd)) - log_notice("Failed to parse cgroups agent fd, ignoring.: %s", val); - else { + fd = deserialize_fd(fds, val); + if (fd >= 0) { m->cgroups_agent_event_source = sd_event_source_disable_unref(m->cgroups_agent_event_source); safe_close(m->cgroups_agent_fd); - m->cgroups_agent_fd = fdset_remove(fds, fd); + m->cgroups_agent_fd = fd; } } else if ((val = startswith(l, "user-lookup="))) { diff --git a/src/core/service.c b/src/core/service.c index b4bd343bc94..8846d04d230 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -3223,27 +3223,27 @@ static int service_deserialize_item(Unit *u, const char *key, const char *value, } } else if (streq(key, "socket-fd")) { - int fd; + asynchronous_close(s->socket_fd); + s->socket_fd = deserialize_fd(fds, value); - if ((fd = parse_fd(value)) < 0 || !fdset_contains(fds, fd)) - log_unit_debug(u, "Failed to parse socket-fd value: %s", value); - else { - asynchronous_close(s->socket_fd); - s->socket_fd = fdset_remove(fds, fd); - } } else if (streq(key, "fd-store-fd")) { _cleanup_free_ char *fdv = NULL, *fdn = NULL, *fdp = NULL; - int fd, do_poll; + _cleanup_close_ int fd = -EBADF; + int do_poll; r = extract_first_word(&value, &fdv, NULL, 0); - if (r <= 0 || (fd = parse_fd(fdv)) < 0 || !fdset_contains(fds, fd)) { - log_unit_debug(u, "Failed to parse fd-store-fd value: %s", value); + if (r <= 0) { + log_unit_debug(u, "Failed to parse fd-store-fd value, ignoring: %s", value); return 0; } + fd = deserialize_fd(fds, fdv); + if (fd < 0) + return 0; + r = extract_first_word(&value, &fdn, NULL, EXTRACT_CUNESCAPE | EXTRACT_UNQUOTE); if (r <= 0) { - log_unit_debug(u, "Failed to parse fd-store-fd value: %s", value); + log_unit_debug(u, "Failed to parse fd-store-fd value, ignoring: %s", value); return 0; } @@ -3251,23 +3251,18 @@ static int service_deserialize_item(Unit *u, const char *key, const char *value, if (r == 0) { /* If the value is not present, we assume the default */ do_poll = 1; - } else if (r < 0 || safe_atoi(fdp, &do_poll) < 0) { - log_unit_debug_errno(u, r, "Failed to parse fd-store-fd value \"%s\": %m", value); + } else if (r < 0 || (r = safe_atoi(fdp, &do_poll)) < 0) { + log_unit_debug_errno(u, r, "Failed to parse fd-store-fd value \"%s\", ignoring: %m", value); return 0; } - r = fdset_remove(fds, fd); - if (r < 0) { - log_unit_error_errno(u, r, "Could not find deserialized fd %i in fdset: %m", fd); - return 0; - } - assert(r == fd); - r = service_add_fd_store(s, fd, fdn, do_poll); if (r < 0) { - log_unit_error_errno(u, r, "Failed to store deserialized fd %i: %m", fd); + log_unit_debug_errno(u, r, "Failed to store deserialized fd %i, ignoring: %m", fd); return 0; } + + TAKE_FD(fd); } else if (streq(key, "main-exec-status-pid")) { pid_t pid; @@ -3312,47 +3307,37 @@ static int service_deserialize_item(Unit *u, const char *key, const char *value, else s->forbid_restart = b; } else if (streq(key, "stdin-fd")) { - int fd; - if ((fd = parse_fd(value)) < 0 || !fdset_contains(fds, fd)) - log_unit_debug(u, "Failed to parse stdin-fd value: %s", value); - else { - asynchronous_close(s->stdin_fd); - s->stdin_fd = fdset_remove(fds, fd); + asynchronous_close(s->stdin_fd); + s->stdin_fd = deserialize_fd(fds, value); + if (s->stdin_fd >= 0) s->exec_context.stdio_as_fds = true; - } + } else if (streq(key, "stdout-fd")) { - int fd; - if ((fd = parse_fd(value)) < 0 || !fdset_contains(fds, fd)) - log_unit_debug(u, "Failed to parse stdout-fd value: %s", value); - else { - asynchronous_close(s->stdout_fd); - s->stdout_fd = fdset_remove(fds, fd); + asynchronous_close(s->stdout_fd); + s->stdout_fd = deserialize_fd(fds, value); + if (s->stdout_fd >= 0) s->exec_context.stdio_as_fds = true; - } + } else if (streq(key, "stderr-fd")) { - int fd; - if ((fd = parse_fd(value)) < 0 || !fdset_contains(fds, fd)) - log_unit_debug(u, "Failed to parse stderr-fd value: %s", value); - else { - asynchronous_close(s->stderr_fd); - s->stderr_fd = fdset_remove(fds, fd); + asynchronous_close(s->stderr_fd); + s->stderr_fd = deserialize_fd(fds, value); + if (s->stderr_fd >= 0) s->exec_context.stdio_as_fds = true; - } - } else if (streq(key, "exec-fd")) { - int fd; - if ((fd = parse_fd(value)) < 0 || !fdset_contains(fds, fd)) - log_unit_debug(u, "Failed to parse exec-fd value: %s", value); - else { + } else if (streq(key, "exec-fd")) { + _cleanup_close_ int fd = -EBADF; + + fd = deserialize_fd(fds, value); + if (fd >= 0) { s->exec_fd_event_source = sd_event_source_disable_unref(s->exec_fd_event_source); - fd = fdset_remove(fds, fd); - if (service_allocate_exec_fd_event_source(s, fd, &s->exec_fd_event_source) < 0) - safe_close(fd); + if (service_allocate_exec_fd_event_source(s, fd, &s->exec_fd_event_source) >= 0) + TAKE_FD(fd); } + } else if (streq(key, "watchdog-override-usec")) { if (deserialize_usec(value, &s->watchdog_override_usec) < 0) log_unit_debug(u, "Failed to parse watchdog_override_usec value: %s", value); diff --git a/src/core/unit-serialize.c b/src/core/unit-serialize.c index be041010312..0ffcceaf53d 100644 --- a/src/core/unit-serialize.c +++ b/src/core/unit-serialize.c @@ -387,16 +387,9 @@ int unit_deserialize_state(Unit *u, FILE *f, FDSet *fds) { else if (STR_IN_SET(l, "ipv4-socket-bind-bpf-link-fd", "ipv6-socket-bind-bpf-link-fd")) { int fd; - if ((fd = parse_fd(v)) < 0 || !fdset_contains(fds, fd)) - log_unit_debug(u, "Failed to parse %s value: %s, ignoring.", l, v); - else { - if (fdset_remove(fds, fd) < 0) { - log_unit_debug(u, "Failed to remove %s value=%d from fdset", l, fd); - continue; - } - + fd = deserialize_fd(fds, v); + if (fd >= 0) (void) bpf_socket_bind_add_initial_link_fd(u, fd); - } continue; } else if (streq(l, "ip-bpf-ingress-installed")) { @@ -419,16 +412,10 @@ int unit_deserialize_state(Unit *u, FILE *f, FDSet *fds) { } else if (streq(l, "restrict-ifaces-bpf-fd")) { int fd; - if ((fd = parse_fd(v)) < 0 || !fdset_contains(fds, fd)) { - log_unit_debug(u, "Failed to parse restrict-ifaces-bpf-fd value: %s", v); - continue; - } - if (fdset_remove(fds, fd) < 0) { - log_unit_debug(u, "Failed to remove restrict-ifaces-bpf-fd %d from fdset", fd); - continue; - } + fd = deserialize_fd(fds, v); + if (fd >= 0) + (void) restrict_network_interfaces_add_initial_link_fd(u, fd); - (void) restrict_network_interfaces_add_initial_link_fd(u, fd); continue; } else if (streq(l, "ref-uid")) { diff --git a/src/shared/serialize.c b/src/shared/serialize.c index ee35055aeae..d62dfaa2d7a 100644 --- a/src/shared/serialize.c +++ b/src/shared/serialize.c @@ -285,6 +285,23 @@ int deserialize_read_line(FILE *f, char **ret) { return 1; } +int deserialize_fd(FDSet *fds, const char *value) { + _cleanup_close_ int our_fd = -EBADF; + int parsed_fd; + + assert(value); + + parsed_fd = parse_fd(value); + if (parsed_fd < 0) + return log_debug_errno(parsed_fd, "Failed to parse file descriptor serialization: %s", value); + + our_fd = fdset_remove(fds, parsed_fd); /* Take possession of the fd */ + if (our_fd < 0) + return log_debug_errno(our_fd, "Failed to acquire fd from serialization fds: %m"); + + return TAKE_FD(our_fd); +} + int deserialize_strv(char ***l, const char *value) { ssize_t unescaped_len; char *unescaped; @@ -372,18 +389,12 @@ int deserialize_pidref(FDSet *fds, const char *value, PidRef *ret) { e = startswith(value, "@"); if (e) { - _cleanup_close_ int our_fd = -EBADF; - int parsed_fd; + int fd = deserialize_fd(fds, e); - parsed_fd = parse_fd(e); - if (parsed_fd < 0) - return log_debug_errno(parsed_fd, "Failed to parse file descriptor specification: %s", e); + if (fd < 0) + return fd; - our_fd = fdset_remove(fds, parsed_fd); /* Take possession of the fd */ - if (our_fd < 0) - return log_debug_errno(our_fd, "Failed to acquire pidfd from serialization fds: %m"); - - r = pidref_set_pidfd_consume(ret, TAKE_FD(our_fd)); + r = pidref_set_pidfd_consume(ret, fd); } else { pid_t pid; diff --git a/src/shared/serialize.h b/src/shared/serialize.h index 5122c427936..decad81de7d 100644 --- a/src/shared/serialize.h +++ b/src/shared/serialize.h @@ -37,6 +37,7 @@ static inline int serialize_item_tristate(FILE *f, const char *key, int value) { int deserialize_read_line(FILE *f, char **ret); +int deserialize_fd(FDSet *fds, const char *value); int deserialize_usec(const char *value, usec_t *timestamp); int deserialize_dual_timestamp(const char *value, dual_timestamp *t); int deserialize_environment(const char *value, char ***environment); From af6321857dc9f0b9c1d58ec2fe4959aebc595c1b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 6 Oct 2023 17:53:13 +0200 Subject: [PATCH 2/7] serialize: modernize deserialize_{usec|dual_timestamp}() a bit Rename the return parameters "ret", and use compound initialization. Add an assert() on input. --- src/shared/serialize.c | 11 +++++++---- src/shared/serialize.h | 4 ++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/shared/serialize.c b/src/shared/serialize.c index d62dfaa2d7a..276e2b7b912 100644 --- a/src/shared/serialize.c +++ b/src/shared/serialize.c @@ -320,6 +320,7 @@ int deserialize_usec(const char *value, usec_t *ret) { int r; assert(value); + assert(ret); r = safe_atou64(value, ret); if (r < 0) @@ -328,12 +329,12 @@ int deserialize_usec(const char *value, usec_t *ret) { return 0; } -int deserialize_dual_timestamp(const char *value, dual_timestamp *t) { +int deserialize_dual_timestamp(const char *value, dual_timestamp *ret) { uint64_t a, b; int r, pos; assert(value); - assert(t); + assert(ret); pos = strspn(value, WHITESPACE); if (value[pos] == '-') @@ -353,8 +354,10 @@ int deserialize_dual_timestamp(const char *value, dual_timestamp *t) { /* trailing garbage */ return -EINVAL; - t->realtime = a; - t->monotonic = b; + *ret = (dual_timestamp) { + .realtime = a, + .monotonic = b, + }; return 0; } diff --git a/src/shared/serialize.h b/src/shared/serialize.h index decad81de7d..c4f78b3bd9a 100644 --- a/src/shared/serialize.h +++ b/src/shared/serialize.h @@ -38,8 +38,8 @@ static inline int serialize_item_tristate(FILE *f, const char *key, int value) { int deserialize_read_line(FILE *f, char **ret); int deserialize_fd(FDSet *fds, const char *value); -int deserialize_usec(const char *value, usec_t *timestamp); -int deserialize_dual_timestamp(const char *value, dual_timestamp *t); +int deserialize_usec(const char *value, usec_t *ret); +int deserialize_dual_timestamp(const char *value, dual_timestamp *ret); int deserialize_environment(const char *value, char ***environment); int deserialize_strv(char ***l, const char *value); int deserialize_pidref(FDSet *fds, const char *value, PidRef *ret); From c2e42d4b88216a6820ac39d37888ca1a285e6df6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 6 Oct 2023 17:56:30 +0200 Subject: [PATCH 3/7] serialize: change order of deserialize_strv() parameters The other deserializers put value first, and return parameter second, let's do so here too. --- src/core/execute-serialize.c | 36 ++++++++++++++++++------------------ src/shared/serialize.c | 2 +- src/shared/serialize.h | 2 +- src/test/test-serialize.c | 2 +- 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/core/execute-serialize.c b/src/core/execute-serialize.c index 1886a59b84c..ca65ca2c421 100644 --- a/src/core/execute-serialize.c +++ b/src/core/execute-serialize.c @@ -1008,11 +1008,11 @@ static int exec_cgroup_context_deserialize(CGroupContext *c, FILE *f) { return r; c->ip_address_deny_reduced = r; } else if ((val = startswith(l, "exec-cgroup-context-ip-ingress-filter-path="))) { - r = deserialize_strv(&c->ip_filters_ingress, val); + r = deserialize_strv(val, &c->ip_filters_ingress); if (r < 0) return r; } else if ((val = startswith(l, "exec-cgroup-context-ip-egress-filter-path="))) { - r = deserialize_strv(&c->ip_filters_egress, val); + r = deserialize_strv(val, &c->ip_filters_egress); if (r < 0) return r; } else if ((val = startswith(l, "exec-cgroup-context-bpf-program="))) { @@ -1544,7 +1544,7 @@ static int exec_parameters_deserialize(ExecParameters *p, FILE *f, FDSet *fds) { if (p->runtime_scope < 0) return p->runtime_scope; } else if ((val = startswith(l, "exec-parameters-environment="))) { - r = deserialize_strv(&p->environment, val); + r = deserialize_strv(val, &p->environment); if (r < 0) return r; } else if ((val = startswith(l, "exec-parameters-n-socket-fds="))) { @@ -1604,7 +1604,7 @@ static int exec_parameters_deserialize(ExecParameters *p, FILE *f, FDSet *fds) { p->fds[i] = fd; } } else if ((val = startswith(l, "exec-parameters-fd-names="))) { - r = deserialize_strv(&p->fd_names, val); + r = deserialize_strv(val, &p->fd_names); if (r < 0) return r; } else if ((val = startswith(l, "exec-parameters-flags="))) { @@ -1780,7 +1780,7 @@ static int exec_parameters_deserialize(ExecParameters *p, FILE *f, FDSet *fds) { p->user_lookup_fd = fd; } else if ((val = startswith(l, "exec-parameters-files-env="))) { - r = deserialize_strv(&p->files_env, val); + r = deserialize_strv(val, &p->files_env); if (r < 0) return r; } else if ((val = startswith(l, "exec-parameters-unit-id="))) { @@ -2730,19 +2730,19 @@ static int exec_context_deserialize(ExecContext *c, FILE *f) { break; if ((val = startswith(l, "exec-context-environment="))) { - r = deserialize_strv(&c->environment, val); + r = deserialize_strv(val, &c->environment); if (r < 0) return r; } else if ((val = startswith(l, "exec-context-environment-files="))) { - r = deserialize_strv(&c->environment_files, val); + r = deserialize_strv(val, &c->environment_files); if (r < 0) return r; } else if ((val = startswith(l, "exec-context-pass-environment="))) { - r = deserialize_strv(&c->pass_environment, val); + r = deserialize_strv(val, &c->pass_environment); if (r < 0) return r; } else if ((val = startswith(l, "exec-context-unset-environment="))) { - r = deserialize_strv(&c->unset_environment, val); + r = deserialize_strv(val, &c->unset_environment); if (r < 0) return r; } else if ((val = startswith(l, "exec-context-working-directory="))) { @@ -3288,7 +3288,7 @@ static int exec_context_deserialize(ExecContext *c, FILE *f) { return r; c->dynamic_user = r; } else if ((val = startswith(l, "exec-context-supplementary-groups="))) { - r = deserialize_strv(&c->supplementary_groups, val); + r = deserialize_strv(val, &c->supplementary_groups); if (r < 0) return r; } else if ((val = startswith(l, "exec-context-set-login-environment="))) { @@ -3300,27 +3300,27 @@ static int exec_context_deserialize(ExecContext *c, FILE *f) { if (r < 0) return r; } else if ((val = startswith(l, "exec-context-read-write-paths="))) { - r = deserialize_strv(&c->read_write_paths, val); + r = deserialize_strv(val, &c->read_write_paths); if (r < 0) return r; } else if ((val = startswith(l, "exec-context-read-only-paths="))) { - r = deserialize_strv(&c->read_only_paths, val); + r = deserialize_strv(val, &c->read_only_paths); if (r < 0) return r; } else if ((val = startswith(l, "exec-context-inaccessible-paths="))) { - r = deserialize_strv(&c->inaccessible_paths, val); + r = deserialize_strv(val, &c->inaccessible_paths); if (r < 0) return r; } else if ((val = startswith(l, "exec-context-exec-paths="))) { - r = deserialize_strv(&c->exec_paths, val); + r = deserialize_strv(val, &c->exec_paths); if (r < 0) return r; } else if ((val = startswith(l, "exec-context-no-exec-paths="))) { - r = deserialize_strv(&c->no_exec_paths, val); + r = deserialize_strv(val, &c->no_exec_paths); if (r < 0) return r; } else if ((val = startswith(l, "exec-context-exec-search-path="))) { - r = deserialize_strv(&c->exec_search_path, val); + r = deserialize_strv(val, &c->exec_search_path); if (r < 0) return r; } else if ((val = startswith(l, "exec-context-mount-propagation-flag="))) { @@ -3794,7 +3794,7 @@ static int exec_context_deserialize(ExecContext *c, FILE *f) { if (r < 0) return log_oom_debug(); } else if ((val = startswith(l, "exec-context-extension-directories="))) { - r = deserialize_strv(&c->extension_directories, val); + r = deserialize_strv(val, &c->extension_directories); if (r < 0) return r; } else if ((val = startswith(l, "exec-context-set-credentials="))) { @@ -3938,7 +3938,7 @@ static int exec_command_deserialize(ExecCommand *c, FILE *f) { if (r < 0) return r; } else if ((val = startswith(l, "exec-command-argv="))) { - r = deserialize_strv(&c->argv, val); + r = deserialize_strv(val, &c->argv); if (r < 0) return r; } else if ((val = startswith(l, "exec-command-flags="))) { diff --git a/src/shared/serialize.c b/src/shared/serialize.c index 276e2b7b912..5fecaa87028 100644 --- a/src/shared/serialize.c +++ b/src/shared/serialize.c @@ -302,7 +302,7 @@ int deserialize_fd(FDSet *fds, const char *value) { return TAKE_FD(our_fd); } -int deserialize_strv(char ***l, const char *value) { +int deserialize_strv(const char *value, char ***l) { ssize_t unescaped_len; char *unescaped; diff --git a/src/shared/serialize.h b/src/shared/serialize.h index c4f78b3bd9a..b2dcfd21102 100644 --- a/src/shared/serialize.h +++ b/src/shared/serialize.h @@ -41,7 +41,7 @@ int deserialize_fd(FDSet *fds, const char *value); int deserialize_usec(const char *value, usec_t *ret); int deserialize_dual_timestamp(const char *value, dual_timestamp *ret); int deserialize_environment(const char *value, char ***environment); -int deserialize_strv(char ***l, const char *value); +int deserialize_strv(const char *value, char ***l); int deserialize_pidref(FDSet *fds, const char *value, PidRef *ret); int open_serialization_fd(const char *ident); diff --git a/src/test/test-serialize.c b/src/test/test-serialize.c index 9229e3f5c77..8f744727c67 100644 --- a/src/test/test-serialize.c +++ b/src/test/test-serialize.c @@ -128,7 +128,7 @@ TEST(serialize_strv) { const char *t = startswith(line, "strv3="); assert_se(t); - assert_se(deserialize_strv(&strv2, t) >= 0); + assert_se(deserialize_strv(t, &strv2) >= 0); } assert_se(strv_equal(strv, strv2)); From 3b444970e609410951b4a96a9dcb648453c8b3f2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 16 Oct 2023 15:56:58 +0200 Subject: [PATCH 4/7] serialize: add serialize_fd_many() helper --- src/basic/fd-util.c | 7 ++ src/basic/fd-util.h | 1 + src/core/execute-serialize.c | 159 +++++------------------------------ src/shared/serialize.c | 69 +++++++++++++++ src/shared/serialize.h | 2 + 5 files changed, 99 insertions(+), 139 deletions(-) diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index 5292e41937a..33c4a057adf 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -99,6 +99,13 @@ void close_many(const int fds[], size_t n_fd) { safe_close(fds[i]); } +void close_many_and_free(int *fds, size_t n_fds) { + assert(fds || n_fds <= 0); + + close_many(fds, n_fds); + free(fds); +} + int fclose_nointr(FILE *f) { assert(f); diff --git a/src/basic/fd-util.h b/src/basic/fd-util.h index 6aecb91fab3..1f5ed828b7b 100644 --- a/src/basic/fd-util.h +++ b/src/basic/fd-util.h @@ -30,6 +30,7 @@ static inline int safe_close_above_stdio(int fd) { } void close_many(const int fds[], size_t n_fd); +void close_many_and_free(int *fds, size_t n_fds); int fclose_nointr(FILE *f); FILE* safe_fclose(FILE *f); diff --git a/src/core/execute-serialize.c b/src/core/execute-serialize.c index ca65ca2c421..55f5bdc8c97 100644 --- a/src/core/execute-serialize.c +++ b/src/core/execute-serialize.c @@ -1117,33 +1117,13 @@ static int exec_runtime_serialize(const ExecRuntime *rt, FILE *f, FDSet *fds) { return r; if (rt->shared->netns_storage_socket[0] >= 0 && rt->shared->netns_storage_socket[1] >= 0) { - int a, b; - - a = fdset_put_dup(fds, rt->shared->netns_storage_socket[0]); - if (a < 0) - return a; - - b = fdset_put_dup(fds, rt->shared->netns_storage_socket[1]); - if (b < 0) - return b; - - r = serialize_item_format(f, "exec-runtime-netns-storage-socket", "%d %d", a, b); + r = serialize_fd_many(f, fds, "exec-runtime-netns-storage-socket", rt->shared->netns_storage_socket, 2); if (r < 0) return r; } if (rt->shared->ipcns_storage_socket[0] >= 0 && rt->shared->ipcns_storage_socket[1] >= 0) { - int a, b; - - a = fdset_put_dup(fds, rt->shared->ipcns_storage_socket[0]); - if (a < 0) - return a; - - b = fdset_put_dup(fds, rt->shared->ipcns_storage_socket[1]); - if (b < 0) - return b; - - r = serialize_item_format(f, "exec-runtime-ipcns-storage-socket", "%d %d", a, b); + r = serialize_fd_many(f, fds, "exec-runtime-ipcns-storage-socket", rt->shared->ipcns_storage_socket, 2); if (r < 0) return r; } @@ -1170,17 +1150,7 @@ static int exec_runtime_serialize(const ExecRuntime *rt, FILE *f, FDSet *fds) { return r; if (rt->ephemeral_storage_socket[0] >= 0 && rt->ephemeral_storage_socket[1] >= 0) { - int a, b; - - a = fdset_put_dup(fds, rt->ephemeral_storage_socket[0]); - if (a < 0) - return a; - - b = fdset_put_dup(fds, rt->ephemeral_storage_socket[1]); - if (b < 0) - return b; - - r = serialize_item_format(f, "exec-runtime-ephemeral-storage-socket", "%d %d", a, b); + r = serialize_fd_many(f, fds, "exec-runtime-ephemeral-storage-socket", rt->ephemeral_storage_socket, 2); if (r < 0) return r; } @@ -1222,39 +1192,17 @@ static int exec_runtime_deserialize(ExecRuntime *rt, FILE *f, FDSet *fds) { if (r < 0) return r; } else if ((val = startswith(l, "exec-runtime-netns-storage-socket="))) { - for (size_t i = 0; i < 2; ++i) { - _cleanup_free_ char *w = NULL; - int fd; - r = extract_first_word(&val, &w, WHITESPACE, 0); - if (r < 0) - return r; - if (r == 0) - break; + r = deserialize_fd_many(fds, val, 2, rt->shared->netns_storage_socket); + if (r < 0) + continue; - fd = deserialize_fd(fds, w); - if (fd < 0) - continue; - - rt->shared->netns_storage_socket[i] = fd; - } } else if ((val = startswith(l, "exec-runtime-ipcns-storage-socket="))) { - for (size_t i = 0; i < 2; ++i) { - _cleanup_free_ char *w = NULL; - int fd; - r = extract_first_word(&val, &w, WHITESPACE, 0); - if (r < 0) - return r; - if (r == 0) - break; + r = deserialize_fd_many(fds, val, 2, rt->shared->ipcns_storage_socket); + if (r < 0) + continue; - fd = deserialize_fd(fds, w); - if (fd < 0) - continue; - - rt->shared->ipcns_storage_socket[i] = fd; - } } else if ((val = startswith(l, "exec-runtime-dynamic-creds-user="))) dynamic_user_deserialize_one(/* m= */ NULL, val, fds, &rt->dynamic_creds->user); else if ((val = startswith(l, "exec-runtime-dynamic-creds-group="))) @@ -1275,22 +1223,10 @@ static int exec_runtime_deserialize(ExecRuntime *rt, FILE *f, FDSet *fds) { if (r < 0) return r; } else if ((val = startswith(l, "exec-runtime-ephemeral-storage-socket="))) { - for (size_t i = 0; i < 2; ++i) { - _cleanup_free_ char *w = NULL; - int fd; - r = extract_first_word(&val, &w, WHITESPACE, 0); - if (r < 0) - return r; - if (r == 0) - break; - - fd = deserialize_fd(fds, w); - if (fd < 0) - continue; - - rt->ephemeral_storage_socket[i] = fd; - } + r = deserialize_fd_many(fds, val, 2, rt->ephemeral_storage_socket); + if (r < 0) + continue; } else log_warning("Failed to parse serialized line, ignoring: %s", l); } @@ -1338,26 +1274,7 @@ static int exec_parameters_serialize(const ExecParameters *p, FILE *f, FDSet *fd } if (p->n_socket_fds + p->n_storage_fds > 0) { - _cleanup_free_ char *serialized_fds = NULL; - - if (!p->fds) - return -EINVAL; - - FOREACH_ARRAY(fd, p->fds, p->n_socket_fds + p->n_storage_fds) { - int copy = -EBADF; - - if (*fd >= 0) { - copy = fdset_put_dup(fds, *fd); - if (copy < 0) - return copy; - } - - r = strextendf(&serialized_fds, "%d ", copy); - if (r < 0) - return r; - } - - r = serialize_item(f, "exec-parameters-fds", serialized_fds); + r = serialize_fd_many(f, fds, "exec-parameters-fds", p->fds, p->n_socket_fds + p->n_storage_fds); if (r < 0) return r; } @@ -1427,19 +1344,7 @@ static int exec_parameters_serialize(const ExecParameters *p, FILE *f, FDSet *fd } if (exec_parameters_is_idle_pipe_set(p)) { - _cleanup_free_ char *serialized_fds = NULL; - - for (size_t i = 0; i < 4; ++i) { - int copy = fdset_put_dup(fds, p->idle_pipe[i]); - if (copy < 0) - return copy; - - r = strextendf(&serialized_fds, "%d ", copy); - if (r < 0) - return r; - } - - r = serialize_item(f, "exec-parameters-idle-pipe", serialized_fds); + r = serialize_fd_many(f, fds, "exec-parameters-idle-pipe", p->idle_pipe, 4); if (r < 0) return r; } @@ -1587,22 +1492,10 @@ static int exec_parameters_deserialize(ExecParameters *p, FILE *f, FDSet *fds) { for (size_t i = 0; i < p->n_socket_fds + p->n_storage_fds; ++i) p->fds[i] = -EBADF; - for (size_t i = 0; i < p->n_socket_fds + p->n_storage_fds; ++i) { - _cleanup_free_ char *w = NULL; - int fd; + r = deserialize_fd_many(fds, val, p->n_socket_fds + p->n_storage_fds, p->fds); + if (r < 0) + continue; - r = extract_first_word(&val, &w, WHITESPACE, 0); - if (r < 0) - return r; - if (r == 0) - break; - - fd = deserialize_fd(fds, w); - if (fd < 0) - continue; - - p->fds[i] = fd; - } } else if ((val = startswith(l, "exec-parameters-fd-names="))) { r = deserialize_strv(val, &p->fd_names); if (r < 0) @@ -1691,22 +1584,10 @@ static int exec_parameters_deserialize(ExecParameters *p, FILE *f, FDSet *fds) { p->idle_pipe[0] = p->idle_pipe[1] = p->idle_pipe[2] = p->idle_pipe[3] = -EBADF; - for (size_t i = 0; i < 4; ++i) { - _cleanup_free_ char *w = NULL; - int fd; + r = deserialize_fd_many(fds, val, 4, p->idle_pipe); + if (r < 0) + continue; - r = extract_first_word(&val, &w, WHITESPACE, 0); - if (r < 0) - return r; - if (r == 0) - break; - - fd = deserialize_fd(fds, w); - if (fd < 0) - continue; - - p->idle_pipe[i] = fd; - } } else if ((val = startswith(l, "exec-parameters-stdin-fd="))) { int fd; diff --git a/src/shared/serialize.c b/src/shared/serialize.c index 5fecaa87028..cb1255932bb 100644 --- a/src/shared/serialize.c +++ b/src/shared/serialize.c @@ -116,6 +116,33 @@ int serialize_fd(FILE *f, FDSet *fds, const char *key, int fd) { return serialize_item_format(f, key, "%i", copy); } +int serialize_fd_many(FILE *f, FDSet *fds, const char *key, const int fd_array[], size_t n_fd_array) { + _cleanup_free_ char *t = NULL; + + assert(f); + + if (n_fd_array == 0) + return 0; + + assert(fd_array); + + for (size_t i = 0; i < n_fd_array; i++) { + int copy; + + if (fd_array[i] < 0) + return -EBADF; + + copy = fdset_put_dup(fds, fd_array[i]); + if (copy < 0) + return log_error_errno(copy, "Failed to add file descriptor to serialization set: %m"); + + if (strextendf_with_separator(&t, " ", "%i", copy) < 0) + return log_oom(); + } + + return serialize_item(f, key, t); +} + int serialize_usec(FILE *f, const char *key, usec_t usec) { assert(f); assert(key); @@ -302,6 +329,48 @@ int deserialize_fd(FDSet *fds, const char *value) { return TAKE_FD(our_fd); } +int deserialize_fd_many(FDSet *fds, const char *value, size_t n, int *ret) { + int r, *fd_array = NULL; + size_t m = 0; + + assert(value); + + fd_array = new(int, n); + if (!fd_array) + return -ENOMEM; + + CLEANUP_ARRAY(fd_array, m, close_many_and_free); + + for (;;) { + _cleanup_free_ char *w = NULL; + int fd; + + r = extract_first_word(&value, &w, NULL, 0); + if (r < 0) + return r; + if (r == 0) { + if (m < n) /* Too few */ + return -EINVAL; + + break; + } + + if (m >= n) /* Too many */ + return -EINVAL; + + fd = deserialize_fd(fds, w); + if (fd < 0) + return fd; + + fd_array[m++] = fd; + } + + memcpy(ret, fd_array, m * sizeof(int)); + fd_array = mfree(fd_array); + + return 0; +} + int deserialize_strv(const char *value, char ***l) { ssize_t unescaped_len; char *unescaped; diff --git a/src/shared/serialize.h b/src/shared/serialize.h index b2dcfd21102..c5211191f03 100644 --- a/src/shared/serialize.h +++ b/src/shared/serialize.h @@ -17,6 +17,7 @@ int serialize_item_format(FILE *f, const char *key, const char *value, ...) _pri int serialize_item_hexmem(FILE *f, const char *key, const void *p, size_t l); int serialize_item_base64mem(FILE *f, const char *key, const void *p, size_t l); int serialize_fd(FILE *f, FDSet *fds, const char *key, int fd); +int serialize_fd_many(FILE *f, FDSet *fds, const char *key, const int fd_array[], size_t n_fd_array); int serialize_usec(FILE *f, const char *key, usec_t usec); int serialize_dual_timestamp(FILE *f, const char *key, const dual_timestamp *t); int serialize_strv(FILE *f, const char *key, char **l); @@ -38,6 +39,7 @@ static inline int serialize_item_tristate(FILE *f, const char *key, int value) { int deserialize_read_line(FILE *f, char **ret); int deserialize_fd(FDSet *fds, const char *value); +int deserialize_fd_many(FDSet *fds, const char *value, size_t n, int *ret); int deserialize_usec(const char *value, usec_t *ret); int deserialize_dual_timestamp(const char *value, dual_timestamp *ret); int deserialize_environment(const char *value, char ***environment); From 26f937e5764a7eadf8f2dfd9a0b7868642ec5464 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 18 Oct 2023 12:12:30 +0200 Subject: [PATCH 5/7] execute: use close_many_and_free() more --- src/core/execute.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index d9c8a19c380..ce947a7a52f 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2260,9 +2260,10 @@ void exec_params_serialized_done(ExecParameters *p) { p->received_credentials_directory = mfree(p->received_credentials_directory); p->received_encrypted_credentials_directory = mfree(p->received_encrypted_credentials_directory); - for (size_t i = 0; p->idle_pipe && i < 4; i++) - p->idle_pipe[i] = safe_close(p->idle_pipe[i]); - p->idle_pipe = mfree(p->idle_pipe); + if (p->idle_pipe) { + close_many_and_free(p->idle_pipe, 4); + p->idle_pipe = NULL; + } p->stdin_fd = safe_close(p->stdin_fd); p->stdout_fd = safe_close(p->stdout_fd); From d3eb74f8f844cf3dc7913d861070ffda66035933 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 18 Oct 2023 14:03:36 +0200 Subject: [PATCH 6/7] fd-util: add new helper close_many_unset() --- src/basic/fd-util.c | 7 +++++++ src/basic/fd-util.h | 1 + src/core/execute.c | 3 +-- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index 33c4a057adf..5fdbf8f2693 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -99,6 +99,13 @@ void close_many(const int fds[], size_t n_fd) { safe_close(fds[i]); } +void close_many_unset(int fds[], size_t n_fd) { + assert(fds || n_fd <= 0); + + for (size_t i = 0; i < n_fd; i++) + fds[i] = safe_close(fds[i]); +} + void close_many_and_free(int *fds, size_t n_fds) { assert(fds || n_fds <= 0); diff --git a/src/basic/fd-util.h b/src/basic/fd-util.h index 1f5ed828b7b..b2efdc9c234 100644 --- a/src/basic/fd-util.h +++ b/src/basic/fd-util.h @@ -30,6 +30,7 @@ static inline int safe_close_above_stdio(int fd) { } void close_many(const int fds[], size_t n_fd); +void close_many_unset(int fds[], size_t n_fd); void close_many_and_free(int *fds, size_t n_fds); int fclose_nointr(FILE *f); diff --git a/src/core/execute.c b/src/core/execute.c index ce947a7a52f..46fb8805b3a 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2251,8 +2251,7 @@ void exec_params_serialized_done(ExecParameters *p) { if (!p) return; - for (size_t i = 0; p->fds && i < p->n_socket_fds + p->n_storage_fds; i++) - p->fds[i] = safe_close(p->fds[i]); + close_many_unset(p->fds, p->n_socket_fds + p->n_storage_fds); p->cgroup_path = mfree(p->cgroup_path); From cf45db559859f844d084259e47e291dc8f212b39 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 18 Oct 2023 14:03:47 +0200 Subject: [PATCH 7/7] fd-util: use close_many() where appropriate --- src/basic/fd-util.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index 5fdbf8f2693..59c243e70d1 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -752,8 +752,7 @@ finish: safe_close_above_stdio(original_error_fd); /* Close the copies we moved > 2 */ - for (int i = 0; i < 3; i++) - safe_close(copy_fd[i]); + close_many(copy_fd, 3); /* Close our null fd, if it's > 2 */ safe_close_above_stdio(null_fd);