Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-134698: Hold a lock when the thread state is detached inssl#134724
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
@Conobi Would you mind pointing me to the downstream FastAPI issue? |
Conobi commentedMay 26, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
UsePySSL_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would probably be good to add a test creating ssl contexts in multiple threads as well.
ZeroIntensityMay 26, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Great to see the improved thread safety! 🚀
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You're missing a call to_PySSL_FIX_ERRNO here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See the discussion on issue, I am not convinced that adding more locks is the solution, it will degrade performance in multi threaded "safe" workloads which work today.
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. |
Uh oh!
There was an error while loading.Please reload this page.
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.