Merge lp:~maxb/bzr/2.2-close-ssh-subprocess-socket into lp:bzr/2.2

[Max Bowsher]

Close leaked socket to SSH subprocesses

This change fixes a bug introduced in 2.2b4 when bzr was made to use a socketpair to talk to ssh subprocesses if supported on the platform. The resultant socket was never actually closed - I've not observed this breaking anything in bzr, but it causes dput sftp uploads (which use bzrlib) to hang on completion.

[Andrew Bennetts (spiv)]

The code looks reasonable. I wonder if to be pedantically correct we should also be attempting to close the other side of the socketpair too, in case the subprocess doesn't close it for us? That would mean SubprocessVendor._connect should subproc_sock into SSHSubprocessConnection so it can pass it along to your improved _close_ssh_proc function with the other half. This may actually explain some test suite problems — I think this may actually be an fd leak. So please extend your patch to do this.

That brings me to my other suggestion: I think we should strive to write at least one direct unit test for this code. It's now had multiple bugs, and I think it's proven that the indirect test coverage so far isn't enough. Off the top of my head a test that constructs a SSHSubprocessConnection directly may be a reasonable way to do it, but perhaps instantiating a custom SubprocessVendor would be better? So if you can think of a way to write some direct tests for the fd/process management in ssh.py, please do. I won't block this patch for lack of tests as it's a clear improvement, but I'd love to see some :)

Finally, add a NEWS entry! :)

review: Approve

[Max Bowsher (maxb)]

NEWS entry added.

Yes, for full correctness we would close the other end of the socketpair within bzrlib - but no need to defer that to _close_ssh_proc time, we can close it as soon as we have handed it off to the subprocess - done.

As for testing, I can't see any meaningful way to test this short of spinning up a test ssh server and trying to put a file across a transport. Unfortunately that's a outside my knowledge of how to achieve it, without a fairly large tangent of research, right now.

[Andrew Bennetts (spiv)]

Max Bowsher wrote:
> NEWS entry added.
>
> Yes, for full correctness we would close the other end of the socketpair
> within bzrlib - but no need to defer that to _close_ssh_proc time, we can
> close it as soon as we have handed it off to the subprocess - done.

Great, thank you!

> As for testing, I can't see any meaningful way to test this short of spinning
> up a test ssh server and trying to put a file across a transport.
> Unfortunately that's a outside my knowledge of how to achieve it, without a
> fairly large tangent of research, right now.

That's a broader test than I was thinking of — we don't need to run SSH, any
subprocess would do (as an extreme the test might substitute some sort of Test
Double, http://www.martinfowler.com/bliki/TestDouble.html, instead of a
subprocess.Popen instance, if you wanted to unit test SSHSubprocessConnection
and/or _close_ssh_proc in isolation from SubprocessVendor). /bin/echo would
work, for instance (where available).

So maybe something like:

    def fd_count(self):
        return len(os.listdir('/proc/self/fd'))

    def test_ssh_subprocess_fd_closing(self):
        class EchoSubprocessVendor(SubprocessVendor):
            def _get_vendor_specific_argv(self, *args):
                return ['/bin/echo']
        vendor = EchoSubprocessVendor()
        initial_fd_count = self.fd_count()
        ssh_conn = vendor.connect_ssh(
            'dummy user', 'dummy password', 'dummy host', 1, 'dummy command')
        ssh_conn.close()
        self.assertEqual(initial_fd_count, self.fd_count())

Perhaps that's a bit too hacky (it would obviously need some guards to skip on
platforms without /bin/echo and /proc/self/fd), but that's the sort of thing I
had in mind. You could also perhaps just try asserting directly that
ssh_conn's proc is terminated and _sock is closed, etc. But certainly I'm
thinking of tests entirely under the transport layer.

-Andrew.

[Max Bowsher (maxb)]

Counting fds by means of /proc doesn't feel very nice to me. Does it risk creating issues with attempts to run tests in parallel, or can we guarantee that any such attempt will always be in forked processes? It's also problematically platform-specific, especially when testing code which already has two platform-specific codepaths (with/without socketpair support). I think it would be enough to just run a python subprocess that read stdin until eof, then exited, and let the test merely confirm that this happens without hanging or raising.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/9/2010 6:38 PM, Max Bowsher wrote:
> Max Bowsher has proposed merging lp:~maxb/bzr/2.2-close-ssh-subprocess-socket into lp:bzr/2.2.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #659590 sftp upload hangs after all files successfully uploaded
> https://bugs.launchpad.net/bugs/659590
>
>
> This change fixes a bug introduced in 2.2b4 when bzr was made to use a socketpair to talk to ssh subprocesses if supported on the platform. The resultant socket was never actually closed - I've not observed this breaking anything in bzr, but it causes dput sftp uploads (which use bzrlib) to hang on completion.

 review: approve

This looks good to me.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkza2WgACgkQJdeBCYSNAAPezgCfUCcNnF0ncxH7T0wieulAcX0k
MmcAoJx+DsepUUfF2GflA00FL4sxVWTC
=XYc0
-----END PGP SIGNATURE-----

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-11-08 06:10:41 +0000
+++ NEWS	2010-11-10 08:46:44 +0000
@@ -49,6 +49,9 @@
   installed.
   (Martin [gz], Gary van der Merwe, #632465)
 
+* Close leaked socket to SSH subprocesses, which caused dput sftp uploads
+  to hang.  (Max Bowsher, #659590)
+
 Improvements
 ************
 

=== modified file 'bzrlib/transport/ssh.py'
--- bzrlib/transport/ssh.py	2010-09-09 07:31:02 +0000
+++ bzrlib/transport/ssh.py	2010-11-10 08:46:44 +0000
@@ -361,13 +361,14 @@
             # This platform doesn't support socketpair(), so just use ordinary
             # pipes instead.
             stdin = stdout = subprocess.PIPE
-            sock = None
+            my_sock, subproc_sock = None, None
         else:
             stdin = stdout = subproc_sock
-            sock = my_sock
         proc = subprocess.Popen(argv, stdin=stdin, stdout=stdout,
                                 **os_specific_subprocess_params())
-        return SSHSubprocessConnection(proc, sock=sock)
+        if subproc_sock is not None:
+            subproc_sock.close()
+        return SSHSubprocessConnection(proc, sock=my_sock)
 
     def connect_sftp(self, username, password, host, port):
         try:
@@ -644,25 +645,24 @@
 import weakref
 _subproc_weakrefs = set()
 
-def _close_ssh_proc(proc):
+def _close_ssh_proc(proc, sock):
     """Carefully close stdin/stdout and reap the SSH process.
 
     If the pipes are already closed and/or the process has already been
     wait()ed on, that's ok, and no error is raised.  The goal is to do our best
     to clean up (whether or not a clean up was already tried).
     """
-    dotted_names = ['stdin.close', 'stdout.close', 'wait']
-    for dotted_name in dotted_names:
-        attrs = dotted_name.split('.')
-        try:
-            obj = proc
-            for attr in attrs:
-                obj = getattr(obj, attr)
-        except AttributeError:
-            # It's ok for proc.stdin or proc.stdout to be None.
-            continue
-        try:
-            obj()
+    funcs = []
+    for closeable in (proc.stdin, proc.stdout, sock):
+        # We expect that either proc (a subprocess.Popen) will have stdin and
+        # stdout streams to close, or that we will have been passed a socket to
+        # close, with the option not in use being None.
+        if closeable is not None:
+            funcs.append(closeable.close)
+    funcs.append(proc.wait)
+    for func in funcs:
+        try:
+            func()
         except OSError:
             # It's ok for the pipe to already be closed, or the process to
             # already be finished.
@@ -707,7 +707,7 @@
         # to avoid leaving processes lingering indefinitely.
         def terminate(ref):
             _subproc_weakrefs.remove(ref)
-            _close_ssh_proc(proc)
+            _close_ssh_proc(proc, sock)
         _subproc_weakrefs.add(weakref.ref(self, terminate))
 
     def send(self, data):
@@ -723,7 +723,7 @@
             return os.read(self.proc.stdout.fileno(), count)
 
     def close(self):
-        _close_ssh_proc(self.proc)
+        _close_ssh_proc(self.proc, self._sock)
 
     def get_sock_or_pipes(self):
         if self._sock is not None: