gh-134698: Hold a lock when the thread state is detached in ssl
#134724
Conversation
|
@Conobi Would you mind pointing me to the downstream FastAPI issue? |
Didn't find any. Howewer, here are the issues probably linked to this: |
|
FastAPI was having problems with this in their test suite, right? Is there someone I should CC? |
Modules/_ssl.c
Outdated
|
|
||
| /* Make sure the SSL error state is initialized */ | ||
| ERR_clear_error(); | ||
|
|
||
| PySSL_BEGIN_ALLOW_THREADS | ||
| Py_BEGIN_ALLOW_THREADS |
There was a problem hiding this comment.
Use PySSL_BEGIN_ALLOW_THREADS(ssl->ctx) here. The PySSLContext's ctx is being accessed by SSL_new so we should lock it.
There was a problem hiding this comment.
Would probably be good to add a test creating ssl contexts in multiple threads as well.
There was a problem hiding this comment.
There's nothing too special about this function, though. I'm not sure it's worth the effort to add a test for every single function fixed here.
Modules/_ssl.c
Outdated
| self->ssl = SSL_new(ctx); | ||
| PySSL_END_ALLOW_THREADS | ||
| PySSL_END_ALLOW_THREADS(sslctx) | ||
| _PySSL_FIX_ERRNO; |
There was a problem hiding this comment.
This one shouldn't be needed, or am I wrong?
| BIO_free_all(bio); | ||
| PySSL_END_ALLOW_THREADS | ||
| Py_END_ALLOW_THREADS |
There was a problem hiding this comment.
You're missing a call to _PySSL_FIX_ERRNO here
|
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 |
Wait a minute, there shouldn't be any multithreaded workloads that get hit by this, right? They'll all crash right now. |
Inside of
Py_BEGIN_ALLOW_THREADSblocks, OpenSSL calls need to be synchronized to prevent crashes. I'm doing this with a per-object mutex that is only held inside when the GIL or critical section is released.