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

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.

* PySSLCertificate_Type
*/

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

Choose a reason for hiding this comment

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

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 encukou and vstinner 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