machinectl: fix race when opening new shells with "machinectl shell"

Previously, we'd allocate the TTY, spawn a service on it, but
immediately start processing the TTY and forwarding it to whatever the
commnd was started on. This is however problematic, as the TTY might get
actually opened only much later by the service. We'll hence first get
EIOs on the master as the other side is still closed, and hence
considered it hung up and terminated the session.

With this change we add a flag to the pty forwarding logic:
PTY_FORWARD_IGNORE_INITIAL_VHANGUP. If set, we'll ignore all hangups
(i.e. EIOs) on the master PTY until the first byte is successfully read.
From that point on we consider a hangup/EIO a regular connection termination. This
way, we handle the race: when we get EIO initially we'll ignore it,
until the connection is properly set up, at which time we start
honouring it.

2 files changed, 43 insertions, 18 deletions

diff --git a/src/shared/ptyfwd.c b/src/shared/ptyfwd.c
index 789f217efc..7749f20540 100644
--- a/src/shared/ptyfwd.c
+++ b/src/shared/ptyfwd.c
@@ -32,6 +32,8 @@ struct PTYForward {
 
         int master;
 
+        PTYForwardFlags flags;
+
         sd_event_source *stdin_event_source;
         sd_event_source *stdout_event_source;
         sd_event_source *master_event_source;
@@ -41,8 +43,6 @@ struct PTYForward {
         struct termios saved_stdin_attr;
         struct termios saved_stdout_attr;
 
-        bool read_only:1;
-
         bool saved_stdin:1;
         bool saved_stdout:1;
 
@@ -54,8 +54,7 @@ struct PTYForward {
         bool master_writable:1;
         bool master_hangup:1;
 
-        /* Continue reading after hangup? */
-        bool ignore_vhangup:1;
+        bool read_from_master:1;
 
         bool last_char_set:1;
         char last_char;
@@ -100,6 +99,18 @@ static bool look_for_escape(PTYForward *f, const char *buffer, size_t n) {
         return false;
 }
 
+static bool ignore_vhangup(PTYForward *f) {
+        assert(f);
+
+        if (f->flags & PTY_FORWARD_IGNORE_VHANGUP)
+                return true;
+
+        if ((f->flags & PTY_FORWARD_IGNORE_INITIAL_VHANGUP) && !f->read_from_master)
+                return true;
+
+        return false;
+}
+
 static int shovel(PTYForward *f) {
         ssize_t k;
 
@@ -179,7 +190,7 @@ static int shovel(PTYForward *f) {
                                  * EAGAIN here and try again, unless
                                  * ignore_vhangup is off. */
 
-                                if (errno == EAGAIN || (errno == EIO && f->ignore_vhangup))
+                                if (errno == EAGAIN || (errno == EIO && ignore_vhangup(f)))
                                         f->master_readable = false;
                                 else if (errno == EPIPE || errno == ECONNRESET || errno == EIO) {
                                         f->master_readable = f->master_writable = false;
@@ -190,8 +201,10 @@ static int shovel(PTYForward *f) {
                                         log_error_errno(errno, "read(): %m");
                                         return sd_event_exit(f->event, EXIT_FAILURE);
                                 }
-                        }  else
+                        }  else {
+                                f->read_from_master = true;
                                 f->out_buffer_full += (size_t) k;
+                        }
                 }
 
                 if (f->stdout_writable && f->out_buffer_full > 0) {
@@ -302,8 +315,7 @@ static int on_sigwinch_event(sd_event_source *e, const struct signalfd_siginfo *
 int pty_forward_new(
                 sd_event *event,
                 int master,
-                bool ignore_vhangup,
-                bool read_only,
+                PTYForwardFlags flags,
                 PTYForward **ret) {
 
         _cleanup_(pty_forward_freep) PTYForward *f = NULL;
@@ -314,8 +326,7 @@ int pty_forward_new(
         if (!f)
                 return -ENOMEM;
 
-        f->read_only = read_only;
-        f->ignore_vhangup = ignore_vhangup;
+        f->flags = flags;
 
         if (event)
                 f->event = sd_event_ref(event);
@@ -325,7 +336,7 @@ int pty_forward_new(
                         return r;
         }
 
-        if (!read_only) {
+        if (!(flags & PTY_FORWARD_READ_ONLY)) {
                 r = fd_nonblock(STDIN_FILENO, true);
                 if (r < 0)
                         return r;
@@ -344,7 +355,7 @@ int pty_forward_new(
         if (ioctl(STDOUT_FILENO, TIOCGWINSZ, &ws) >= 0)
                 (void) ioctl(master, TIOCSWINSZ, &ws);
 
-        if (!read_only) {
+        if (!(flags & PTY_FORWARD_READ_ONLY)) {
                 if (tcgetattr(STDIN_FILENO, &f->saved_stdin_attr) >= 0) {
                         struct termios raw_stdin_attr;
 
@@ -429,16 +440,20 @@ int pty_forward_get_last_char(PTYForward *f, char *ch) {
         return 0;
 }
 
-int pty_forward_set_ignore_vhangup(PTYForward *f, bool ignore_vhangup) {
+int pty_forward_set_ignore_vhangup(PTYForward *f, bool b) {
         int r;
 
         assert(f);
 
-        if (f->ignore_vhangup == ignore_vhangup)
+        if (!!(f->flags & PTY_FORWARD_IGNORE_VHANGUP) == b)
                 return 0;
 
-        f->ignore_vhangup = ignore_vhangup;
-        if (!f->ignore_vhangup) {
+        if (b)
+                f->flags |= PTY_FORWARD_IGNORE_VHANGUP;
+        else
+                f->flags &= ~PTY_FORWARD_IGNORE_VHANGUP;
+
+        if (!ignore_vhangup(f)) {
 
                 /* We shall now react to vhangup()s? Let's check
                  * immediately if we might be in one */
@@ -455,5 +470,5 @@ int pty_forward_set_ignore_vhangup(PTYForward *f, bool ignore_vhangup) {
 int pty_forward_get_ignore_vhangup(PTYForward *f) {
         assert(f);
 
-        return f->ignore_vhangup;
+        return !!(f->flags & PTY_FORWARD_IGNORE_VHANGUP);
 }
diff --git a/src/shared/ptyfwd.h b/src/shared/ptyfwd.h
index 6f84e4036a..9b3214221b 100644
--- a/src/shared/ptyfwd.h
+++ b/src/shared/ptyfwd.h
@@ -27,7 +27,17 @@
 
 typedef struct PTYForward PTYForward;
 
-int pty_forward_new(sd_event *event, int master, bool ignore_vhangup, bool read_only, PTYForward **f);
+typedef enum PTYForwardFlags {
+        PTY_FORWARD_READ_ONLY = 1,
+
+        /* Continue reading after hangup? */
+        PTY_FORWARD_IGNORE_VHANGUP = 2,
+
+        /* Continue reading after hangup but only if we never read anything else? */
+        PTY_FORWARD_IGNORE_INITIAL_VHANGUP = 4,
+} PTYForwardFlags;
+
+int pty_forward_new(sd_event *event, int master, PTYForwardFlags flags, PTYForward **f);
 PTYForward *pty_forward_free(PTYForward *f);
 
 int pty_forward_get_last_char(PTYForward *f, char *ch);