Skip to content

gh-133157: remove use of _Py_NO_SANITIZE_UNDEFINED in faulthandler #134047

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 18, 2025
Merged

gh-133157: remove use of _Py_NO_SANITIZE_UNDEFINED in faulthandler #134047

merged 3 commits into from
May 18, 2025
+2 −52

Follow Lee on X/Twitter - Father, Husband, Serial builder creating AI, crypto, games & web tools. We are friends :) AI Will Come To Life!

Check out: eBank.nz (Art Generator) | Netwrck.com (AI Tools) | Text-Generator.io (AI API) | BitBank.nz (Crypto AI) | ReadingTime (Kids Reading) | RewordGame | BigMultiplayerChess | WebFiddle | How.nz | Helix AI Assistant

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented May 15, 2025 •
edited
Loading

  • remove _read_null() as it's a tricky one (and the compiler didn't seem to complain)
  • instead of using 1/0 arithmetic, we explicitly raise SIGFPE

@picnixz
fix UBSan failures in faulthandler.c
a0b2967 
- reading from NULL is no more an undefined behavior for C11
- instead of using 1/0 arithmetic, we explicitly raise SIGFPE
@bedevere-app bedevere-app bot mentioned this pull request May 15, 2025
Open
@picnixz picnixz requested review from vstinner and encukou May 15, 2025 12:17
vstinner
vstinner reviewed May 15, 2025
View reviewed changes
vstinner
vstinner reviewed May 15, 2025
View reviewed changes
picnixz
picnixz commented May 15, 2025
View reviewed changes
picnixz added 2 commits May 15, 2025 17:30
@picnixz
Update Modules/faulthandler.c
130da64
@picnixz
remove faulthandler._read_null()
05d7ff1 
Reading from NULL is an undefined behavior and `faulthandler`
should not check for low-level C about undefined behaviors.
@picnixz picnixz requested a review from vstinner May 16, 2025 09:03
@picnixz
Copy link
Member Author

picnixz commented May 16, 2025

Should I actually add a NEWS entry? the function was not documented but it had a docstring.

vstinner
vstinner approved these changes May 17, 2025
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

@vstinner
Copy link
Member

Should I actually add a NEWS entry? the function was not documented but it had a docstring.

It doesn't need a NEWS entry since it's a private function written for test_faulthandler.

@picnixz picnixz merged commit 0a160bf into python:main May 18, 2025
42 checks passed
@picnixz picnixz deleted the fix/ubsan/faulthandler-133157 branch May 18, 2025 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner

@encukou encukou

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet

Follow Lee on X/Twitter - Father, Husband, Serial builder creating AI, crypto, games & web tools. We are friends :) AI Will Come To Life!

Check out: eBank.nz (Art Generator) | Netwrck.com (AI Tools) | Text-Generator.io (AI API) | BitBank.nz (Crypto AI) | ReadingTime (Kids Reading) | RewordGame | BigMultiplayerChess | WebFiddle | How.nz | Helix AI Assistant

2 participants
@picnixz @vstinner

Follow Lee on X/Twitter - Father, Husband, Serial builder creating AI, crypto, games & web tools. We are friends :) AI Will Come To Life!

Check out: eBank.nz (Art Generator) | Netwrck.com (AI Tools) | Text-Generator.io (AI API) | BitBank.nz (Crypto AI) | ReadingTime (Kids Reading) | RewordGame | BigMultiplayerChess | WebFiddle | How.nz | Helix AI Assistant