From 149bc84aacd06dccf2b0e4def6a2fa04fa902f2b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Feb 2018 19:17:17 +0100 Subject: [PATCH 01/23] tty-ask-password-agent: make code a tiny bit shorter --- src/tty-ask-password-agent/tty-ask-password-agent.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tty-ask-password-agent/tty-ask-password-agent.c b/src/tty-ask-password-agent/tty-ask-password-agent.c index 9dfb0d80de..baed728bf9 100644 --- a/src/tty-ask-password-agent/tty-ask-password-agent.c +++ b/src/tty-ask-password-agent/tty-ask-password-agent.c @@ -363,7 +363,7 @@ static int parse_password(const char *filename, char **wall) { int tty_fd = -1; if (arg_console) { - const char *con = arg_device ? arg_device : "/dev/console"; + const char *con = arg_device ?: "/dev/console"; tty_fd = acquire_terminal(con, false, false, false, USEC_INFINITY); if (tty_fd < 0) From 8854d7950449911dafc2aad945ad97584dd5b9f1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Feb 2018 21:24:37 +0100 Subject: [PATCH 02/23] terminal-util: rework acquire_terminal() This modernizes acquire_terminal() in a couple of ways: 1. The three boolean arguments are replaced by a flags parameter, that should be more descriptive in what it does. 2. We now properly handle inotify queue overruns 3. We use _cleanup_ for closing the fds now, to shorten the code quite a bit. Behaviour should not be altered by this. --- src/basic/terminal-util.c | 134 ++++++++---------- src/basic/terminal-util.h | 18 ++- src/core/execute.c | 8 +- .../tty-ask-password-agent.c | 2 +- 4 files changed, 81 insertions(+), 81 deletions(-) diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index 42336e8fdf..395adc1ea0 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -365,44 +365,36 @@ int open_terminal(const char *name, int mode) { int acquire_terminal( const char *name, - bool fail, - bool force, - bool ignore_tiocstty_eperm, + AcquireTerminalFlags flags, usec_t timeout) { - int fd = -1, notify = -1, r = 0, wd = -1; - usec_t ts = 0; + _cleanup_close_ int notify = -1, fd = -1; + usec_t ts = USEC_INFINITY; + int r, wd = -1; assert(name); + assert(IN_SET(flags & ~ACQUIRE_TERMINAL_PERMISSIVE, ACQUIRE_TERMINAL_TRY, ACQUIRE_TERMINAL_FORCE, ACQUIRE_TERMINAL_WAIT)); - /* We use inotify to be notified when the tty is closed. We - * create the watch before checking if we can actually acquire - * it, so that we don't lose any event. + /* We use inotify to be notified when the tty is closed. We create the watch before checking if we can actually + * acquire it, so that we don't lose any event. * - * Note: strictly speaking this actually watches for the - * device being closed, it does *not* really watch whether a - * tty loses its controlling process. However, unless some - * rogue process uses TIOCNOTTY on /dev/tty *after* closing - * its tty otherwise this will not become a problem. As long - * as the administrator makes sure not configure any service - * on the same tty as an untrusted user this should not be a - * problem. (Which he probably should not do anyway.) */ + * Note: strictly speaking this actually watches for the device being closed, it does *not* really watch + * whether a tty loses its controlling process. However, unless some rogue process uses TIOCNOTTY on /dev/tty + * *after* closing its tty otherwise this will not become a problem. As long as the administrator makes sure + * not configure any service on the same tty as an untrusted user this should not be a problem. (Which he + * probably should not do anyway.) */ - if (timeout != USEC_INFINITY) - ts = now(CLOCK_MONOTONIC); - - if (!fail && !force) { + if ((flags & ~ACQUIRE_TERMINAL_PERMISSIVE) == ACQUIRE_TERMINAL_WAIT) { notify = inotify_init1(IN_CLOEXEC | (timeout != USEC_INFINITY ? IN_NONBLOCK : 0)); - if (notify < 0) { - r = -errno; - goto fail; - } + if (notify < 0) + return -errno; wd = inotify_add_watch(notify, name, IN_CLOSE); - if (wd < 0) { - r = -errno; - goto fail; - } + if (wd < 0) + return -errno; + + if (timeout != USEC_INFINITY) + ts = now(CLOCK_MONOTONIC); } for (;;) { @@ -414,41 +406,43 @@ int acquire_terminal( if (notify >= 0) { r = flush_fd(notify); if (r < 0) - goto fail; + return r; } - /* We pass here O_NOCTTY only so that we can check the return - * value TIOCSCTTY and have a reliable way to figure out if we - * successfully became the controlling process of the tty */ + /* We pass here O_NOCTTY only so that we can check the return value TIOCSCTTY and have a reliable way + * to figure out if we successfully became the controlling process of the tty */ fd = open_terminal(name, O_RDWR|O_NOCTTY|O_CLOEXEC); if (fd < 0) return fd; - /* Temporarily ignore SIGHUP, so that we don't get SIGHUP'ed - * if we already own the tty. */ + /* Temporarily ignore SIGHUP, so that we don't get SIGHUP'ed if we already own the tty. */ assert_se(sigaction(SIGHUP, &sa_new, &sa_old) == 0); /* First, try to get the tty */ - if (ioctl(fd, TIOCSCTTY, force) < 0) - r = -errno; + r = ioctl(fd, TIOCSCTTY, + (flags & ~ACQUIRE_TERMINAL_PERMISSIVE) == ACQUIRE_TERMINAL_FORCE) < 0 ? -errno : 0; + /* Reset signal handler to old value */ assert_se(sigaction(SIGHUP, &sa_old, NULL) == 0); - /* Sometimes, it makes sense to ignore TIOCSCTTY - * returning EPERM, i.e. when very likely we already - * are have this controlling terminal. */ - if (r < 0 && r == -EPERM && ignore_tiocstty_eperm) - r = 0; - - if (r < 0 && (force || fail || r != -EPERM)) - goto fail; - + /* Success? Exit the loop now! */ if (r >= 0) break; - assert(!fail); - assert(!force); + /* Any failure besides -EPERM? Fail, regardless of the mode. */ + if (r != -EPERM) + return r; + + if (flags & ACQUIRE_TERMINAL_PERMISSIVE) /* If we are in permissive mode, then EPERM is fine, turn this + * into a success. Note that EPERM is also returned if we + * already are the owner of the TTY. */ + break; + + if (flags != ACQUIRE_TERMINAL_WAIT) /* If we are in TRY or FORCE mode, then propagate EPERM as EPERM */ + return r; + assert(notify >= 0); + assert(wd >= 0); for (;;) { union inotify_event_buffer buffer; @@ -458,20 +452,17 @@ int acquire_terminal( if (timeout != USEC_INFINITY) { usec_t n; + assert(ts != USEC_INFINITY); + n = now(CLOCK_MONOTONIC); - if (ts + timeout < n) { - r = -ETIMEDOUT; - goto fail; - } + if (ts + timeout < n) + return -ETIMEDOUT; r = fd_wait_for_event(notify, POLLIN, ts + timeout - n); if (r < 0) - goto fail; - - if (r == 0) { - r = -ETIMEDOUT; - goto fail; - } + return r; + if (r == 0) + return -ETIMEDOUT; } l = read(notify, &buffer, sizeof(buffer)); @@ -479,34 +470,27 @@ int acquire_terminal( if (IN_SET(errno, EINTR, EAGAIN)) continue; - r = -errno; - goto fail; + return -errno; } FOREACH_INOTIFY_EVENT(e, buffer, l) { - if (e->wd != wd || !(e->mask & IN_CLOSE)) { - r = -EIO; - goto fail; - } + if (e->mask & IN_Q_OVERFLOW) /* If we hit an inotify queue overflow, simply check if the terminal is up for grabs now. */ + break; + + if (e->wd != wd || !(e->mask & IN_CLOSE)) /* Safety checks */ + return -EIO; } break; } - /* We close the tty fd here since if the old session - * ended our handle will be dead. It's important that - * we do this after sleeping, so that we don't enter - * an endless loop. */ + /* We close the tty fd here since if the old session ended our handle will be dead. It's important that + * we do this after sleeping, so that we don't enter an endless loop. */ fd = safe_close(fd); } - safe_close(notify); - - return fd; - -fail: - safe_close(fd); - safe_close(notify); + r = fd; + fd = -1; return r; } @@ -630,7 +614,7 @@ int make_console_stdio(void) { /* Make /dev/console the controlling terminal and stdin/stdout/stderr */ - fd = acquire_terminal("/dev/console", false, true, true, USEC_INFINITY); + fd = acquire_terminal("/dev/console", ACQUIRE_TERMINAL_FORCE|ACQUIRE_TERMINAL_PERMISSIVE, USEC_INFINITY); if (fd < 0) return log_error_errno(fd, "Failed to acquire terminal: %m"); diff --git a/src/basic/terminal-util.h b/src/basic/terminal-util.h index e82719b11b..257dbe71da 100644 --- a/src/basic/terminal-util.h +++ b/src/basic/terminal-util.h @@ -52,7 +52,23 @@ int reset_terminal_fd(int fd, bool switch_to_text); int reset_terminal(const char *name); int open_terminal(const char *name, int mode); -int acquire_terminal(const char *name, bool fail, bool force, bool ignore_tiocstty_eperm, usec_t timeout); + +/* Flags for tweaking the way we become the controlling process of a terminal. */ +typedef enum AcquireTerminalFlags { + /* Try to become the controlling process of the TTY. If we can't return -EPERM. */ + ACQUIRE_TERMINAL_TRY = 0, + + /* Tell the kernel to forcibly make us the controlling process of the TTY. Returns -EPERM if the kernel doesn't allow that. */ + ACQUIRE_TERMINAL_FORCE = 1, + + /* If we can't become the controlling process of the TTY right-away, then wait until we can. */ + ACQUIRE_TERMINAL_WAIT = 2, + + /* Pick one of the above, and then OR this flag in, in order to request permissive behaviour, if we can't become controlling process then don't mind */ + ACQUIRE_TERMINAL_PERMISSIVE = 4, +} AcquireTerminalFlags; + +int acquire_terminal(const char *name, AcquireTerminalFlags flags, usec_t timeout); int release_terminal(void); int terminal_vhangup_fd(int fd); diff --git a/src/core/execute.c b/src/core/execute.c index be9ef20725..1381ab4091 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -509,9 +509,9 @@ static int setup_input( int fd; fd = acquire_terminal(exec_context_tty_path(context), - i == EXEC_INPUT_TTY_FAIL, - i == EXEC_INPUT_TTY_FORCE, - false, + i == EXEC_INPUT_TTY_FAIL ? ACQUIRE_TERMINAL_TRY : + i == EXEC_INPUT_TTY_FORCE ? ACQUIRE_TERMINAL_FORCE : + ACQUIRE_TERMINAL_WAIT, USEC_INFINITY); if (fd < 0) return fd; @@ -753,7 +753,7 @@ static int setup_confirm_stdio(const char *vc, int *_saved_stdin, int *_saved_st if (saved_stdout < 0) return -errno; - fd = acquire_terminal(vc, false, false, false, DEFAULT_CONFIRM_USEC); + fd = acquire_terminal(vc, ACQUIRE_TERMINAL_WAIT, DEFAULT_CONFIRM_USEC); if (fd < 0) return fd; diff --git a/src/tty-ask-password-agent/tty-ask-password-agent.c b/src/tty-ask-password-agent/tty-ask-password-agent.c index baed728bf9..33b7e6010f 100644 --- a/src/tty-ask-password-agent/tty-ask-password-agent.c +++ b/src/tty-ask-password-agent/tty-ask-password-agent.c @@ -365,7 +365,7 @@ static int parse_password(const char *filename, char **wall) { if (arg_console) { const char *con = arg_device ?: "/dev/console"; - tty_fd = acquire_terminal(con, false, false, false, USEC_INFINITY); + tty_fd = acquire_terminal(con, ACQUIRE_TERMINAL_WAIT, USEC_INFINITY); if (tty_fd < 0) return log_error_errno(tty_fd, "Failed to acquire /dev/console: %m"); From c6063244db32c0e1b0195e5d653d5e159db06bc7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Feb 2018 23:50:26 +0100 Subject: [PATCH 03/23] terminal-util: when making /dev/null or the console stdio, forget cached terminal features Let's forget all relevant terminal features we learnt when we make a console or /dev/null stdin/stdout/stderr. Also, while we are at it, let's drop the various _unlikely_ and _likely_ annotiations around the terminal feature caches. In many cases we call the relevant functions only once in which cases the annotations are likely to do just harm and no good. After all we can't know if the specific code will call us just once or many times... --- src/basic/terminal-util.c | 55 +++++++++++++++++++++++++-------------- src/basic/terminal-util.h | 2 ++ 2 files changed, 37 insertions(+), 20 deletions(-) diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index 395adc1ea0..40f8282e99 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -61,6 +61,10 @@ static volatile unsigned cached_columns = 0; static volatile unsigned cached_lines = 0; +static volatile int cached_on_tty = -1; +static volatile int cached_colors_enabled = -1; +static volatile int cached_underline_enabled = -1; + int chvt(int vt) { _cleanup_close_ int fd; @@ -626,6 +630,8 @@ int make_console_stdio(void) { if (r < 0) return log_error_errno(r, "Failed to duplicate terminal fd: %m"); + reset_terminal_feature_caches(); + return 0; } @@ -791,7 +797,7 @@ unsigned columns(void) { const char *e; int c; - if (_likely_(cached_columns > 0)) + if (cached_columns > 0) return cached_columns; c = 0; @@ -825,7 +831,7 @@ unsigned lines(void) { const char *e; int l; - if (_likely_(cached_lines > 0)) + if (cached_lines > 0) return cached_lines; l = 0; @@ -849,10 +855,17 @@ void columns_lines_cache_reset(int signum) { cached_lines = 0; } -bool on_tty(void) { - static int cached_on_tty = -1; +void reset_terminal_feature_caches(void) { + cached_columns = 0; + cached_lines = 0; - if (_unlikely_(cached_on_tty < 0)) + cached_colors_enabled = -1; + cached_underline_enabled = -1; + cached_on_tty = -1; +} + +bool on_tty(void) { + if (cached_on_tty < 0) cached_on_tty = isatty(STDOUT_FILENO) > 0; return cached_on_tty; @@ -863,7 +876,7 @@ int make_stdio(int fd) { assert(fd >= 0); - if (dup2(fd, STDIN_FILENO) < 0 && r >= 0) + if (dup2(fd, STDIN_FILENO) < 0) r = -errno; if (dup2(fd, STDOUT_FILENO) < 0 && r >= 0) r = -errno; @@ -880,13 +893,17 @@ int make_stdio(int fd) { } int make_null_stdio(void) { - int null_fd; + int null_fd, r; null_fd = open("/dev/null", O_RDWR|O_NOCTTY|O_CLOEXEC); if (null_fd < 0) return -errno; - return make_stdio(null_fd); + r = make_stdio(null_fd); + + reset_terminal_feature_caches(); + + return r; } int getttyname_malloc(int fd, char **ret) { @@ -1189,38 +1206,36 @@ bool terminal_is_dumb(void) { } bool colors_enabled(void) { - static int enabled = -1; - if (_unlikely_(enabled < 0)) { + if (cached_colors_enabled < 0) { int val; val = getenv_bool("SYSTEMD_COLORS"); if (val >= 0) - enabled = val; + cached_colors_enabled = val; else if (getpid_cached() == 1) /* PID1 outputs to the console without holding it open all the time */ - enabled = !getenv_terminal_is_dumb(); + cached_colors_enabled = !getenv_terminal_is_dumb(); else - enabled = !terminal_is_dumb(); + cached_colors_enabled = !terminal_is_dumb(); } - return enabled; + return cached_colors_enabled; } bool underline_enabled(void) { - static int enabled = -1; - if (enabled < 0) { + if (cached_underline_enabled < 0) { /* The Linux console doesn't support underlining, turn it off, but only there. */ - if (!colors_enabled()) - enabled = false; + if (colors_enabled()) + cached_underline_enabled = !streq_ptr(getenv("TERM"), "linux"); else - enabled = !streq_ptr(getenv("TERM"), "linux"); + cached_underline_enabled = false; } - return enabled; + return cached_underline_enabled; } int vt_default_utf8(void) { diff --git a/src/basic/terminal-util.h b/src/basic/terminal-util.h index 257dbe71da..9a060f5c59 100644 --- a/src/basic/terminal-util.h +++ b/src/basic/terminal-util.h @@ -98,7 +98,9 @@ int fd_columns(int fd); unsigned columns(void); int fd_lines(int fd); unsigned lines(void); + void columns_lines_cache_reset(int _unused_ signum); +void reset_terminal_feature_caches(void); bool on_tty(void); bool terminal_is_dumb(void); From 87964ec7d1c6edf69d18dba53add7d1b70a3aaf0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Feb 2018 23:53:34 +0100 Subject: [PATCH 04/23] terminal-util: minor, trivial fixes and improvements --- src/basic/terminal-util.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index 40f8282e99..97c876af0c 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -327,8 +327,8 @@ int reset_terminal(const char *name) { } int open_terminal(const char *name, int mode) { - int fd, r; unsigned c = 0; + int fd; /* * If a TTY is in the process of being closed opening it might @@ -358,8 +358,7 @@ int open_terminal(const char *name, int mode) { c++; } - r = isatty(fd); - if (r == 0) { + if (isatty(fd) <= 0) { safe_close(fd); return -ENOTTY; } @@ -507,7 +506,7 @@ int release_terminal(void) { _cleanup_close_ int fd = -1; struct sigaction sa_old; - int r = 0; + int r; fd = open("/dev/tty", O_RDWR|O_NOCTTY|O_CLOEXEC|O_NONBLOCK); if (fd < 0) @@ -517,8 +516,7 @@ int release_terminal(void) { * by our own TIOCNOTTY */ assert_se(sigaction(SIGHUP, &sa_new, &sa_old) == 0); - if (ioctl(fd, TIOCNOTTY) < 0) - r = -errno; + r = ioctl(fd, TIOCNOTTY) < 0 ? -errno : 0; assert_se(sigaction(SIGHUP, &sa_old, NULL) == 0); From 5439206bc721bc2e7b8453601e9c99d8b0fe5e9a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Feb 2018 23:53:59 +0100 Subject: [PATCH 05/23] tty-ask-password-agent: assing sendto() result to a ssize_t variable, not an int We should be careful with these types, and if we do convert between "int" and "ssize_t" we should do so explicitly rather than implicitly. Otherwise this just looks like a bug. --- src/tty-ask-password-agent/tty-ask-password-agent.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/tty-ask-password-agent/tty-ask-password-agent.c b/src/tty-ask-password-agent/tty-ask-password-agent.c index 33b7e6010f..871ac27f0e 100644 --- a/src/tty-ask-password-agent/tty-ask-password-agent.c +++ b/src/tty-ask-password-agent/tty-ask-password-agent.c @@ -254,6 +254,7 @@ static int send_passwords(const char *socket_name, char **passwords) { union sockaddr_union sa = { .un.sun_family = AF_UNIX }; size_t packet_length = 1; char **p, *d; + ssize_t n; int r; assert(socket_name); @@ -279,9 +280,13 @@ static int send_passwords(const char *socket_name, char **passwords) { strncpy(sa.un.sun_path, socket_name, sizeof(sa.un.sun_path)); - r = sendto(socket_fd, packet, packet_length, MSG_NOSIGNAL, &sa.sa, SOCKADDR_UN_LEN(sa.un)); - if (r < 0) + n = sendto(socket_fd, packet, packet_length, MSG_NOSIGNAL, &sa.sa, SOCKADDR_UN_LEN(sa.un)); + if (n < 0) { r = log_debug_errno(errno, "sendto(): %m"); + goto finish; + } + + r = (int) n; finish: explicit_bzero(packet, packet_length); From befd657b00a627b0661cdf94b06c125e7be6a6ee Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Feb 2018 23:55:19 +0100 Subject: [PATCH 06/23] tty-ask-password-agent: show right TTY path in error message --- src/tty-ask-password-agent/tty-ask-password-agent.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/tty-ask-password-agent/tty-ask-password-agent.c b/src/tty-ask-password-agent/tty-ask-password-agent.c index 871ac27f0e..ed9adab067 100644 --- a/src/tty-ask-password-agent/tty-ask-password-agent.c +++ b/src/tty-ask-password-agent/tty-ask-password-agent.c @@ -372,11 +372,12 @@ static int parse_password(const char *filename, char **wall) { tty_fd = acquire_terminal(con, ACQUIRE_TERMINAL_WAIT, USEC_INFINITY); if (tty_fd < 0) - return log_error_errno(tty_fd, "Failed to acquire /dev/console: %m"); + return log_error_errno(tty_fd, "Failed to acquire %s: %m", con); r = reset_terminal_fd(tty_fd, true); if (r < 0) log_warning_errno(r, "Failed to reset terminal, ignoring: %m"); + } r = ask_password_tty(message, NULL, not_after, echo ? ASK_PASSWORD_ECHO : 0, filename, &password); From 0f13392851f3bb5e9e091f2f34b0315aaed67ac8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Feb 2018 23:55:58 +0100 Subject: [PATCH 07/23] ask-password: don't use ttyfd if it is not set --- src/shared/ask-password-api.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c index 99d6a9b143..25cd9eee7f 100644 --- a/src/shared/ask-password-api.c +++ b/src/shared/ask-password-api.c @@ -373,7 +373,8 @@ int ask_password_tty( loop_write(ttyfd, "(no echo) ", 10, false); } else { if (p >= sizeof(passphrase)-1) { - loop_write(ttyfd, "\a", 1, false); + if (ttyfd >= 0) + loop_write(ttyfd, "\a", 1, false); continue; } From a497a2966e39dccca0b50626972f6aaafb64fb56 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Feb 2018 23:56:57 +0100 Subject: [PATCH 08/23] ask-password: bypass clean-up if we don't need it --- src/shared/ask-password-api.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c index 25cd9eee7f..fdb53e49bf 100644 --- a/src/shared/ask-password-api.c +++ b/src/shared/ask-password-api.c @@ -232,24 +232,18 @@ int ask_password_tty( if (flag_file) { notify = inotify_init1(IN_CLOEXEC|IN_NONBLOCK); - if (notify < 0) { - r = -errno; - goto finish; - } + if (notify < 0) + return -errno; - if (inotify_add_watch(notify, flag_file, IN_ATTRIB /* for the link count */) < 0) { - r = -errno; - goto finish; - } + if (inotify_add_watch(notify, flag_file, IN_ATTRIB /* for the link count */) < 0) + return -errno; } ttyfd = open("/dev/tty", O_RDWR|O_NOCTTY|O_CLOEXEC); if (ttyfd >= 0) { - if (tcgetattr(ttyfd, &old_termios) < 0) { - r = -errno; - goto finish; - } + if (tcgetattr(ttyfd, &old_termios) < 0) + return -errno; if (colors_enabled()) loop_write(ttyfd, ANSI_HIGHLIGHT, From ac7a9674e438f9048fcf1e35df4e6362843781ec Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Feb 2018 23:57:13 +0100 Subject: [PATCH 09/23] ask-password: let's (void) cast where appropriate --- src/shared/ask-password-api.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c index fdb53e49bf..6f5a72dd46 100644 --- a/src/shared/ask-password-api.c +++ b/src/shared/ask-password-api.c @@ -246,13 +246,13 @@ int ask_password_tty( return -errno; if (colors_enabled()) - loop_write(ttyfd, ANSI_HIGHLIGHT, - STRLEN(ANSI_HIGHLIGHT), false); - loop_write(ttyfd, message, strlen(message), false); - loop_write(ttyfd, " ", 1, false); + (void) loop_write(ttyfd, ANSI_HIGHLIGHT, STRLEN(ANSI_HIGHLIGHT), false); + + (void) loop_write(ttyfd, message, strlen(message), false); + (void) loop_write(ttyfd, " ", 1, false); + if (colors_enabled()) - loop_write(ttyfd, ANSI_NORMAL, STRLEN(ANSI_NORMAL), - false); + (void) loop_write(ttyfd, ANSI_NORMAL, STRLEN(ANSI_NORMAL), false); new_termios = old_termios; new_termios.c_lflag &= ~(ICANON|ECHO); @@ -351,10 +351,10 @@ int ask_password_tty( * as first key (and only as first * key), or ... */ if (ttyfd >= 0) - loop_write(ttyfd, "(no echo) ", 10, false); + (void) loop_write(ttyfd, "(no echo) ", 10, false); } else if (ttyfd >= 0) - loop_write(ttyfd, "\a", 1, false); + (void) loop_write(ttyfd, "\a", 1, false); } else if (c == '\t' && !(flags & ASK_PASSWORD_SILENT)) { @@ -364,11 +364,11 @@ int ask_password_tty( /* ... or by pressing TAB at any time. */ if (ttyfd >= 0) - loop_write(ttyfd, "(no echo) ", 10, false); + (void) loop_write(ttyfd, "(no echo) ", 10, false); } else { if (p >= sizeof(passphrase)-1) { if (ttyfd >= 0) - loop_write(ttyfd, "\a", 1, false); + (void) loop_write(ttyfd, "\a", 1, false); continue; } @@ -378,7 +378,7 @@ int ask_password_tty( n = utf8_encoded_valid_unichar(passphrase + codepoint); if (n >= 0) { codepoint = p; - loop_write(ttyfd, (flags & ASK_PASSWORD_ECHO) ? &c : "*", 1, false); + (void) loop_write(ttyfd, (flags & ASK_PASSWORD_ECHO) ? &c : "*", 1, false); } } @@ -403,8 +403,8 @@ int ask_password_tty( finish: if (ttyfd >= 0 && reset_tty) { - loop_write(ttyfd, "\n", 1, false); - tcsetattr(ttyfd, TCSADRAIN, &old_termios); + (void) loop_write(ttyfd, "\n", 1, false); + (void) tcsetattr(ttyfd, TCSADRAIN, &old_termios); } return r; From 70dee4755a573f538e8897f97dde868bc1741170 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Feb 2018 23:57:40 +0100 Subject: [PATCH 10/23] ask-password: let's use structure initialization properly --- src/shared/ask-password-api.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c index 6f5a72dd46..fbe8d06c34 100644 --- a/src/shared/ask-password-api.c +++ b/src/shared/ask-password-api.c @@ -267,11 +267,14 @@ int ask_password_tty( reset_tty = true; } - zero(pollfd); - pollfd[POLL_TTY].fd = ttyfd >= 0 ? ttyfd : STDIN_FILENO; - pollfd[POLL_TTY].events = POLLIN; - pollfd[POLL_INOTIFY].fd = notify; - pollfd[POLL_INOTIFY].events = POLLIN; + pollfd[POLL_TTY] = (struct pollfd) { + .fd = ttyfd >= 0 ? ttyfd : STDIN_FILENO, + .events = POLLIN, + }; + pollfd[POLL_INOTIFY] = (struct pollfd) { + .fd = notify, + .events = POLLIN, + }; for (;;) { char c; From c9eb4a00542e9183a624190cef64d27c560254bd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Feb 2018 23:57:57 +0100 Subject: [PATCH 11/23] ask-password: round up when determining sleep time We should rather sleep to much than too little. This otherwise might result in a busy loop, because we slept too little and then recheck again coming to the conclusion we need to go to sleep again, and so on. --- src/shared/ask-password-api.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c index fbe8d06c34..d727f5b142 100644 --- a/src/shared/ask-password-api.c +++ b/src/shared/ask-password-api.c @@ -291,7 +291,7 @@ int ask_password_tty( goto finish; } - sleep_for = (int) ((until - y) / USEC_PER_MSEC); + sleep_for = (int) DIV_ROUND_UP(until - y, USEC_PER_MSEC); } if (flag_file) From 088dcd8e41c589f1a180124370a90768a8bd521d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Feb 2018 00:02:23 +0100 Subject: [PATCH 12/23] ask-password: derive pollfd array from enum It's prettier that way! --- src/shared/ask-password-api.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c index d727f5b142..8664513a89 100644 --- a/src/shared/ask-password-api.c +++ b/src/shared/ask-password-api.c @@ -209,19 +209,21 @@ int ask_password_tty( const char *flag_file, char **ret) { - struct termios old_termios, new_termios; - char passphrase[LINE_MAX + 1] = {}, *x; - size_t p = 0, codepoint = 0; - int r; - _cleanup_close_ int ttyfd = -1, notify = -1; - struct pollfd pollfd[2]; - bool reset_tty = false; - bool dirty = false; enum { POLL_TTY, - POLL_INOTIFY + POLL_INOTIFY, + _POLL_MAX, }; + _cleanup_close_ int ttyfd = -1, notify = -1; + struct termios old_termios, new_termios; + char passphrase[LINE_MAX + 1] = {}, *x; + struct pollfd pollfd[_POLL_MAX]; + size_t p = 0, codepoint = 0; + bool reset_tty = false; + bool dirty = false; + int r; + assert(ret); if (flags & ASK_PASSWORD_NO_TTY) From daa557208de1d77d4aa4f8f8fb162b10d796678f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Feb 2018 00:10:00 +0100 Subject: [PATCH 13/23] tty-ask-password-agent: don't open terminal multiple times We already have the terminal open, hence pass the fd we got to ask_password_tty(), so that it doesn't have to reopen it a second time. This is mostly an optimization, but it has the nice benefit of making us independent from RLIMIT_NOFILE issues and so on, as we don't need to allocate another fd needlessly. --- src/firstboot/firstboot.c | 4 ++-- src/shared/ask-password-api.c | 11 +++++++---- src/shared/ask-password-api.h | 2 +- src/test/test-ask-password-api.c | 2 +- src/tty-ask-password-agent/tty-ask-password-agent.c | 2 +- 5 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/firstboot/firstboot.c b/src/firstboot/firstboot.c index 262e520d56..effa092ec9 100644 --- a/src/firstboot/firstboot.c +++ b/src/firstboot/firstboot.c @@ -558,7 +558,7 @@ static int prompt_root_password(void) { for (;;) { _cleanup_string_free_erase_ char *a = NULL, *b = NULL; - r = ask_password_tty(msg1, NULL, 0, 0, NULL, &a); + r = ask_password_tty(-1, msg1, NULL, 0, 0, NULL, &a); if (r < 0) return log_error_errno(r, "Failed to query root password: %m"); @@ -567,7 +567,7 @@ static int prompt_root_password(void) { break; } - r = ask_password_tty(msg2, NULL, 0, 0, NULL, &b); + r = ask_password_tty(-1, msg2, NULL, 0, 0, NULL, &b); if (r < 0) return log_error_errno(r, "Failed to query root password: %m"); diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c index 8664513a89..c37920b7b9 100644 --- a/src/shared/ask-password-api.c +++ b/src/shared/ask-password-api.c @@ -202,6 +202,7 @@ static void backspace_chars(int ttyfd, size_t p) { } int ask_password_tty( + int ttyfd, const char *message, const char *keyname, usec_t until, @@ -215,7 +216,7 @@ int ask_password_tty( _POLL_MAX, }; - _cleanup_close_ int ttyfd = -1, notify = -1; + _cleanup_close_ int cttyfd = -1, notify = -1; struct termios old_termios, new_termios; char passphrase[LINE_MAX + 1] = {}, *x; struct pollfd pollfd[_POLL_MAX]; @@ -241,9 +242,11 @@ int ask_password_tty( return -errno; } - ttyfd = open("/dev/tty", O_RDWR|O_NOCTTY|O_CLOEXEC); - if (ttyfd >= 0) { + /* If the caller didn't specify a TTY, then use the controlling tty, if we can. */ + if (ttyfd < 0) + ttyfd = cttyfd = open("/dev/tty", O_RDWR|O_NOCTTY|O_CLOEXEC); + if (ttyfd >= 0) { if (tcgetattr(ttyfd, &old_termios) < 0) return -errno; @@ -715,7 +718,7 @@ int ask_password_auto( if (!(flags & ASK_PASSWORD_NO_TTY) && isatty(STDIN_FILENO)) { char *s = NULL, **l = NULL; - r = ask_password_tty(message, keyname, until, flags, NULL, &s); + r = ask_password_tty(-1, message, keyname, until, flags, NULL, &s); if (r < 0) return r; diff --git a/src/shared/ask-password-api.h b/src/shared/ask-password-api.h index f3ca6743a9..41d45c62ce 100644 --- a/src/shared/ask-password-api.h +++ b/src/shared/ask-password-api.h @@ -33,7 +33,7 @@ typedef enum AskPasswordFlags { ASK_PASSWORD_NO_AGENT = 32, } AskPasswordFlags; -int ask_password_tty(const char *message, const char *keyname, usec_t until, AskPasswordFlags flags, const char *flag_file, char **ret); +int ask_password_tty(int tty_fd, const char *message, const char *keyname, usec_t until, AskPasswordFlags flags, const char *flag_file, char **ret); int ask_password_agent(const char *message, const char *icon, const char *id, const char *keyname, usec_t until, AskPasswordFlags flag, char ***ret); int ask_password_keyring(const char *keyname, AskPasswordFlags flags, char ***ret); int ask_password_auto(const char *message, const char *icon, const char *id, const char *keyname, usec_t until, AskPasswordFlags flag, char ***ret); diff --git a/src/test/test-ask-password-api.c b/src/test/test-ask-password-api.c index da44465821..562a774531 100644 --- a/src/test/test-ask-password-api.c +++ b/src/test/test-ask-password-api.c @@ -26,7 +26,7 @@ static void ask_password(void) { int r; _cleanup_free_ char *ret; - r = ask_password_tty("hello?", "da key", 0, 0, NULL, &ret); + r = ask_password_tty(-1, "hello?", "da key", 0, 0, NULL, &ret); assert(r >= 0); log_info("Got %s", ret); diff --git a/src/tty-ask-password-agent/tty-ask-password-agent.c b/src/tty-ask-password-agent/tty-ask-password-agent.c index ed9adab067..be7889202c 100644 --- a/src/tty-ask-password-agent/tty-ask-password-agent.c +++ b/src/tty-ask-password-agent/tty-ask-password-agent.c @@ -380,7 +380,7 @@ static int parse_password(const char *filename, char **wall) { } - r = ask_password_tty(message, NULL, not_after, echo ? ASK_PASSWORD_ECHO : 0, filename, &password); + r = ask_password_tty(tty_fd, message, NULL, not_after, echo ? ASK_PASSWORD_ECHO : 0, filename, &password); if (arg_console) { tty_fd = safe_close(tty_fd); From f612f8fb93209b23d2efab811b6a55288c19a6b7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Feb 2018 00:13:26 +0100 Subject: [PATCH 14/23] ask-password: pretty flags enum definition a bit --- src/shared/ask-password-api.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/shared/ask-password-api.h b/src/shared/ask-password-api.h index 41d45c62ce..7a01029ae3 100644 --- a/src/shared/ask-password-api.h +++ b/src/shared/ask-password-api.h @@ -25,12 +25,12 @@ #include "time-util.h" typedef enum AskPasswordFlags { - ASK_PASSWORD_ACCEPT_CACHED = 1, - ASK_PASSWORD_PUSH_CACHE = 2, - ASK_PASSWORD_ECHO = 4, /* show the password literally while reading, instead of "*" */ - ASK_PASSWORD_SILENT = 8, /* do no show any password at all while reading */ - ASK_PASSWORD_NO_TTY = 16, - ASK_PASSWORD_NO_AGENT = 32, + ASK_PASSWORD_ACCEPT_CACHED = 1U << 0, + ASK_PASSWORD_PUSH_CACHE = 1U << 1, + ASK_PASSWORD_ECHO = 1U << 2, /* show the password literally while reading, instead of "*" */ + ASK_PASSWORD_SILENT = 1U << 3, /* do no show any password at all while reading */ + ASK_PASSWORD_NO_TTY = 1U << 4, + ASK_PASSWORD_NO_AGENT = 1U << 5, } AskPasswordFlags; int ask_password_tty(int tty_fd, const char *message, const char *keyname, usec_t until, AskPasswordFlags flags, const char *flag_file, char **ret); From e70f445306b6c348cb581eed5e954006ff93bfaf Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Feb 2018 14:27:31 +0100 Subject: [PATCH 15/23] process: shortcut getenv_for_pid() for our own process --- src/basic/process-util.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 7f8644ea9f..bed78cc599 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -826,17 +826,33 @@ int kill_and_sigcont(pid_t pid, int sig) { return r; } -int getenv_for_pid(pid_t pid, const char *field, char **_value) { +int getenv_for_pid(pid_t pid, const char *field, char **ret) { _cleanup_fclose_ FILE *f = NULL; char *value = NULL; - int r; bool done = false; - size_t l; const char *path; + size_t l; assert(pid >= 0); assert(field); - assert(_value); + assert(ret); + + if (pid == 0 || pid == getpid_cached()) { + const char *e; + + e = getenv(field); + if (!e) { + *ret = NULL; + return 0; + } + + value = strdup(e); + if (!value) + return -ENOMEM; + + *ret = value; + return 1; + } path = procfs_file_alloca(pid, "environ"); @@ -844,13 +860,13 @@ int getenv_for_pid(pid_t pid, const char *field, char **_value) { if (!f) { if (errno == ENOENT) return -ESRCH; + return -errno; } (void) __fsetlocking(f, FSETLOCKING_BYCALLER); l = strlen(field); - r = 0; do { char line[LINE_MAX]; @@ -875,14 +891,14 @@ int getenv_for_pid(pid_t pid, const char *field, char **_value) { if (!value) return -ENOMEM; - r = 1; - break; + *ret = value; + return 1; } } while (!done); - *_value = value; - return r; + *ret = NULL; + return 0; } bool pid_is_unwaited(pid_t pid) { From 6b7b0f394734d003241dcb601cbf795ba755e286 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Feb 2018 14:28:00 +0100 Subject: [PATCH 16/23] update TODO --- TODO | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/TODO b/TODO index a6cdae84a0..0b1b6c84ef 100644 --- a/TODO +++ b/TODO @@ -24,6 +24,15 @@ Janitorial Clean-ups: Features: +* introduce DefaultSlice= or so in system.conf that allows changing where we + place our units by default, i.e. change system.slice to something + else. Similar, ManagerSlice= should exist so that PID1's own scope unit could + be moved somewhere else too. Finally machined and logind should get similar + options so that it is possible to move user session scopes and machines to a + different slice too by default. Usecase: people who want to put resources on + the entire system, with the exception of one specific service. See: + https://lists.freedesktop.org/archives/systemd-devel/2018-February/040369.html + * check what setting the login shell to /bin/false vs. /sbin/nologin means and do the right thing in get_user_creds_clean() with it. From 0295642dda314559c8a5b188b21dbca8a99bf09a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Feb 2018 14:28:17 +0100 Subject: [PATCH 17/23] terminal-util: add some explanatory comments --- src/basic/terminal-util.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index 97c876af0c..26c35ebe86 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -68,6 +68,9 @@ static volatile int cached_underline_enabled = -1; int chvt(int vt) { _cleanup_close_ int fd; + /* Switch to the specified vt number. If the VT is specified <= 0 switch to the VT the kernel log messages go, + * if that's configured. */ + fd = open_terminal("/dev/tty0", O_RDWR|O_NOCTTY|O_CLOEXEC|O_NONBLOCK); if (fd < 0) return -errno; @@ -1205,6 +1208,12 @@ bool terminal_is_dumb(void) { bool colors_enabled(void) { + /* Returns true if colors are considered supported on our stdout. For that we check $SYSTEMD_COLORS first + * (which is the explicit way to turn off/on colors). If that didn't work we turn off colors unless we are on a + * TTY. And if we are on a TTY we turn it off if $TERM is set to "dumb". There's one special tweak though: if + * we are PID 1 then we do not check whether we are connected to a TTY, because we don't keep /dev/console open + * continously due to fear of SAK, and hence things are a bit weird. */ + if (cached_colors_enabled < 0) { int val; From c2b32159413aec1dc8162c4423f04ebb6da68476 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Feb 2018 14:30:30 +0100 Subject: [PATCH 18/23] tty-ask-password-agent: reenable color for boot-time password prompt The password prompt used to be highlighted, and that was a good thing. Let's fix things to make the prompt highlighted again. Fixes: #3853 --- src/basic/terminal-util.c | 24 ++++++++++++++++++- src/basic/terminal-util.h | 1 + src/shared/ask-password-api.c | 12 ++++++---- src/shared/ask-password-api.h | 1 + .../tty-ask-password-agent.c | 6 +++-- 5 files changed, 37 insertions(+), 7 deletions(-) diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index 26c35ebe86..ee540bb37f 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -48,6 +48,8 @@ #include "log.h" #include "macro.h" #include "parse-util.h" +#include "path-util.h" +#include "proc-cmdline.h" #include "process-util.h" #include "socket-util.h" #include "stat-util.h" @@ -56,7 +58,6 @@ #include "terminal-util.h" #include "time-util.h" #include "util.h" -#include "path-util.h" static volatile unsigned cached_columns = 0; static volatile unsigned cached_lines = 0; @@ -1230,6 +1231,27 @@ bool colors_enabled(void) { return cached_colors_enabled; } +bool dev_console_colors_enabled(void) { + _cleanup_free_ char *s = NULL; + int b; + + /* Returns true if we assume that color is supported on /dev/console. + * + * For that we first check if we explicitly got told to use colors or not, by checking $SYSTEMD_COLORS. If that + * didn't tell us anything we check whether PID 1 has $TERM set, and if not whether $TERM is set on the kernel + * command line. If we find $TERM set we assume color if it's not set to "dumb", similar to regular + * colors_enabled() operates. */ + + b = getenv_bool("SYSTEMD_COLORS"); + if (b >= 0) + return b; + + if (getenv_for_pid(1, "TERM", &s) <= 0) + (void) proc_cmdline_get_key("TERM", 0, &s); + + return !streq_ptr(s, "dumb"); +} + bool underline_enabled(void) { if (cached_underline_enabled < 0) { diff --git a/src/basic/terminal-util.h b/src/basic/terminal-util.h index 9a060f5c59..470892d38b 100644 --- a/src/basic/terminal-util.h +++ b/src/basic/terminal-util.h @@ -106,6 +106,7 @@ bool on_tty(void); bool terminal_is_dumb(void); bool colors_enabled(void); bool underline_enabled(void); +bool dev_console_colors_enabled(void); #define DEFINE_ANSI_FUNC(name, NAME) \ static inline const char *ansi_##name(void) { \ diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c index c37920b7b9..845fd9dc08 100644 --- a/src/shared/ask-password-api.c +++ b/src/shared/ask-password-api.c @@ -216,13 +216,12 @@ int ask_password_tty( _POLL_MAX, }; + bool reset_tty = false, dirty = false, use_color = false; _cleanup_close_ int cttyfd = -1, notify = -1; struct termios old_termios, new_termios; char passphrase[LINE_MAX + 1] = {}, *x; struct pollfd pollfd[_POLL_MAX]; size_t p = 0, codepoint = 0; - bool reset_tty = false; - bool dirty = false; int r; assert(ret); @@ -250,13 +249,18 @@ int ask_password_tty( if (tcgetattr(ttyfd, &old_termios) < 0) return -errno; - if (colors_enabled()) + if (flags & ASK_PASSWORD_CONSOLE_COLOR) + use_color = dev_console_colors_enabled(); + else + use_color = colors_enabled(); + + if (use_color) (void) loop_write(ttyfd, ANSI_HIGHLIGHT, STRLEN(ANSI_HIGHLIGHT), false); (void) loop_write(ttyfd, message, strlen(message), false); (void) loop_write(ttyfd, " ", 1, false); - if (colors_enabled()) + if (use_color) (void) loop_write(ttyfd, ANSI_NORMAL, STRLEN(ANSI_NORMAL), false); new_termios = old_termios; diff --git a/src/shared/ask-password-api.h b/src/shared/ask-password-api.h index 7a01029ae3..4b2eb3fe92 100644 --- a/src/shared/ask-password-api.h +++ b/src/shared/ask-password-api.h @@ -31,6 +31,7 @@ typedef enum AskPasswordFlags { ASK_PASSWORD_SILENT = 1U << 3, /* do no show any password at all while reading */ ASK_PASSWORD_NO_TTY = 1U << 4, ASK_PASSWORD_NO_AGENT = 1U << 5, + ASK_PASSWORD_CONSOLE_COLOR = 1U << 6, /* Use color if /dev/console points to a console that supports color */ } AskPasswordFlags; int ask_password_tty(int tty_fd, const char *message, const char *keyname, usec_t until, AskPasswordFlags flags, const char *flag_file, char **ret); diff --git a/src/tty-ask-password-agent/tty-ask-password-agent.c b/src/tty-ask-password-agent/tty-ask-password-agent.c index be7889202c..c52a19dc37 100644 --- a/src/tty-ask-password-agent/tty-ask-password-agent.c +++ b/src/tty-ask-password-agent/tty-ask-password-agent.c @@ -377,10 +377,12 @@ static int parse_password(const char *filename, char **wall) { r = reset_terminal_fd(tty_fd, true); if (r < 0) log_warning_errno(r, "Failed to reset terminal, ignoring: %m"); - } - r = ask_password_tty(tty_fd, message, NULL, not_after, echo ? ASK_PASSWORD_ECHO : 0, filename, &password); + r = ask_password_tty(tty_fd, message, NULL, not_after, + (echo ? ASK_PASSWORD_ECHO : 0) | + (arg_console ? ASK_PASSWORD_CONSOLE_COLOR : 0), + filename, &password); if (arg_console) { tty_fd = safe_close(tty_fd); From bef41af2332252e4e1774ccba23eba6aef959074 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Feb 2018 14:56:17 +0100 Subject: [PATCH 19/23] terminal-util: modernize get_kernel_consoles() a bit Also, make sure when we run in a container, we don't use the data from /sys at all, but immediately fall back to /dev/console itself. --- src/basic/terminal-util.c | 41 ++++++++++++++++++++++++++------------- src/basic/terminal-util.h | 2 +- 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index ee540bb37f..34878d3192 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -705,24 +705,29 @@ char *resolve_dev_console(char **active) { return tty; } -int get_kernel_consoles(char ***consoles) { - _cleanup_strv_free_ char **con = NULL; +int get_kernel_consoles(char ***ret) { + _cleanup_strv_free_ char **l = NULL; _cleanup_free_ char *line = NULL; - const char *active; + const char *p; int r; - assert(consoles); + assert(ret); + + /* If we /sys is mounted read-only this means we are running in some kind of container environment. In that + * case /sys would reflect the host system, not us, hence ignore the data we can read from it. */ + if (path_is_read_only_fs("/sys") > 0) + goto fallback; r = read_one_line_file("/sys/class/tty/console/active", &line); if (r < 0) return r; - active = line; + p = line; for (;;) { _cleanup_free_ char *tty = NULL; char *path; - r = extract_first_word(&active, &tty, NULL, 0); + r = extract_first_word(&p, &tty, NULL, 0); if (r < 0) return r; if (r == 0) @@ -745,21 +750,29 @@ int get_kernel_consoles(char ***consoles) { continue; } - r = strv_consume(&con, path); + r = strv_consume(&l, path); if (r < 0) return r; } - if (strv_isempty(con)) { + if (strv_isempty(l)) { log_debug("No devices found for system console"); - - r = strv_extend(&con, "/dev/console"); - if (r < 0) - return r; + goto fallback; } - *consoles = con; - con = NULL; + *ret = l; + l = NULL; + + return 0; + +fallback: + r = strv_extend(&l, "/dev/console"); + if (r < 0) + return r; + + *ret = l; + l = NULL; + return 0; } diff --git a/src/basic/terminal-util.h b/src/basic/terminal-util.h index 470892d38b..e13d441a25 100644 --- a/src/basic/terminal-util.h +++ b/src/basic/terminal-util.h @@ -83,7 +83,7 @@ int ask_string(char **ret, const char *text, ...) _printf_(2, 3); int vt_disallocate(const char *name); char *resolve_dev_console(char **active); -int get_kernel_consoles(char ***consoles); +int get_kernel_consoles(char ***ret); bool tty_is_vc(const char *tty); bool tty_is_vc_resolve(const char *tty); bool tty_is_console(const char *tty) _pure_; From 7b91264852f63ba7f775dfb4a82cc2b64d4a61bc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Feb 2018 17:30:37 +0100 Subject: [PATCH 20/23] terminal-util: make resolve_dev_console() less weird Let's normalize the behaviour: return a negative errno style error code, and return the resolved string directly as argument. --- src/basic/terminal-util.c | 57 ++++++++++++++++++++++++++------------- src/basic/terminal-util.h | 2 +- src/core/execute.c | 11 +++----- 3 files changed, 43 insertions(+), 27 deletions(-) diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index 34878d3192..a897a6367d 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -672,37 +672,55 @@ int vtnr_from_tty(const char *tty) { return i; } -char *resolve_dev_console(char **active) { + int resolve_dev_console(char **ret) { + _cleanup_free_ char *active = NULL; char *tty; + int r; - /* Resolve where /dev/console is pointing to, if /sys is actually ours - * (i.e. not read-only-mounted which is a sign for container setups) */ + assert(ret); + + /* Resolve where /dev/console is pointing to, if /sys is actually ours (i.e. not read-only-mounted which is a + * sign for container setups) */ if (path_is_read_only_fs("/sys") > 0) - return NULL; + return -ENOMEDIUM; - if (read_one_line_file("/sys/class/tty/console/active", active) < 0) - return NULL; + r = read_one_line_file("/sys/class/tty/console/active", &active); + if (r < 0) + return r; - /* If multiple log outputs are configured the last one is what - * /dev/console points to */ - tty = strrchr(*active, ' '); + /* If multiple log outputs are configured the last one is what /dev/console points to */ + tty = strrchr(active, ' '); if (tty) tty++; else - tty = *active; + tty = active; if (streq(tty, "tty0")) { - char *tmp; + active = mfree(active); /* Get the active VC (e.g. tty1) */ - if (read_one_line_file("/sys/class/tty/tty0/active", &tmp) >= 0) { - free(*active); - tty = *active = tmp; - } + r = read_one_line_file("/sys/class/tty/tty0/active", &active); + if (r < 0) + return r; + + tty = active; } - return tty; + if (tty == active) { + *ret = active; + active = NULL; + } else { + char *tmp; + + tmp = strdup(tty); + if (!tmp) + return -ENOMEM; + + *ret = tmp; + } + + return 0; } int get_kernel_consoles(char ***ret) { @@ -777,16 +795,17 @@ fallback: } bool tty_is_vc_resolve(const char *tty) { - _cleanup_free_ char *active = NULL; + _cleanup_free_ char *resolved = NULL; assert(tty); tty = skip_dev_prefix(tty); if (streq(tty, "console")) { - tty = resolve_dev_console(&active); - if (!tty) + if (resolve_dev_console(&resolved) < 0) return false; + + tty = resolved; } return tty_is_vc(tty); diff --git a/src/basic/terminal-util.h b/src/basic/terminal-util.h index e13d441a25..f6e6020b66 100644 --- a/src/basic/terminal-util.h +++ b/src/basic/terminal-util.h @@ -82,7 +82,7 @@ int ask_string(char **ret, const char *text, ...) _printf_(2, 3); int vt_disallocate(const char *name); -char *resolve_dev_console(char **active); +int resolve_dev_console(char **ret); int get_kernel_consoles(char ***ret); bool tty_is_vc(const char *tty); bool tty_is_vc_resolve(const char *tty); diff --git a/src/core/execute.c b/src/core/execute.c index 1381ab4091..bebd4eca80 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -3871,8 +3871,7 @@ static int exec_context_load_environment(const Unit *unit, const ExecContext *c, } static bool tty_may_match_dev_console(const char *tty) { - _cleanup_free_ char *active = NULL; - char *console; + _cleanup_free_ char *resolved = NULL; if (!tty) return true; @@ -3883,13 +3882,11 @@ static bool tty_may_match_dev_console(const char *tty) { if (streq(tty, "console")) return true; - console = resolve_dev_console(&active); - /* if we could not resolve, assume it may */ - if (!console) - return true; + if (resolve_dev_console(&resolved) < 0) + return true; /* if we could not resolve, assume it may */ /* "tty0" means the active VC, so it may be the same sometimes */ - return streq(console, tty) || (streq(console, "tty0") && tty_is_vc(tty)); + return streq(resolved, tty) || (streq(resolved, "tty0") && tty_is_vc(tty)); } bool exec_context_may_touch_console(const ExecContext *ec) { From 65ee8660dfe6991ef15dee9cc588a91f317be059 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Feb 2018 18:41:03 +0100 Subject: [PATCH 21/23] utf8: add utf8_n_codepoints() for counting complete utf8 codepoints in a string --- src/basic/utf8.c | 19 +++++++++++++++++++ src/basic/utf8.h | 2 ++ 2 files changed, 21 insertions(+) diff --git a/src/basic/utf8.c b/src/basic/utf8.c index 4da9a405cb..b17f420264 100644 --- a/src/basic/utf8.c +++ b/src/basic/utf8.c @@ -408,3 +408,22 @@ int utf8_encoded_valid_unichar(const char *str) { return len; } + +size_t utf8_n_codepoints(const char *str) { + size_t n = 0; + + /* Returns the number of UTF-8 codepoints in this string, or (size_t) -1 if the string is not valid UTF-8. */ + + while (*str != 0) { + int k; + + k = utf8_encoded_valid_unichar(str); + if (k < 0) + return (size_t) -1; + + str += k; + n++; + } + + return n; +} diff --git a/src/basic/utf8.h b/src/basic/utf8.h index b0a7485aed..7128615181 100644 --- a/src/basic/utf8.h +++ b/src/basic/utf8.h @@ -59,3 +59,5 @@ static inline bool utf16_is_trailing_surrogate(char16_t c) { static inline char32_t utf16_surrogate_pair_to_unichar(char16_t lead, char16_t trail) { return ((lead - 0xd800) << 10) + (trail - 0xdc00) + 0x10000; } + +size_t utf8_n_codepoints(const char *str); From fd6ac62c717dcef472b9b50f5e54f89b0105a7e2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Feb 2018 18:41:37 +0100 Subject: [PATCH 22/23] ask-password-api: many fixes to ask_password_tty() A couple of fixes: 1. always bzero_explicit() away what we remove from the passphrase buffer. The UTF-8 code assumes the string remains NUL-terminated, and we hence should enforce that. memzero() would do too here, but let's be paranoid after all this is key material. 2. when clearing '*' characters from string, do so counting UTF-8 codepoints properly. We already have code in place to count UTF-8 codepoints when generating '*' characters, hence we should take the same care when clearing them again. 3. Treat NUL on input as an alternative terminator to newline or EOF. 4. When removing characters from the password always also reset the "codepoint" index properly. --- src/shared/ask-password-api.c | 87 +++++++++++++++++++++++++++-------- 1 file changed, 67 insertions(+), 20 deletions(-) diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c index 845fd9dc08..a0ee05e2d8 100644 --- a/src/shared/ask-password-api.c +++ b/src/shared/ask-password-api.c @@ -201,6 +201,27 @@ static void backspace_chars(int ttyfd, size_t p) { } } +static void backspace_string(int ttyfd, const char *str) { + size_t m; + + assert(str); + + if (ttyfd < 0) + return; + + /* Backspaces back for enough characters to entirely undo printing of the specified string. */ + + m = utf8_n_codepoints(str); + if (m == (size_t) -1) + m = strlen(str); /* Not a valid UTF-8 string? If so, let's backspace the number of bytes output. Most + * likely this happened because we are not in an UTF-8 locale, and in that case that + * is the correct thing to do. And even if it's not, terminals tend to stop + * backspacing at the leftmost column, hence backspacing too much should be mostly + * OK. */ + + backspace_chars(ttyfd, m); +} + int ask_password_tty( int ttyfd, const char *message, @@ -286,9 +307,9 @@ int ask_password_tty( }; for (;;) { - char c; int sleep_for = -1, k; ssize_t n; + char c; if (until > 0) { usec_t y; @@ -335,33 +356,56 @@ int ask_password_tty( r = -errno; goto finish; - } else if (n == 0) + } + + /* We treat EOF, newline and NUL byte all as valid end markers */ + if (n == 0 || c == '\n' || c == 0) break; - if (c == '\n') - break; - else if (c == 21) { /* C-u */ + if (c == 21) { /* C-u */ if (!(flags & ASK_PASSWORD_SILENT)) - backspace_chars(ttyfd, p); - p = 0; + backspace_string(ttyfd, passphrase); + + explicit_bzero(passphrase, sizeof(passphrase)); + p = codepoint = 0; } else if (IN_SET(c, '\b', 127)) { if (p > 0) { + size_t q; if (!(flags & ASK_PASSWORD_SILENT)) backspace_chars(ttyfd, 1); - p--; + /* Remove a full UTF-8 codepoint from the end. For that, figure out where the last one + * begins */ + q = 0; + for (;;) { + size_t z; + + z = utf8_encoded_valid_unichar(passphrase + q); + if (z == 0) { + q = (size_t) -1; /* Invalid UTF8! */ + break; + } + + if (q + z >= p) /* This one brings us over the edge */ + break; + + q += z; + } + + p = codepoint = q == (size_t) -1 ? p - 1 : q; + explicit_bzero(passphrase + p, sizeof(passphrase) - p); + } else if (!dirty && !(flags & ASK_PASSWORD_SILENT)) { flags |= ASK_PASSWORD_SILENT; - /* There are two ways to enter silent - * mode. Either by pressing backspace - * as first key (and only as first - * key), or ... */ + /* There are two ways to enter silent mode. Either by pressing backspace as first key + * (and only as first key), or ... */ + if (ttyfd >= 0) (void) loop_write(ttyfd, "(no echo) ", 10, false); @@ -370,23 +414,25 @@ int ask_password_tty( } else if (c == '\t' && !(flags & ASK_PASSWORD_SILENT)) { - backspace_chars(ttyfd, p); + backspace_string(ttyfd, passphrase); flags |= ASK_PASSWORD_SILENT; /* ... or by pressing TAB at any time. */ if (ttyfd >= 0) (void) loop_write(ttyfd, "(no echo) ", 10, false); - } else { - if (p >= sizeof(passphrase)-1) { - if (ttyfd >= 0) - (void) loop_write(ttyfd, "\a", 1, false); - continue; - } + } else if (p >= sizeof(passphrase)-1) { + + /* Reached the size limit */ + if (ttyfd >= 0) + (void) loop_write(ttyfd, "\a", 1, false); + + } else { passphrase[p++] = c; if (!(flags & ASK_PASSWORD_SILENT) && ttyfd >= 0) { + /* Check if we got a complete UTF-8 character now. If so, let's output one '*'. */ n = utf8_encoded_valid_unichar(passphrase + codepoint); if (n >= 0) { codepoint = p; @@ -397,11 +443,12 @@ int ask_password_tty( dirty = true; } + /* Let's forget this char, just to not keep needlessly copies of key material around */ c = 'x'; } x = strndup(passphrase, p); - explicit_bzero(passphrase, p); + explicit_bzero(passphrase, sizeof(passphrase)); if (!x) { r = -ENOMEM; goto finish; From 1496ceaf301835497cde6e7a74ef90f21893566c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Feb 2018 19:56:24 +0100 Subject: [PATCH 23/23] mkosi: add pcre2 to our build deps, as we can now link to it --- .mkosi/mkosi.fedora | 1 + 1 file changed, 1 insertion(+) diff --git a/.mkosi/mkosi.fedora b/.mkosi/mkosi.fedora index 4d7168c00b..a7713271da 100644 --- a/.mkosi/mkosi.fedora +++ b/.mkosi/mkosi.fedora @@ -67,6 +67,7 @@ BuildPackages= m4 meson pam-devel + pcre2-devel pkgconfig python3-devel python3-lxml