Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.4k
gh-137583: Only lock the SSL context, not the SSL socket#137588
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ZeroIntensity commentedAug 9, 2025
!buildbot ^((?!refleak).)* PR$ |
This comment was marked as outdated.
This comment was marked as outdated.
picnixz commentedAug 9, 2025
(You'll get failures due to the buildbots without any built-in hashes and with FIPS mode on) |
I spent all that time trying to make sure the test worked and of courseit was the blurb that was wrong too.
picnixz commentedAug 9, 2025
The blurb was ok. We just don't document those methods because I think they are actually available simply as |
ZeroIntensity commentedAug 9, 2025
That's frustrating. I can't reproduce it emitting warnings locally, so I thoughtd45f6a8 fixed it. |
picnixz commentedAug 9, 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.
Oh no, actually, recv() doesn't exist on SSLSockets. It's socket.socket.recv(). Same for send(). If you want you can do: :meth:`SSLSocket.recv <socket.socket.recv>` |
ZeroIntensity commentedAug 9, 2025
We'll deal with the blurb later, I avoided the reference altogether now. There's an actual test failure here:https://github.com/python/cpython/actions/runs/16849797228/job/47734333301 I think this is that stupid race condition. I'll look into fixing |
Apparently, the thread can start waiting at a point where the serverisn't ready to receive messages, which caused it to block indefinitelyfor some reason. Before we create any threads or set any events, do asimple exchange with the server to ensure that it's in its message loop.This took way too damn long to figure out.
ZeroIntensity commentedAug 9, 2025
There we go, that race is finally fixed. Let's try the buildbots again. |
bedevere-bot commentedAug 9, 2025
🤖 New build scheduled with the buildbot fleet by@ZeroIntensity for commit38e29f9 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F137588%2Fmerge If you want to schedule another build, you need to add the🔨 test-with-buildbots label again. |
| @@ -0,0 +1,4 @@ | |||
| Fix a deadlock introduced in the :mod:`ssl` module when a call to | |||
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.
mention the specific Python 3.13 version the deadlock was introduced in.
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.
(and 3.14.0rc1)
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.
The backport didn't make it to 3.14, so it's fine there. Only 3.13.6 is affected.
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.
Done.
| Fix a deadlock introduced in 3.13.6 when a call to | ||
| :meth:`ssl.SSLSocket.recv <socket.socket.recv>` was blocked in one thread, while another | ||
| attempted to call :meth:`ssl.SSLSocket.send <socket.socket.send>` to unblock it in another | ||
| thread. |
aaugustinAug 10, 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.
I'd simplify this as follows:
Fix a regression in 3.13.6 when a call to:meth:`ssl.SSLSocket.recv <socket.socket.recv>` in a thread would block calling:meth:`ssl.SSLSocket.send <socket.socket.send>` in another thread.Indeed, the problem happens even without a dependency betweenrecv andsend.
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.
I don't think we even need to mentionsend at all. Ifrecv was blocked, it's not possible to access most methods on anSSLSocket, because they'll also try to acquire that same lock.
55788a9 intopython:mainUh oh!
There was an error while loading.Please reload this page.
Thanks@ZeroIntensity for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
Sorry,@ZeroIntensity, I could not cleanly backport this to |
Sorry,@ZeroIntensity, I could not cleanly backport this to |
…nGH-137588)Fixes a deadlock in 3.13.6.(cherry picked from commit55788a9)
…pythonGH-137588)Fixes a deadlock in 3.13.6.(cherry picked from commit55788a9)Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
GH-137613 is a backport of this pull request to the3.13 branch. |
mcepl commentedAug 12, 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.
Is it just me or this hunk really doesn't apply cleanly on 3.13.6? --- Modules/_ssl.c+++ Modules/_ssl.c@@ -2612,10 +2609,11 @@ _ssl__SSLSocket_sendfile_impl(PySSLSocket *self, int fd, Py_off_t offset, } do {- PySSL_BEGIN_ALLOW_THREADS(self)+ Py_BEGIN_ALLOW_THREADS retval = SSL_sendfile(self->ssl, fd, (off_t)offset, size, flags); err = _PySSL_errno(retval < 0, self->ssl, (int)retval);- PySSL_END_ALLOW_THREADS(self)+ Py_END_ALLOW_THREADS;+ _PySSL_FIX_ERRNO; self->err = err; if (PyErr_CheckSignals()) { If I am not mistaken Oh, I see, so it is not this whole PR which would be a fix forgh-137583, onlythis one commit? |
ZeroIntensity commentedAug 12, 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.
That was part of the conflict that prevented the automated backport. I had to manually remove that call so we had something that worked for 3.13. (To clarify, |
serhiy-storchaka commentedAug 14, 2025
Reminder about backporting.@ZeroIntensity |
ZeroIntensity commentedAug 14, 2025
Sorry, forgot to update here -- I manually pushed the fix from this PR to the original backport:#137107. |
…nGH-137588)Fixes a deadlock in 3.13.6.
picnixz commentedSep 9, 2025
@python/organization-owners Could you block this user? TiA. |
Uh oh!
There was an error while loading.Please reload this page.
Remove locking around
SSLSocketcalls (and more importantly, calls tosendandrecv). I think they were unnecessary, as OpenSSL seems to be fine with those being called concurrently.The test here is likely a bit flaky and I'm not really sure why. It deadlocks on the current(Test now works correctly without flakiness.)main, but also seems to have periodic deadlocks even whene047a35 is not applied. It's either a bug in OpenSSL, an existing bug inssl, or most likely, a bug in theThreadedEchoServerused for the tests. I've added atime.sleep(0)to intentionally yield the GIL, and that seems to somewhat fix the race condition.