Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-124984: Fix ssl thread safety #124993

Merged
merged 60 commits into from
Oct 19, 2024
Merged

Conversation

ZeroIntensity
Copy link
Member

@ZeroIntensity ZeroIntensity commented Oct 5, 2024

As it turns out, OpenSSL doesn't like being called in multiple threads. This adds a per-socket (and per-context and per-session) lock for all OpenSSL calls.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks, that's a lot of locks.

Modules/_ssl.c Outdated Show resolved Hide resolved
Modules/_ssl.c Outdated Show resolved Hide resolved
Modules/_ssl.c Outdated Show resolved Hide resolved
Modules/_ssl.c Outdated Show resolved Hide resolved
Modules/_ssl.c Outdated Show resolved Hide resolved
Modules/_ssl.c Outdated Show resolved Hide resolved
Modules/_ssl.c Outdated Show resolved Hide resolved
ZeroIntensity and others added 3 commits October 5, 2024 10:14
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@vstinner vstinner merged commit 4c53b25 into python:main Oct 19, 2024
37 checks passed
@vstinner
Copy link
Member

Merged, thank you! I replaced "Fix" with "Enhance" in the commit message: "Enhance ssl thread safety" :-)

@ZeroIntensity ZeroIntensity deleted the fix-ssl-threadsafety branch October 19, 2024 21:18
@vstinner
Copy link
Member

Remaining question: should we backport this enhancement (bugfix?) to Python 3.13?

@ZeroIntensity
Copy link
Member Author

Remaining question: should we backport this enhancement (bugfix?) to Python 3.13?

Yeah, I had the same thought. I'll leave that decision to @Yhg1s

@Yhg1s
Copy link
Member

Yhg1s commented Oct 20, 2024

Backport this to 3.13? Uuhm...

image

No.

@vstinner
Copy link
Member

Without this change, the ssl module is not usable on a Free Threaded build (it does easily crash). The change only affects the Free Threaded build: adding @critical_section does nothing in the regular build, and the added test is ignored on regular Python (only run on Free Threading). I would be fine with no backporting it, since Free Threading remains experimental.

@colesbury
Copy link
Contributor

colesbury commented Oct 20, 2024

I think that fixing the SSL module crash in the 3.13 free threading build is important -- lots of basic tasks around HTTP requests are likely to crash without it.

I don't think "lines of code changed" is a good measure of the complexity here -- the Modules/_ssl.c changes are mostly mechanical, and the only behavioral changes is additional locking in the free threading build.

If backporting this PR is a nonstarter than we should consider a smaller, more targeted change that only adds @critical_section to SSLContext methods (without changing getters and setters). That seems less ideal to me, but should fix the common SSL crash and be a pretty small code change.

@colesbury
Copy link
Contributor

colesbury commented Oct 20, 2024

Also, thank you @ZeroIntensity for fixing this bug and @vstinner, @corona10, and everyone else that reviewed the PR.

@vstinner vstinner added the needs backport to 3.13 bugs and security fixes label Oct 21, 2024
@miss-islington-app
Copy link

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

@miss-islington-app
Copy link

Sorry, @ZeroIntensity and @vstinner, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 4c53b2577531c77193430cdcd66ad6385fcda81f 3.13

@vstinner
Copy link
Member

@ZeroIntensity: Automated backport failed. Would you mind to backport the change manually?

With a backport, it might be easier to take a decision on fixing 3.13 or not.

@ZeroIntensity
Copy link
Member Author

Yeah, I can do it later today.

@bedevere-app
Copy link

bedevere-app bot commented Oct 21, 2024

GH-125780 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 21, 2024
ZeroIntensity added a commit to ZeroIntensity/cpython that referenced this pull request Oct 21, 2024
Make SSL objects thread safe in Free Theaded build by
using critical sections.

(cherry picked from commit 4c53b25)

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@vstinner
Copy link
Member

@Yhg1s: Are you still against the backport to 3.13 after @colesbury's comment?

@Yhg1s
Copy link
Member

Yhg1s commented Nov 4, 2024

I'm okay with a backport of just the @critical_section changes (since those expand to nothing in the normal build, and the free-threaded build is experimental anyway). It's the larger refactorings that worry me.

@vstinner
Copy link
Member

vstinner commented Nov 4, 2024

It's the larger refactorings that worry me.

Other changes are tests and changes to use the code declared with @critical_section.

@ZeroIntensity
Copy link
Member Author

There isn't any other refactoring going on here, I just had to switch the getters and setters over to AC for the critical section. Another issue is that not backporting this to 3.13 will also hurt any automatic backports for ssl in the future.

Yhg1s pushed a commit that referenced this pull request Dec 2, 2024
Make SSL objects thread safe in Free Theaded build by
using critical sections.

(cherry picked from commit 4c53b25)

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.