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

3 files changed, 37 insertions, 20 deletions

diff --git a/daemon/gvfsbackendsftp.c b/daemon/gvfsbackendsftp.c
index 2bdba3ed..75725e1c 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 ca9a4b7e..fecd0d10 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 1ed47242..5c6241df 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