From 1300f91149bf8477ec1b55ea0bd2ac6010f7b807 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Nov 2018 15:52:33 +0100 Subject: [PATCH 01/12] rlimit: add new rlimit_nofile_safe() helper This helper sets RLIMIT_NOFILE's soft limit to 1024 (FD_SETSIZE) for compatibility with apps using select(). The idea is that we use this helper to reset the limit whenever we invoke foreign code from our own processes which have bumped RLIMIT_NOFILE high. --- src/shared/rlimit-util.c | 19 +++++++++++++++++++ src/shared/rlimit-util.h | 1 + 2 files changed, 20 insertions(+) diff --git a/src/shared/rlimit-util.c b/src/shared/rlimit-util.c index c133f84b7e..74b3a023f1 100644 --- a/src/shared/rlimit-util.c +++ b/src/shared/rlimit-util.c @@ -389,3 +389,22 @@ int rlimit_nofile_bump(int limit) { return 0; } + +int rlimit_nofile_safe(void) { + struct rlimit rl; + + /* Resets RLIMIT_NOFILE's soft limit FD_SETSIZE (i.e. 1024), for compatibility with software still using + * select() */ + + if (getrlimit(RLIMIT_NOFILE, &rl) < 0) + return log_debug_errno(errno, "Failed to query RLIMIT_NOFILE: %m"); + + if (rl.rlim_cur <= FD_SETSIZE) + return 0; + + rl.rlim_cur = FD_SETSIZE; + if (setrlimit(RLIMIT_NOFILE, &rl) < 0) + return log_debug_errno(errno, "Failed to lower RLIMIT_NOFILE's soft limit to " RLIM_FMT ": %m", rl.rlim_cur); + + return 1; +} diff --git a/src/shared/rlimit-util.h b/src/shared/rlimit-util.h index 6139af3ff5..d4fca2b855 100644 --- a/src/shared/rlimit-util.h +++ b/src/shared/rlimit-util.h @@ -22,3 +22,4 @@ void rlimit_free_all(struct rlimit **rl); #define RLIMIT_MAKE_CONST(lim) ((struct rlimit) { lim, lim }) int rlimit_nofile_bump(int limit); +int rlimit_nofile_safe(void); From 3c069cdac439c21b8df12caf0dd7bd6e6a502141 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Nov 2018 15:55:26 +0100 Subject: [PATCH 02/12] =?UTF-8?q?move=20src/shared/rlimit-util.[ch]=20?= =?UTF-8?q?=E2=86=92=20src/basic/?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is really basic stuff and in a follow-up commit will use it all across the codebase, including in process-util.[ch] which is in src/basic/. Hence let's move it back to src/basic/ itself. --- src/basic/meson.build | 2 ++ src/{shared => basic}/rlimit-util.c | 0 src/{shared => basic}/rlimit-util.h | 0 src/shared/meson.build | 2 -- 4 files changed, 2 insertions(+), 2 deletions(-) rename src/{shared => basic}/rlimit-util.c (100%) rename src/{shared => basic}/rlimit-util.h (100%) diff --git a/src/basic/meson.build b/src/basic/meson.build index 124a3e9f84..4cd6911da9 100644 --- a/src/basic/meson.build +++ b/src/basic/meson.build @@ -120,6 +120,8 @@ basic_sources = files(''' refcnt.h replace-var.c replace-var.h + rlimit-util.c + rlimit-util.h rm-rf.c rm-rf.h securebits.h diff --git a/src/shared/rlimit-util.c b/src/basic/rlimit-util.c similarity index 100% rename from src/shared/rlimit-util.c rename to src/basic/rlimit-util.c diff --git a/src/shared/rlimit-util.h b/src/basic/rlimit-util.h similarity index 100% rename from src/shared/rlimit-util.h rename to src/basic/rlimit-util.h diff --git a/src/shared/meson.build b/src/shared/meson.build index 85c546eb5b..b03ae287b2 100644 --- a/src/shared/meson.build +++ b/src/shared/meson.build @@ -118,8 +118,6 @@ shared_sources = files(''' reboot-util.h resolve-util.c resolve-util.h - rlimit-util.c - rlimit-util.h seccomp-util.h securebits-util.c securebits-util.h From 909106ebdf9a128627cd5974d4d388c71d694464 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Nov 2018 15:59:17 +0100 Subject: [PATCH 03/12] process-util: add new FORK_RLIMIT_NOFILE_SAFE flag for safe_fork() The new flag simply means rlimit_nofile_safe() is called in the child after all fds are rearranged. --- src/basic/process-util.c | 9 +++++++++ src/basic/process-util.h | 19 ++++++++++--------- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index d1a34338f6..5cf4e37f24 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -35,6 +35,7 @@ #include "missing.h" #include "process-util.h" #include "raw-clone.h" +#include "rlimit-util.h" #include "signal-util.h" #include "stat-util.h" #include "string-table.h" @@ -1401,6 +1402,14 @@ int safe_fork_full( } } + if (flags & FORK_RLIMIT_NOFILE_SAFE) { + r = rlimit_nofile_safe(); + if (r < 0) { + log_full_errno(prio, r, "Failed to lower RLIMIT_NOFILE's soft limit to 1K: %m"); + _exit(EXIT_FAILURE); + } + } + if (ret_pid) *ret_pid = getpid_cached(); diff --git a/src/basic/process-util.h b/src/basic/process-util.h index af47513fab..496e14d3de 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -142,15 +142,16 @@ void reset_cached_pid(void); int must_be_root(void); typedef enum ForkFlags { - FORK_RESET_SIGNALS = 1 << 0, - FORK_CLOSE_ALL_FDS = 1 << 1, - FORK_DEATHSIG = 1 << 2, - FORK_NULL_STDIO = 1 << 3, - FORK_REOPEN_LOG = 1 << 4, - FORK_LOG = 1 << 5, - FORK_WAIT = 1 << 6, - FORK_NEW_MOUNTNS = 1 << 7, - FORK_MOUNTNS_SLAVE = 1 << 8, + FORK_RESET_SIGNALS = 1 << 0, /* Reset all signal handlers and signal mask */ + FORK_CLOSE_ALL_FDS = 1 << 1, /* Close all open file descriptors in the child, except for 0,1,2 */ + FORK_DEATHSIG = 1 << 2, /* Set PR_DEATHSIG in the child */ + FORK_NULL_STDIO = 1 << 3, /* Connect 0,1,2 to /dev/null */ + FORK_REOPEN_LOG = 1 << 4, /* Reopen log connection */ + FORK_LOG = 1 << 5, /* Log above LOG_DEBUG log level about failures */ + FORK_WAIT = 1 << 6, /* Wait until child exited */ + FORK_NEW_MOUNTNS = 1 << 7, /* Run child in its own mount namespace */ + FORK_MOUNTNS_SLAVE = 1 << 8, /* Make child's mount namespace MS_SLAVE */ + FORK_RLIMIT_NOFILE_SAFE = 1 << 9, /* Set RLIMIT_NOFILE soft limit to 1K for select() compat */ } ForkFlags; int safe_fork_full(const char *name, const int except_fds[], size_t n_except_fds, ForkFlags flags, pid_t *ret_pid); From 595225af7a4f663788d26b8720e994fed71f9410 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Nov 2018 16:06:26 +0100 Subject: [PATCH 04/12] tree-wide: invoke rlimit_nofile_safe() before various exec{v,ve,l}() invocations Whenever we invoke external, foreign code from code that has RLIMIT_NOFILE's soft limit bumped to high values, revert it to 1024 first. This is a safety precaution for compatibility with programs using select() which cannot operate with fds > 1024. This commit adds the call to rlimit_nofile_safe() to all invocations of exec{v,ve,l}() and friends that either are in code that we know runs with RLIMIT_NOFILE bumped up (which is PID 1 and all journal code for starters) or that is part of shared code that might end up there. The calls are placed as early as we can in processes invoking a flavour of execve(), but after the last time we do fd manipulations, so that we can still take benefit of the high fd limits for that. --- src/basic/process-util.c | 2 ++ src/core/main.c | 2 ++ src/core/shutdown.c | 3 +++ src/fsck/fsck.c | 3 +++ src/import/pull-common.c | 3 +++ src/journal-remote/journal-remote-main.c | 2 ++ src/libsystemd/sd-bus/bus-socket.c | 3 +++ src/nspawn/nspawn-setuid.c | 3 +++ src/shared/exec-util.c | 3 +++ src/shared/pager.c | 1 + src/systemctl/systemctl.c | 1 + src/udev/udev-event.c | 2 ++ 12 files changed, 28 insertions(+) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 5cf4e37f24..e69566c8a4 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -1521,6 +1521,8 @@ int fork_agent(const char *name, const int except[], size_t n_except, pid_t *ret safe_close_above_stdio(fd); } + (void) rlimit_nofile_safe(); + /* Count arguments */ va_start(ap, path); for (n = 0; va_arg(ap, char*); n++) diff --git a/src/core/main.c b/src/core/main.c index 6d03b06684..839dc062ff 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -236,6 +236,7 @@ _noreturn_ static void crash(int sig) { else if (pid == 0) { (void) setsid(); (void) make_console_stdio(); + (void) rlimit_nofile_safe(); (void) execle("/bin/sh", "/bin/sh", NULL, environ); log_emergency_errno(errno, "execle() failed: %m"); @@ -1733,6 +1734,7 @@ static void do_reexecute( /* Reenable any blocked signals, especially important if we switch from initial ramdisk to init=... */ (void) reset_all_signal_handlers(); (void) reset_signal_mask(); + (void) rlimit_nofile_safe(); if (switch_root_init) { args[0] = switch_root_init; diff --git a/src/core/shutdown.c b/src/core/shutdown.c index eae7295acb..368a92dd38 100644 --- a/src/core/shutdown.c +++ b/src/core/shutdown.c @@ -28,6 +28,7 @@ #include "parse-util.h" #include "process-util.h" #include "reboot-util.h" +#include "rlimit-util.h" #include "signal-util.h" #include "string-util.h" #include "switch-root.h" @@ -443,6 +444,8 @@ int main(int argc, char *argv[]) { arguments[2] = NULL; execute_directories(dirs, DEFAULT_TIMEOUT_USEC, NULL, NULL, arguments, NULL); + (void) rlimit_nofile_safe(); + if (can_initrd) { r = switch_root_initramfs(); if (r >= 0) { diff --git a/src/fsck/fsck.c b/src/fsck/fsck.c index 995cf92ef1..7fc4a283ce 100644 --- a/src/fsck/fsck.c +++ b/src/fsck/fsck.c @@ -27,6 +27,7 @@ #include "path-util.h" #include "proc-cmdline.h" #include "process-util.h" +#include "rlimit-util.h" #include "signal-util.h" #include "socket-util.h" #include "special.h" @@ -401,6 +402,8 @@ static int run(int argc, char *argv[]) { cmdline[i++] = device; cmdline[i++] = NULL; + (void) rlimit_nofile_safe(); + execv(cmdline[0], (char**) cmdline); _exit(FSCK_OPERATIONAL_ERROR); } diff --git a/src/import/pull-common.c b/src/import/pull-common.c index a90693c802..acfe380969 100644 --- a/src/import/pull-common.c +++ b/src/import/pull-common.c @@ -14,6 +14,7 @@ #include "process-util.h" #include "pull-common.h" #include "pull-job.h" +#include "rlimit-util.h" #include "rm-rf.h" #include "signal-util.h" #include "siphash24.h" @@ -472,6 +473,8 @@ int pull_verify(PullJob *main_job, _exit(EXIT_FAILURE); } + (void) rlimit_nofile_safe(); + cmd[k++] = strjoina("--homedir=", gpg_home); /* We add the user keyring only to the command line diff --git a/src/journal-remote/journal-remote-main.c b/src/journal-remote/journal-remote-main.c index c46e0acdd3..b82d4b4a1b 100644 --- a/src/journal-remote/journal-remote-main.c +++ b/src/journal-remote/journal-remote-main.c @@ -81,6 +81,8 @@ static int spawn_child(const char* child, char** argv) { _exit(EXIT_FAILURE); } + (void) rlimit_nofile_safe(); + execvp(child, argv); log_error_errno(errno, "Failed to exec child %s: %m", child); _exit(EXIT_FAILURE); diff --git a/src/libsystemd/sd-bus/bus-socket.c b/src/libsystemd/sd-bus/bus-socket.c index f7485211ac..ed185131b8 100644 --- a/src/libsystemd/sd-bus/bus-socket.c +++ b/src/libsystemd/sd-bus/bus-socket.c @@ -21,6 +21,7 @@ #include "missing.h" #include "path-util.h" #include "process-util.h" +#include "rlimit-util.h" #include "selinux-util.h" #include "signal-util.h" #include "stdio-util.h" @@ -932,6 +933,8 @@ int bus_socket_exec(sd_bus *b) { if (rearrange_stdio(s[1], s[1], STDERR_FILENO) < 0) _exit(EXIT_FAILURE); + (void) rlimit_nofile_safe(); + if (b->exec_argv) execvp(b->exec_path, b->exec_argv); else { diff --git a/src/nspawn/nspawn-setuid.c b/src/nspawn/nspawn-setuid.c index e865d5b2a8..86fd9deec0 100644 --- a/src/nspawn/nspawn-setuid.c +++ b/src/nspawn/nspawn-setuid.c @@ -12,6 +12,7 @@ #include "mkdir.h" #include "nspawn-setuid.h" #include "process-util.h" +#include "rlimit-util.h" #include "signal-util.h" #include "string-util.h" #include "strv.h" @@ -44,6 +45,8 @@ static int spawn_getent(const char *database, const char *key, pid_t *rpid) { close_all_fds(NULL, 0); + (void) rlimit_nofile_safe(); + execle("/usr/bin/getent", "getent", database, key, NULL, &empty_env); execle("/bin/getent", "getent", database, key, NULL, &empty_env); _exit(EXIT_FAILURE); diff --git a/src/shared/exec-util.c b/src/shared/exec-util.c index 10d774dfcd..2429915f40 100644 --- a/src/shared/exec-util.c +++ b/src/shared/exec-util.c @@ -16,6 +16,7 @@ #include "hashmap.h" #include "macro.h" #include "process-util.h" +#include "rlimit-util.h" #include "serialize.h" #include "set.h" #include "signal-util.h" @@ -50,6 +51,8 @@ static int do_spawn(const char *path, char *argv[], int stdout_fd, pid_t *pid) { _exit(EXIT_FAILURE); } + (void) rlimit_nofile_safe(); + if (!argv) { _argv[0] = (char*) path; _argv[1] = NULL; diff --git a/src/shared/pager.c b/src/shared/pager.c index 88d9ef349e..86a394e4f8 100644 --- a/src/shared/pager.c +++ b/src/shared/pager.c @@ -19,6 +19,7 @@ #include "macro.h" #include "pager.h" #include "process-util.h" +#include "rlimit-util.h" #include "signal-util.h" #include "string-util.h" #include "strv.h" diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 87ae4eb5d7..f3e1dff499 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -8302,6 +8302,7 @@ static int parse_argv(int argc, char *argv[]) { /* Hmm, so some other init system is running, we need to forward this request to * it. For now we simply guess that it is Upstart. */ + (void) rlimit_nofile_safe(); execv(TELINIT, argv); return log_error_errno(SYNTHETIC_ERRNO(EIO), diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c index 840d20beac..7bfb692a5a 100644 --- a/src/udev/udev-event.c +++ b/src/udev/udev-event.c @@ -20,6 +20,7 @@ #include "netlink-util.h" #include "path-util.h" #include "process-util.h" +#include "rlimit-util.h" #include "signal-util.h" #include "stdio-util.h" #include "string-util.h" @@ -654,6 +655,7 @@ int udev_event_spawn(struct udev_event *event, _exit(EXIT_FAILURE); (void) close_all_fds(NULL, 0); + (void) rlimit_nofile_safe(); execve(argv[0], argv, envp); _exit(EXIT_FAILURE); From 0672e2c6f84e0999ead8ea662360eb6bb8effe4c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Nov 2018 16:11:45 +0100 Subject: [PATCH 05/12] tree-wide: use FORK_RLIMIT_NOFILE_SAFE wherever possible Similar to the previous commit: in many cases no further fd processing needs to be done in forked of children before execve() or any of its flavours are called. In those case we can use FORK_RLIMIT_NOFILE_SAFE instead. --- src/activate/activate.c | 2 +- src/coredump/coredumpctl.c | 2 +- src/delta/delta.c | 2 +- src/login/inhibit.c | 2 +- src/partition/makefs.c | 2 +- src/quotacheck/quotacheck.c | 2 +- src/remount-fs/remount-fs.c | 2 +- src/shared/pager.c | 4 ++-- src/sulogin-shell/sulogin-shell.c | 2 +- src/systemctl/systemctl.c | 6 +++--- src/vconsole/vconsole-setup.c | 4 ++-- 11 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/activate/activate.c b/src/activate/activate.c index 912772d590..9a83bc7f24 100644 --- a/src/activate/activate.c +++ b/src/activate/activate.c @@ -249,7 +249,7 @@ static int fork_and_exec_process(const char* child, char** argv, char **env, int if (!joined) return log_oom(); - r = safe_fork("(activate)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_LOG, &child_pid); + r = safe_fork("(activate)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &child_pid); if (r < 0) return r; if (r == 0) { diff --git a/src/coredump/coredumpctl.c b/src/coredump/coredumpctl.c index de26bee931..8be7399b0c 100644 --- a/src/coredump/coredumpctl.c +++ b/src/coredump/coredumpctl.c @@ -968,7 +968,7 @@ static int run_debug(int argc, char **argv, void *userdata) { fork_name = strjoina("(", arg_debugger, ")"); - r = safe_fork(fork_name, FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_CLOSE_ALL_FDS|FORK_LOG, &pid); + r = safe_fork(fork_name, FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_CLOSE_ALL_FDS|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pid); if (r < 0) goto finish; if (r == 0) { diff --git a/src/delta/delta.c b/src/delta/delta.c index 328f5654e8..379226641e 100644 --- a/src/delta/delta.c +++ b/src/delta/delta.c @@ -169,7 +169,7 @@ static int found_override(const char *top, const char *bottom) { fflush(stdout); - r = safe_fork("(diff)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_CLOSE_ALL_FDS|FORK_LOG, &pid); + r = safe_fork("(diff)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_CLOSE_ALL_FDS|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pid); if (r < 0) return r; if (r == 0) { diff --git a/src/login/inhibit.c b/src/login/inhibit.c index 2394c5d937..383afdb9b0 100644 --- a/src/login/inhibit.c +++ b/src/login/inhibit.c @@ -303,7 +303,7 @@ static int run(int argc, char *argv[]) { if (fd < 0) return log_error_errno(fd, "Failed to inhibit: %s", bus_error_message(&error, fd)); - r = safe_fork("(inhibit)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_CLOSE_ALL_FDS|FORK_LOG, &pid); + r = safe_fork("(inhibit)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_CLOSE_ALL_FDS|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pid); if (r < 0) return r; if (r == 0) { diff --git a/src/partition/makefs.c b/src/partition/makefs.c index 88834092bd..ab19577dca 100644 --- a/src/partition/makefs.c +++ b/src/partition/makefs.c @@ -28,7 +28,7 @@ static int makefs(const char *type, const char *device) { if (access(mkfs, X_OK) != 0) return log_error_errno(errno, "%s is not executable: %m", mkfs); - r = safe_fork("(fsck)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_LOG, &pid); + r = safe_fork("(fsck)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pid); if (r < 0) return r; if (r == 0) { diff --git a/src/quotacheck/quotacheck.c b/src/quotacheck/quotacheck.c index a51a76411e..90f542a058 100644 --- a/src/quotacheck/quotacheck.c +++ b/src/quotacheck/quotacheck.c @@ -78,7 +78,7 @@ static int run(int argc, char *argv[]) { return 0; } - r = safe_fork("(quotacheck)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_LOG|FORK_WAIT, NULL); + r = safe_fork("(quotacheck)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_RLIMIT_NOFILE_SAFE|FORK_WAIT|FORK_LOG, NULL); if (r < 0) return r; if (r == 0) { diff --git a/src/remount-fs/remount-fs.c b/src/remount-fs/remount-fs.c index 28edbbd856..af92ddb96c 100644 --- a/src/remount-fs/remount-fs.c +++ b/src/remount-fs/remount-fs.c @@ -62,7 +62,7 @@ static int run(int argc, char *argv[]) { log_debug("Remounting %s", me->mnt_dir); - r = safe_fork("(remount)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_LOG, &pid); + r = safe_fork("(remount)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pid); if (r < 0) return r; if (r == 0) { diff --git a/src/shared/pager.c b/src/shared/pager.c index 86a394e4f8..d907a60119 100644 --- a/src/shared/pager.c +++ b/src/shared/pager.c @@ -132,7 +132,7 @@ int pager_open(PagerFlags flags) { if (flags & PAGER_JUMP_TO_END) less_opts = strjoina(less_opts, " +G"); - r = safe_fork("(pager)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_LOG, &pager_pid); + r = safe_fork("(pager)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pager_pid); if (r < 0) return r; if (r == 0) { @@ -257,7 +257,7 @@ int show_man_page(const char *desc, bool null_stdio) { } else args[1] = desc; - r = safe_fork("(man)", FORK_RESET_SIGNALS|FORK_DEATHSIG|(null_stdio ? FORK_NULL_STDIO : 0)|FORK_LOG, &pid); + r = safe_fork("(man)", FORK_RESET_SIGNALS|FORK_DEATHSIG|(null_stdio ? FORK_NULL_STDIO : 0)|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pid); if (r < 0) return r; if (r == 0) { diff --git a/src/sulogin-shell/sulogin-shell.c b/src/sulogin-shell/sulogin-shell.c index 82481972f0..6d65efbb9e 100644 --- a/src/sulogin-shell/sulogin-shell.c +++ b/src/sulogin-shell/sulogin-shell.c @@ -69,7 +69,7 @@ static int fork_wait(const char* const cmdline[]) { pid_t pid; int r; - r = safe_fork("(sulogin)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_LOG, &pid); + r = safe_fork("(sulogin)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pid); if (r < 0) return r; if (r == 0) { diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index f3e1dff499..436c6ed8f2 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -3532,7 +3532,7 @@ static int load_kexec_kernel(void) { if (arg_dry_run) return 0; - r = safe_fork("(kexec)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_LOG, &pid); + r = safe_fork("(kexec)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pid); if (r < 0) return r; if (r == 0) { @@ -6005,7 +6005,7 @@ static int enable_sysv_units(const char *verb, char **args) { if (!arg_quiet) log_info("Executing: %s", l); - j = safe_fork("(sysv-install)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_LOG, &pid); + j = safe_fork("(sysv-install)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pid); if (j < 0) return j; if (j == 0) { @@ -6900,7 +6900,7 @@ static int run_editor(char **paths) { assert(paths); - r = safe_fork("(editor)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_LOG|FORK_WAIT, NULL); + r = safe_fork("(editor)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG|FORK_WAIT, NULL); if (r < 0) return r; if (r == 0) { diff --git a/src/vconsole/vconsole-setup.c b/src/vconsole/vconsole-setup.c index 7182be4624..93993e7d80 100644 --- a/src/vconsole/vconsole-setup.c +++ b/src/vconsole/vconsole-setup.c @@ -149,7 +149,7 @@ static int keyboard_load_and_wait(const char *vc, const char *map, const char *m log_debug("Executing \"%s\"...", strnull(cmd)); } - r = safe_fork("(loadkeys)", FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_LOG, &pid); + r = safe_fork("(loadkeys)", FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pid); if (r < 0) return r; if (r == 0) { @@ -192,7 +192,7 @@ static int font_load_and_wait(const char *vc, const char *font, const char *map, log_debug("Executing \"%s\"...", strnull(cmd)); } - r = safe_fork("(setfont)", FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_LOG, &pid); + r = safe_fork("(setfont)", FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pid); if (r < 0) return r; if (r == 0) { From ece0fe12ad172abfc67af662d56384b075cd42da Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Nov 2018 16:17:24 +0100 Subject: [PATCH 06/12] tree-wide: (void)ify some setsid() and related calls --- src/core/shutdown.c | 4 ++-- src/udev/udevd.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/shutdown.c b/src/core/shutdown.c index 368a92dd38..cb47ee8984 100644 --- a/src/core/shutdown.c +++ b/src/core/shutdown.c @@ -451,8 +451,8 @@ int main(int argc, char *argv[]) { if (r >= 0) { argv[0] = (char*) "/shutdown"; - setsid(); - make_console_stdio(); + (void) setsid(); + (void) make_console_stdio(); log_info("Successfully changed into root pivot.\n" "Returning to initrd..."); diff --git a/src/udev/udevd.c b/src/udev/udevd.c index c9e3d426b3..f5d33541f6 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -1850,7 +1850,7 @@ static int run(int argc, char *argv[]) { return 0; /* child */ - setsid(); + (void) setsid(); r = set_oom_score_adjust(-1000); if (r < 0) From 1d78890851531f54aff1f6cebb284009dff0f41a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 14:27:01 +0100 Subject: [PATCH 07/12] pager: log about all error conditions The code so far logged about some errors but was silent on others. Let's stream-line that and make the function fully self-logging on all error conditions. --- src/shared/pager.c | 41 +++++++++++++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/src/shared/pager.c b/src/shared/pager.c index d907a60119..ce4ca9bdb2 100644 --- a/src/shared/pager.c +++ b/src/shared/pager.c @@ -56,7 +56,7 @@ static int no_quit_on_interrupt(int exe_name_fd, const char *less_opts) { file = fdopen(exe_name_fd, "r"); if (!file) { safe_close(exe_name_fd); - return log_debug_errno(errno, "Failed to create FILE object: %m"); + return log_error_errno(errno, "Failed to create FILE object: %m"); } /* Find the last line */ @@ -65,7 +65,7 @@ static int no_quit_on_interrupt(int exe_name_fd, const char *less_opts) { r = read_line(file, LONG_LINE_MAX, &t); if (r < 0) - return r; + return log_error_errno(r, "Failed to read from socket: %m"); if (r == 0) break; @@ -97,7 +97,7 @@ int pager_open(PagerFlags flags) { return 0; if (!is_main_thread()) - return -EPERM; + return log_error_errno(SYNTHETIC_ERRNO(EPERM), "Pager invoked from wrong thread."); pager = getenv("SYSTEMD_PAGER"); if (!pager) @@ -106,7 +106,7 @@ int pager_open(PagerFlags flags) { if (pager) { pager_args = strv_split(pager, WHITESPACE); if (!pager_args) - return -ENOMEM; + return log_oom(); /* If the pager is explicitly turned off, honour it */ if (strv_isempty(pager_args) || strv_equal(pager_args, STRV_MAKE("cat"))) @@ -140,11 +140,17 @@ int pager_open(PagerFlags flags) { /* In the child start the pager */ - (void) dup2(fd[0], STDIN_FILENO); + if (dup2(fd[0], STDIN_FILENO) < 0) { + log_error_errno(errno, "Failed to duplicate file descriptor to STDIN: %m"); + _exit(EXIT_FAILURE); + } + safe_close_pair(fd); - if (setenv("LESS", less_opts, 1) < 0) + if (setenv("LESS", less_opts, 1) < 0) { + log_error_errno(errno, "Failed to set environment variable LESS: %m"); _exit(EXIT_FAILURE); + } /* Initialize a good charset for less. This is * particularly important if we output UTF-8 @@ -153,14 +159,21 @@ int pager_open(PagerFlags flags) { if (!less_charset && is_locale_utf8()) less_charset = "utf-8"; if (less_charset && - setenv("LESSCHARSET", less_charset, 1) < 0) + setenv("LESSCHARSET", less_charset, 1) < 0) { + log_error_errno(errno, "Failed to set environment variable LESSCHARSET: %m"); _exit(EXIT_FAILURE); + } if (pager_args) { - if (loop_write(exe_name_pipe[1], pager_args[0], strlen(pager_args[0]) + 1, false) < 0) + r = loop_write(exe_name_pipe[1], pager_args[0], strlen(pager_args[0]) + 1, false); + if (r < 0) { + log_error_errno(r, "Failed to write pager name to socket: %m"); _exit(EXIT_FAILURE); + } execvp(pager_args[0], pager_args); + log_full_errno(errno == ENOENT ? LOG_DEBUG : LOG_WARNING, errno, + "Failed execute %s, using fallback pagers: %m", pager_args[0]); } /* Debian's alternatives command for pagers is @@ -170,13 +183,21 @@ int pager_open(PagerFlags flags) { * is similar to this one anyway, but is * Debian-specific. */ FOREACH_STRING(exe, "pager", "less", "more") { - if (loop_write(exe_name_pipe[1], exe, strlen(exe) + 1, false) < 0) + r = loop_write(exe_name_pipe[1], exe, strlen(exe) + 1, false); + if (r < 0) { + log_error_errno(r, "Failed to write pager name to socket: %m"); _exit(EXIT_FAILURE); + } execlp(exe, exe, NULL); + log_full_errno(errno == ENOENT ? LOG_DEBUG : LOG_WARNING, errno, + "Failed execute %s, using next fallback pager: %m", exe); } - if (loop_write(exe_name_pipe[1], "(built-in)", strlen("(built-in") + 1, false) < 0) + r = loop_write(exe_name_pipe[1], "(built-in)", strlen("(built-in") + 1, false); + if (r < 0) { + log_error_errno(r, "Failed to write pager name to socket: %m"); _exit(EXIT_FAILURE); + } pager_fallback(); /* not reached */ } From 55844aebb60b42f56335eb147dd395269d8fb686 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 14:27:39 +0100 Subject: [PATCH 08/12] pager: close all fds when forking off pager --- src/shared/pager.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/pager.c b/src/shared/pager.c index ce4ca9bdb2..69484384d3 100644 --- a/src/shared/pager.c +++ b/src/shared/pager.c @@ -132,7 +132,7 @@ int pager_open(PagerFlags flags) { if (flags & PAGER_JUMP_TO_END) less_opts = strjoina(less_opts, " +G"); - r = safe_fork("(pager)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pager_pid); + r = safe_fork_full("(pager)", fd, 2, FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_DEATHSIG|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pager_pid); if (r < 0) return r; if (r == 0) { From f2747bf52b4762aa9623d3d4adf917434e8afc77 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 14:48:47 +0100 Subject: [PATCH 09/12] machined: prefix child process name with 'sd' So far we followed to rule that child processes we fork off without execve() are named "(sd-xyz)", but one child process didn't follow this. Correct that. --- src/machine/image-dbus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/machine/image-dbus.c b/src/machine/image-dbus.c index 89df274544..512e5282b9 100644 --- a/src/machine/image-dbus.c +++ b/src/machine/image-dbus.c @@ -169,7 +169,7 @@ int bus_image_method_clone( if (pipe2(errno_pipe_fd, O_CLOEXEC|O_NONBLOCK) < 0) return sd_bus_error_set_errnof(error, errno, "Failed to create pipe: %m"); - r = safe_fork("(imgclone)", FORK_RESET_SIGNALS, &child); + r = safe_fork("(sd-imgclone)", FORK_RESET_SIGNALS, &child); if (r < 0) return sd_bus_error_set_errnof(error, r, "Failed to fork(): %m"); if (r == 0) { From ae1889068796f6151703f598dc2bae88a167bc9d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 14:49:36 +0100 Subject: [PATCH 10/12] makefs: correct child process name Probably a copy/paste mistake --- src/partition/makefs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/partition/makefs.c b/src/partition/makefs.c index ab19577dca..0b9bae55e7 100644 --- a/src/partition/makefs.c +++ b/src/partition/makefs.c @@ -28,7 +28,7 @@ static int makefs(const char *type, const char *device) { if (access(mkfs, X_OK) != 0) return log_error_errno(errno, "%s is not executable: %m", mkfs); - r = safe_fork("(fsck)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pid); + r = safe_fork("(mkfs)", FORK_RESET_SIGNALS|FORK_DEATHSIG|FORK_RLIMIT_NOFILE_SAFE|FORK_LOG, &pid); if (r < 0) return r; if (r == 0) { From 49f3ee7e74c714f55aab395c080b1099fc17f7fd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 29 Nov 2018 14:50:09 +0100 Subject: [PATCH 11/12] udevd: configure a child process name for worker processes --- src/udev/udevd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/udev/udevd.c b/src/udev/udevd.c index f5d33541f6..fa85f6e263 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -534,7 +534,7 @@ static int worker_spawn(Manager *manager, struct event *event) { if (r < 0) return log_error_errno(r, "Worker: Failed to enable receiving of device: %m"); - r = safe_fork(NULL, FORK_DEATHSIG, &pid); + r = safe_fork("(worker)", FORK_DEATHSIG, &pid); if (r < 0) { event->state = EVENT_QUEUED; return log_error_errno(r, "Failed to fork() worker: %m"); From 707b3fbd5732c630d1fd0bab6f6e5b8f7130b322 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Nov 2018 16:18:41 +0100 Subject: [PATCH 12/12] update TODO --- TODO | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/TODO b/TODO index cafd75a01d..f5c5d6cc22 100644 --- a/TODO +++ b/TODO @@ -23,7 +23,9 @@ Janitorial Clean-ups: Features: -* when we fork off generators and such, lower LIMIT_NOFILE soft limit to 1K +* Maybe introduce a helper safe_exec() or so, which is to execve() which + safe_fork() is to fork(). And then make revert the RLIMIT_NOFILE soft limit + to 1K implicitly, unless explicitly opted-out. * rework seccomp/nnp logic that that even if User= is used in combination with a seccomp option we don't have to set NNP. For that, change uid first whil