From 79bad078bb9403aab6a9c45bf4769e684bdc3f59 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 29 Nov 2023 23:18:23 +0800 Subject: [PATCH] core/executor: avoid double closing serialization fd Before this commit, between fdopen() (in parse_argv()) and fdset_remove(), the serialization fd is owned by both arg_serialization FILE stream and fdset. Therefore, if something wrong happens between the two calls, or if --deserialize= is specified more than once, we end up closing the serialization fd twice. Normally this doesn't matter much, but I still think it's better to fix this. Let's call fdset_new_fill() after parsing serialization fd hence. We set the fd to CLOEXEC in parse_argv(), so it will be filtered when the fdset is created. While at it, also move fdset_new_fill() under the second log_open(), so that we always log to the log target specified in arguments. --- src/core/executor.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/src/core/executor.c b/src/core/executor.c index 2026055..f55bacd 100644 --- a/src/core/executor.c +++ b/src/core/executor.c @@ -132,24 +132,22 @@ static int parse_argv(int argc, char *argv[]) { break; case ARG_DESERIALIZE: { + _cleanup_close_ int fd = -EBADF; FILE *f; - int fd; fd = parse_fd(optarg); if (fd < 0) - return log_error_errno( - fd, - "Failed to parse serialization fd \"%s\": %m", - optarg); + return log_error_errno(fd, + "Failed to parse serialization fd \"%s\": %m", + optarg); r = fd_cloexec(fd, /* cloexec= */ true); if (r < 0) - return log_error_errno( - r, - "Failed to set serialization fd \"%s\" to close-on-exec: %m", - optarg); + return log_error_errno(r, + "Failed to set serialization fd %d to close-on-exec: %m", + fd); - f = fdopen(fd, "r"); + f = take_fdopen(&fd, "r"); if (!f) return log_error_errno(errno, "Failed to open serialization fd %d: %m", fd); @@ -167,8 +165,7 @@ static int parse_argv(int argc, char *argv[]) { } if (!arg_serialization) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "No serialization fd specified."); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "No serialization fd specified."); return 1 /* work to do */; } @@ -199,10 +196,6 @@ int main(int argc, char *argv[]) { log_set_prohibit_ipc(true); log_setup(); - r = fdset_new_fill(/* filter_cloexec= */ 0, &fdset); - if (r < 0) - return log_error_errno(r, "Failed to create fd set: %m"); - r = parse_argv(argc, argv); if (r <= 0) return r; @@ -211,6 +204,11 @@ int main(int argc, char *argv[]) { log_set_prohibit_ipc(false); log_open(); + /* The serialization fd is set to CLOEXEC in parse_argv, so it's also filtered. */ + r = fdset_new_fill(/* filter_cloexec= */ 0, &fdset); + if (r < 0) + return log_error_errno(r, "Failed to create fd set: %m"); + /* Initialize lazily. SMACK is just a few operations, but the SELinux is very slow as it requires * loading the entire database in memory, so we will do it lazily only if it is actually needed, to * avoid wasting 2ms-10ms for each sd-executor that gets spawned. */ @@ -218,10 +216,6 @@ int main(int argc, char *argv[]) { if (r < 0) return log_error_errno(r, "Failed to initialize MAC layer: %m"); - r = fdset_remove(fdset, fileno(arg_serialization)); - if (r < 0) - return log_error_errno(r, "Failed to remove serialization fd from fd set: %m"); - r = exec_deserialize_invocation(arg_serialization, fdset, &context, -- 2.7.4