Skip to content

bpo-46252: Prevent _SelectorSocketTransport from closing on SSLWantRe…#30385

Closed
matan1008 wants to merge 4 commits into
python:mainfrom
matan1008:fix-issue-46252
Closed

bpo-46252: Prevent _SelectorSocketTransport from closing on SSLWantRe…#30385
matan1008 wants to merge 4 commits into
python:mainfrom
matan1008:fix-issue-46252

Conversation

@matan1008

@matan1008 matan1008 commented Jan 4, 2022

Copy link
Copy Markdown
Contributor

@asvetlov asvetlov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think self._sock.recv() can never throw ssl.SSLWant* exception.
The self._sock is a plain TCP socket always, ssl.SSLSocket and ctx.wrap_socket() are never used by asyncio.
SSL encoding/decoding is done in-memory by BIO, see sslproto.py.

@bedevere-bot

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@matan1008

matan1008 commented Jan 4, 2022

Copy link
Copy Markdown
Contributor Author

I think self._sock.recv() can never throw ssl.SSLWant* exception. The self._sock is a plain TCP socket always, ssl.SSLSocket and ctx.wrap_socket() are never used by asyncio. SSL encoding/decoding is done in-memory by BIO, see sslproto.py.

I am facing this issue when calling to asyncio.open_connection with sock argument set to a SSLSocket object.
From your comment I assume that I might be using open_connection in a wrong way. If that is the case, how can StreamReader and StreamWriter be created with an existing SSLSocket?

@matan1008

Copy link
Copy Markdown
Contributor Author

I found out it happens both in writing and reading data, so I think that now it is handled better - I have made the requested changes; please review again

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

@bedevere-bot
bedevere-bot requested a review from asvetlov January 5, 2022 19:37
@asvetlov

asvetlov commented Jan 6, 2022

Copy link
Copy Markdown
Contributor

Using SSLSocket is forbidden in asyncio, sorry.
Because SSLSocket cannot be switched into non-blocking mode, it is always blocking by design.

The only thing that asyncio can do is raise a TypeError fast if SSLSocket is passed.

Actually, it has this check already for sock_* operations but not for create_connection() / create_server().

@matan1008

Copy link
Copy Markdown
Contributor Author

Thanks you for your response!
I am probably missing something but from the SSLSocket documentation is seems that there is a setblocking() method, and the discussed changes solved the issues (at least in my case), so I would really appreciate if you could explain some more about why it is forbidden.
More specific question if I may - In my case I am implementing one of lockdown's services protocol (see pymobiledevice3 ) and the protocol requires creating a socket, exchanging some messages, establishing SSL connection over the same socket, and after that doing some asynchronous logic - if using asyncio is not the solution, how can it be done in python?

@github-actions

github-actions Bot commented Feb 6, 2022

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Feb 6, 2022
@asvetlov

Copy link
Copy Markdown
Contributor

Sorry, pymobiledevice3 discussion is out of the project scope. Try to ask there.

@asvetlov asvetlov closed this Feb 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting change review stale Stale PR or inactive for long period of time.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants