[gvfs] sftp: Don't spin before the SSH process opens the slave pty

[Ross Lagerwall <rossl src gnome org>]

commit 82305d74bdb750ae2dc6f070af85a22ee0e2a76d
Author: Ross Lagerwall <rosslagerwall gmail com>
Date:   Wed Jun 4 21:42:00 2014 +0100

    sftp: Don't spin before the SSH process opens the slave pty
    
    When OpenSSH starts, it closes all its open file descriptors, including
    the slave pty which is used for handling the login.  This would cause
    gvfsd-sftp to spin because the poll() call in handle_login would return
    immediately with POLLHUP.  Open the slave pty in the parent process so
    that poll() doesn't return POLLHUP and spin needlessly.
    
    Once the login has been completed, the slave fd can be closed in the
    parent process (this fixes an fd leak in the BSD openpty codepath).
    
    This change is based on what sshfs does, and is also similar to how the
    BSD openpty codepath behaves.
    
    Although the problem occurs briefly for any login, it is most noticeable
    when trying to connect to a server that drops SSH packets.
    E.g. gvfs-mount sftp://8.8.8.8
    
    https://bugzilla.gnome.org/show_bug.cgi?id=724780

 daemon/gvfsbackendsftp.c |   15 +++++++++++----
 daemon/pty_open.c        |   40 +++++++++++++++++++++++++---------------
 daemon/pty_open.h        |    2 +-
 3 files changed, 37 insertions(+), 20 deletions(-)
---
diff --git a/daemon/gvfsbackendsftp.c b/daemon/gvfsbackendsftp.c
index 2bdba3e..75725e1 100644
--- a/daemon/gvfsbackendsftp.c
+++ b/daemon/gvfsbackendsftp.c
@@ -500,13 +500,14 @@ spawn_ssh (GVfsBackend *backend,
            int *stdin_fd,
            int *stdout_fd,
            int *stderr_fd,
+           int *slave_fd,
            GError **error)
 {
 #ifdef USE_PTY
   *tty_fd = pty_open(pid, PTY_REAP_CHILD, NULL,
                     args[0], args, NULL,
                     300, 300, 
-                    stdin_fd, stdout_fd, stderr_fd);
+                    stdin_fd, stdout_fd, stderr_fd, slave_fd);
   if (*tty_fd == -1)
     {
       g_set_error_literal (error,
@@ -1593,7 +1594,7 @@ do_mount (GVfsBackend *backend,
   GVfsBackendSftp *op_backend = G_VFS_BACKEND_SFTP (backend);
   gchar **args; /* Enough for now, extend if you add more args */
   pid_t pid;
-  int tty_fd, stdout_fd, stdin_fd, stderr_fd;
+  int tty_fd, stdout_fd, stdin_fd, stderr_fd, slave_fd;
   GError *error;
   GInputStream *is;
   GDataOutputStream *command;
@@ -1609,7 +1610,7 @@ do_mount (GVfsBackend *backend,
   error = NULL;
   if (!spawn_ssh (backend,
                  args, &pid,
-                 &tty_fd, &stdin_fd, &stdout_fd, &stderr_fd,
+                 &tty_fd, &stdin_fd, &stdout_fd, &stderr_fd, &slave_fd,
                  &error))
     {
       g_vfs_job_failed_from_error (G_VFS_JOB (job), error);
@@ -1630,7 +1631,13 @@ do_mount (GVfsBackend *backend,
   if (tty_fd == -1)
     res = wait_for_reply (backend, stdout_fd, &error);
   else
-    res = handle_login (backend, mount_source, tty_fd, stdout_fd, stderr_fd, &error);
+    {
+      res = handle_login (backend,
+                          mount_source,
+                          tty_fd, stdout_fd, stderr_fd,
+                          &error);
+      close (slave_fd);
+    }
   
   if (!res)
     {
diff --git a/daemon/pty_open.c b/daemon/pty_open.c
index ca9a4b7..fecd0d1 100644
--- a/daemon/pty_open.c
+++ b/daemon/pty_open.c
@@ -414,7 +414,7 @@ _pty_fork_on_pty_name(const char *path, int parent_fd, char **env_add,
                      const char *command, char **argv,
                      const char *directory,
                      int columns, int rows, 
-                     int *stdin_fd, int *stdout_fd, int *stderr_fd, 
+                     int *stdin_fd, int *stdout_fd, int *stderr_fd, int *slave_fd,
                      pid_t *child, gboolean reapchild, gboolean login)
 {
        int fd, i;
@@ -452,6 +452,14 @@ _pty_fork_on_pty_name(const char *path, int parent_fd, char **env_add,
                goto bail_stderr;
        }
 
+       /* Open the slave PTY in the parent (but not as a controlling terminal)
+        * otherwise later when we want to poll the master fd, POLLHUP is
+        * returned if the process hasn't opened the slave side yet.
+         */
+       *slave_fd = open(path, O_RDWR | O_NOCTTY);
+       if (*slave_fd == -1)
+               goto bail_slavefd;
+
        /* Start up a child. */
        pid = fork();
        switch (pid) {
@@ -470,6 +478,10 @@ _pty_fork_on_pty_name(const char *path, int parent_fd, char **env_add,
                close(stdout_pipe[0]);
                close(stderr_pipe[0]);
 
+               /* Close the slave PTY opened in the parent.  It is later
+                * opened as a controlling terminal. */
+               close (*slave_fd);
+
                if(reapchild) {
                        close(pid_pipe[0]);
 
@@ -595,6 +607,8 @@ _pty_fork_on_pty_name(const char *path, int parent_fd, char **env_add,
        return -1;
 
  bail_fork:
+       close(*slave_fd);
+ bail_slavefd:
        close(stderr_pipe[0]);
        close(stderr_pipe[1]);
  bail_stderr:
@@ -730,7 +744,7 @@ static int
 _pty_open_unix98(pid_t *child, guint flags, char **env_add,
                           const char *command, char **argv,
                           const char *directory, int columns, int rows,
-                          int *stdin_fd, int *stdout_fd, int *stderr_fd)
+                          int *stdin_fd, int *stdout_fd, int *stderr_fd, int *slave_fd)
 {
        int fd;
        char *buf;
@@ -749,7 +763,7 @@ _pty_open_unix98(pid_t *child, guint flags, char **env_add,
                        if (_pty_fork_on_pty_name(buf, fd, env_add, command,
                                                  argv, directory,
                                                  columns, rows,
-                                                 stdin_fd, stdout_fd, stderr_fd, 
+                                                 stdin_fd, stdout_fd, stderr_fd, slave_fd,
                                                  child, 
                                                  flags & PTY_REAP_CHILD, 
                                                  flags & PTY_LOGIN_TTY) != 0) {
@@ -766,9 +780,9 @@ _pty_open_unix98(pid_t *child, guint flags, char **env_add,
 
 static int
 _pty_open_bsd(pid_t *child, const char *command, char **argv,
-             int *stdin_fd, int *stdout_fd, int *stderr_fd)
+             int *stdin_fd, int *stdout_fd, int *stderr_fd, int *slave_fd)
 {
-       int master, slave;
+       int master;
        char **args, *arg;
        int stdin_pipe[2];
        int stdout_pipe[2];
@@ -783,7 +797,7 @@ _pty_open_bsd(pid_t *child, const char *command, char **argv,
        if (pipe(stderr_pipe))
                goto bail_stderr;
 
-       if (openpty(&master, &slave, NULL, NULL, NULL) == -1)
+       if (openpty(&master, slave_fd, NULL, NULL, NULL) == -1)
                return (-1);
 
        switch(pid = fork()) {
@@ -799,7 +813,7 @@ _pty_open_bsd(pid_t *child, const char *command, char **argv,
                close(stderr_pipe[0]);
 
                setsid();
-               if (ioctl(slave, TIOCSCTTY, (char *)NULL) == -1)
+               if (ioctl(*slave_fd, TIOCSCTTY, (char *)NULL) == -1)
                        _exit(0);
 
                /* Set up stdin/out/err */
@@ -835,11 +849,6 @@ _pty_open_bsd(pid_t *child, const char *command, char **argv,
        /*
         * Parent
         */
-
-       /* XXX Don't close the slave pty, it's now the control
-        *     terminal of the child and ssh needs it to authenticate.
-       close(slave);
-        */
        close(stdin_pipe[0]);
        close(stdout_pipe[1]);
        close(stderr_pipe[1]);
@@ -895,16 +904,17 @@ int
 pty_open(pid_t *child, guint flags, char **env_add, 
         const char *command, char **argv, const char *directory,
         int columns, int rows,
-        int *stdin_fd, int *stdout_fd, int *stderr_fd)
+        int *stdin_fd, int *stdout_fd, int *stderr_fd, int *slave_fd)
 {
        int ret = -1;
 
 #if defined(HAVE_UNIX98_PTY)
        ret = _pty_open_unix98(child, flags, env_add, command, argv, directory,
-                              columns, rows, stdin_fd, stdout_fd, stderr_fd);
+                              columns, rows, stdin_fd, stdout_fd, stderr_fd,
+                              slave_fd);
 #elif defined(HAVE_OPENPTY)
        ret = _pty_open_bsd(child, command, argv,
-                           stdin_fd, stdout_fd, stderr_fd);
+                           stdin_fd, stdout_fd, stderr_fd, slave_fd);
 #else
 #error Have neither UNIX98 PTY nor BSD openpty!
 #endif
diff --git a/daemon/pty_open.h b/daemon/pty_open.h
index 1ed4724..5c6241d 100644
--- a/daemon/pty_open.h
+++ b/daemon/pty_open.h
@@ -57,7 +57,7 @@ enum {
 int pty_open(pid_t *child, guint flags, char **env_add,
             const char *command, char **argv, const char *directory,
             int columns, int rows,
-            int *stdin_fd, int *stdout_fd, int *stderr_fd);
+            int *stdin_fd, int *stdout_fd, int *stderr_fd, int *slave_fd);
 int pty_get_size(int master, int *columns, int *rows);
 
 G_END_DECLS