Remove memoryview usage in urllib3.contrib.pyopenssl#sendall

[evnm]

As described in #717, urllib3.contrib.pyopenssl#sendall currently fails when WantWriteError is thrown by OpenSSL due to urllib3 preemptively casting the data to be written as a memoryview. This results in duplicate bytestring allocations by pyopenssl and OpenSSL issuing a SSL3_WRITE_PENDING error.

This change removes memoryview usage from urllib3/contrib/pyopenssl.py. We thus rely on the underlying pyOpenSSL library's ability to cast data to the proper bytestring type.

[Lukasa]

This change looks entirely reasonable to me. I'm going to hold off merging and let @shazow do that, but I don't see any reason he can't. =) Thanks again @evnm!

[shazow]

Looks swell, thank you for finding it, reporting it, and fixing it! (And to @Lukasa for being Captain Maintainer) 🍮

[ScottEAdams]

Sadly the lastest dev doesnt fix it for me, only rolling back to 1.10.4 does. Here is the traceback from latest dev on heroku trying to send email via djrill

app[worker.1]:   File "/app/.heroku/python/lib/python2.7/site-packages/requests/sessions.py", line 576, in send
app[worker.1]:     r = adapter.send(request, **kwargs)
app[worker.1]:   File "/app/.heroku/python/lib/python2.7/site-packages/requests/adapters.py", line 370, in send
app[worker.1]:     timeout=timeout
app[worker.1]:   File "/app/.heroku/python/lib/python2.7/site-packages/requests/packages/urllib3/connectionpool.py", line 559, in urlopen
app[worker.1]:     body=body, headers=headers)
app[worker.1]:   File "/app/.heroku/python/lib/python2.7/site-packages/requests/packages/urllib3/connectionpool.py", line 353, in _make_request
app[worker.1]:     conn.request(method, url, **httplib_request_kw)
app[worker.1]:   File "/app/.heroku/python/lib/python2.7/httplib.py", line 1053, in request
app[worker.1]:     self._send_request(method, url, body, headers)
app[worker.1]:   File "/app/.heroku/python/lib/python2.7/httplib.py", line 1093, in _send_request
app[worker.1]:     self.endheaders(body)
app[worker.1]:   File "/app/.heroku/python/lib/python2.7/site-packages/newrelic-2.58.0.43/newrelic/hooks/external_httplib.py", line 35, in httplib_endheaders_wrapper
app[worker.1]:     return wrapped(*args, **kwargs)
app[worker.1]:   File "/app/.heroku/python/lib/python2.7/httplib.py", line 1049, in endheaders
app[worker.1]:     self._send_output(message_body)
app[worker.1]:   File "/app/.heroku/python/lib/python2.7/httplib.py", line 893, in _send_output
app[worker.1]:     self.send(msg)
app[worker.1]:   File "/app/.heroku/python/lib/python2.7/httplib.py", line 869, in send
app[worker.1]:     self.sock.sendall(data)
app[worker.1]:   File "/app/.heroku/python/lib/python2.7/site-packages/requests/packages/urllib3/contrib/pyopenssl.py", line 220, in sendall
app[worker.1]:     sent = self._send_until_done(data[total_sent:total_sent+SSL_WRITE_BLOCKSIZE])
app[worker.1]:   File "/app/.heroku/python/lib/python2.7/site-packages/requests/packages/urllib3/contrib/pyopenssl.py", line 206, in _send_until_done
app[worker.1]:     return self.connection.send(data)
app[worker.1]:   File "/app/.heroku/python/lib/python2.7/site-packages/OpenSSL/SSL.py", line 1271, in send
app[worker.1]:     self._raise_ssl_error(self._ssl, result)
app[worker.1]:   File "/app/.heroku/python/lib/python2.7/site-packages/OpenSSL/SSL.py", line 1187, in _raise_ssl_error
app[worker.1]:     _raise_current_error()
app[worker.1]:   File "/app/.heroku/python/lib/python2.7/site-packages/OpenSSL/_util.py", line 48, in exception_from_error_queue
app[worker.1]:     raise exception_type(errors)
app[worker.1]: Error: [('SSL routines', 'SSL3_WRITE_PENDING', 'bad write retry')]

[Lukasa]

@7wonders You aren't using the latest master in that traceback. You have a traceback that includes line 220 of urllib3.contrib.pyopenssl, and claims that it's in sendall. In the latest master, line 220 is in close. Want to try again?

[Lukasa]

In fact, your traceback is substantially identical to the one originally posted in #717, which strongly suggests you were using the flawed version of the code. =)

[ScottEAdams]

It was very late/early this morning so you might be right and heroku has been known to cache previous versions :) I did however force a full reinstall by switching python versions to be sure caches were cleared and then even opened bash into heroku and checked the urllib3/contrib/pyopenssl.py that the following was gone.

-try:       
-    _ = memoryview     
-    has_memoryview = True      
-except NameError:      
-    has_memoryview = False     
-

And, like I said, as soon as I installled 1.10.4 it was working again. I will have to wait until later to try again as it is only happening under very specific circumstances.

[Lukasa]

Bear in mind that requests uses its own personal copy of urllib3, and does not use any one that you install from PyPI. This may have caused your misleading result.

[ScottEAdams]

^^yes. Exactly this. But then why would it work when I switched urllib3 to 1.10.4.

[Lukasa]

It should be unrelated. Note, however, that this issue is not totally deterministic, and depends how the writes land on the SSL layer.

[ScottEAdams]

Thanks for the help Lukasa. I will try again later and replace requests with an updated urllib3 fork to test.

[jof]

I'm still running into this issue in the latest version of kennethreitz/requests (2.8.1), which vendors urllib3 into itself.
Since this was fixed since the last release (1.12), there is not yet a newer version to ask requests to vendor, such that it includes the fix.

Might it be possible to cut a release soon so that I can try and get requests rolled forward with this fix included?
cc @shazow

[sjyvmbam]

As described in #717, urllib3.contrib.pyopenssl#sendall currently fails when WantWriteError is thrown by OpenSSL due to urllib3 preemptively casting the data to be written as a memoryview. This results in duplicate bytestring allocations by pyopenssl and OpenSSL issuing a SSL3_WRITE_PENDING error.

This change removes memoryview usage from urllib3/contrib/pyopenssl.py. We thus rely on the underlying pyOpenSSL library's ability to cast data to the proper bytestring type.

Please how to remove this memoryview ???

[pquentin]

@sjyvmbam Can you please open a new issue with the actual problem you're facing? We introduced another memoryview in 1.26.0, this six years old pull request is probably not related to your problem.

[sjyvmbam]

hello, please if you can help me that will be nice i put a picture inside my email. best Regards Yvan Mbami Am 29.09.2021 14:12, schrieb Quentin Pradet:

…

@sjyvmbam [1] Can you please open a new issue with the actual problem you're facing? We introduced another memoryview in 1.26.0, this six years old pull request is probably not related to your problem. -- You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub [2], or unsubscribe [3]. Triage notifications on the go with GitHub Mobile for iOS [4] or Android [5]. Links: ------ [1] https://github.com/sjyvmbam [2] #719 (comment) [3] https://github.com/notifications/unsubscribe-auth/AOTAN6GBISCLBNS7NXOMLHDUEL7EFANCNFSM4BR6PN4Q [4] https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 [5] https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub