From 0f81c8406f0f47175c699715e84de8291057033c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 1 Dec 2024 17:36:33 +0900 Subject: [PATCH 1/6] exec-util: allow to invoke polkit/ask-password agent even if STDIN is not a tty Closes #35018. --- src/shared/ask-password-agent.c | 8 ++------ src/shared/exec-util.c | 23 ++++++++++++++--------- src/shared/polkit-agent.c | 8 ++------ 3 files changed, 18 insertions(+), 21 deletions(-) diff --git a/src/shared/ask-password-agent.c b/src/shared/ask-password-agent.c index 62b73503ca..d02d68a4e1 100644 --- a/src/shared/ask-password-agent.c +++ b/src/shared/ask-password-agent.c @@ -18,12 +18,8 @@ int ask_password_agent_open(void) { if (agent_pid > 0) return 0; - /* We check STDIN here, not STDOUT, since this is about input, not output */ - if (!isatty_safe(STDIN_FILENO)) - return 0; - - /* Also check if we have a controlling terminal. If not (ENXIO here), we aren't actually invoked - * interactively on a terminal, hence fail */ + /* Check if we have a controlling terminal. If not (ENXIO here), we aren't actually invoked + * interactively on a terminal, hence fail. */ r = get_ctty_devnr(0, NULL); if (r == -ENXIO) return 0; diff --git a/src/shared/exec-util.c b/src/shared/exec-util.c index 8435c4f118..599b925a99 100644 --- a/src/shared/exec-util.c +++ b/src/shared/exec-util.c @@ -544,7 +544,6 @@ int fexecve_or_execve(int executable_fd, const char *executable, char *const arg } int _fork_agent(const char *name, const 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; @@ -567,17 +566,18 @@ int _fork_agent(const char *name, const int except[], size_t n_except, pid_t *re /* In the child: */ - stdout_is_tty = isatty_safe(STDOUT_FILENO); - stderr_is_tty = isatty_safe(STDERR_FILENO); + bool stdin_is_tty = isatty_safe(STDIN_FILENO), + stdout_is_tty = isatty_safe(STDOUT_FILENO), + stderr_is_tty = isatty_safe(STDERR_FILENO); - if (!stdout_is_tty || !stderr_is_tty) { + if (!stdin_is_tty || !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 + /* Detach from stdin/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); + fd = open("/dev/tty", stdin_is_tty ? O_WRONLY : (stdout_is_tty && stderr_is_tty) ? O_RDONLY : O_RDWR); if (fd < 0) { if (errno != ENXIO) { log_error_errno(errno, "Failed to open /dev/tty: %m"); @@ -588,13 +588,18 @@ int _fork_agent(const char *name, const int except[], size_t n_except, pid_t *re * connected to a TTY. That's a weird setup, but let's handle it gracefully: let's * skip the forking of the agents, given the TTY setup is not in order. */ } else { + if (!stdin_is_tty && dup2(fd, STDIN_FILENO) < 0) { + log_error_errno(errno, "Failed to dup2 /dev/tty to STDIN: %m"); + _exit(EXIT_FAILURE); + } + if (!stdout_is_tty && dup2(fd, STDOUT_FILENO) < 0) { - log_error_errno(errno, "Failed to dup2 /dev/tty: %m"); + log_error_errno(errno, "Failed to dup2 /dev/tty to STDOUT: %m"); _exit(EXIT_FAILURE); } if (!stderr_is_tty && dup2(fd, STDERR_FILENO) < 0) { - log_error_errno(errno, "Failed to dup2 /dev/tty: %m"); + log_error_errno(errno, "Failed to dup2 /dev/tty to STDERR: %m"); _exit(EXIT_FAILURE); } diff --git a/src/shared/polkit-agent.c b/src/shared/polkit-agent.c index 842e41e8db..d87eb56164 100644 --- a/src/shared/polkit-agent.c +++ b/src/shared/polkit-agent.c @@ -31,12 +31,8 @@ int polkit_agent_open(void) { if (geteuid() == 0) return 0; - /* We check STDIN here, not STDOUT, since this is about input, not output */ - if (!isatty_safe(STDIN_FILENO)) - return 0; - - /* Also check if we have a controlling terminal. If not (ENXIO here), we aren't actually invoked - * interactively on a terminal, hence fail */ + /* Check if we have a controlling terminal. If not (ENXIO here), we aren't actually invoked + * interactively on a terminal, hence fail. */ r = get_ctty_devnr(0, NULL); if (r == -ENXIO) return 0; From 388d6c5f37238a9f5c6a2abe231d9b2633ba8b3b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 3 Dec 2024 18:36:15 +0900 Subject: [PATCH 2/6] polkit-agent: modernize code a bit - Use _cleanup_close_pair_ attribute for the pipe FDs, - Return earlier on failure in forking polkit agent. --- src/shared/polkit-agent.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/shared/polkit-agent.c b/src/shared/polkit-agent.c index d87eb56164..4a0d83e8ba 100644 --- a/src/shared/polkit-agent.c +++ b/src/shared/polkit-agent.c @@ -21,8 +21,9 @@ static pid_t agent_pid = 0; int polkit_agent_open(void) { + _cleanup_close_pair_ int pipe_fd[2] = EBADF_PAIR; char notify_fd[DECIMAL_STR_MAX(int) + 1]; - int pipe_fd[2], r; + int r; if (agent_pid > 0) return 0; @@ -55,19 +56,16 @@ int polkit_agent_open(void) { POLKIT_AGENT_BINARY_PATH, "--notify-fd", notify_fd, "--fallback"); + if (r < 0) + return log_error_errno(r, "Failed to fork polkit agent: %m"); /* Close the writing side, because that's the one for the agent */ - safe_close(pipe_fd[1]); + pipe_fd[1] = safe_close(pipe_fd[1]); - if (r < 0) - log_error_errno(r, "Failed to fork polkit agent: %m"); - else - /* Wait until the agent closes the fd */ - (void) fd_wait_for_event(pipe_fd[0], POLLHUP, USEC_INFINITY); + /* Wait until the agent closes the fd */ + (void) fd_wait_for_event(pipe_fd[0], POLLHUP, USEC_INFINITY); - safe_close(pipe_fd[0]); - - return r; + return 1; } void polkit_agent_close(void) { From fc3691a70adefd5519bd529fe3a204015caa476b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 3 Dec 2024 18:40:06 +0900 Subject: [PATCH 3/6] exec-util: split out common checks before fork_agent() to can_fork_agent() No functional change, just refactoring. --- src/shared/ask-password-agent.c | 12 ++---------- src/shared/exec-util.c | 17 +++++++++++++++++ src/shared/exec-util.h | 1 + src/shared/polkit-agent.c | 12 ++---------- 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/shared/ask-password-agent.c b/src/shared/ask-password-agent.c index d02d68a4e1..b16d222359 100644 --- a/src/shared/ask-password-agent.c +++ b/src/shared/ask-password-agent.c @@ -8,7 +8,6 @@ #include "exec-util.h" #include "log.h" #include "process-util.h" -#include "terminal-util.h" static pid_t agent_pid = 0; @@ -18,17 +17,10 @@ int ask_password_agent_open(void) { if (agent_pid > 0) return 0; - /* Check if we have a controlling terminal. If not (ENXIO here), we aren't actually invoked - * interactively on a terminal, hence fail. */ - r = get_ctty_devnr(0, NULL); - if (r == -ENXIO) - return 0; - if (r < 0) + r = shall_fork_agent(); + if (r <= 0) return r; - if (!is_main_thread()) - return -EPERM; - r = fork_agent("(sd-askpwagent)", NULL, 0, &agent_pid, diff --git a/src/shared/exec-util.c b/src/shared/exec-util.c index 599b925a99..e044cea330 100644 --- a/src/shared/exec-util.c +++ b/src/shared/exec-util.c @@ -543,6 +543,23 @@ int fexecve_or_execve(int executable_fd, const char *executable, char *const arg return -errno; } +int shall_fork_agent(void) { + int r; + + /* Check if we have a controlling terminal. If not (ENXIO here), we aren't actually invoked + * interactively on a terminal, hence fail. */ + r = get_ctty_devnr(0, NULL); + if (r == -ENXIO) + return false; + if (r < 0) + return r; + + if (!is_main_thread()) + return -EPERM; + + return true; +} + int _fork_agent(const char *name, const int except[], size_t n_except, pid_t *ret_pid, const char *path, ...) { size_t n, i; va_list ap; diff --git a/src/shared/exec-util.h b/src/shared/exec-util.h index ca7d30d45e..4f8eae26e7 100644 --- a/src/shared/exec-util.h +++ b/src/shared/exec-util.h @@ -61,5 +61,6 @@ 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 shall_fork_agent(void); int _fork_agent(const char *name, const int except[], size_t n_except, pid_t *ret_pid, const char *path, ...) _sentinel_; #define fork_agent(name, ...) _fork_agent(name, __VA_ARGS__, NULL) diff --git a/src/shared/polkit-agent.c b/src/shared/polkit-agent.c index 4a0d83e8ba..ce1212e15e 100644 --- a/src/shared/polkit-agent.c +++ b/src/shared/polkit-agent.c @@ -14,7 +14,6 @@ #include "polkit-agent.h" #include "process-util.h" #include "stdio-util.h" -#include "terminal-util.h" #include "time-util.h" #if ENABLE_POLKIT @@ -32,17 +31,10 @@ int polkit_agent_open(void) { if (geteuid() == 0) return 0; - /* Check if we have a controlling terminal. If not (ENXIO here), we aren't actually invoked - * interactively on a terminal, hence fail. */ - r = get_ctty_devnr(0, NULL); - if (r == -ENXIO) - return 0; - if (r < 0) + r = shall_fork_agent(); + if (r <= 0) return r; - if (!is_main_thread()) - return -EPERM; - if (pipe2(pipe_fd, 0) < 0) return -errno; From 90579fd0b31b489988b9414fd15a883fb89bea00 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 3 Dec 2024 18:50:25 +0900 Subject: [PATCH 4/6] exec-util: drop handling of ENXIO in opening /dev/tty This effectively reverts 0bcf1679007e71d1d37666c10ab1f8d46de8d570. The handling is not necessary anymore after 61242b1f0f9cac399deb67c88c3b62d38218dba3. --- src/shared/exec-util.c | 44 ++++++++++++++++++------------------------ 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/src/shared/exec-util.c b/src/shared/exec-util.c index e044cea330..725962ef83 100644 --- a/src/shared/exec-util.c +++ b/src/shared/exec-util.c @@ -596,32 +596,26 @@ int _fork_agent(const char *name, const int except[], size_t n_except, pid_t *re * stdin around. */ fd = open("/dev/tty", stdin_is_tty ? O_WRONLY : (stdout_is_tty && stderr_is_tty) ? O_RDONLY : O_RDWR); if (fd < 0) { - if (errno != ENXIO) { - log_error_errno(errno, "Failed to open /dev/tty: %m"); - _exit(EXIT_FAILURE); - } - - /* If we get ENXIO here we have no controlling TTY even though stdout/stderr are - * connected to a TTY. That's a weird setup, but let's handle it gracefully: let's - * skip the forking of the agents, given the TTY setup is not in order. */ - } else { - if (!stdin_is_tty && dup2(fd, STDIN_FILENO) < 0) { - log_error_errno(errno, "Failed to dup2 /dev/tty to STDIN: %m"); - _exit(EXIT_FAILURE); - } - - if (!stdout_is_tty && dup2(fd, STDOUT_FILENO) < 0) { - log_error_errno(errno, "Failed to dup2 /dev/tty to STDOUT: %m"); - _exit(EXIT_FAILURE); - } - - if (!stderr_is_tty && dup2(fd, STDERR_FILENO) < 0) { - log_error_errno(errno, "Failed to dup2 /dev/tty to STDERR: %m"); - _exit(EXIT_FAILURE); - } - - fd = safe_close_above_stdio(fd); + log_error_errno(errno, "Failed to open /dev/tty: %m"); + _exit(EXIT_FAILURE); } + + if (!stdin_is_tty && dup2(fd, STDIN_FILENO) < 0) { + log_error_errno(errno, "Failed to dup2 /dev/tty to STDIN: %m"); + _exit(EXIT_FAILURE); + } + + if (!stdout_is_tty && dup2(fd, STDOUT_FILENO) < 0) { + log_error_errno(errno, "Failed to dup2 /dev/tty to STDOUT: %m"); + _exit(EXIT_FAILURE); + } + + if (!stderr_is_tty && dup2(fd, STDERR_FILENO) < 0) { + log_error_errno(errno, "Failed to dup2 /dev/tty to STDERR: %m"); + _exit(EXIT_FAILURE); + } + + fd = safe_close_above_stdio(fd); } /* Count arguments */ From f0ace1655db51278da005bfe08660f00ca601f2c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 3 Dec 2024 18:58:11 +0900 Subject: [PATCH 5/6] exec-util: use open_terminal() in fork_agent() for safety --- src/shared/exec-util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shared/exec-util.c b/src/shared/exec-util.c index 725962ef83..6ab32bea92 100644 --- a/src/shared/exec-util.c +++ b/src/shared/exec-util.c @@ -594,9 +594,9 @@ int _fork_agent(const char *name, const int except[], size_t n_except, pid_t *re * 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", stdin_is_tty ? O_WRONLY : (stdout_is_tty && stderr_is_tty) ? O_RDONLY : O_RDWR); + fd = open_terminal("/dev/tty", stdin_is_tty ? O_WRONLY : (stdout_is_tty && stderr_is_tty) ? O_RDONLY : O_RDWR); if (fd < 0) { - log_error_errno(errno, "Failed to open /dev/tty: %m"); + log_error_errno(fd, "Failed to open /dev/tty: %m"); _exit(EXIT_FAILURE); } From 46c26454bd061c6ebda0fd40317018c08b32c7fd Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 1 Dec 2024 17:43:31 +0900 Subject: [PATCH 6/6] exec-util: use strv_from_stdarg_alloca() No functional change, just refactoring. --- src/shared/ask-password-agent.c | 1 - src/shared/exec-util.c | 18 +----------------- src/shared/polkit-agent.c | 1 - 3 files changed, 1 insertion(+), 19 deletions(-) diff --git a/src/shared/ask-password-agent.c b/src/shared/ask-password-agent.c index b16d222359..c5898ca564 100644 --- a/src/shared/ask-password-agent.c +++ b/src/shared/ask-password-agent.c @@ -25,7 +25,6 @@ int ask_password_agent_open(void) { NULL, 0, &agent_pid, SYSTEMD_TTY_ASK_PASSWORD_AGENT_BINARY_PATH, - SYSTEMD_TTY_ASK_PASSWORD_AGENT_BINARY_PATH, "--watch"); if (r < 0) return log_error_errno(r, "Failed to fork TTY ask password agent: %m"); diff --git a/src/shared/exec-util.c b/src/shared/exec-util.c index 6ab32bea92..35ceab980b 100644 --- a/src/shared/exec-util.c +++ b/src/shared/exec-util.c @@ -561,9 +561,6 @@ int shall_fork_agent(void) { } int _fork_agent(const char *name, const int except[], size_t n_except, pid_t *ret_pid, const char *path, ...) { - size_t n, i; - va_list ap; - char **l; int r; assert(path); @@ -619,20 +616,7 @@ int _fork_agent(const char *name, const int except[], size_t n_except, pid_t *re } /* 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); - + char **l = strv_from_stdarg_alloca(path); execv(path, l); log_error_errno(errno, "Failed to execute %s: %m", path); _exit(EXIT_FAILURE); diff --git a/src/shared/polkit-agent.c b/src/shared/polkit-agent.c index ce1212e15e..a652b465b9 100644 --- a/src/shared/polkit-agent.c +++ b/src/shared/polkit-agent.c @@ -45,7 +45,6 @@ int polkit_agent_open(void) { 1, &agent_pid, POLKIT_AGENT_BINARY_PATH, - POLKIT_AGENT_BINARY_PATH, "--notify-fd", notify_fd, "--fallback"); if (r < 0)