From 5b6059c4bd1b75121023b50003f5191345ef789c Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Wed, 12 Feb 2014 09:48:04 -0800 Subject: [PATCH 01/11] Nuke old merge mistake (or what looks like one anyway.) --- paramiko/transport.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/paramiko/transport.py b/paramiko/transport.py index 6fb3797..6aae083 100644 --- a/paramiko/transport.py +++ b/paramiko/transport.py @@ -1539,10 +1539,6 @@ class Transport (threading.Thread): # containers. Random.atfork() - # Hold reference to 'sys' so we can test sys.modules to detect - # interpreter shutdown. - self.sys = sys - # active=True occurs before the thread is launched, to avoid a race _active_threads.append(self) if self.server_mode: From 58489c893e3f62947ee8235c2a07fc5465949aeb Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Wed, 12 Feb 2014 17:16:34 -0800 Subject: [PATCH 02/11] Potentially horrible attempt at manual subprocess-read timeouts --- paramiko/proxy.py | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/paramiko/proxy.py b/paramiko/proxy.py index 218b76e..abdd157 100644 --- a/paramiko/proxy.py +++ b/paramiko/proxy.py @@ -20,10 +20,13 @@ L{ProxyCommand}. """ +from datetime import datetime import os from shlex import split as shlsplit import signal from subprocess import Popen, PIPE +from select import select +import socket from paramiko.ssh_exception import ProxyCommandFailure @@ -48,6 +51,7 @@ class ProxyCommand(object): """ self.cmd = shlsplit(command_line) self.process = Popen(self.cmd, stdin=PIPE, stdout=PIPE, stderr=PIPE) + self.timeout = None def send(self, content): """ @@ -78,7 +82,25 @@ class ProxyCommand(object): @rtype: int """ try: - return os.read(self.process.stdout.fileno(), size) + start = datetime.now() + read = [] + while len(read) < size: + if self.timeout is not None: + elapsed = (datetime.now() - start).microseconds + timeout = self.timeout * 1000 * 1000 # to microseconds + # Unsure why the 'default' timeout is too short here - + # causes us to raise before e.g. the SSH banner is read, + # probably generally awful for slow connections. + # Try inflating it some. + timeout = timeout * 2 + if elapsed >= timeout: + raise socket.timeout() + r, w, x = select([self.process.stdout], [], [], 0.0) + if r and r[0] == self.process.stdout: + b = os.read(self.process.stdout.fileno(), 1) + read.append(b) + result = ''.join(read) + return result except IOError, e: raise BadProxyCommand(' '.join(self.cmd), e.strerror) @@ -86,6 +108,4 @@ class ProxyCommand(object): os.kill(self.process.pid, signal.SIGTERM) def settimeout(self, timeout): - # Timeouts are meaningless for this implementation, but are part of the - # spec, so must be present. - pass + self.timeout = timeout From cfd1efe64878912964797e1a6f2303387f7e1848 Mon Sep 17 00:00:00 2001 From: Marius Gedminas Date: Thu, 13 Feb 2014 09:47:13 +0200 Subject: [PATCH 03/11] Fix NameError in error-handling case Fixes #268 --- paramiko/proxy.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/paramiko/proxy.py b/paramiko/proxy.py index abdd157..a24fbb2 100644 --- a/paramiko/proxy.py +++ b/paramiko/proxy.py @@ -68,7 +68,7 @@ class ProxyCommand(object): # died and we can't proceed. The best option here is to # raise an exception informing the user that the informed # ProxyCommand is not working. - raise BadProxyCommand(' '.join(self.cmd), e.strerror) + raise ProxyCommandFailure(' '.join(self.cmd), e.strerror) return len(content) def recv(self, size): @@ -102,7 +102,7 @@ class ProxyCommand(object): result = ''.join(read) return result except IOError, e: - raise BadProxyCommand(' '.join(self.cmd), e.strerror) + raise ProxyCommandFailure(' '.join(self.cmd), e.strerror) def close(self): os.kill(self.process.pid, signal.SIGTERM) From 0c493541ebc817d187c87a4a7cf81a8e90c72f5c Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Thu, 13 Feb 2014 09:40:53 -0800 Subject: [PATCH 04/11] Changelog re #268 --- sites/www/changelog.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index 1c533f9..2c479b6 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -2,6 +2,8 @@ Changelog ========= +* :bug:`268` Fix some missed renames of `ProxyCommand` related error classes. + Thanks to Marius Gedminas for catch & patch. * :bug:`34` (PR :issue:`35`) Fix SFTP prefetching incompatibility with some SFTP servers regarding request/response ordering. Thanks to Richard Kettlewell for catch & patch. From c3eb903ed35a2ddd3387d495e844efb2d536cc01 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Thu, 13 Feb 2014 09:48:58 -0800 Subject: [PATCH 05/11] Preliminary changelog entry re #252 --- sites/www/changelog.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index 2c479b6..08a459c 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -2,6 +2,11 @@ Changelog ========= +* :bug:`252` (`Fabric #1020 `_) + Tried enhancing the implementation of `ProxyCommand` to avoid a deadlock/hang + condition that frequently occurs at `Transport` shutdown time. Thanks to + Mateusz Kobos, Matthijs van der Vleuten and Guillaume Zitta for the original + reports and to Marius Gedminas for helping test nontrivial test cases. * :bug:`268` Fix some missed renames of `ProxyCommand` related error classes. Thanks to Marius Gedminas for catch & patch. * :bug:`34` (PR :issue:`35`) Fix SFTP prefetching incompatibility with some From 8003c738ca3ea567bfec876bcbf3e7c6facae6a6 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Thu, 13 Feb 2014 09:50:05 -0800 Subject: [PATCH 06/11] I hate you sometimes, ReST --- sites/www/changelog.rst | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index 08a459c..5034641 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -3,11 +3,12 @@ Changelog ========= * :bug:`252` (`Fabric #1020 `_) - Tried enhancing the implementation of `ProxyCommand` to avoid a deadlock/hang - condition that frequently occurs at `Transport` shutdown time. Thanks to - Mateusz Kobos, Matthijs van der Vleuten and Guillaume Zitta for the original - reports and to Marius Gedminas for helping test nontrivial test cases. -* :bug:`268` Fix some missed renames of `ProxyCommand` related error classes. + Tried enhancing the implementation of ``ProxyCommand`` to avoid a + deadlock/hang condition that frequently occurs at ``Transport`` shutdown + time. Thanks to Mateusz Kobos, Matthijs van der Vleuten and Guillaume Zitta + for the original reports and to Marius Gedminas for helping test nontrivial + test cases. +* :bug:`268` Fix some missed renames of ``ProxyCommand`` related error classes. Thanks to Marius Gedminas for catch & patch. * :bug:`34` (PR :issue:`35`) Fix SFTP prefetching incompatibility with some SFTP servers regarding request/response ordering. Thanks to Richard From 244e09f57a6670fbdda9979882d81b1d7e40b75c Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Thu, 13 Feb 2014 12:54:23 -0800 Subject: [PATCH 07/11] Slightly safer socket.error handling --- paramiko/transport.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/paramiko/transport.py b/paramiko/transport.py index 6aae083..990c9d3 100644 --- a/paramiko/transport.py +++ b/paramiko/transport.py @@ -1608,7 +1608,10 @@ class Transport (threading.Thread): self.saved_exception = e except socket.error, e: if type(e.args) is tuple: - emsg = '%s (%d)' % (e.args[1], e.args[0]) + if e.args: + emsg = '%s (%d)' % (e.args[1], e.args[0]) + else: # empty tuple, e.g. socket.timeout + emsg = str(e) or repr(e) else: emsg = e.args self._log(ERROR, 'Socket exception: ' + emsg) From 4402f67fa61c1c43a838d4414e42ad8047876ebf Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Thu, 13 Feb 2014 13:44:27 -0800 Subject: [PATCH 08/11] Don't drop/lose data when our inner timeout goges off. --- paramiko/proxy.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/paramiko/proxy.py b/paramiko/proxy.py index a24fbb2..722590c 100644 --- a/paramiko/proxy.py +++ b/paramiko/proxy.py @@ -52,6 +52,7 @@ class ProxyCommand(object): self.cmd = shlsplit(command_line) self.process = Popen(self.cmd, stdin=PIPE, stdout=PIPE, stderr=PIPE) self.timeout = None + self.buffer = [] def send(self, content): """ @@ -83,23 +84,21 @@ class ProxyCommand(object): """ try: start = datetime.now() - read = [] - while len(read) < size: + while len(self.buffer) < size: if self.timeout is not None: elapsed = (datetime.now() - start).microseconds timeout = self.timeout * 1000 * 1000 # to microseconds - # Unsure why the 'default' timeout is too short here - - # causes us to raise before e.g. the SSH banner is read, - # probably generally awful for slow connections. - # Try inflating it some. - timeout = timeout * 2 if elapsed >= timeout: raise socket.timeout() r, w, x = select([self.process.stdout], [], [], 0.0) if r and r[0] == self.process.stdout: b = os.read(self.process.stdout.fileno(), 1) - read.append(b) - result = ''.join(read) + # Store in class-level buffer for persistence across + # timeouts; this makes us act more like a real socket + # (where timeouts don't actually drop data.) + self.buffer.append(b) + result = ''.join(self.buffer) + self.buffer = [] return result except IOError, e: raise ProxyCommandFailure(' '.join(self.cmd), e.strerror) From d438ff6b64f1c20b78fc1824bacba9659653ecd1 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Thu, 13 Feb 2014 13:44:46 -0800 Subject: [PATCH 09/11] Don't raise timeouts as ProxyCommand failures, thanks @mgedmin --- paramiko/proxy.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/paramiko/proxy.py b/paramiko/proxy.py index 722590c..3f5c8bb 100644 --- a/paramiko/proxy.py +++ b/paramiko/proxy.py @@ -100,6 +100,8 @@ class ProxyCommand(object): result = ''.join(self.buffer) self.buffer = [] return result + except socket.timeout: + raise # socket.timeout is a subclass of IOError except IOError, e: raise ProxyCommandFailure(' '.join(self.cmd), e.strerror) From e3a16fac5af103c48ee64034cfca36f6c8c1dfd1 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Thu, 13 Feb 2014 13:48:21 -0800 Subject: [PATCH 10/11] Braino in changelog text --- sites/www/changelog.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index 5034641..451d695 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -7,7 +7,7 @@ Changelog deadlock/hang condition that frequently occurs at ``Transport`` shutdown time. Thanks to Mateusz Kobos, Matthijs van der Vleuten and Guillaume Zitta for the original reports and to Marius Gedminas for helping test nontrivial - test cases. + use cases. * :bug:`268` Fix some missed renames of ``ProxyCommand`` related error classes. Thanks to Marius Gedminas for catch & patch. * :bug:`34` (PR :issue:`35`) Fix SFTP prefetching incompatibility with some From fb772a8695a95df3aa1cd3dd45aa6c42302e8640 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Fri, 14 Feb 2014 09:16:35 -0800 Subject: [PATCH 11/11] Tweak changelog language --- sites/www/changelog.rst | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index 451d695..2faa964 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -3,11 +3,10 @@ Changelog ========= * :bug:`252` (`Fabric #1020 `_) - Tried enhancing the implementation of ``ProxyCommand`` to avoid a - deadlock/hang condition that frequently occurs at ``Transport`` shutdown - time. Thanks to Mateusz Kobos, Matthijs van der Vleuten and Guillaume Zitta - for the original reports and to Marius Gedminas for helping test nontrivial - use cases. + Enhanced the implementation of ``ProxyCommand`` to avoid a deadlock/hang + condition that frequently occurs at ``Transport`` shutdown time. Thanks to + Mateusz Kobos, Matthijs van der Vleuten and Guillaume Zitta for the original + reports and to Marius Gedminas for helping test nontrivial use cases. * :bug:`268` Fix some missed renames of ``ProxyCommand`` related error classes. Thanks to Marius Gedminas for catch & patch. * :bug:`34` (PR :issue:`35`) Fix SFTP prefetching incompatibility with some