Repository navigation
asyncio.sslproto.SSLProtocol exception handling triggers TypeError: SSLProtocol._abort() takes 1 positional argument but 2 were given #113214
Description
Activity
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?
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 toself._transport._force_close()feels scary -- we don't know thatself._transporteven has a_force_close()method, since that's not a standardTransportmethod. (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.
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 toself._transport._force_close()feels scary -- we don't know thatself._transporteven has a_force_close()method, since that's not a standardTransportmethod. (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 inSSLProtocol._check_shutdown_timeout, and bothasyncio.proactor_events._ProactorBasePipeTransportandasyncio.selector_events._SelectorTransportimplement a_force_close()method. The latter two use the exception passed in to then pass the exception to theconnection_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.
Okay, thanks and good luck!
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 callsself._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_SSLProtocolTransportclass 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 toProtocol.connection_lost(exc); both implementations end withself._loop.call_soon(self._call_connection_lost, exc).What goes wrong here is that
_SSLProtocolTransportshould 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 theSSLProtocol._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 toloop.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 hereself._transportis 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 becauseSSLProtocolis 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 whereNoneneeds to be passed in now. Plus, we can remove the redundancy in_SSLProtocolTransport.abort()by just callingself._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_ssltest 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.Reacted by Jamie PhanThanks 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?
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.
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.SSLWantReadErrorline in the test caseconnection_madeutility method.I'll wrap this in a pull request next.
Reacted by Guido van Rossum and adamsilkey-resideo- added a commit that references this issue
on Dec 20, 2023
Bug report
Bug description:
There are several pathways to this bug, all which call
asyncio.sslproto._SSLProtocolTransport._force_close()with an exception instance:asyncio.sslproto.SSLProtocolTransport._check_shutdown_timeout()is called)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:
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
At any rate, the
self._ssl_protocolattribute is an instance ofSSLProtocolin 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
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