-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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-111178: fix UBSan failures in Objects/complexobject.c
#128241
Conversation
picnixz
commented
Dec 25, 2024
•
edited by bedevere-app
bot
Loading
edited by bedevere-app
bot
- Issue: UBSan: Calling a function through pointer to incorrect function type is undefined behavior #111178
@@ -14,6 +14,8 @@ | |||
#include "pycore_pymath.h" // _Py_ADJUST_ERANGE2() | |||
|
|||
|
|||
#define _PyComplexObject_CAST(op) ((PyComplexObject *)(op)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate this cleanup, but, perhaps, you should change other type casts too. E.g. in the COMPLEX_BINOP macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to keep a small diff but I can do that (as you can see, I've opened 20 PRs doing the same mechanical changes...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do this mechanically (i.e. with sed), I would guess you should take some extra steps to prevent other changes in same file.
Perhaps, it's better to not introduce a new macro and stick with (PyComplexObject *)
casts. Or change everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When Victor and I started fixing #111178, we introduced a macro so that we can (in the future), possibly add type-checks easily. By "mechanical" changes, I manually changed everything (but everytime I repeated the same steps)
(binaryfunc)complex_add, /* nb_add */ | ||
(binaryfunc)complex_sub, /* nb_subtract */ | ||
(binaryfunc)complex_mul, /* nb_multiply */ | ||
complex_add, /* nb_add */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This structure modified a lot. Maybe it's an opportunity to change it to include only added entries in style:
static PyNumberMethods complex_as_number = {
.nb_add = complex_add,
.nb_subtract = complex_sub,
.nb_multiply = complex_mul,
.nb_power = complex_pow,
[...]
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to do that as per #127679 but since Serhiy wanted a discussion first (#127679 (comment)) and since others wanted PEP-7 to be updated, I decided not to do that kind of cosmetic change (I admit that there are other cosmetic changes that I've done but those wouldn't cause conflicts or require a discussion IMO, they're just for consistency)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no discussion opened so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh.. then.. maybe I can do the change? well.. I actually did it for internal structures, but for exported ones, I don't think I did it in any of the other PR. I'll ask on the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do the change?
Well, discussion is not started so far. That's all:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to expand the PR's scope. Feel free to send a follow-up PR.