Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry on WANT_WRITE & WANT_READ in sendall() #954

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

webknjaz
Copy link

@webknjaz webknjaz commented Nov 9, 2020

This change introduces retries in OpenSSL.SSL.Connection.sendall()
when WANT_WRITE_ERROR or WANT_READ_ERROR happen.

It relies on SSL_MODE_ENABLE_PARTIAL_WRITE being set on the context,
that changes the mode of SSL_write() to return errors only if zero
bytes has been sent making it safe to retry in these cases.

Ideally, the calling code is supposed to poll()/select() the
socket to know when it's okay to attempt the next retry (hence it is
readable or writable) but it's not available in the sendall() method
and just retrying the operation is good enough.

Fixes #176

Refs:

@webknjaz webknjaz force-pushed the bugfixes/176-sendall-want-write-read-processing branch from c945229 to 873866a Compare November 9, 2020 00:13
@reaperhulk
Copy link
Member

Is there a way we can cover this with a test? And is SSL_MODE_ENABLE_PARTIAL_WRITE being set anywhere?

@webknjaz
Copy link
Author

webknjaz commented Nov 9, 2020

And is SSL_MODE_ENABLE_PARTIAL_WRITE being set anywhere?

It's set in the context initializer: https://github.com/pyca/pyopenssl/blob/124a013/src/OpenSSL/SSL.py#L660. Also, the docstring of send() seems to imply the same: https://github.com/pyca/pyopenssl/blob/124a013/src/OpenSSL/SSL.py#L1623-L1625 — I did not understand why is that at first but now it makes sense.

Is there a way we can cover this with a test?

I'm not sure how to properly trigger this in a stable manner at the moment. Although, the suspicion is that maybe if we could limit the buffer size of the test socket and feed a sufficiently big chunk of data into it, it should work.

pyopenssl/tests/test_ssl.py

Lines 2659 to 2690 in 124a013

def test_wantWriteError(self):
"""
`Connection` methods which generate output raise
`OpenSSL.SSL.WantWriteError` if writing to the connection's BIO
fail indicating a should-write state.
"""
client_socket, server_socket = socket_pair()
# Fill up the client's send buffer so Connection won't be able to write
# anything. Only write a single byte at a time so we can be sure we
# completely fill the buffer. Even though the socket API is allowed to
# signal a short write via its return value it seems this doesn't
# always happen on all platforms (FreeBSD and OS X particular) for the
# very last bit of available buffer space.
msg = b"x"
for i in range(1024 * 1024 * 64):
try:
client_socket.send(msg)
except error as e:
if e.errno == EWOULDBLOCK:
break
raise
else:
pytest.fail(
"Failed to fill socket buffer, cannot test BIO want write"
)
ctx = Context(SSLv23_METHOD)
conn = Connection(ctx, client_socket)
# Client's speak first, so make it an SSL client
conn.set_connect_state()
with pytest.raises(WantWriteError):
conn.do_handshake()
seems to be doing this exactly.

@webknjaz
Copy link
Author

webknjaz commented Nov 9, 2020

I'm trying to come up with with a reproducer @ #955 but it still needs polishing.

Base automatically changed from master to main February 13, 2021 17:50
@@ -25,6 +25,10 @@ Deprecations:
Changes:
^^^^^^^^

- Fixed the inability of ``OpenSSL.SSL.Connection.sendall()`` to
keep with sending data over the wire after ``SSL_ERROR_WANT_READ``
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
keep with sending data over the wire after ``SSL_ERROR_WANT_READ``
keep sending data over the wire after ``SSL_ERROR_WANT_READ``

This change introduces retries in `OpenSSL.SSL.Connection.sendall()`
when `WANT_WRITE_ERROR` or `WANT_READ_ERROR` happen.

It relies on `SSL_MODE_ENABLE_PARTIAL_WRITE` being set on the context,
that changes the mode of `SSL_write()` to return errors only if zero
bytes has been sent making it safe to retry in these cases.

Ideally, the calling code is supposed to `poll()`/`select()` the
socket to know when it's okay to attempt the next retry (hence it is
readable or writable) but it's not available in the `sendall()` method
and just retrying the operation is good enough.

Fixes pyca#176

Refs:
* http://openssl.6102.n7.nabble.com/SSL-MODE-ACCEPT-MOVING-WRITE-BUFFER-td6421.html
* https://stackoverflow.com/a/28992313/595220
* https://www.openssl.org/docs/manmaster/man3/SSL_write.html
* https://stackoverflow.com/a/20817394/595220
@webknjaz webknjaz force-pushed the bugfixes/176-sendall-want-write-read-processing branch from 873866a to 9836c42 Compare January 23, 2024 21:27
@webknjaz webknjaz marked this pull request as draft January 24, 2024 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

WANT_WRITE_ERROR occurs for sendall
2 participants