From: Lennart Poettering Date: Fri, 31 Jan 2025 14:32:01 +0000 (+0100) Subject: terminal-util: change conditioning in terminal_reset_defensive() X-Git-Tag: v258-rc1~1227^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5b3eaf9e680d98e5247bd0af16afc004cca1d754;p=thirdparty%2Fsystemd.git terminal-util: change conditioning in terminal_reset_defensive() So far we conditioned the logic that issues ansi sequences for resetting the TTY based on whether something is a pty is not (under the assumption we need no reset on ptys, since they are shortlived). This is simply wrong though. The pty that a container getty is invoked on is generally long-lived: as long as the container is up, and it will be reused between getty instances/sessions all the time. In such a case we really should reset properly. Let's instead make the logic dependent on whether TERM is set to anything other than "dumb". The previous commit made sure we always set TERM in a sensible way in systemd-run, hence this *explicit* logic sounds like a much better choice now, as it mea --- diff --git a/src/basic/terminal-util.c b/src/basic/terminal-util.c index 1ff0dbcedc3..787e698738a 100644 --- a/src/basic/terminal-util.c +++ b/src/basic/terminal-util.c @@ -1934,21 +1934,18 @@ int terminal_reset_defensive(int fd, TerminalResetFlags flags) { int r = 0; assert(fd >= 0); + assert(!FLAGS_SET(flags, TERMINAL_RESET_AVOID_ANSI_SEQ|TERMINAL_RESET_FORCE_ANSI_SEQ)); - /* Resets the terminal comprehensively, but defensively. i.e. both resets the tty via ioctl()s and - * via ANSI sequences, but avoids the latter in case we are talking to a pty. That's a safety measure - * because ptys might be connected to shell pipelines where we cannot expect such ansi sequences to - * work. Given that ptys are generally short-lived (and not recycled) this restriction shouldn't hurt - * much. - * - * The specified fd should be open for *writing*! */ + /* Resets the terminal comprehensively, i.e. via both ioctl()s and via ANSI sequences, but do so only + * if $TERM is unset or set to "dumb" */ if (!isatty_safe(fd)) return -ENOTTY; RET_GATHER(r, terminal_reset_ioctl(fd, FLAGS_SET(flags, TERMINAL_RESET_SWITCH_TO_TEXT))); - if (terminal_is_pty_fd(fd) == 0) + if (!FLAGS_SET(flags, TERMINAL_RESET_AVOID_ANSI_SEQ) && + (FLAGS_SET(flags, TERMINAL_RESET_FORCE_ANSI_SEQ) || !getenv_terminal_is_dumb())) RET_GATHER(r, terminal_reset_ansi_seq(fd)); return r; diff --git a/src/basic/terminal-util.h b/src/basic/terminal-util.h index e2c4a832304..9decf42f221 100644 --- a/src/basic/terminal-util.h +++ b/src/basic/terminal-util.h @@ -43,6 +43,8 @@ bool isatty_safe(int fd); typedef enum TerminalResetFlags { TERMINAL_RESET_SWITCH_TO_TEXT = 1 << 0, + TERMINAL_RESET_AVOID_ANSI_SEQ = 1 << 1, + TERMINAL_RESET_FORCE_ANSI_SEQ = 1 << 2, } TerminalResetFlags; int terminal_reset_defensive(int fd, TerminalResetFlags flags); diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index 8d48d1f75a1..490b3a522ae 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -4367,6 +4367,10 @@ static void prepare_terminal( p->stdout_fd >= 0)) return; + /* Let's explicitly determine whether to reset via ANSI sequences or not, taking our ExecContext + * information into account */ + bool use_ansi = exec_context_shall_ansi_seq_reset(context); + if (context->tty_reset) { /* When we are resetting the TTY, then let's create a lock first, to synchronize access. This * in particular matters as concurrent resets and the TTY size ANSI DSR logic done by the @@ -4375,7 +4379,10 @@ static void prepare_terminal( if (lock_fd < 0) log_exec_debug_errno(context, p, lock_fd, "Failed to lock /dev/console, ignoring: %m"); - (void) terminal_reset_defensive(STDOUT_FILENO, /* flags= */ 0); + /* We explicitly control whether to send ansi sequences or not here, since we want to consult + * the env vars explicitly configured in the ExecContext, rather than our own environment + * block. */ + (void) terminal_reset_defensive(STDOUT_FILENO, use_ansi ? TERMINAL_RESET_FORCE_ANSI_SEQ : TERMINAL_RESET_AVOID_ANSI_SEQ); } (void) exec_context_apply_tty_size(context, STDIN_FILENO, STDOUT_FILENO, /* tty_path= */ NULL); diff --git a/src/core/execute.c b/src/core/execute.c index 62b9e6e221b..7d99e723288 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -129,11 +129,12 @@ int exec_context_apply_tty_size( rows == UINT_MAX ? &rows : NULL, cols == UINT_MAX ? &cols : NULL); - /* If we got nothing so far and we are talking to a physical device, and the TTY reset logic is on, - * then let's query dimensions from the ANSI driver. */ + /* If we got nothing so far and we are talking to a physical device, then let's query dimensions from + * the ANSI terminal driver. Note that we will not bother with this in case terminal reset via ansi + * sequences is not enabled, as the DSR logic relies on ANSI sequences after all, and if we shall not + * use those during initialization we need to skip it. */ if (rows == UINT_MAX && cols == UINT_MAX && - context->tty_reset && - terminal_is_pty_fd(output_fd) == 0 && + exec_context_shall_ansi_seq_reset(context) && isatty_safe(input_fd)) { r = terminal_get_size_by_dsr(input_fd, output_fd, &rows, &cols); if (r < 0) @@ -178,7 +179,10 @@ void exec_context_tty_reset(const ExecContext *context, const ExecParameters *p) log_warning_errno(lock_fd, "Failed to lock /dev/console, proceeding without lock: %m"); if (context->tty_reset) - (void) terminal_reset_defensive(fd, TERMINAL_RESET_SWITCH_TO_TEXT); + (void) terminal_reset_defensive( + fd, + TERMINAL_RESET_SWITCH_TO_TEXT | + (exec_context_shall_ansi_seq_reset(context) ? TERMINAL_RESET_FORCE_ANSI_SEQ : TERMINAL_RESET_AVOID_ANSI_SEQ)); r = exec_context_apply_tty_size(context, fd, fd, path); if (r < 0) @@ -974,6 +978,22 @@ bool exec_context_may_touch_console(const ExecContext *ec) { tty_may_match_dev_console(exec_context_tty_path(ec)); } +bool exec_context_shall_ansi_seq_reset(const ExecContext *c) { + assert(c); + + /* Determines whether ANSI sequences shall be used during any terminal initialisation: + * + * 1. If the reset logic is enabled at all, this is an immediate no. + * + * 2. If $TERM is set to anything other than "dumb", it's a yes. + */ + + if (!c->tty_reset) + return false; + + return !streq_ptr(strv_env_get(c->environment, "TERM"), "dumb"); +} + static void strv_fprintf(FILE *f, char **l) { assert(f); diff --git a/src/core/execute.h b/src/core/execute.h index 6421f19cc44..abfa31afcb6 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -530,6 +530,7 @@ const char* exec_context_fdname(const ExecContext *c, int fd_index); bool exec_context_may_touch_console(const ExecContext *c); bool exec_context_maintains_privileges(const ExecContext *c); +bool exec_context_shall_ansi_seq_reset(const ExecContext *c); int exec_context_get_effective_ioprio(const ExecContext *c); bool exec_context_get_effective_mount_apivfs(const ExecContext *c);