From 14d84d7ba002383f37d007d5b0e03174a2a230f4 Mon Sep 17 00:00:00 2001 From: Fantix King Date: Fri, 1 Feb 2019 20:19:30 -0600 Subject: [PATCH 1/4] Fix handshake timeout leak in asyncio/sslproto Refs MagicStack/uvloop#222 --- Lib/asyncio/sslproto.py | 2 ++ Lib/test/test_asyncio/test_sslproto.py | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py index 42785609dcd2c0..4aab75a092a7d2 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -498,6 +498,8 @@ def connection_lost(self, exc): self._app_transport._closed = True self._transport = None self._app_transport = None + if getattr(self, '_handshake_timeout_handle', None): + self._handshake_timeout_handle.cancel() self._wakeup_waiter(exc) def pause_writing(self): diff --git a/Lib/test/test_asyncio/test_sslproto.py b/Lib/test/test_asyncio/test_sslproto.py index 19b7a4366b2e82..45df903a406b66 100644 --- a/Lib/test/test_asyncio/test_sslproto.py +++ b/Lib/test/test_asyncio/test_sslproto.py @@ -4,6 +4,7 @@ import socket import sys import unittest +import weakref from unittest import mock try: import ssl @@ -562,6 +563,11 @@ async def client(addr): # exception or log an error, even if the handshake failed self.assertEqual(messages, []) + # The 10s handshake timeout should be cancelled to free related + # objects without really waiting for 10s + client_sslctx = weakref.ref(client_sslctx) + self.assertIsNone(client_sslctx()) + def test_create_connection_ssl_slow_handshake(self): client_sslctx = test_utils.simple_client_sslcontext() From 78b34521772e5e3aafb9a1ae15c205609041cc46 Mon Sep 17 00:00:00 2001 From: Fantix King Date: Fri, 22 Feb 2019 19:12:50 -0600 Subject: [PATCH 2/4] Break circular ref _SSLPipe <-> SSLProtocol --- Lib/asyncio/sslproto.py | 1 + Lib/test/test_asyncio/test_sslproto.py | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py index 4aab75a092a7d2..b4703e3c33a517 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -501,6 +501,7 @@ def connection_lost(self, exc): if getattr(self, '_handshake_timeout_handle', None): self._handshake_timeout_handle.cancel() self._wakeup_waiter(exc) + self._sslpipe = None def pause_writing(self): """Called when the low-level transport's buffer goes over diff --git a/Lib/test/test_asyncio/test_sslproto.py b/Lib/test/test_asyncio/test_sslproto.py index 45df903a406b66..c6670338cb4143 100644 --- a/Lib/test/test_asyncio/test_sslproto.py +++ b/Lib/test/test_asyncio/test_sslproto.py @@ -275,6 +275,10 @@ async def client(addr): self.loop.run_until_complete( asyncio.wait_for(client(srv.addr), timeout=10)) + # No garbage is left if SSL is closed uncleanly + client_context = weakref.ref(client_context) + self.assertIsNone(client_context()) + def test_start_tls_client_buf_proto_1(self): HELLO_MSG = b'1' * self.PAYLOAD_SIZE From 2e67f0dc91b925292944009214f6d028c4674c40 Mon Sep 17 00:00:00 2001 From: Fantix King Date: Fri, 22 Feb 2019 19:38:21 -0600 Subject: [PATCH 3/4] bpo-34745: Fix asyncio ssl memory leak * Break circular ref SSLProtocol <-> UserProtocol --- Lib/asyncio/sslproto.py | 1 + Lib/test/test_asyncio/test_sslproto.py | 62 ++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py index b4703e3c33a517..97a6fc66a92735 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -501,6 +501,7 @@ def connection_lost(self, exc): if getattr(self, '_handshake_timeout_handle', None): self._handshake_timeout_handle.cancel() self._wakeup_waiter(exc) + self._app_protocol = None self._sslpipe = None def pause_writing(self): diff --git a/Lib/test/test_asyncio/test_sslproto.py b/Lib/test/test_asyncio/test_sslproto.py index c6670338cb4143..7bc2ccf0bddcd1 100644 --- a/Lib/test/test_asyncio/test_sslproto.py +++ b/Lib/test/test_asyncio/test_sslproto.py @@ -279,6 +279,68 @@ async def client(addr): client_context = weakref.ref(client_context) self.assertIsNone(client_context()) + def test_create_connection_memory_leak(self): + HELLO_MSG = b'1' * self.PAYLOAD_SIZE + + server_context = test_utils.simple_server_sslcontext() + client_context = test_utils.simple_client_sslcontext() + + def serve(sock): + sock.settimeout(self.TIMEOUT) + + sock.start_tls(server_context, server_side=True) + + sock.sendall(b'O') + data = sock.recv_all(len(HELLO_MSG)) + self.assertEqual(len(data), len(HELLO_MSG)) + + sock.shutdown(socket.SHUT_RDWR) + sock.close() + + class ClientProto(asyncio.Protocol): + def __init__(self, on_data, on_eof): + self.on_data = on_data + self.on_eof = on_eof + self.con_made_cnt = 0 + + def connection_made(proto, tr): + # XXX: We assume user stores the transport in protocol + proto.tr = tr + proto.con_made_cnt += 1 + # Ensure connection_made gets called only once. + self.assertEqual(proto.con_made_cnt, 1) + + def data_received(self, data): + self.on_data.set_result(data) + + def eof_received(self): + self.on_eof.set_result(True) + + async def client(addr): + await asyncio.sleep(0.5) + + on_data = self.loop.create_future() + on_eof = self.loop.create_future() + + tr, proto = await self.loop.create_connection( + lambda: ClientProto(on_data, on_eof), *addr, + ssl=client_context) + + self.assertEqual(await on_data, b'O') + tr.write(HELLO_MSG) + await on_eof + + tr.close() + + with self.tcp_server(serve, timeout=self.TIMEOUT) as srv: + self.loop.run_until_complete( + asyncio.wait_for(client(srv.addr), timeout=10)) + + # No garbage is left for SSL client from loop.create_connection, even + # if user stores the SSLTransport in corresponding protocol instance + client_context = weakref.ref(client_context) + self.assertIsNone(client_context()) + def test_start_tls_client_buf_proto_1(self): HELLO_MSG = b'1' * self.PAYLOAD_SIZE From 49d1cff0f405de753435bbd18e7d89c440eeb52b Mon Sep 17 00:00:00 2001 From: Fantix King Date: Sun, 17 Mar 2019 16:44:00 -0500 Subject: [PATCH 4/4] Add NEWS entry --- .../NEWS.d/next/Library/2019-03-17-16-43-29.bpo-34745.nOfm7_.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2019-03-17-16-43-29.bpo-34745.nOfm7_.rst diff --git a/Misc/NEWS.d/next/Library/2019-03-17-16-43-29.bpo-34745.nOfm7_.rst b/Misc/NEWS.d/next/Library/2019-03-17-16-43-29.bpo-34745.nOfm7_.rst new file mode 100644 index 00000000000000..d88f36a6d21736 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2019-03-17-16-43-29.bpo-34745.nOfm7_.rst @@ -0,0 +1 @@ +Fix :mod:`asyncio` ssl memory issues caused by circular references