8000 gh-111178: fix UBSan failures in `Modules/_ssl.c` by picnixz · Pull Request #130719 · python/cpython · GitHub
[go: up one dir, main page]

Skip to content

gh-111178: fix UBSan failures in Modules/_ssl.c #130719

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

Merged
merged 10 commits into from
Mar 17, 2025

Conversation

picnixz
Copy link
Member
@picnixz picnixz commented Mar 1, 2025

@ZeroIntensity
Copy link
Member

Probably not the best place to discuss, but I just realized something while applying labels: we probably should have been backporting some of these, otherwise other changes that get backported to these modules will conflict. WDYT? (It's good that Petr is tagged on this, I'd like to hear his opinion too.)

@encukou
Copy link
Member
encukou commented Mar 3, 2025

I don't think we should be backporting all of them preemptively. But if you get conflicts, it's probably best to backport the UB PR first.

@picnixz
Copy link
Member Author
picnixz commented Mar 3, 2025

I haven't backported the changes which caused conflicts on a recent PR but I resolved them manually. I'm afraid that the backports of UB PRs would actually be harder because #128447 was not backported.

@@ -153,13 +153,13 @@ _x509name_print(_sslmodulestate *state, X509_NAME *name, int indent, unsigned lo
* PySSLCertificate_Type
*/

#define _PySSLCertificate_CAST(op) ((PySSLCertificate *)(op))
Copy link
Member

I don't thinkt that this change is useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I myself did the change and I at that time put an underscore. But considering I used the convention "objectname+_CAST" for the series of PR and that I amended some changes I've done recently I think we can refactor this one as well (if the code is however not by me and/or is older, I can revert that change)

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: it's something I've changed last month, and I thought I fixed all the UBSan issues in the SSL module but this wasn't the case. I can revert it if you don't want this additional diff.

@picnixz picnixz requested review from vstinner and encukou March 16, 2025 16:52
@encukou encukou merged commit e708ac8 into python:main Mar 17, 2025
45 checks passed
@picnixz picnixz deleted the fix/ubsan/ssl-111178 branch March 17, 2025 15:50
plashchynski pushed a commit to plashchynski/cpython that referenced this pull request Mar 17, 2025
)

* fix UBSan failures for `PySSLContext`, `PySSLSocket`, `PySSLMemoryBIO`, `PySSLSession`
seehwan pushed a commit to seehwan/cpython that referenced this pull request Apr 16, 2025
)

* fix UBSan failures for `PySSLContext`, `PySSLSocket`, `PySSLMemoryBIO`, `PySSLSession`
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.

4 participants
0