From 0e557eef37c9ebcc8f5c19fc6fc44b6fd617cc5d Mon Sep 17 00:00:00 2001 From: Sergey Bugaev Date: Mon, 22 Mar 2021 18:31:12 +0300 Subject: [PATCH 1/3] log: protect errno in log_open() Commit 0b1f3c768ce1bd1490a5e53f539976dcef8ca765 has introduced log_open() calls after exec fails post-fork. However, the log_open() call itself could change the value of errno, which, for me, manifested in: $ coredumpctl gdb ... Failed to invoke gdb: Success Fix this by using PROTECT_ERRNO in log_open(). --- src/basic/log.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/basic/log.c b/src/basic/log.c index c8cca96bca..0e6023cff2 100644 --- a/src/basic/log.c +++ b/src/basic/log.c @@ -252,6 +252,13 @@ int log_open(void) { /* Do not call from library code. */ + /* This function is often called in preparation for being able + * to log. Let's make sure we don't clobber errno, so that a call + * to a logging function immediately following a log_open() call + * can still easily reference an error that happened immediately + * before the log_open() call. */ + PROTECT_ERRNO; + /* If we don't use the console we close it here, to not get * killed by SAK. If we don't use syslog we close it here so * that we are not confused by somebody deleting the socket in From 7e0ed2e9a2fea20b21d17b374fdb709589e02008 Mon Sep 17 00:00:00 2001 From: Sergey Bugaev Date: Sat, 20 Mar 2021 18:12:14 +0300 Subject: [PATCH 2/3] tree-wide: reopen log after fork when needed This follows up on 0b1f3c768ce1bd1490a5e53f539976dcef8ca765, adding more places where we should reopen the log after forking with FORK_CLOSE_ALL_FDS. When immediately calling exec in the child, prefer to explicitly reopen the log after exec fails. In other cases, just use FORK_REOPEN_LOG. --- src/basic/process-util.c | 6 +++++- src/home/homed-home.c | 6 ++++-- src/home/homework-fscrypt.c | 4 +++- src/login/logind-brightness.c | 2 +- src/shared/dissect-image.c | 1 + 5 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index eb257144f0..264ecc276b 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -1467,7 +1467,11 @@ int fork_agent(const char *name, const int except[], size_t n_except, pid_t *ret /* 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, ret_pid); + 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) diff --git a/src/home/homed-home.c b/src/home/homed-home.c index 5a777e88ef..b0c5ce4232 100644 --- a/src/home/homed-home.c +++ b/src/home/homed-home.c @@ -1007,7 +1007,7 @@ static int home_start_work(Home *h, const char *verb, UserRecord *hr, UserRecord r = safe_fork_full("(sd-homework)", (int[]) { stdin_fd, stdout_fd }, 2, - FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_DEATHSIG|FORK_LOG, &pid); + FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_DEATHSIG|FORK_LOG|FORK_REOPEN_LOG, &pid); if (r < 0) return r; if (r == 0) { @@ -1838,7 +1838,9 @@ int home_killall(Home *h) { assert(h->uid > 0); /* We never should be UID 0 */ /* Let's kill everything matching the specified UID */ - r = safe_fork("(sd-killer)", FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_DEATHSIG|FORK_WAIT|FORK_LOG, NULL); + r = safe_fork("(sd-killer)", + FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_DEATHSIG|FORK_WAIT|FORK_LOG|FORK_REOPEN_LOG, + NULL); if (r < 0) return r; if (r == 0) { diff --git a/src/home/homework-fscrypt.c b/src/home/homework-fscrypt.c index d0676f8ae6..037e4853fd 100644 --- a/src/home/homework-fscrypt.c +++ b/src/home/homework-fscrypt.c @@ -324,7 +324,9 @@ int home_prepare_fscrypt( /* Also install the access key in the user's own keyring */ if (uid_is_valid(h->uid)) { - r = safe_fork("(sd-addkey)", FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_DEATHSIG|FORK_LOG|FORK_WAIT, NULL); + r = safe_fork("(sd-addkey)", + FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_DEATHSIG|FORK_LOG|FORK_WAIT|FORK_REOPEN_LOG, + NULL); if (r < 0) return log_error_errno(r, "Failed install encryption key in user's keyring: %m"); if (r == 0) { diff --git a/src/login/logind-brightness.c b/src/login/logind-brightness.c index 5eb3534f53..d6b9289ea6 100644 --- a/src/login/logind-brightness.c +++ b/src/login/logind-brightness.c @@ -137,7 +137,7 @@ static int brightness_writer_fork(BrightnessWriter *w) { assert(w->child == 0); assert(!w->child_event_source); - r = safe_fork("(sd-bright)", FORK_DEATHSIG|FORK_NULL_STDIO|FORK_CLOSE_ALL_FDS|FORK_LOG, &w->child); + r = safe_fork("(sd-bright)", FORK_DEATHSIG|FORK_NULL_STDIO|FORK_CLOSE_ALL_FDS|FORK_LOG|FORK_REOPEN_LOG, &w->child); if (r < 0) return r; if (r == 0) { diff --git a/src/shared/dissect-image.c b/src/shared/dissect-image.c index 6f9a457614..cca92e7fb4 100644 --- a/src/shared/dissect-image.c +++ b/src/shared/dissect-image.c @@ -1287,6 +1287,7 @@ static int run_fsck(const char *node, const char *fstype) { if (r == 0) { /* Child */ execl("/sbin/fsck", "/sbin/fsck", "-aT", node, NULL); + log_open(); log_debug_errno(errno, "Failed to execl() fsck: %m"); _exit(FSCK_OPERATIONAL_ERROR); } From fbdacd72680f38ce10980ade5badb1fac21d8788 Mon Sep 17 00:00:00 2001 From: Sergey Bugaev Date: Sun, 21 Mar 2021 21:21:57 +0300 Subject: [PATCH 3/3] homework: use FORK_CLOSE_ALL_FDS in a few more places And make sure to reopen the log appropriately. --- src/home/homework-luks.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/home/homework-luks.c b/src/home/homework-luks.c index 1d012c5d02..07d5bcfdb6 100644 --- a/src/home/homework-luks.c +++ b/src/home/homework-luks.c @@ -199,12 +199,15 @@ static int run_fsck(const char *node, const char *fstype) { return 0; } - r = safe_fork("(fsck)", FORK_RESET_SIGNALS|FORK_RLIMIT_NOFILE_SAFE|FORK_DEATHSIG|FORK_LOG|FORK_STDOUT_TO_STDERR, &fsck_pid); + r = safe_fork("(fsck)", + FORK_RESET_SIGNALS|FORK_RLIMIT_NOFILE_SAFE|FORK_DEATHSIG|FORK_LOG|FORK_STDOUT_TO_STDERR|FORK_CLOSE_ALL_FDS, + &fsck_pid); if (r < 0) return r; if (r == 0) { /* Child */ execl("/sbin/fsck", "/sbin/fsck", "-aTl", node, NULL); + log_open(); log_error_errno(errno, "Failed to execute fsck: %m"); _exit(FSCK_OPERATIONAL_ERROR); } @@ -2351,12 +2354,15 @@ static int ext4_offline_resize_fs(HomeSetup *setup, uint64_t new_size, bool disc log_info("Temporary unmounting of file system completed."); /* resize2fs requires that the file system is force checked first, do so. */ - r = safe_fork("(e2fsck)", FORK_RESET_SIGNALS|FORK_RLIMIT_NOFILE_SAFE|FORK_DEATHSIG|FORK_LOG|FORK_STDOUT_TO_STDERR, &fsck_pid); + r = safe_fork("(e2fsck)", + FORK_RESET_SIGNALS|FORK_RLIMIT_NOFILE_SAFE|FORK_DEATHSIG|FORK_LOG|FORK_STDOUT_TO_STDERR|FORK_CLOSE_ALL_FDS, + &fsck_pid); if (r < 0) return r; if (r == 0) { /* Child */ execlp("e2fsck" ,"e2fsck", "-fp", setup->dm_node, NULL); + log_open(); log_error_errno(errno, "Failed to execute e2fsck: %m"); _exit(EXIT_FAILURE); } @@ -2380,12 +2386,15 @@ static int ext4_offline_resize_fs(HomeSetup *setup, uint64_t new_size, bool disc return log_oom(); /* Resize the thing */ - r = safe_fork("(e2resize)", FORK_RESET_SIGNALS|FORK_RLIMIT_NOFILE_SAFE|FORK_DEATHSIG|FORK_LOG|FORK_WAIT|FORK_STDOUT_TO_STDERR, &resize_pid); + r = safe_fork("(e2resize)", + FORK_RESET_SIGNALS|FORK_RLIMIT_NOFILE_SAFE|FORK_DEATHSIG|FORK_LOG|FORK_WAIT|FORK_STDOUT_TO_STDERR|FORK_CLOSE_ALL_FDS, + &resize_pid); if (r < 0) return r; if (r == 0) { /* Child */ execlp("resize2fs" ,"resize2fs", setup->dm_node, size_str, NULL); + log_open(); log_error_errno(errno, "Failed to execute resize2fs: %m"); _exit(EXIT_FAILURE); }