exec-invoke: move terminal initialization a bit

It's a bit confusing, but we actually initialize the terminal twice for
each service, potentially. One earlier time, where we might end up
firing vhangup() and vt_disallocate(), which is a pretty brutal way to
reset things, by disconnecting and possibly invalidating the tty
completely. When we do this we do not keep any fd open afterwards, since
it quite likely points to a dead connection of a tty.

The 2nd time we initialize things when we actually want to use it.

The first initialization is hence "destructive" (killing any left-overs
from previous uses) the 2nd one "constructive" (preparing things for our
new use), if you so will.

Let's document this distinction in comments, and let's also move both
initializations to exec_invoke(), so that they are easier to see in their
symmetric behaviour. Moreover, let's run the tty initialization after we
opened both input and output, since we need both for doing the fancy
dimension auto init stuff now.

Oh, and of course, one thing to mention: we nowadays initialize
terminals both with ioctl() and with ansi sequences. But the latter
means we need an fd that is open for *write* (since we are *writing*
those ansi sequences to the tty). Hence, resetting via the input fd is
conceptually wrong, it worked only so far if we had O_RDWR open mode
selected)

2 files changed, 52 insertions, 13 deletions

diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c
index 7d8270bc1c..08f9188ec7 100644
--- a/src/core/exec-invoke.c
+++ b/src/core/exec-invoke.c
@@ -32,6 +32,7 @@
 #include "chown-recursive.h"
 #include "copy.h"
 #include "data-fd-util.h"
+#include "dev-setup.h"
 #include "env-util.h"
 #include "escape.h"
 #include "exec-credential.h"
@@ -353,16 +354,10 @@ static int setup_input(
                 if (dup2(params->stdin_fd, STDIN_FILENO) < 0)
                         return -errno;
 
-                /* Try to make this the controlling tty, if it is a tty, and reset it */
-                if (isatty(STDIN_FILENO)) {
+                /* Try to make this the controlling tty, if it is a tty */
+                if (isatty(STDIN_FILENO))
                         (void) ioctl(STDIN_FILENO, TIOCSCTTY, context->std_input == EXEC_INPUT_TTY_FORCE);
 
-                        if (context->tty_reset)
-                                (void) terminal_reset_defensive(STDIN_FILENO, /* switch_to_text= */ true);
-
-                        (void) exec_context_apply_tty_size(context, STDIN_FILENO, STDIN_FILENO, /* tty_path= */ NULL);
-                }
-
                 return STDIN_FILENO;
         }
 
@@ -389,10 +384,6 @@ static int setup_input(
                 if (tty_fd < 0)
                         return tty_fd;
 
-                r = exec_context_apply_tty_size(context, tty_fd, tty_fd, tty_path);
-                if (r < 0)
-                        return r;
-
                 r = move_fd(tty_fd, STDIN_FILENO, /* cloexec= */ false);
                 if (r < 0)
                         return r;
@@ -554,7 +545,6 @@ static int setup_output(
                 if (is_terminal_input(i))
                         return RET_NERRNO(dup2(STDIN_FILENO, fileno));
 
-                /* We don't reset the terminal if this is just about output */
                 return open_terminal_as(exec_context_tty_path(context), O_WRONLY, fileno);
 
         case EXEC_OUTPUT_KMSG:
@@ -4032,6 +4022,39 @@ static int send_handoff_timestamp(
         return 1;
 }
 
+static void prepare_terminal(
+                const ExecContext *context,
+                ExecParameters *p) {
+
+        _cleanup_close_ int lock_fd = -EBADF;
+
+        /* This is the "constructive" reset, i.e. is about preparing things for our invocation rather than
+         * cleaning up things from older invocations. */
+
+        assert(context);
+        assert(p);
+
+        /* We only try to reset things if we there's the chance our stdout points to a TTY */
+        if (!(is_terminal_output(context->std_output) ||
+              (context->std_output == EXEC_OUTPUT_INHERIT && is_terminal_input(context->std_input)) ||
+              context->std_output == EXEC_OUTPUT_NAMED_FD ||
+              p->stdout_fd >= 0))
+                return;
+
+        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
+                 * exec_context_apply_tty_size() below might interfere */
+                lock_fd = lock_dev_console();
+                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, /* switch_to_text= */ false);
+        }
+
+        (void) exec_context_apply_tty_size(context, STDIN_FILENO, STDOUT_FILENO, /* tty_path= */ NULL);
+}
+
 int exec_invoke(
                 const ExecCommand *command,
                 const ExecContext *context,
@@ -4199,6 +4222,11 @@ int exec_invoke(
                 return log_exec_error_errno(context, params, errno, "Failed to create new process session: %m");
         }
 
+        /* Now, reset the TTY associated to this service "destructively" (i.e. possibly even hang up or
+         * disallocate the VT), to get rid of any prior uses of the device. Note that we do not keep any fd
+         * open here, hence some of the settings made here might vanish again, depending on the TTY driver
+         * used. A 2nd ("constructive") initialization after we opened the input/output fds we actually want
+         * will fix this. */
         exec_context_tty_reset(context, params);
 
         if (params->shall_confirm_spawn && exec_context_shall_confirm_spawn(context)) {
@@ -4382,6 +4410,12 @@ int exec_invoke(
                 return log_exec_error_errno(context, params, r, "Failed to set up standard error output: %m");
         }
 
+        /* Now that stdin/stdout are definiely opened, properly initialize it with our desired
+         * settings. Note: this is a "constructive" reset, it prepares things for us to use. This is
+         * different from the "destructive" TTY reset further up. Also note: we apply this on stdin/stdout in
+         * case this is a tty, regardless if we opened it ourselves or got it passed in pre-opened. */
+        prepare_terminal(context, params);
+
         if (context->oom_score_adjust_set) {
                 /* When we can't make this change due to EPERM, then let's silently skip over it. User
                  * namespaces prohibit write access to this file, and we shouldn't trip up over that. */
diff --git a/src/core/execute.c b/src/core/execute.c
index 8982af10aa..dc418fd14c 100644
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -149,6 +149,11 @@ void exec_context_tty_reset(const ExecContext *context, const ExecParameters *p)
 
         assert(context);
 
+        /* Note that this is potentially a "destructive" reset of a TTY device. It's about getting rid of the
+         * remains of previous uses of the TTY. It's *not* about getting things set up for coming uses. We'll
+         * potentially invalidate the TTY here through hangups or VT disallocations, and hence do not keep a
+         * continous fd open. */
+
         const char *path = exec_context_tty_path(context);
 
         if (p && p->stdout_fd >= 0 && isatty_safe(p->stdout_fd))