Skip to content

gh-111178: fix UBSan failures in Modules/posixmodule.c - #129788

Merged
encukou merged 8 commits into
python:mainfrom
picnixz:fix/ubsan/posix-111178
Feb 24, 2025
Merged

gh-111178: fix UBSan failures in Modules/posixmodule.c#129788
encukou merged 8 commits into
python:mainfrom
picnixz:fix/ubsan/posix-111178

Conversation

@picnixz

@picnixzpicnixz commented Feb 7, 2025

Copy link
Copy Markdown
Member

This PR fixes the UBSan failures and addresses some minor cosmetic changes. PEP-7 changes were not applied since they could scramble the diff but other semantic changes affecting the signature of touched functions may have been done.

Comment threadModules/posixmodule.c Outdated

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@picnixz

Copy link
Copy Markdown
MemberAuthor

There was a redundant cast that I missed. I also renamed some parameters that I incorrectly renamed (using Py_UNUSED(ignored) is useless, it doesn't tell you at first glance whether it's a NOARGS or a ONEARG/VARARGS method while Py_UNUSED(dummy), Py_UNUSED(arg) and Py_UNUSED(args) would tell you more).

@encukou
encukou merged commit de13293 into python:mainFeb 24, 2025
@encukou

Copy link
Copy Markdown
Member

using Py_UNUSED(ignored) is useless, it doesn't tell you at first glance whether it's a NOARGS or a ONEARG/VARARGS method while Py_UNUSED(dummy), Py_UNUSED(arg) and Py_UNUSED(args) would tell you more

FWIW, I'm skeptical here -- I don't think we can make it reliably tell you more. But, if it doesn't matter, why not use your suggestions.

@picnixz

Copy link
Copy Markdown
MemberAuthor

don't think we can make it reliably tell you more

The thing is that I saw one usage of void *ignored although it should have been PyObject *unused (#130446), however this was hidden behind a #ifdef MS_WINDOWS and removing the PyCFunction cast introduced a compilation error that I didn't see. For me "ignored" or "unused" means that we can possibly use the argument later. However "dummy" really means for me "it's just a placeholder, we'll never use it later" (I'm only using dummy for NOARGS method, not for VARARGS that actually ignore their args value).

The semantic change I usually do is unused to closure because the closure parameter is generally really for getter/setter and not for methods.

@picnixz
picnixz deleted the fix/ubsan/posix-111178 branch February 24, 2025 12:50
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@picnixz@encukou@vstinner