Remove Python OpenSSL locking callback and replace it with one in C #3201
Conversation
The Python OpenSSL locking callback is unsafe; if GC is triggered during the callback's invocation, it can result in the callback being invoked reentrantly, which can lead to deadlocks. This patch replaces it with one in C that gets built at compile time via cffi along with the rest of the OpenSSL binding.
|
@aglasgall, thanks for your PR! By analyzing the history of the files in this pull request, we identified @public, @tiran and @alex to be potential reviewers. |
|
Jenkins, ok to test. |
|
Build failures are I think because I imported sysconfig to get at WITH_THREAD to mimic _ssl.c's behavior of only installing the callback if threading is enabled. There's probably a better way of doing that. |
|
|
||
|
|
||
| if sysconfig.get_config_var("WITH_THREAD"): | ||
| FUNCTIONS += """ |
There was a problem hiding this comment.
I think you need an additional guard for OpenSSL < 1.1.0 and not LibreSSL. OpenSSL 1.1.0 has its own locking code.
There was a problem hiding this comment.
I think 1.1.0 just noops the methods, but I agree we should guard.
| if sysconfig.get_config_var("WITH_THREAD"): | ||
| # Use Python's implementation if available, importing _ssl | ||
| # triggers the setup for this. | ||
| __import__("ssl") |
There was a problem hiding this comment.
You don't have to import the ssl module. Just try to import the _ssl C extension module.
try:
__import__('_ssl')
except ImportError:
pass
|
@tiran Is it possible to reconcile the Python license with our Apache/BSD dual license? I want to be sure we're in the clear on code import like this, but I am utterly unfamiliar with the actual provisions of the license CPython is provided under. |
| """ | ||
|
|
||
| if sysconfig.get_config_var("WITH_THREAD"): | ||
| INCLUDES += """ |
There was a problem hiding this comment.
Style nit, let's put this all on one line.
| @@ -34,6 +42,7 @@ | |||
| */ | |||
| extern "Python" int Cryptography_rand_bytes(unsigned char *, int); | |||
| extern "Python" int Cryptography_rand_status(void); | |||
|
|
|||
| # If nothing else has setup a locking callback already, we set up | ||
| # our own | ||
| if not lib._setup_ssl_threads(): | ||
| raise InternalError("Threading is enabled and cryptography is" |
There was a problem hiding this comment.
Instead of having this branch let's just do an _openssl_assert. Otherwise we'll have a coverage issue since we can't test this branch.
|
|
||
|
|
||
| if sysconfig.get_config_var("WITH_THREAD"): | ||
| FUNCTIONS += """ |
There was a problem hiding this comment.
I think 1.1.0 just noops the methods, but I agree we should guard.
|
@reaperhulk I'm not a lawyer. You can ask VanL about the license. |
|
To keep this PR updated: I have an open question to Van :) |
|
Closing this in favor of #3226 which I'll use to drive this to completion. Thanks for diagnosing this and getting the ball rolling on a patch. |
The Python OpenSSL locking callback is unsafe; if GC is triggered
during the callback's invocation, it can result in the callback being
invoked reentrantly, which can lead to deadlocks. This patch replaces
it with one in C that gets built at compile time via cffi along with
the rest of the OpenSSL binding.
This is a straight lift of the callback and setup code from Python 3.5.2's _ssl module; the only changes were removing #ifs conditioned on various openssl versions because cryptography's minimum openssl version is higher than any of the conditioned blocks.
There are still several issues remaining after replacing it; in particular, the interpreter at least sometimes hangs on exit unless cryptography.hazmat.backends.openssl.backend.activate_builtin_random() is used (and though I've not seen it deadlock/hang if it is, that doesn't mean it won't), and I've also seen it dump core and abort because:
Fatal Python error: could not acquire lock for <_io.BufferedWriter name=''> at interpreter shutdown, possibly due to daemon threads
So we still have work to do, but this is a start.
This is for #3195.