asyncio.sslproto.SSLProtocol exception handling triggers TypeError: SSLProtocol._abort() takes 1 positional argument but 2 were given

[mjpieters]

Bug report

Bug description:

There are several pathways to this bug, all which call asyncio.sslproto._SSLProtocolTransport._force_close() with an exception instance:

• During shutdown, if the flushing state takes too long (asyncio.sslproto.SSLProtocolTransport._check_shutdown_timeout() is called)
• Anything that triggers a call to asyncio.sslproto.SSLProtocol._fatal_error(), e.g. SSL handshake timeout or exception, SSL shutdown timeout or exception, an exception during reading, exception raised in the app transport EOF handler, etc.

I'm seeing this when using a HTTPS proxy with a aiohttp client session (which wraps TLS in TLS), but I don't think it is specific to that context. I'm seeing these tracebacks:

Fatal error on SSL protocol
protocol: <asyncio.sslproto.SSLProtocol object at 0x7fe36f3a1350>
transport: <_SelectorSocketTransport closing fd=6 read=idle write=<idle, bufsize=0>>
Traceback (most recent call last):
  File ".../lib/python3.11/asyncio/sslproto.py", line 644, in _do_shutdown
    self._sslobj.unwrap()
  File ".../lib/python3.11/ssl.py", line 983, in unwrap
    return self._sslobj.shutdown()
           ^^^^^^^^^^^^^^^^^^^^^^^
ssl.SSLError: [SSL: APPLICATION_DATA_AFTER_CLOSE_NOTIFY] application data after close notify (_ssl.c:2702)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File ".../lib/python3.11/asyncio/sslproto.py", line 731, in _do_read
    self._do_read__buffered()
  File ".../lib/python3.11/asyncio/sslproto.py", line 765, in _do_read__buffered
    self._app_protocol_buffer_updated(offset)
  File ".../lib/python3.11/asyncio/sslproto.py", line 445, in buffer_updated
    self._do_shutdown()
  File ".../lib/python3.11/asyncio/sslproto.py", line 648, in _do_shutdown
    self._on_shutdown_complete(exc)
  File ".../lib/python3.11/asyncio/sslproto.py", line 660, in _on_shutdown_complete
    self._fatal_error(shutdown_exc)
  File ".../lib/python3.11/asyncio/sslproto.py", line 911, in _fatal_error
    self._transport._force_close(exc)
  File ".../lib/python3.11/asyncio/sslproto.py", line 252, in _force_close
    self._ssl_protocol._abort(exc)
TypeError: SSLProtocol._abort() takes 1 positional argument but 2 were given

To me, the implementation of _SSLProtocolTransport._force_close() looks like an unfinished copy of the _SSLProtocolTransport.abort() method:

cpython/Lib/asyncio/sslproto.py

Lines 239 to 252 in 1583c40

	def abort(self):
	"""Close the transport immediately.
	Buffered data will be lost. No more data will be received.
	The protocol's connection_lost() method will (eventually) be
	called with None as its argument.
	"""
	self._closed = True
	if self._ssl_protocol is not None:
	self._ssl_protocol._abort()
	def _force_close(self, exc):
	self._closed = True
	self._ssl_protocol._abort(exc)

At any rate, the self._ssl_protocol attribute is an instance of SSLProtocol in the same module, and the _abort() method on that class doesn't accept an exception instance:

cpython/Lib/asyncio/sslproto.py

Lines 664 to 667 in 1583c40

	def _abort(self):
	self._set_state(SSLProtocolState.UNWRAPPED)
	if self._transport is not None:
	self._transport.abort()

I find the test suite surrounding the SSL protocol to be dense enough that I can't easily spot how to provide an update there to reproduce this issue more easily, but the fix looks simple enough: don't pass an argument to _abort().

CPython versions tested on:

3.11, 3.12, CPython main branch

Operating systems tested on:

Linux

Linked PRs

• GH-113214: Fix SSLProto exception handling in SSL-over-SSL scenarios #113334
• [3.12] GH-113214: Fix SSLProto exception handling in SSL-over-SSL scenarios (GH-113334) #113339
• [3.11] GH-113214: Fix SSLProto exception handling in SSL-over-SSL scenarios (GH-113334) #113340

[mjpieters]

Ping @asvetlov: this is your baby (introduced in #31275)

[mjpieters]

Ah, I just found the trail to UVLoop, and I see that there the implementation of SSLProtocol._abort() does accept an exception object.

The implementation of _SSLTransport.abort() delegates to _SSLTransport._force_close():

    def abort(self):
        """Close the transport immediately.


        Buffered data will be lost.  No more data will be received.
        The protocol's connection_lost() method will (eventually) be
        called with None as its argument.
        """
        self._force_close(None)


    def _force_close(self, exc):
        self._closed = True
        self._ssl_protocol._abort(exc)

and the implementation in SSLProtocol._abort() passes the exception back to the transport _force_close() method:

    cdef _abort(self, exc):
        self._set_state(UNWRAPPED)
        if self._transport is not None:
            self._transport._force_close(exc)

Is there any reason why the Python port didn't follow this same pattern?

[gvanrossum]

Hallo Martijn,

Is there any reason why the Python port didn't follow this same pattern?

I doubt that anyone can answer your question. Andrew has been distracted by a war, and Yury (uvloop creator) by a company. Kumar I believe is in his final year of school. I personally refuse to learn anything about SSL, its complexity just depresses me.

Nevertheless, I read your bug description and, well, SSL is still bizarre. I think the best fix at this point is to do as you say -- don't pass the exception to _abort(). Changing _abort() to take an optional exception and pass it to self._transport._force_close() feels scary -- we don't know that self._transport even has a _force_close() method, since that's not a standard Transport method. (I guess uvloop knows its own structure better.)

If you don't have a test to trigger the bug, so be it. It's only a three-character fix, and you've convinced me that it doesn't make things worse -- possibly the code will crash a few lines later, but so be it.

[mjpieters]

Hi Guido,

Nevertheless, I read your bug description and, well, SSL is still bizarre. I think the best fix at this point is to do as you say -- don't pass the exception to _abort(). Changing _abort() to take an optional exception and pass it to self._transport._force_close() feels scary -- we don't know that self._transport even has a _force_close() method, since that's not a standard Transport method. (I guess uvloop knows its own structure better.)

There is actually other code that calls self._transport._force_close(), passing in an exception; that's in SSLProtocol._check_shutdown_timeout, and both asyncio.proactor_events._ProactorBasePipeTransport and asyncio.selector_events._SelectorTransport implement a _force_close() method. The latter two use the exception passed in to then pass the exception to the connection_lost() callback, but there is no equivalent code path in the sslproto module.

I'll try to take a look at the original uvloop PR and may have to face the test suite anyway. If I can't get round to that in the next week I'll cop out and just make a PR for the 3-character change.

[gvanrossum]

Okay, thanks and good luck!

[mjpieters]

After a little bit of time with the code today I understand much better what is going on.

First of all: the SSLProtocol implementation is, basically, private to the event loop implementations. It is shared between the proactor and selector event loops, and it has knowledge of the internal private methods of the transport implementations used by each loop. This includes the ._force_close(exc) methods that both loop implementations have, and the current implementation already has two locations where it calls self._transport._force_close(exc).

Either event loop implementation will insert the SSLProtocol in between a normal application protocol object and the socket transport when you include a ssl context. Normally the relationship between protocol, transport and socket is simply this:

<protocol> - <transport> - <socket>

but with ssl, this becomes

<protocol> - <sslproto transport> - <sslproto protocol> - <transport>

where <sslproto transport> is nothing more than a shim really; this is what the _SSLProtocolTransport class is for. I'm making this explicit because it is this relationship that I wasn't aware of before and I got my transports all muddled up.

The most important detail that I stumbled over is what transport._force_close(exc) is there for. Both the loops implement this method to ensure that any exception during closing can be passed on to Protocol.connection_lost(exc); both implementations end with self._loop.call_soon(self._call_connection_lost, exc).

What goes wrong here is that _SSLProtocolTransport should act just like the internal implementations of the normal socket transports for either event loop, and mostly, it succeeds at this. Except when it comes to the _force_close() method, that is, because it doesn't correctly pass on the exception through the SSLProtocol._abort() method to the transport handling the raw socket. And you don't see this in the test cases because none of the tests have a protocol that tries to talk to the private implementation details of the transport; it's just a shim implementation and this detail on how this should work must have been missed. It was apparently dropped from earlier version of the SSL PR but a review comment asked for it to be put back in but I think by that stage the knowledge of how that would work wasn't quite there any more.

But my case is special because I was using SSL-over-SSL, in the form of a HTTPS proxy used by aiohttp (passing in the shim _SSLProtocolTransport() instance to loop.start_tls()). In that case, the whole chain basically becomes:

<protocol> - <sslproto outer transport> - <sslproto outer protocol> - <sslproto inner transport> - <sslproto inner protocol> - <transport>

SSLProtocol does know about the internal methods of the transport classes and so calls self._transport._force_close(exc) when exceptions occur, and the whole thing falls over because here self._transport is the inner SSL protocol / transport combo, not the raw socket transport.

To correct this, the SSLProtocol._abort() method definitely should accept an exception argument and pass this on to the 'raw' transport:

    def _abort(self, exc):
        self._set_state(SSLProtocolState.UNWRAPPED)
        if self._transport is not None:
            self._transport._force_close(exc)

Note that this calls _transport._force_close(...), not _transport.abort(). This is safe because SSLProtocol is a private implementation that is only meant to work with the private transport internals, anyway.

There is a bit more to a patch in that there are other calls to the same _abort() method where None needs to be passed in now. Plus, we can remove the redundancy in _SSLProtocolTransport.abort() by just calling self._force_close(None), just like the uvloop implementation:

ff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py
index 3eb65a8a08b..cbb6527d0b2 100644
--- a/Lib/asyncio/sslproto.py
+++ b/Lib/asyncio/sslproto.py
@@ -243,13 +243,12 @@ def abort(self):
         The protocol's connection_lost() method will (eventually) be
         called with None as its argument.
         """
-        self._closed = True
-        if self._ssl_protocol is not None:
-            self._ssl_protocol._abort()
+        self._force_close(None)
 
     def _force_close(self, exc):
         self._closed = True
-        self._ssl_protocol._abort(exc)
+        if self._ssl_protocol is not None:
+            self._ssl_protocol._abort(exc)
 
     def _test__append_write_backlog(self, data):
         # for test only
@@ -614,7 +613,7 @@ def _start_shutdown(self):
         if self._app_transport is not None:
             self._app_transport._closed = True
         if self._state == SSLProtocolState.DO_HANDSHAKE:
-            self._abort()
+            self._abort(None)
         else:
             self._set_state(SSLProtocolState.FLUSHING)
             self._shutdown_timeout_handle = self._loop.call_later(
@@ -661,10 +660,10 @@ def _on_shutdown_complete(self, shutdown_exc):
         else:
             self._loop.call_soon(self._transport.close)
 
-    def _abort(self):
+    def _abort(self, exc):
         self._set_state(SSLProtocolState.UNWRAPPED)
         if self._transport is not None:
-            self._transport.abort()
+            self._transport._force_close(exc)
 
     # Outgoing flow

My next step is to try and port the uvloop test_create_server_ssl_over_ssl test to the asyncio test suite to ensure that this keeps working in future. At any rate, the current asyncio test suite passes with and without the above change, but my traceback is no longer reproducible with the change.

[gvanrossum]

Thanks for the analysis, that looks convincing.

Do you think you can come up with a new test case that exercises this code path? That would be ideal, since it would prove the bug exists in the previous version and is fixed by the patch.

We should probably backport this to 3.12 and 3.11, right?

[mjpieters]

Do you think you can come up with a new test case that exercises this code path? That would be ideal, since it would prove the bug exists in the previous version and is fixed by the patch.

I certainly was hoping I could; implement a SSL-over-SSL test case, verify that it fails without the change, and succeeds with it.

We should probably backport this to 3.12 and 3.11, right?

Yes, I came to this issue via a 3.11 project.

[mjpieters]

Right, the test case is actually a lot simpler than I anticipated; closing a wrapped protocol transport is all that's needed:

    def test_close_during_ssl_over_ssl(self):
        outer = self.ssl_protocol(proto=self.ssl_protocol())
        self.connection_made(outer)
        # Closing the outer app transport should not raise issues
        messages = []
        self.loop.set_exception_handler(lambda loop, ctx: messages.append(ctx))
        outer._app_transport.close()
        self.assertEqual(messages, [])

This reproduces the exception. It actually produces 2 exceptions, but the other is an artefact of the ssl object being a mock and can be silenced by adding a sslobj.write.side_effect = ssl.SSLWantReadError line in the test case connection_made utility method.

I'll wrap this in a pull request next.