Skip to content

Commit fca7de8

Browse files
gpsheadmiss-islington
authored andcommitted
gh-160499: Pause asyncio TLS reads when SSL replies pile up (GH-160500)
(cherry picked from commit 8a5e24a) Co-authored-by: Gregory P. Smith <68491+gpshead@users.noreply.github.com>
1 parent 3d6d2a6 commit fca7de8

3 files changed

Lines changed: 61 additions & 1 deletion

File tree

‎Lib/asyncio/sslproto.py‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,9 @@ def __init__(self, loop, app_protocol, sslcontext, waiter,
308308
# App data write buffering
309309
self._write_backlog = collections.deque()
310310
self._write_buffer_size = 0
311+
# How much of self._outgoing was produced by _do_write(): encrypted
312+
# app data, plus anything else OpenSSL flushed along with it
313+
self._outgoing_app_size = 0
311314

312315
self._waiter = waiter
313316
self._loop = loop
@@ -700,6 +703,7 @@ def _write_appdata(self, list_of_data):
700703
self._fatal_error(ex, 'Fatal error on SSL protocol')
701704

702705
def _do_write(self):
706+
pending = self._outgoing.pending
703707
try:
704708
while self._write_backlog:
705709
data = self._write_backlog[0]
@@ -713,11 +717,13 @@ def _do_write(self):
713717
self._write_buffer_size -= data_len
714718
except SSLAgainErrors:
715719
pass
720+
self._outgoing_app_size += self._outgoing.pending - pending
716721
self._process_outgoing()
717722

718723
def _process_outgoing(self):
719724
if not self._ssl_writing_paused:
720725
data = self._outgoing.read()
726+
self._outgoing_app_size = 0
721727
if len(data):
722728
self._transport.write(data)
723729
self._control_app_writing()
@@ -879,7 +885,15 @@ def resume():
879885
# Flow control for reads from SSL socket
880886

881887
def _control_ssl_reading(self):
882-
size = self._get_read_buffer_size()
888+
# Also count the records OpenSSL writes on its own while reading
889+
# (alerts, KeyUpdate replies, ...) which the peer does not read:
890+
# the app is not involved in producing them, so pausing it does
891+
# not bound them; only pausing reads from the peer does. Whatever
892+
# was produced while writing app data (_outgoing_app_size) is left
893+
# out so that two peers which both write before they read cannot
894+
# deadlock.
895+
size = (self._get_read_buffer_size() +
896+
self._outgoing.pending - self._outgoing_app_size)
883897
if size >= self._incoming_high_water and not self._ssl_reading_paused:
884898
self._ssl_reading_paused = True
885899
self._transport.pause_reading()
@@ -912,6 +926,7 @@ def resume_writing(self):
912926
assert self._ssl_writing_paused
913927
self._ssl_writing_paused = False
914928
self._process_outgoing()
929+
self._control_ssl_reading()
915930

916931
def _fatal_error(self, exc, message='Fatal error on transport'):
917932
if self._transport:

‎Lib/test/test_asyncio/test_sslproto.py‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,46 @@ def test_write_after_closing(self):
271271
# should not raise
272272
self.assertIsNone(transp.write(b'data'))
273273

274+
def test_pause_reading_if_ssl_replies_are_not_read(self):
275+
# Output which OpenSSL produces on its own while reading (alerts,
276+
# KeyUpdate, ...) must not pile up if the peer does not read it.
277+
ssl_proto = self.ssl_protocol()
278+
transport = self.connection_made(ssl_proto, do_handshake=mock.Mock())
279+
reply = b'x' * (ssl_proto._incoming_high_water // 2)
280+
281+
def read(*args):
282+
ssl_proto._outgoing.write(reply)
283+
raise ssl.SSLWantReadError
284+
ssl_proto._sslobj.read.side_effect = read
285+
286+
ssl_proto.pause_writing()
287+
ssl_proto.buffer_updated(0)
288+
transport.pause_reading.assert_not_called()
289+
ssl_proto.buffer_updated(0)
290+
transport.pause_reading.assert_called_once_with()
291+
transport.resume_reading.assert_not_called()
292+
transport.write.assert_not_called()
293+
294+
ssl_proto.resume_writing()
295+
transport.write.assert_called_once_with(reply * 2)
296+
transport.resume_reading.assert_called_once_with()
297+
298+
def test_no_pause_reading_if_app_data_is_not_read(self):
299+
# Peers which both write a lot before they read must not deadlock.
300+
ssl_proto = self.ssl_protocol()
301+
transport = self.connection_made(ssl_proto, do_handshake=mock.Mock())
302+
ssl_proto._sslobj.write.side_effect = ssl_proto._outgoing.write
303+
data = b'x' * ssl_proto._incoming_high_water
304+
305+
ssl_proto.pause_writing()
306+
ssl_proto._app_transport.write(data)
307+
ssl_proto.buffer_updated(0)
308+
transport.pause_reading.assert_not_called()
309+
310+
ssl_proto.resume_writing()
311+
transport.write.assert_called_once_with(data)
312+
transport.resume_reading.assert_not_called()
313+
274314

275315
##############################################################################
276316
# Start TLS Tests
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Fix unbounded memory growth in :mod:`asyncio` TLS connections when a peer
2+
stops reading but keeps sending records that make OpenSSL generate its own
3+
replies (such as TLS 1.2 renegotiation alerts). Reading from the peer is now
4+
paused when this output accumulates, instead of only when application data
5+
does.

0 commit comments

Comments
 (0)