Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
ZeroIntensity merged 16 commits intopython:mainfromZeroIntensity:ssl-deadlock
Aug 10, 2025

Conversation

@ZeroIntensity
Copy link
Member

@ZeroIntensityZeroIntensity commentedAug 9, 2025
edited
Loading

Remove locking aroundSSLSocket calls (and more importantly, calls tosend andrecv). 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 currentmain, 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 theThreadedEchoServer used for the tests. I've added atime.sleep(0) to intentionally yield the GIL, and that seems to somewhat fix the race condition. (Test now works correctly without flakiness.)

@ZeroIntensity
Copy link
MemberAuthor

!buildbot ^((?!refleak).)* PR$

@bedevere-bot

This comment was marked as outdated.

@picnixz
Copy link
Member

(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
Copy link
Member

The blurb was ok. We just don't document those methods because I think they are actually available simply asSSLSocket.send. Actually, I think they are wrongly documented (they are documented as methods of the module, not the class)

@ZeroIntensity
Copy link
MemberAuthor

That's frustrating. I can't reproduce it emitting warnings locally, so I thoughtd45f6a8 fixed it.

@picnixz
Copy link
Member

picnixz commentedAug 9, 2025
edited
Loading

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
Copy link
MemberAuthor

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 fixingThreadedEchoServer or switching the test to something more robust.

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
Copy link
MemberAuthor

There we go, that race is finally fixed. Let's try the buildbots again.

@ZeroIntensityZeroIntensity added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelAug 9, 2025
@bedevere-bot
Copy link

🤖 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.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelAug 9, 2025
@@ -0,0 +1,4 @@
Fix a deadlock introduced in the :mod:`ssl` module when a call to
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

(and 3.14.0rc1)

Copy link
MemberAuthor

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Done.

@gpsheadgpshead self-assigned thisAug 9, 2025
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.
Copy link

@aaugustinaaugustinAug 10, 2025
edited
Loading

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.

Copy link
MemberAuthor

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.

aaugustin reacted with thumbs up emoji
@ZeroIntensityZeroIntensityenabled auto-merge (squash)August 10, 2025 13:33
@ZeroIntensityZeroIntensity merged commit55788a9 intopython:mainAug 10, 2025
42 checks passed
@miss-islington-app
Copy link

Thanks@ZeroIntensity for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry,@ZeroIntensity, I could not cleanly backport this to3.14 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker 55788a90967e82a9ea05b45c06a293b46ec53d72 3.14

@miss-islington-app
Copy link

Sorry,@ZeroIntensity, I could not cleanly backport this to3.13 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker 55788a90967e82a9ea05b45c06a293b46ec53d72 3.13

@ZeroIntensityZeroIntensity deleted the ssl-deadlock branchAugust 10, 2025 14:56
ZeroIntensity added a commit to ZeroIntensity/cpython that referenced this pull requestAug 10, 2025
ZeroIntensity added a commit to ZeroIntensity/cpython that referenced this pull requestAug 10, 2025
…pythonGH-137588)Fixes a deadlock in 3.13.6.(cherry picked from commit55788a9)Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@bedevere-app
Copy link

GH-137613 is a backport of this pull request to the3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13bugs and security fixes labelAug 10, 2025
ZeroIntensity added a commit that referenced this pull requestAug 12, 2025
…37588) (GH-137613)Fixes a deadlock introduced in 3.13.6.(cherry picked from commit55788a9)
@mcepl
Copy link
Contributor

mcepl commentedAug 12, 2025
edited
Loading

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 mistakenModules/_ssl.c doesn’t containSSL_sendfile call at all? Is there some other PR which is pre-requisite of this one on the top of the plain 3.13.6?

Oh, I see, so it is not this whole PR which would be a fix forgh-137583, onlythis one commit?

@ZeroIntensity
Copy link
MemberAuthor

ZeroIntensity commentedAug 12, 2025
edited
Loading

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,SSL_sendfile was added in 3.14.)

@serhiy-storchaka
Copy link
Member

Reminder about backporting.@ZeroIntensity

@ZeroIntensity
Copy link
MemberAuthor

Sorry, forgot to update here -- I manually pushed the fix from this PR to the original backport:#137107.

serhiy-storchaka reacted with thumbs up emoji

@ZeroIntensityZeroIntensity removed the needs backport to 3.14bugs and security fixes labelAug 14, 2025
Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull requestAug 19, 2025
@pythonpython deleted a commentSep 9, 2025
@picnixz
Copy link
Member

@python/organization-owners Could you block this user? TiA.

@pythonpython locked asspamand limited conversation to collaboratorsSep 21, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@gpsheadgpsheadgpshead approved these changes

@picnixzpicnixzAwaiting requested review from picnixzpicnixz is a code owner

+1 more reviewer

@aaugustinaaugustinaaugustin left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@gpsheadgpshead

Labels

release-blockertopic-SSLtype-bugAn unexpected behavior, bug, or error

Projects

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ZeroIntensity@bedevere-bot@picnixz@mcepl@serhiy-storchaka@gpshead@aaugustin

[8]ページ先頭

©2009-2025 Movatter.jp