From b27f791691b149ea660618dd93e7b9792d4e7680 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 26 Jan 2025 01:32:42 +0100 Subject: [PATCH 1/6] terminal-util: stop doing 0/upper bound check in tty_is_vc() tty_is_vc() is more often than not used for simple "categorization" than validity check. E.g. in logind, we first recognize the tty "looks like vc", and then use vtnr_from_tty() where range check is performed and vtnr is extracted. In such cases, we want to reject invalid vtnr from clients rather than silently carry on, hence let's remove bound check in tty_is_vc(). Fixes #36166 Replaces #36167 and #36175 --- src/basic/terminal-util.c | 43 ++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index eeeb657623..2baf2830b1 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -764,21 +764,7 @@ int make_console_stdio(void) { return 0; } -bool tty_is_vc(const char *tty) { - assert(tty); - - return vtnr_from_tty(tty) >= 0; -} - -bool tty_is_console(const char *tty) { - assert(tty); - - return streq(skip_dev_prefix(tty), "console"); -} - -int vtnr_from_tty(const char *tty) { - int r; - +static int vtnr_from_tty_raw(const char *tty, unsigned *ret) { assert(tty); tty = skip_dev_prefix(tty); @@ -787,8 +773,16 @@ int vtnr_from_tty(const char *tty) { if (!e) return -EINVAL; + return safe_atou(e, ret); +} + +int vtnr_from_tty(const char *tty) { unsigned u; - r = safe_atou(e, &u); + int r; + + assert(tty); + + r = vtnr_from_tty_raw(tty, &u); if (r < 0) return r; if (!vtnr_is_valid(u)) @@ -797,6 +791,23 @@ int vtnr_from_tty(const char *tty) { return (int) u; } +bool tty_is_vc(const char *tty) { + assert(tty); + + /* NB: for >= 0 values no range check is conducted here, on the assumption that the caller will + * either extract vtnr through vtnr_from_tty() later where ERANGE would be reported, or doesn't care + * about whether it's strictly valid, but only asking "does this fall into the vt catogory?", for which + * "yes" seems to be a better answer. */ + + return vtnr_from_tty_raw(tty, /* ret = */ NULL) >= 0; +} + +bool tty_is_console(const char *tty) { + assert(tty); + + return streq(skip_dev_prefix(tty), "console"); +} + int resolve_dev_console(char **ret) { int r; From d25b55b95a38785412cbbd171860ae45212c0095 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 25 Jan 2025 22:32:16 +0100 Subject: [PATCH 2/6] login/pam_systemd: drop redundant vtnr == 0 handling vtnr_from_tty() treats 0 as invalid. --- src/login/pam_systemd.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/login/pam_systemd.c b/src/login/pam_systemd.c index e97485e9c9..ffca6045b6 100644 --- a/src/login/pam_systemd.c +++ b/src/login/pam_systemd.c @@ -348,8 +348,7 @@ static int get_seat_from_display(const char *display, const char **seat, uint32_ v = vtnr_from_tty(tty); if (v < 0) return v; - else if (v == 0) - return -ENOENT; + assert(v > 0); if (seat) *seat = "seat0"; From daf13202373e4723aa11f0565468dc7b7cf420f5 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 25 Jan 2025 20:19:26 +0100 Subject: [PATCH 3/6] terminal-util: make get_kernel_consoles() return > 0 if /dev/console is resolved --- src/basic/terminal-util.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index 2baf2830b1..678cd4ed4d 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -866,13 +866,12 @@ int resolve_dev_console(char **ret) { int get_kernel_consoles(char ***ret) { _cleanup_strv_free_ char **l = NULL; _cleanup_free_ char *line = NULL; - const char *p; int r; assert(ret); - /* If /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 /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; @@ -880,8 +879,7 @@ int get_kernel_consoles(char ***ret) { if (r < 0) return r; - p = line; - for (;;) { + for (const char *p = line;;) { _cleanup_free_ char *tty = NULL, *path = NULL; r = extract_first_word(&p, &tty, NULL, 0); @@ -917,8 +915,7 @@ int get_kernel_consoles(char ***ret) { } *ret = TAKE_PTR(l); - - return 0; + return strv_length(*ret); fallback: r = strv_extend(&l, "/dev/console"); @@ -926,7 +923,6 @@ fallback: return r; *ret = TAKE_PTR(l); - return 0; } From 68cf1fc34d3e86a664fb948d413e1db4a24e16dd Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 25 Jan 2025 20:30:10 +0100 Subject: [PATCH 4/6] tty-ask-password-agent: do not needlessly spawn subprocesses if there's only one console --- .../tty-ask-password-agent.c | 30 +++++++++++++------ 1 file changed, 21 insertions(+), 9 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 5ff892c25d..1c1b95bef3 100644 --- a/src/tty-ask-password-agent/tty-ask-password-agent.c +++ b/src/tty-ask-password-agent/tty-ask-password-agent.c @@ -40,6 +40,7 @@ #include "set.h" #include "signal-util.h" #include "socket-util.h" +#include "static-destruct.h" #include "string-util.h" #include "strv.h" #include "terminal-util.h" @@ -54,7 +55,9 @@ static enum { static bool arg_plymouth = false; static bool arg_console = false; -static const char *arg_device = NULL; +static char *arg_device = NULL; + +STATIC_DESTRUCTOR_REGISTER(arg_device, freep); static int send_passwords(const char *socket_name, char **passwords) { int r; @@ -530,12 +533,13 @@ static int parse_argv(int argc, char *argv[]) { case ARG_CONSOLE: arg_console = true; if (optarg) { - if (isempty(optarg)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Empty console device path is not allowed."); - arg_device = optarg; + arg_device = strdup(optarg); + if (!arg_device) + return log_oom(); } break; @@ -667,6 +671,11 @@ static int ask_on_consoles(char *argv[]) { r = get_kernel_consoles(&consoles); if (r < 0) return log_error_errno(r, "Failed to determine devices of /dev/console: %m"); + if (r <= 1) { + /* No need to spawn subprocesses, there's only one console or using /dev/console as fallback */ + arg_device = TAKE_PTR(consoles[0]); + return 0; + } pids = set_new(NULL); if (!pids) @@ -714,7 +723,7 @@ static int ask_on_consoles(char *argv[]) { } terminate_agents(pids); - return 0; + return 1; } static int run(int argc, char *argv[]) { @@ -728,11 +737,14 @@ static int run(int argc, char *argv[]) { if (r <= 0) return r; - if (arg_console && !arg_device) - /* - * Spawn a separate process for each console device. - */ - return ask_on_consoles(argv); + /* Spawn a separate process for each console device if there're multiple. */ + if (arg_console && !arg_device) { + r = ask_on_consoles(argv); + if (r != 0) + return r; + + assert(arg_device); + } if (arg_device) /* Later on, a controlling terminal will be acquired, therefore the current process has to From 1cd3c49d09bf78a2a2e4cf25cb3d388e1f08a709 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 25 Jan 2025 20:58:47 +0100 Subject: [PATCH 5/6] getty-generator: use generator_add_symlink() where appropriate --- src/getty-generator/getty-generator.c | 52 +++++++++------------------ 1 file changed, 16 insertions(+), 36 deletions(-) diff --git a/src/getty-generator/getty-generator.c b/src/getty-generator/getty-generator.c index 9ad5597b05..fdcd5c28b6 100644 --- a/src/getty-generator/getty-generator.c +++ b/src/getty-generator/getty-generator.c @@ -25,56 +25,39 @@ static const char *arg_dest = NULL; static bool arg_enabled = true; -static int add_symlink(const char *fservice, const char *tservice) { - const char *from, *to; - - assert(fservice); - assert(tservice); - - from = strjoina(SYSTEM_DATA_UNIT_DIR "/", fservice); - to = strjoina(arg_dest, "/getty.target.wants/", tservice); - - (void) mkdir_parents_label(to, 0755); - - if (symlink(from, to) < 0) { - /* In case console=hvc0 is passed this will very likely result in EEXIST */ - if (errno == EEXIST) - return 0; - - return log_error_errno(errno, "Failed to create symlink %s: %m", to); - } - - return 0; -} - static int add_serial_getty(const char *tty) { - _cleanup_free_ char *n = NULL; + _cleanup_free_ char *instance = NULL; int r; assert(tty); log_debug("Automatically adding serial getty for /dev/%s.", tty); - r = unit_name_from_path_instance("serial-getty", tty, ".service", &n); + r = unit_name_path_escape(tty, &instance); if (r < 0) - return log_error_errno(r, "Failed to generate service name: %m"); + return log_error_errno(r, "Failed to escape tty path: %m"); - return add_symlink("serial-getty@.service", n); + return generator_add_symlink_full(arg_dest, + "getty.target", "wants", + SYSTEM_DATA_UNIT_DIR "/serial-getty@.service", instance); } static int add_container_getty(const char *tty) { - _cleanup_free_ char *n = NULL; + _cleanup_free_ char *instance = NULL; int r; assert(tty); + assert(!path_startswith(tty, "/dev/")); log_debug("Automatically adding container getty for /dev/pts/%s.", tty); - r = unit_name_from_path_instance("container-getty", tty, ".service", &n); + r = unit_name_path_escape(tty, &instance); if (r < 0) - return log_error_errno(r, "Failed to generate service name: %m"); + return log_error_errno(r, "Failed to escape tty path: %m"); - return add_symlink("container-getty@.service", n); + return generator_add_symlink_full(arg_dest, + "getty.target", "wants", + SYSTEM_DATA_UNIT_DIR "/container-getty@.service", instance); } static int verify_tty(const char *name) { @@ -101,18 +84,17 @@ static int verify_tty(const char *name) { } static int run_container(void) { - _cleanup_free_ char *container_ttys = NULL; int r; log_debug("Automatically adding console shell."); - r = add_symlink("console-getty.service", "console-getty.service"); + r = generator_add_symlink(arg_dest, "getty.target", "wants", SYSTEM_DATA_UNIT_DIR "/console-getty.service"); if (r < 0) return r; /* When $container_ttys is set for PID 1, spawn gettys on all ptys named therein. * Note that despite the variable name we only support ptys here. */ - + _cleanup_free_ char *container_ttys = NULL; (void) getenv_for_pid(1, "container_ttys", &container_ttys); for (const char *p = container_ttys;;) { @@ -124,10 +106,8 @@ static int run_container(void) { if (r == 0) return 0; - const char *tty = word; - /* First strip off /dev/ if it is specified */ - tty = path_startswith(tty, "/dev/") ?: tty; + const char *tty = skip_dev_prefix(word); /* Then, make sure it's actually a pty */ tty = path_startswith(tty, "pts/"); From 26cd3a2fcefd82804f4cf7a966263a4aba59c2e0 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 25 Jan 2025 20:42:58 +0100 Subject: [PATCH 6/6] getty-generator: port to get_kernel_consoles() --- src/getty-generator/getty-generator.c | 56 ++++++++++++--------------- 1 file changed, 24 insertions(+), 32 deletions(-) diff --git a/src/getty-generator/getty-generator.c b/src/getty-generator/getty-generator.c index fdcd5c28b6..e71b5ec101 100644 --- a/src/getty-generator/getty-generator.c +++ b/src/getty-generator/getty-generator.c @@ -31,6 +31,8 @@ static int add_serial_getty(const char *tty) { assert(tty); + tty = skip_dev_prefix(tty); + log_debug("Automatically adding serial getty for /dev/%s.", tty); r = unit_name_path_escape(tty, &instance); @@ -60,20 +62,16 @@ static int add_container_getty(const char *tty) { SYSTEM_DATA_UNIT_DIR "/container-getty@.service", instance); } -static int verify_tty(const char *name) { +static int verify_tty(const char *path) { _cleanup_close_ int fd = -EBADF; - const char *p; - /* Some TTYs are weird and have been enumerated but don't work - * when you try to use them, such as classic ttyS0 and - * friends. Let's check that and open the device and run - * isatty() on it. */ + assert(path); - p = strjoina("/dev/", name); + /* Some TTYs are weird and have been enumerated but don't work when you try to use them, such as + * classic ttyS0 and friends. Let's check that and open the device and run isatty() on it. */ - /* O_NONBLOCK is essential here, to make sure we don't wait - * for DCD */ - fd = open(p, O_RDWR|O_NONBLOCK|O_NOCTTY|O_CLOEXEC|O_NOFOLLOW); + /* O_NONBLOCK is essential here, to make sure we don't wait for DCD */ + fd = open(path, O_RDWR|O_NONBLOCK|O_NOCTTY|O_CLOEXEC|O_NOFOLLOW); if (fd < 0) return -errno; @@ -225,30 +223,24 @@ static int run(const char *dest, const char *dest_early, const char *dest_late) return run_container(); /* Automatically add in a serial getty on all active kernel consoles */ - _cleanup_free_ char *active = NULL; - (void) read_one_line_file("/sys/class/tty/console/active", &active); - for (const char *p = active;;) { - _cleanup_free_ char *tty = NULL; + _cleanup_strv_free_ char **consoles = NULL; + r = get_kernel_consoles(&consoles); + if (r < 0) + log_warning_errno(r, "Failed to get active kernel consoles, ignoring: %m"); + else if (r > 0) + STRV_FOREACH(i, consoles) { + /* We assume that gettys on virtual terminals are started via manual configuration + * and do this magic only for non-VC terminals. */ + if (tty_is_vc(*i)) + continue; - r = extract_first_word(&p, &tty, NULL, 0); - if (r < 0) - return log_error_errno(r, "Failed to parse /sys/class/tty/console/active: %m"); - if (r == 0) - break; + if (verify_tty(*i) < 0) + continue; - /* We assume that gettys on virtual terminals are started via manual configuration and do - * this magic only for non-VC terminals. */ - - if (isempty(tty) || tty_is_vc(tty)) - continue; - - if (verify_tty(tty) < 0) - continue; - - r = add_serial_getty(tty); - if (r < 0) - return r; - } + r = add_serial_getty(*i); + if (r < 0) + return r; + } /* Automatically add in a serial getty on the first virtualizer console */ FOREACH_STRING(j,