From caa842bf5d5b59002e8277f55ccd29c531aea08e Mon Sep 17 00:00:00 2001 From: Krzysztof Rusek Date: Thu, 11 Feb 2016 16:51:19 +0100 Subject: Issue #537 reproduction test and fix --- paramiko/buffered_pipe.py | 14 +++++++++----- tests/test_transport.py | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/paramiko/buffered_pipe.py b/paramiko/buffered_pipe.py index d5fe164e..e81e9b3d 100644 --- a/paramiko/buffered_pipe.py +++ b/paramiko/buffered_pipe.py @@ -70,11 +70,15 @@ class BufferedPipe (object): :param threading.Event event: the event to set/clear """ - self._event = event - if len(self._buffer) > 0: - event.set() - else: - event.clear() + self._lock.acquire() + try: + self._event = event + if self._closed or len(self._buffer) > 0: + event.set() + else: + event.clear() + finally: + self._lock.release() def feed(self, data): """ diff --git a/tests/test_transport.py b/tests/test_transport.py index 5069e5b0..d81ad8f3 100644 --- a/tests/test_transport.py +++ b/tests/test_transport.py @@ -828,3 +828,21 @@ class TransportTest(unittest.TestCase): hostkey=public_host_key, username='slowdive', password='pygmalion') + + def test_M_select_after_close(self): + """ + verify that select works when a channel is already closed. + """ + self.setup_test_server() + chan = self.tc.open_session() + chan.invoke_shell() + schan = self.ts.accept(1.0) + schan.close() + + # give client a moment to receive close notification + time.sleep(0.1) + + r, w, e = select.select([chan], [], [], 0.1) + self.assertEqual([chan], r) + self.assertEqual([], w) + self.assertEqual([], e) -- cgit v1.2.3 From f1994dc8da8f30e383508f2c001065e2e2741a91 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Thu, 9 Jun 2016 10:39:53 -0700 Subject: Add explicit commentary re #537 in BufferedPipe.set_event --- paramiko/buffered_pipe.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/paramiko/buffered_pipe.py b/paramiko/buffered_pipe.py index e81e9b3d..605f51e9 100644 --- a/paramiko/buffered_pipe.py +++ b/paramiko/buffered_pipe.py @@ -73,6 +73,11 @@ class BufferedPipe (object): self._lock.acquire() try: self._event = event + # Make sure the event starts in `set` state if we appear to already + # be closed; otherwise, if we start in `clear` state & are closed, + # nothing will ever call `.feed` and the event (& OS pipe, if we're + # wrapping one - see `Channel.fileno`) will permanently stay in + # `clear`, causing deadlock if e.g. `select`ed upon. if self._closed or len(self._buffer) > 0: event.set() else: -- cgit v1.2.3 From e0a458847beefa85959742f92faaea9830155255 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Thu, 9 Jun 2016 10:47:28 -0700 Subject: Changelog re #537 --- sites/www/changelog.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index 225e5778..9fb7d20c 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -2,6 +2,13 @@ Changelog ========= +* :bug:`537` Fix a bug in `BufferedPipe.set_event + ` which could cause + deadlocks/hangs when one uses `select.select` against + `~paramiko.channel.Channel` objects (or otherwise calls `Channel.fileno + ` after the channel has closed). Thanks to + Przemysław Strzelczak for the report & reproduction case, and to Krzysztof + Rusek for the fix. * :release:`1.16.1 <2016-04-28>` * :release:`1.15.5 <2016-04-28>` * :bug:`670` Due to an earlier bugfix, less-specific ``Host`` blocks' -- cgit v1.2.3 From 20aa3230e32a5018cb5b4245eab1af19dd28acf7 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Thu, 9 Jun 2016 21:02:09 -0700 Subject: Update tasks to use Invoke/Invocations 0.13 --- dev-requirements.txt | 4 ++-- tasks.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dev-requirements.txt b/dev-requirements.txt index 0c1fc020..2547fb5f 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,8 +1,8 @@ # Older junk tox>=1.4,<1.5 # For newer tasks like building Sphinx docs. -invoke>=0.11.1,<2.0 -invocations>=0.11.0,<2.0 +invoke>=0.13,<2.0 +invocations>=0.13,<2.0 sphinx>=1.1.3,<2.0 alabaster>=0.7.5,<2.0 releases>=1.1.0,<2.0 diff --git a/tasks.py b/tasks.py index 9e8c56d2..90ee758c 100644 --- a/tasks.py +++ b/tasks.py @@ -2,7 +2,7 @@ from os import mkdir from os.path import join from shutil import rmtree, copytree -from invoke import Collection, ctask as task +from invoke import Collection, task from invocations.docs import docs, www, sites from invocations.packaging import publish -- cgit v1.2.3