WANT_WRITE_ERROR occurs for sendall #176

Lothsahn · 2014-12-05T22:33:49Z

The OpenSSL documentation says that in the event of a WANT_WRITE_ERROR or WANT_READ_ERROR, the same OpenSSL method call is to be repeated, otherwise you will get a bad write retry error.

See here:
https://www.openssl.org/docs/ssl/SSL_write.html
http://stackoverflow.com/questions/2997218/why-am-i-getting-error1409f07fssl-routinesssl3-write-pending-bad-write-retr

For pyOpenSSL.sendAll() this is problematic. Because python's sendAll() call does not return the number of bytes already sent if an error is thrown, it's impossible to continue with an identical SSL.Write() call. The state of the last SSL.Write() call is only known inside the pyOpenSSL.sendAll() function, and since this function has thrown an exception, this state is lost.

To illustrate the problem, see the following source for SSL.sendall() in pyOpenSSL:

    left_to_send = len(buf)
    total_sent = 0
    data = _ffi.new("char[]", buf)

    while left_to_send:
        result = _lib.SSL_write(self._ssl, data + total_sent, left_to_send)
        self._raise_ssl_error(self._ssl, result)
        total_sent += result
        left_to_send -= result

Specifically, inside the while loop, there are repeated calls to _lib.SSL_write(). _lib is the openSSL library, so the correct contract is that if SSL_write throws a WantWriteError or WantReadError, it should be handled by sleeping for a short duration and re-issuing the same call. However, pyOpenSSL does not do this, and the caller cannot either.

As a workaround, I implemented a wrapper around pyOpenSSL, and implemented my own sendAll() to handle any WANT_WRITE_ERROR or WANT_READ_ERRORS in a loop, re-issuing the same send() call that the error threw (after a small sleep).

Finally, keep in mind that any write or read calls that are made to pyOpenSSL can fail with either a WANT_WRITE_ERROR or a WANT_READ_ERROR, because these calls could trigger an SSL handshake. If network buffers are full or nearly full when the handshake triggers, these errors can be thrown. For this reason, both error conditions must be handled by all network calls, either in pyOpenSSL or by developer using pyOpenSSL.

Recommended fix:
pyOpenSSL should write exception handlers to catch WANT_WRITE_ERROR and WANT_READ_ERRORS inside all functions to catch those exceptions, sleep for a small period of time, and re-issue the same SSL_write() or SSL_read() call. The specific implementation will need to be considerate of blocking vs non-blocking mode, as well as timeouts, and is likely not trivial.

Thank you for all the work you do on pyOpenSSL. It really is a fantastic library.

exarkun · 2014-12-07T12:19:44Z

pyOpenSSL should write exception handlers to catch WANT_WRITE_ERROR and WANT_READ_ERRORS inside all functions to catch those exceptions

Can you explain why you think this needs to be applied to all methods, not just sendall?

Lothsahn · 2015-05-14T16:52:00Z

exarkun: sorry--I did not see this for a long time.

sendall needs the fix, as it's impossible for the user to retry the call as described above.
For the rest of the methods, the user can retry the call manually, but it makes the userspace code uglier. It would be ideal if the library handled such conditions. It would also be nice to be consistent between the various methods (send vs sendall).

However, if you chose not to handle these errors, the userspace code CAN handle them.

exarkun · 2015-05-18T16:27:52Z

For the rest of the methods, the user can retry the call manually, but it makes the userspace code uglier.

pyOpenSSL is meant as a fairly low-level interface. It supports a variety of use-cases - for example, non-blocking TLS. That use-case would be impeded by automatic retries in send and recv. I suspect that non-blocking code is already prevented from using sendall so fixing it there seems like it could be reasonable. I don't think the change should be applied elsewhere, though. User code that doesn't want to deal with such low-level details should use a higher-level library (eg Twisted).

Lothsahn · 2015-05-19T18:00:00Z

Understood. Thanks for your time. We've implemented a workaround in sendall for our wrapper, so we're all set with this issue.

exarkun · 2015-05-20T13:34:35Z

Great. Thanks for the follow-up.

exarkun · 2015-05-20T13:35:17Z

Actually, oops. Leaving this open because I still think doing retry in sendall in pyOpenSSL would be fine. 😄

hynek · 2015-05-21T09:30:05Z

Yes totally. Happy for PRs. :)

webknjaz · 2020-11-02T13:28:14Z

Looks like the patch with retries needs to be applied around these lines: https://github.com/pyca/pyopenssl/blob/124a013/src/OpenSSL/SSL.py#L1670-L1673

webknjaz · 2020-11-02T14:20:51Z

Per https://www.openssl.org/docs/man1.1.1/man3/SSL_write_ex.html, sendall() should use _lib.SSL_write_ex() instead of _lib.SSL_write() so that it's possible to figure out how many bytes have been written by a failed operation.
But it looks like it's not exposed via cryptography so that should probably happen first: https://github.com/pyca/cryptography/blob/b0a3d89e0f69d6e460a4ae65a57ea2c721f9370b/src/_cffi_src/openssl/ssl.py#L167.

webknjaz · 2020-11-02T15:09:09Z

Hm... This https://github.com/pyca/pyopenssl/blob/124a013/src/OpenSSL/SSL.py#L1623-L1625 suggests that SSL_write() could be retried with the same buffer, although I don't understand why.

webknjaz · 2020-11-08T22:25:31Z

So looking at the source, it seems like SSL_MODE_ENABLE_PARTIAL_WRITE is set and I think calls to SSL_write() return the bytes written when at least 1 byte has been sent. This means that we can expect that if self._raise_ssl_error(self._ssl, result) raises an error, then 0 bytes have been sent.

I think that the patch would roughly be wrapping

pyopenssl/src/OpenSSL/SSL.py

Line 1673 in 124a013

self._raise_ssl_error(self._ssl, result)

with try/except and doing a retry:

     def sendall(self, buf, flags=0):
         """
         Send "all" data on the connection. This calls send() repeatedly until
         all data is sent. If an error occurs, it's impossible to tell how much
         data has been sent.
         :param buf: The string, buffer or memoryview to send
         :param flags: (optional) Included for compatibility with the socket
                       API, the value is ignored
         :return: The number of bytes written
         """
         buf = _text_to_bytes_and_warn("buf", buf)

         with _from_buffer(buf) as data:

             left_to_send = len(buf)
             total_sent = 0

             while left_to_send:
                 # SSL_write's num arg is an int,
                 # so we cannot send more than 2**31-1 bytes at once.
                 result = _lib.SSL_write(
                     self._ssl, data + total_sent, min(left_to_send, 2147483647)
                 )
-                self._raise_ssl_error(self._ssl, result)
+                try:
+                    self._raise_ssl_error(self._ssl, result)
+                except (WantReadError, WantWriteError):
+                    continue
                 total_sent += result
                 left_to_send -= result

             return total_sent

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 · 2020-11-09T00:13:56Z

Here's the PR: #954

webknjaz · 2020-11-09T09:31:45Z

I don't think the change should be applied elsewhere, though.

@exarkun FTR CPython implements retries in their send() implementation: https://github.com/python/cpython/blob/c32f2976b8f4034724c3270397aa16f38daf470f/Modules/_ssl.c#L2467-L2468
It's implemented without the use of SSL_MODE_ENABLE_PARTIAL_WRITE — they have an immutable buffer that they keep passing into SSL_write() unchanged and the internal machinery in OpenSSL keeps a pointer to the slice that's still unsent.

And their sendall() also does that with a memoryview object:
https://github.com/python/cpython/blob/c32f2976b8f4034724c3270397aa16f38daf470f/Lib/ssl.py#L1193-L1207

tiran · 2020-11-09T09:37:18Z

CPython's approach may not be the best approach. Some decisions and workarounds pre-date OpenSSL 0.9.8.

exarkun · 2020-11-09T19:24:24Z

I'm not too involved in pyOpenSSL development anymore (and haven't been for years). But I think it would be great if this PR were augmented with automated tests demonstrating the desired behavior has been achieved.

webknjaz · 2020-11-09T22:52:18Z

@exarkun I wish. Here's my attempt: #955. But when applying it to implementation in the PR I get ssl3_read_bytes and I'm not sure what I'm doing wrong..

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

exarkun closed this as completed May 20, 2015

exarkun reopened this May 20, 2015

dalai4git mentioned this issue Sep 25, 2015

SSL-related bug with websocket send call and multithreading Lawouach/WebSocket-for-Python#174

Open

vashek mentioned this issue Nov 2, 2020

Unhandled OpenSSL.SSL.WantReadError and WantWriteError raised by PyOpenSSL cherrypy/cheroot#245

Open

3 tasks

webknjaz mentioned this issue Nov 2, 2020

Handle OpenSSL.SSL.WantReadError and WantWriteError in pyopenssl adapter cherrypy/cheroot#332

Open

15 tasks

webknjaz mentioned this issue Nov 2, 2020

Expose SSL_write_ex pyca/cryptography#5546

Closed

webknjaz linked a pull request Nov 9, 2020 that will close this issue

Retry on WANT_WRITE & WANT_READ in sendall() #954

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WANT_WRITE_ERROR occurs for sendall #176

WANT_WRITE_ERROR occurs for sendall #176

Lothsahn commented Dec 5, 2014

exarkun commented Dec 7, 2014

Lothsahn commented May 14, 2015

exarkun commented May 18, 2015

Lothsahn commented May 19, 2015

exarkun commented May 20, 2015

exarkun commented May 20, 2015

hynek commented May 21, 2015

webknjaz commented Nov 2, 2020

webknjaz commented Nov 2, 2020

webknjaz commented Nov 2, 2020

webknjaz commented Nov 8, 2020

webknjaz commented Nov 9, 2020

webknjaz commented Nov 9, 2020

tiran commented Nov 9, 2020

exarkun commented Nov 9, 2020

webknjaz commented Nov 9, 2020

WANT_WRITE_ERROR occurs for sendall #176

WANT_WRITE_ERROR occurs for sendall #176

Comments

Lothsahn commented Dec 5, 2014

exarkun commented Dec 7, 2014

Lothsahn commented May 14, 2015

exarkun commented May 18, 2015

Lothsahn commented May 19, 2015

exarkun commented May 20, 2015

exarkun commented May 20, 2015

hynek commented May 21, 2015

webknjaz commented Nov 2, 2020

webknjaz commented Nov 2, 2020

webknjaz commented Nov 2, 2020

webknjaz commented Nov 8, 2020

webknjaz commented Nov 9, 2020

webknjaz commented Nov 9, 2020

tiran commented Nov 9, 2020

exarkun commented Nov 9, 2020

webknjaz commented Nov 9, 2020