From 9c46228b7deb53d6384545535b37b2844a102b2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Jul 2021 10:51:14 +0200 Subject: [PATCH 1/6] basic/fd-util: sort the 'except' array in place We need a sorted list of fds to skip over when closing. We would allocate a copy of the passed array to do the sort. But all callers construct a temporary array to pass to us, so it is pointless to copy it again. close_all_fds/safe_fork_full/namespace_fork/fork_agent are changed to pass a non-const int array. I checked all users, and all callers are fine with the array being sorted. The function was returning some number (sometimes 1, sometimes the extent of the range passed over to close_range(), ???). Anyway, all callers only check for error, so let's return 0 on success. --- src/basic/fd-util.c | 146 ++++++++++++++++----------------------- src/basic/fd-util.h | 2 +- src/basic/process-util.c | 9 +-- src/basic/process-util.h | 6 +- 4 files changed, 69 insertions(+), 94 deletions(-) diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index 008f474344..99b517d8c2 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -208,10 +208,9 @@ static int get_max_fd(void) { return (int) (m - 1); } -int close_all_fds(const int except[], size_t n_except) { +int close_all_fds(int except[], size_t n_except) { static bool have_close_range = true; /* Assume we live in the future */ _cleanup_closedir_ DIR *d = NULL; - struct dirent *de; int r = 0; assert(n_except == 0 || except); @@ -227,129 +226,104 @@ int close_all_fds(const int except[], size_t n_except) { /* Close everything. Yay! */ if (close_range(3, -1, 0) >= 0) - return 1; + return 0; - if (!ERRNO_IS_NOT_SUPPORTED(errno) && !ERRNO_IS_PRIVILEGE(errno)) + if (ERRNO_IS_NOT_SUPPORTED(errno) || ERRNO_IS_PRIVILEGE(errno)) + have_close_range = false; + else return -errno; - have_close_range = false; } else { - _cleanup_free_ int *sorted_malloc = NULL; - size_t n_sorted; - int *sorted; + typesafe_qsort(except, n_except, cmp_int); - assert(n_except < SIZE_MAX); - n_sorted = n_except + 1; + for (size_t i = 0; i < n_except; i++) { + int start = i == 0 ? 2 : MAX(except[i-1], 2); /* The first three fds shall always remain open */ + int end = MAX(except[i], 2); - if (n_sorted > 64) /* Use heap for large numbers of fds, stack otherwise */ - sorted = sorted_malloc = new(int, n_sorted); - else - sorted = newa(int, n_sorted); + assert(end >= start); - if (sorted) { - int c = 0; - - memcpy(sorted, except, n_except * sizeof(int)); - - /* Let's add fd 2 to the list of fds, to simplify the loop below, as this - * allows us to cover the head of the array the same way as the body */ - sorted[n_sorted-1] = 2; - - typesafe_qsort(sorted, n_sorted, cmp_int); - - for (size_t i = 0; i < n_sorted-1; i++) { - int start, end; - - start = MAX(sorted[i], 2); /* The first three fds shall always remain open */ - end = MAX(sorted[i+1], 2); - - assert(end >= start); - - if (end - start <= 1) - continue; - - /* Close everything between the start and end fds (both of which shall stay open) */ - if (close_range(start + 1, end - 1, 0) < 0) { - if (!ERRNO_IS_NOT_SUPPORTED(errno) && !ERRNO_IS_PRIVILEGE(errno)) - return -errno; + if (end - start <= 1) + continue; + /* Close everything between the start and end fds (both of which shall stay open) */ + if (close_range(start + 1, end - 1, 0) < 0) { + if (ERRNO_IS_NOT_SUPPORTED(errno) || ERRNO_IS_PRIVILEGE(errno)) have_close_range = false; - break; - } - - c += end - start - 1; - } - - if (have_close_range) { - /* The loop succeeded. Let's now close everything beyond the end */ - - if (sorted[n_sorted-1] >= INT_MAX) /* Dont let the addition below overflow */ - return c; - - if (close_range(sorted[n_sorted-1] + 1, -1, 0) >= 0) - return c + 1; - - if (!ERRNO_IS_NOT_SUPPORTED(errno) && !ERRNO_IS_PRIVILEGE(errno)) + else return -errno; - - have_close_range = false; + goto opendir_fallback; } } - } - /* Fallback on OOM or if close_range() is not supported */ + /* The loop succeeded. Let's now close everything beyond the end */ + + if (except[n_except-1] >= INT_MAX) /* Don't let the addition below overflow */ + return 0; + + int start = MAX(except[n_except-1], 2); + + if (close_range(start + 1, -1, 0) >= 0) + return 0; + + if (ERRNO_IS_NOT_SUPPORTED(errno) || ERRNO_IS_PRIVILEGE(errno)) + have_close_range = false; + else + return -errno; + } } + /* Fallback for when close_range() is not supported */ + opendir_fallback: d = opendir("/proc/self/fd"); - if (!d) { - int fd, max_fd; + if (d) { + struct dirent *de; - /* When /proc isn't available (for example in chroots) the fallback is brute forcing through - * the fd table */ + FOREACH_DIRENT(de, d, return -errno) { + int fd = -1, q; - max_fd = get_max_fd(); - if (max_fd < 0) - return max_fd; + if (safe_atoi(de->d_name, &fd) < 0) + /* Let's better ignore this, just in case */ + continue; - /* Refuse to do the loop over more too many elements. It's better to fail immediately than to - * spin the CPU for a long time. */ - if (max_fd > MAX_FD_LOOP_LIMIT) - return log_debug_errno(SYNTHETIC_ERRNO(EPERM), - "/proc/self/fd is inaccessible. Refusing to loop over %d potential fds.", - max_fd); + if (fd < 3) + continue; - for (fd = 3; fd >= 0; fd = fd < max_fd ? fd + 1 : -1) { - int q; + if (fd == dirfd(d)) + continue; if (fd_in_set(fd, except, n_except)) continue; q = close_nointr(fd); - if (q < 0 && q != -EBADF && r >= 0) + if (q < 0 && q != -EBADF && r >= 0) /* Valgrind has its own FD and doesn't want to have it closed */ r = q; } return r; } - FOREACH_DIRENT(de, d, return -errno) { - int fd = -1, q; + /* Fallback for when /proc isn't available (for example in chroots) by brute-forcing through the file + * descriptor table. */ - if (safe_atoi(de->d_name, &fd) < 0) - /* Let's better ignore this, just in case */ - continue; + int max_fd = get_max_fd(); + if (max_fd < 0) + return max_fd; - if (fd < 3) - continue; + /* Refuse to do the loop over more too many elements. It's better to fail immediately than to + * spin the CPU for a long time. */ + if (max_fd > MAX_FD_LOOP_LIMIT) + return log_debug_errno(SYNTHETIC_ERRNO(EPERM), + "/proc/self/fd is inaccessible. Refusing to loop over %d potential fds.", + max_fd); - if (fd == dirfd(d)) - continue; + for (int fd = 3; fd >= 0; fd = fd < max_fd ? fd + 1 : -1) { + int q; if (fd_in_set(fd, except, n_except)) continue; q = close_nointr(fd); - if (q < 0 && q != -EBADF && r >= 0) /* Valgrind has its own FD and doesn't want to have it closed */ + if (q < 0 && q != -EBADF && r >= 0) r = q; } diff --git a/src/basic/fd-util.h b/src/basic/fd-util.h index 9529a4723d..d3c2944d86 100644 --- a/src/basic/fd-util.h +++ b/src/basic/fd-util.h @@ -56,7 +56,7 @@ DEFINE_TRIVIAL_CLEANUP_FUNC_FULL(DIR*, closedir, NULL); int fd_nonblock(int fd, bool nonblock); int fd_cloexec(int fd, bool cloexec); -int close_all_fds(const int except[], size_t n_except); +int close_all_fds(int except[], size_t n_except); int same_fd(int a, int b); diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 14259ea8df..30a608231e 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -1271,7 +1271,7 @@ static void restore_sigsetp(sigset_t **ssp) { int safe_fork_full( const char *name, - const int except_fds[], + int except_fds[], size_t n_except_fds, ForkFlags flags, pid_t *ret_pid) { @@ -1466,7 +1466,7 @@ int safe_fork_full( int namespace_fork( const char *outer_name, const char *inner_name, - const int except_fds[], + int except_fds[], size_t n_except_fds, ForkFlags flags, int pidns_fd, @@ -1482,7 +1482,8 @@ int namespace_fork( * process. This ensures that we are fully a member of the destination namespace, with pidns an all, so that * /proc/self/fd works correctly. */ - r = safe_fork_full(outer_name, except_fds, n_except_fds, (flags|FORK_DEATHSIG) & ~(FORK_REOPEN_LOG|FORK_NEW_MOUNTNS|FORK_MOUNTNS_SLAVE), ret_pid); + r = safe_fork_full(outer_name, except_fds, n_except_fds, + (flags|FORK_DEATHSIG) & ~(FORK_REOPEN_LOG|FORK_NEW_MOUNTNS|FORK_MOUNTNS_SLAVE), ret_pid); if (r < 0) return r; if (r == 0) { @@ -1517,7 +1518,7 @@ int namespace_fork( return 1; } -int fork_agent(const char *name, const int except[], size_t n_except, pid_t *ret_pid, const char *path, ...) { +int fork_agent(const char *name, int except[], size_t n_except, pid_t *ret_pid, const char *path, ...) { bool stdout_is_tty, stderr_is_tty; size_t n, i; va_list ap; diff --git a/src/basic/process-util.h b/src/basic/process-util.h index 0e064de85e..73717dd3a4 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -168,15 +168,15 @@ typedef enum ForkFlags { FORK_NEW_USERNS = 1 << 13, /* Run child in its own user namespace */ } ForkFlags; -int safe_fork_full(const char *name, const int except_fds[], size_t n_except_fds, ForkFlags flags, pid_t *ret_pid); +int safe_fork_full(const char *name, int except_fds[], size_t n_except_fds, ForkFlags flags, pid_t *ret_pid); static inline int safe_fork(const char *name, ForkFlags flags, pid_t *ret_pid) { return safe_fork_full(name, NULL, 0, flags, ret_pid); } -int namespace_fork(const char *outer_name, const char *inner_name, const int except_fds[], size_t n_except_fds, ForkFlags flags, int pidns_fd, int mntns_fd, int netns_fd, int userns_fd, int root_fd, pid_t *ret_pid); +int namespace_fork(const char *outer_name, const char *inner_name, int except_fds[], size_t n_except_fds, ForkFlags flags, int pidns_fd, int mntns_fd, int netns_fd, int userns_fd, int root_fd, pid_t *ret_pid); -int fork_agent(const char *name, const int except[], size_t n_except, pid_t *pid, const char *path, ...) _sentinel_; +int fork_agent(const char *name, int except[], size_t n_except, pid_t *pid, const char *path, ...) _sentinel_; int set_oom_score_adjust(int value); From 6bf3c6c9007ca87376d5dff1e029186a38736cdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Jul 2021 11:06:13 +0200 Subject: [PATCH 2/6] Make oom_score_adjust_is_valid() static It has only one user and we don't need to put it in basic/. --- src/basic/process-util.c | 4 ---- src/basic/process-util.h | 2 -- src/core/dbus-execute.c | 5 +++++ 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 30a608231e..4ca22a583f 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -1061,10 +1061,6 @@ _noreturn_ void freeze(void) { pause(); } -bool oom_score_adjust_is_valid(int oa) { - return oa >= OOM_SCORE_ADJ_MIN && oa <= OOM_SCORE_ADJ_MAX; -} - unsigned long personality_from_string(const char *p) { int architecture; diff --git a/src/basic/process-util.h b/src/basic/process-util.h index 73717dd3a4..58a3ebd577 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -84,8 +84,6 @@ bool is_main_thread(void); _noreturn_ void freeze(void); -bool oom_score_adjust_is_valid(int oa); - #ifndef PERSONALITY_INVALID /* personality(7) documents that 0xffffffffUL is used for querying the * current personality, hence let's use that here as error diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index f6783e924a..f7784bb73d 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include #include #include @@ -94,6 +95,10 @@ static int property_get_environment_files( return sd_bus_message_close_container(reply); } +static bool oom_score_adjust_is_valid(int oa) { + return oa >= OOM_SCORE_ADJ_MIN && oa <= OOM_SCORE_ADJ_MAX; +} + static int property_get_oom_score_adjust( sd_bus *bus, const char *path, From 3bd6a01c0190655f50fb7f171c00f1c4c48a4069 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Jul 2021 11:06:26 +0200 Subject: [PATCH 3/6] basic/process-util: use xsprintf() in one more place --- src/basic/process-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 4ca22a583f..a78e8141b4 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -1593,7 +1593,7 @@ int fork_agent(const char *name, int except[], size_t n_except, pid_t *ret_pid, int set_oom_score_adjust(int value) { char t[DECIMAL_STR_MAX(int)]; - sprintf(t, "%i", value); + xsprintf(t, "%i", value); return write_string_file("/proc/self/oom_score_adj", t, WRITE_STRING_FILE_VERIFY_ON_FAILURE|WRITE_STRING_FILE_DISABLE_BUFFER); From 3e24e8cd647478b3d161f1887785132e334e5df5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Jul 2021 11:20:39 +0200 Subject: [PATCH 4/6] Move fork_agent() into shared/ Currently it's only used in two places in src/shared/, so the function was already included just once in compiled code. But it seems appropriate to move it there anyway, because library code should have no need to fork agents, so it doesn't belong in basic/. --- src/basic/process-util.c | 76 --------------------------- src/basic/process-util.h | 2 - src/shared/exec-util.c | 73 +++++++++++++++++++++++++ src/shared/exec-util.h | 2 + src/shared/spawn-ask-password-agent.c | 1 + src/shared/spawn-polkit-agent.c | 1 + 6 files changed, 77 insertions(+), 78 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index a78e8141b4..e325820584 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -1514,82 +1514,6 @@ int namespace_fork( return 1; } -int fork_agent(const char *name, int except[], size_t n_except, pid_t *ret_pid, const char *path, ...) { - bool stdout_is_tty, stderr_is_tty; - size_t n, i; - va_list ap; - char **l; - int r; - - assert(path); - - /* Spawns a temporary TTY agent, making sure it goes away when we go away */ - - r = safe_fork_full(name, - except, - n_except, - FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_CLOSE_ALL_FDS|FORK_REOPEN_LOG, - ret_pid); - if (r < 0) - return r; - if (r > 0) - return 0; - - /* In the child: */ - - stdout_is_tty = isatty(STDOUT_FILENO); - stderr_is_tty = isatty(STDERR_FILENO); - - if (!stdout_is_tty || !stderr_is_tty) { - int fd; - - /* Detach from stdout/stderr. and reopen - * /dev/tty for them. This is important to - * ensure that when systemctl is started via - * popen() or a similar call that expects to - * read EOF we actually do generate EOF and - * not delay this indefinitely by because we - * keep an unused copy of stdin around. */ - fd = open("/dev/tty", O_WRONLY); - if (fd < 0) { - log_error_errno(errno, "Failed to open /dev/tty: %m"); - _exit(EXIT_FAILURE); - } - - if (!stdout_is_tty && dup2(fd, STDOUT_FILENO) < 0) { - log_error_errno(errno, "Failed to dup2 /dev/tty: %m"); - _exit(EXIT_FAILURE); - } - - if (!stderr_is_tty && dup2(fd, STDERR_FILENO) < 0) { - log_error_errno(errno, "Failed to dup2 /dev/tty: %m"); - _exit(EXIT_FAILURE); - } - - safe_close_above_stdio(fd); - } - - (void) rlimit_nofile_safe(); - - /* Count arguments */ - va_start(ap, path); - for (n = 0; va_arg(ap, char*); n++) - ; - va_end(ap); - - /* Allocate strv */ - l = newa(char*, n + 1); - - /* Fill in arguments */ - va_start(ap, path); - for (i = 0; i <= n; i++) - l[i] = va_arg(ap, char*); - va_end(ap); - - execv(path, l); - _exit(EXIT_FAILURE); -} - int set_oom_score_adjust(int value) { char t[DECIMAL_STR_MAX(int)]; diff --git a/src/basic/process-util.h b/src/basic/process-util.h index 58a3ebd577..b130c5ae02 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -174,8 +174,6 @@ static inline int safe_fork(const char *name, ForkFlags flags, pid_t *ret_pid) { int namespace_fork(const char *outer_name, const char *inner_name, int except_fds[], size_t n_except_fds, ForkFlags flags, int pidns_fd, int mntns_fd, int netns_fd, int userns_fd, int root_fd, pid_t *ret_pid); -int fork_agent(const char *name, int except[], size_t n_except, pid_t *pid, const char *path, ...) _sentinel_; - int set_oom_score_adjust(int value); /* The highest possibly (theoretic) pid_t value on this architecture. */ diff --git a/src/shared/exec-util.c b/src/shared/exec-util.c index 031ca4cb4c..68e896a8fa 100644 --- a/src/shared/exec-util.c +++ b/src/shared/exec-util.c @@ -470,3 +470,76 @@ int fexecve_or_execve(int executable_fd, const char *executable, char *const arg execve(executable, argv, envp); return -errno; } + +int fork_agent(const char *name, int except[], size_t n_except, pid_t *ret_pid, const char *path, ...) { + bool stdout_is_tty, stderr_is_tty; + size_t n, i; + va_list ap; + char **l; + int r; + + assert(path); + + /* Spawns a temporary TTY agent, making sure it goes away when we go away */ + + r = safe_fork_full(name, + except, + n_except, + FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_CLOSE_ALL_FDS|FORK_REOPEN_LOG, + ret_pid); + if (r < 0) + return r; + if (r > 0) + return 0; + + /* In the child: */ + + stdout_is_tty = isatty(STDOUT_FILENO); + stderr_is_tty = isatty(STDERR_FILENO); + + if (!stdout_is_tty || !stderr_is_tty) { + int fd; + + /* Detach from stdout/stderr and reopen /dev/tty for them. This is important to ensure that + * when systemctl is started via popen() or a similar call that expects to read EOF we + * actually do generate EOF and not delay this indefinitely by keeping an unused copy of + * stdin around. */ + fd = open("/dev/tty", O_WRONLY); + if (fd < 0) { + log_error_errno(errno, "Failed to open /dev/tty: %m"); + _exit(EXIT_FAILURE); + } + + if (!stdout_is_tty && dup2(fd, STDOUT_FILENO) < 0) { + log_error_errno(errno, "Failed to dup2 /dev/tty: %m"); + _exit(EXIT_FAILURE); + } + + if (!stderr_is_tty && dup2(fd, STDERR_FILENO) < 0) { + log_error_errno(errno, "Failed to dup2 /dev/tty: %m"); + _exit(EXIT_FAILURE); + } + + safe_close_above_stdio(fd); + } + + (void) rlimit_nofile_safe(); + + /* Count arguments */ + va_start(ap, path); + for (n = 0; va_arg(ap, char*); n++) + ; + va_end(ap); + + /* Allocate strv */ + l = newa(char*, n + 1); + + /* Fill in arguments */ + va_start(ap, path); + for (i = 0; i <= n; i++) + l[i] = va_arg(ap, char*); + va_end(ap); + + execv(path, l); + _exit(EXIT_FAILURE); +} diff --git a/src/shared/exec-util.h b/src/shared/exec-util.h index 9ce5324de9..21d28608f9 100644 --- a/src/shared/exec-util.h +++ b/src/shared/exec-util.h @@ -48,3 +48,5 @@ const char* exec_command_flags_to_string(ExecCommandFlags i); ExecCommandFlags exec_command_flags_from_string(const char *s); int fexecve_or_execve(int executable_fd, const char *executable, char *const argv[], char *const envp[]); + +int fork_agent(const char *name, int except[], size_t n_except, pid_t *ret_pid, const char *path, ...) _sentinel_; diff --git a/src/shared/spawn-ask-password-agent.c b/src/shared/spawn-ask-password-agent.c index 1f07b198fa..38fab21203 100644 --- a/src/shared/spawn-ask-password-agent.c +++ b/src/shared/spawn-ask-password-agent.c @@ -4,6 +4,7 @@ #include #include +#include "exec-util.h" #include "log.h" #include "process-util.h" #include "spawn-ask-password-agent.h" diff --git a/src/shared/spawn-polkit-agent.c b/src/shared/spawn-polkit-agent.c index a0024eb2ea..cd0b4601da 100644 --- a/src/shared/spawn-polkit-agent.c +++ b/src/shared/spawn-polkit-agent.c @@ -6,6 +6,7 @@ #include #include +#include "exec-util.h" #include "fd-util.h" #include "io-util.h" #include "log.h" From 45a68ed3073a006c44703acf2928be156805ef9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Jul 2021 11:34:00 +0200 Subject: [PATCH 5/6] Move freeze() into shared/ Library code should not call freeze(), this is something that should only be done by "application code", so moving it into shared/ is appropriate. The fallback to call _exit() is dropped: let's trust that the infinite loop is infinite. --- src/basic/process-util.c | 24 ------------------------ src/basic/process-util.h | 2 -- src/libsystemd/sd-event/test-event.c | 1 + src/shared/exec-util.c | 23 +++++++++++++++++++++++ src/shared/exec-util.h | 2 ++ src/shared/mount-util.c | 5 ++--- 6 files changed, 28 insertions(+), 29 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index e325820584..ce4bfb783d 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -1037,30 +1037,6 @@ bool is_main_thread(void) { return cached > 0; } -_noreturn_ void freeze(void) { - - log_close(); - - /* Make sure nobody waits for us on a socket anymore */ - (void) close_all_fds(NULL, 0); - - sync(); - - /* Let's not freeze right away, but keep reaping zombies. */ - for (;;) { - int r; - siginfo_t si = {}; - - r = waitid(P_ALL, 0, &si, WEXITED); - if (r < 0 && errno != EINTR) - break; - } - - /* waitid() failed with an unexpected error, things are really borked. Freeze now! */ - for (;;) - pause(); -} - unsigned long personality_from_string(const char *p) { int architecture; diff --git a/src/basic/process-util.h b/src/basic/process-util.h index b130c5ae02..a591fc32a4 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -82,8 +82,6 @@ int pid_from_same_root_fs(pid_t pid); bool is_main_thread(void); -_noreturn_ void freeze(void); - #ifndef PERSONALITY_INVALID /* personality(7) documents that 0xffffffffUL is used for querying the * current personality, hence let's use that here as error diff --git a/src/libsystemd/sd-event/test-event.c b/src/libsystemd/sd-event/test-event.c index 7ff1452e08..9b92dac650 100644 --- a/src/libsystemd/sd-event/test-event.c +++ b/src/libsystemd/sd-event/test-event.c @@ -5,6 +5,7 @@ #include "sd-event.h" #include "alloc-util.h" +#include "exec-util.h" #include "fd-util.h" #include "fs-util.h" #include "log.h" diff --git a/src/shared/exec-util.c b/src/shared/exec-util.c index 68e896a8fa..42f6c4d75a 100644 --- a/src/shared/exec-util.c +++ b/src/shared/exec-util.c @@ -448,6 +448,29 @@ ExecCommandFlags exec_command_flags_from_string(const char *s) { return 1 << idx; } +_noreturn_ void freeze(void) { + log_close(); + + /* Make sure nobody waits for us on a socket anymore */ + (void) close_all_fds(NULL, 0); + + sync(); + + /* Let's not freeze right away, but keep reaping zombies. */ + for (;;) { + int r; + siginfo_t si = {}; + + r = waitid(P_ALL, 0, &si, WEXITED); + if (r < 0 && errno != EINTR) + break; + } + + /* waitid() failed with an unexpected error, things are really borked. Freeze now! */ + for (;;) + pause(); +} + int fexecve_or_execve(int executable_fd, const char *executable, char *const argv[], char *const envp[]) { #if ENABLE_FEXECVE execveat(executable_fd, "", argv, envp, AT_EMPTY_PATH); diff --git a/src/shared/exec-util.h b/src/shared/exec-util.h index 21d28608f9..05f8e1af83 100644 --- a/src/shared/exec-util.h +++ b/src/shared/exec-util.h @@ -47,6 +47,8 @@ extern const gather_stdout_callback_t gather_environment[_STDOUT_CONSUME_MAX]; const char* exec_command_flags_to_string(ExecCommandFlags i); ExecCommandFlags exec_command_flags_from_string(const char *s); +_noreturn_ void freeze(void); + int fexecve_or_execve(int executable_fd, const char *executable, char *const argv[], char *const envp[]); int fork_agent(const char *name, int except[], size_t n_except, pid_t *ret_pid, const char *path, ...) _sentinel_; diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 594efea989..cf8ca8d9d3 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -11,6 +11,7 @@ #include "alloc-util.h" #include "dissect-image.h" +#include "exec-util.h" #include "extract-word.h" #include "fd-util.h" #include "fileio.h" @@ -1010,11 +1011,9 @@ static int make_userns(uid_t uid_shift, uid_t uid_range) { r = safe_fork("(sd-mkuserns)", FORK_CLOSE_ALL_FDS|FORK_DEATHSIG|FORK_NEW_USERNS, &pid); if (r < 0) return r; - if (r == 0) { + if (r == 0) /* Child. We do nothing here, just freeze until somebody kills us. */ freeze(); - _exit(EXIT_FAILURE); - } xsprintf(line, UID_FMT " " UID_FMT " " UID_FMT "\n", 0, uid_shift, uid_range); From cbcf371abc328167fa869721c1add4850c793240 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Jul 2021 11:36:44 +0200 Subject: [PATCH 6/6] Add variant of close_all_fds() that does not allocate and use it in freeze() Even though it's just a fallback path, let's not be sloppy and allocate in the crash handler. > The deadlock happens because systemd crash in malloc() then in signal > handler, it calls malloc() (close_all_fds()-> opendir()-> __alloc_dir()) > again. malloc() is not a signal-safe function, maybe we should re-think > the logic here. Fixes #20266. --- src/basic/fd-util.c | 8 ++++---- src/basic/fd-util.h | 5 ++++- src/shared/exec-util.c | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index 99b517d8c2..6b6457dbc2 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -208,7 +208,7 @@ static int get_max_fd(void) { return (int) (m - 1); } -int close_all_fds(int except[], size_t n_except) { +int close_all_fds_full(int except[], size_t n_except, bool allow_alloc) { static bool have_close_range = true; /* Assume we live in the future */ _cleanup_closedir_ DIR *d = NULL; int r = 0; @@ -274,7 +274,7 @@ int close_all_fds(int except[], size_t n_except) { /* Fallback for when close_range() is not supported */ opendir_fallback: - d = opendir("/proc/self/fd"); + d = allow_alloc ? opendir("/proc/self/fd") : NULL; if (d) { struct dirent *de; @@ -302,8 +302,8 @@ int close_all_fds(int except[], size_t n_except) { return r; } - /* Fallback for when /proc isn't available (for example in chroots) by brute-forcing through the file - * descriptor table. */ + /* Fallback for when /proc isn't available (for example in chroots) or when we cannot allocate by + * brute-forcing through the file descriptor table. */ int max_fd = get_max_fd(); if (max_fd < 0) diff --git a/src/basic/fd-util.h b/src/basic/fd-util.h index d3c2944d86..61b6684cb3 100644 --- a/src/basic/fd-util.h +++ b/src/basic/fd-util.h @@ -56,7 +56,10 @@ DEFINE_TRIVIAL_CLEANUP_FUNC_FULL(DIR*, closedir, NULL); int fd_nonblock(int fd, bool nonblock); int fd_cloexec(int fd, bool cloexec); -int close_all_fds(int except[], size_t n_except); +int close_all_fds_full(int except[], size_t n_except, bool allow_alloc); +static inline int close_all_fds(int except[], size_t n_except) { + return close_all_fds_full(except, n_except, true); +} int same_fd(int a, int b); diff --git a/src/shared/exec-util.c b/src/shared/exec-util.c index 42f6c4d75a..cffa3fe96e 100644 --- a/src/shared/exec-util.c +++ b/src/shared/exec-util.c @@ -452,7 +452,7 @@ _noreturn_ void freeze(void) { log_close(); /* Make sure nobody waits for us on a socket anymore */ - (void) close_all_fds(NULL, 0); + (void) close_all_fds_full(NULL, 0, false); sync();