From 9016eb58e51104c3836adb93a459b5716bc16f1d Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Wed, 15 Nov 2017 12:29:21 +0200 Subject: [PATCH 1/4] Implement bpo-29406: Prevent SSL socket leaking in asyncio --- Lib/asyncio/sslproto.py | 22 +++++++++++- Lib/test/test_asyncio/test_sslproto.py | 34 +++++++++++++++++++ Misc/ACKS | 1 + .../2017-11-14-19-15-23.bpo-29406.6DeUWP.rst | 2 ++ 4 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Library/2017-11-14-19-15-23.bpo-29406.6DeUWP.rst diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py index 53e6d2b667b12c..85885533c85ace 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -6,6 +6,7 @@ ssl = None from . import base_events +from . import futures from . import protocols from . import transports from .log import logger @@ -406,7 +407,7 @@ class SSLProtocol(protocols.Protocol): def __init__(self, loop, app_protocol, sslcontext, waiter, server_side=False, server_hostname=None, - call_connection_made=True): + call_connection_made=True, shutdown_timeout=5.0): if ssl is None: raise RuntimeError('stdlib ssl module not available') @@ -436,6 +437,8 @@ def __init__(self, loop, app_protocol, sslcontext, waiter, self._session_established = False self._in_handshake = False self._in_shutdown = False + self._shutdown_timeout = shutdown_timeout + self._shutdown_timeout_handle = None # transport, ex: SelectorSocketTransport self._transport = None self._call_connection_made = call_connection_made @@ -550,6 +553,15 @@ def _start_shutdown(self): self._in_shutdown = True self._write_appdata(b'') + if self._shutdown_timeout is not None: + self._shutdown_timeout_handle = self._loop.call_later( + self._shutdown_timeout, self._on_shutdown_timeout) + + def _on_shutdown_timeout(self): + if self._transport is not None: + self._fatal_error( + futures.TimeoutError(), 'Can not complete shitdown operation') + def _write_appdata(self, data): self._write_backlog.append((data, 0)) self._write_buffer_size += len(data) @@ -677,12 +689,20 @@ def _fatal_error(self, exc, message='Fatal error on transport'): }) if self._transport: self._transport._force_close(exc) + self._transport = None + + if self._shutdown_timeout_handle is not None: + self._shutdown_timeout_handle.cancel() + self._shutdown_timeout_handle = None def _finalize(self): self._sslpipe = None if self._transport is not None: self._transport.close() + if self._shutdown_timeout_handle is not None: + self._shutdown_timeout_handle.cancel() + self._shutdown_timeout_handle = None def _abort(self): try: diff --git a/Lib/test/test_asyncio/test_sslproto.py b/Lib/test/test_asyncio/test_sslproto.py index f573ae8fe779e7..600722720e08fc 100644 --- a/Lib/test/test_asyncio/test_sslproto.py +++ b/Lib/test/test_asyncio/test_sslproto.py @@ -96,6 +96,40 @@ def test_connection_lost(self): test_utils.run_briefly(self.loop) self.assertIsInstance(waiter.exception(), ConnectionAbortedError) + def test_close_abort(self): + # From issue #bpo-29406 + # abort connection if server does not complete shutdown procedure + ssl_proto = self.ssl_protocol() + transport = self.connection_made(ssl_proto) + ssl_proto._on_handshake_complete(None) + ssl_proto._start_shutdown() + self.assertIsNotNone(ssl_proto._shutdown_timeout_handle) + + exc_handler = mock.Mock() + self.loop.set_exception_handler(exc_handler) + ssl_proto._shutdown_timeout_handle._run() + + exc_handler.assert_called_with( + self.loop, {'message': 'Can not complete shitdown operation', + 'exception': mock.ANY, + 'transport': transport, + 'protocol': ssl_proto} + ) + self.assertIsNone(ssl_proto._shutdown_timeout_handle) + + def test_close(self): + # From issue #bpo-29406 + # abort connection if server does not complete shutdown procedure + ssl_proto = self.ssl_protocol() + transport = self.connection_made(ssl_proto) + ssl_proto._on_handshake_complete(None) + ssl_proto._start_shutdown() + self.assertIsNotNone(ssl_proto._shutdown_timeout_handle) + + ssl_proto._finalize() + self.assertIsNone(ssl_proto._transport) + self.assertIsNone(ssl_proto._shutdown_timeout_handle) + def test_close_during_handshake(self): # bpo-29743 Closing transport during handshake process leaks socket waiter = asyncio.Future(loop=self.loop) diff --git a/Misc/ACKS b/Misc/ACKS index 05113903f06de7..022bad47c59c12 100644 --- a/Misc/ACKS +++ b/Misc/ACKS @@ -1785,3 +1785,4 @@ Jelle Zijlstra Gennadiy Zlobin Doug Zongker Peter Åstrand +Nikolay Kim diff --git a/Misc/NEWS.d/next/Library/2017-11-14-19-15-23.bpo-29406.6DeUWP.rst b/Misc/NEWS.d/next/Library/2017-11-14-19-15-23.bpo-29406.6DeUWP.rst new file mode 100644 index 00000000000000..fe5273804bedca --- /dev/null +++ b/Misc/NEWS.d/next/Library/2017-11-14-19-15-23.bpo-29406.6DeUWP.rst @@ -0,0 +1,2 @@ +asyncio SSL contexts leak sockets after calling close with certain servers. +Patch by Nikolay Kim From bdfb6530ef91adbb81799409b34da37be845844a Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Wed, 15 Nov 2017 12:43:47 +0200 Subject: [PATCH 2/4] Fix typo --- Lib/asyncio/sslproto.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py index 85885533c85ace..a63f51709f2799 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -560,7 +560,7 @@ def _start_shutdown(self): def _on_shutdown_timeout(self): if self._transport is not None: self._fatal_error( - futures.TimeoutError(), 'Can not complete shitdown operation') + futures.TimeoutError(), 'Can not complete shutdown operation') def _write_appdata(self, data): self._write_backlog.append((data, 0)) From a6b9cad2ace5ad3fb8500dcdbe0e326909074034 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Wed, 15 Nov 2017 13:19:03 +0200 Subject: [PATCH 3/4] Cleanup transport --- Lib/asyncio/sslproto.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py index a63f51709f2799..dd8c1bc8cdbabf 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -700,6 +700,7 @@ def _finalize(self): if self._transport is not None: self._transport.close() + self._transport = None if self._shutdown_timeout_handle is not None: self._shutdown_timeout_handle.cancel() self._shutdown_timeout_handle = None From 5b2ee640929d26ee6a197d2a31d44e8b6ca92a35 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Wed, 15 Nov 2017 15:10:52 +0200 Subject: [PATCH 4/4] Fix typo --- Lib/test/test_asyncio/test_sslproto.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_asyncio/test_sslproto.py b/Lib/test/test_asyncio/test_sslproto.py index 600722720e08fc..a7f71085a9c3a0 100644 --- a/Lib/test/test_asyncio/test_sslproto.py +++ b/Lib/test/test_asyncio/test_sslproto.py @@ -110,7 +110,7 @@ def test_close_abort(self): ssl_proto._shutdown_timeout_handle._run() exc_handler.assert_called_with( - self.loop, {'message': 'Can not complete shitdown operation', + self.loop, {'message': 'Can not complete shutdown operation', 'exception': mock.ANY, 'transport': transport, 'protocol': ssl_proto}