Skip to content

gh-111178: fix UBSan failures in Objects/complexobject.c #128241

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

Merged
merged 1 commit into from
Jan 10, 2025
Merged

gh-111178: fix UBSan failures in Objects/complexobject.c #128241

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 29 additions & 25 deletions Objects/complexobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
#include "pycore_pymath.h" // _Py_ADJUST_ERANGE2()


#define _PyComplexObject_CAST(op) ((PyComplexObject *)(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 appreciate this cleanup, but, perhaps, you should change other type casts too. E.g. in the COMPLEX_BINOP macro.

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 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...)

Copy link
Member

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.

Copy link
Member Author

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)



/*[clinic input]
class complex "PyComplexObject *" "&PyComplex_Type"
Expand Down Expand Up @@ -553,11 +555,12 @@ PyComplex_AsCComplex(PyObject *op)
}

static PyObject *
complex_repr(PyComplexObject *v)
complex_repr(PyObject *op)
{
int precision = 0;
char format_code = 'r';
PyObject *result = NULL;
PyComplexObject *v = _PyComplexObject_CAST(op);

/* If these are non-NULL, they'll need to be freed. */
char *pre = NULL;
Expand Down Expand Up @@ -609,13 +612,14 @@ complex_repr(PyComplexObject *v)
}

static Py_hash_t
complex_hash(PyComplexObject *v)
complex_hash(PyObject *op)
{
Py_uhash_t hashreal, hashimag, combined;
hashreal = (Py_uhash_t)_Py_HashDouble((PyObject *) v, v->cval.real);
PyComplexObject *v = _PyComplexObject_CAST(op);
hashreal = (Py_uhash_t)_Py_HashDouble(op, v->cval.real);
if (hashreal == (Py_uhash_t)-1)
return -1;
hashimag = (Py_uhash_t)_Py_HashDouble((PyObject *)v, v->cval.imag);
hashimag = (Py_uhash_t)_Py_HashDouble(op, v->cval.imag);
if (hashimag == (Py_uhash_t)-1)
return -1;
/* Note: if the imaginary part is 0, hashimag is 0 now,
Expand Down Expand Up @@ -753,31 +757,30 @@ complex_pow(PyObject *v, PyObject *w, PyObject *z)
}

static PyObject *
complex_neg(PyComplexObject *v)
complex_neg(PyObject *op)
{
PyComplexObject *v = _PyComplexObject_CAST(op);
Py_complex neg;
neg.real = -v->cval.real;
neg.imag = -v->cval.imag;
return PyComplex_FromCComplex(neg);
}

static PyObject *
complex_pos(PyComplexObject *v)
complex_pos(PyObject *op)
{
PyComplexObject *v = _PyComplexObject_CAST(op);
if (PyComplex_CheckExact(v)) {
return Py_NewRef(v);
}
else
return PyComplex_FromCComplex(v->cval);
return PyComplex_FromCComplex(v->cval);
}

static PyObject *
complex_abs(PyComplexObject *v)
complex_abs(PyObject *op)
{
double result;

result = _Py_c_abs(v->cval);

PyComplexObject *v = _PyComplexObject_CAST(op);
double result = _Py_c_abs(v->cval);
if (errno == ERANGE) {
PyErr_SetString(PyExc_OverflowError,
"absolute value too large");
Expand All @@ -787,8 +790,9 @@ complex_abs(PyComplexObject *v)
}

static int
complex_bool(PyComplexObject *v)
complex_bool(PyObject *op)
{
PyComplexObject *v = _PyComplexObject_CAST(op);
return v->cval.real != 0.0 || v->cval.imag != 0.0;
}

Expand Down Expand Up @@ -1339,16 +1343,16 @@ static PyMemberDef complex_members[] = {
};

static PyNumberMethods complex_as_number = {
(binaryfunc)complex_add, /* nb_add */
(binaryfunc)complex_sub, /* nb_subtract */
(binaryfunc)complex_mul, /* nb_multiply */
complex_add, /* nb_add */
Copy link
Member

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,
    [...]
};

Copy link
Member Author

@picnixz picnixz Dec 25, 2024 •
edited
Loading

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)

Copy link
Member

@skirpichev skirpichev Dec 25, 2024 •
edited
Loading

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.

Copy link
Member Author

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.

Copy link
Member

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:)

complex_sub, /* nb_subtract */
complex_mul, /* nb_multiply */
0, /* nb_remainder */
0, /* nb_divmod */
(ternaryfunc)complex_pow, /* nb_power */
(unaryfunc)complex_neg, /* nb_negative */
(unaryfunc)complex_pos, /* nb_positive */
(unaryfunc)complex_abs, /* nb_absolute */
(inquiry)complex_bool, /* nb_bool */
complex_pow, /* nb_power */
complex_neg, /* nb_negative */
complex_pos, /* nb_positive */
complex_abs, /* nb_absolute */
complex_bool, /* nb_bool */
0, /* nb_invert */
0, /* nb_lshift */
0, /* nb_rshift */
Expand All @@ -1369,7 +1373,7 @@ static PyNumberMethods complex_as_number = {
0, /* nb_inplace_xor */
0, /* nb_inplace_or */
0, /* nb_floor_divide */
(binaryfunc)complex_div, /* nb_true_divide */
complex_div, /* nb_true_divide */
0, /* nb_inplace_floor_divide */
0, /* nb_inplace_true_divide */
};
Expand All @@ -1384,11 +1388,11 @@ PyTypeObject PyComplex_Type = {
0, /* tp_getattr */
0, /* tp_setattr */
0, /* tp_as_async */
(reprfunc)complex_repr, /* tp_repr */
complex_repr, /* tp_repr */
&complex_as_number, /* tp_as_number */
0, /* tp_as_sequence */
0, /* tp_as_mapping */
(hashfunc)complex_hash, /* tp_hash */
complex_hash, /* tp_hash */
0, /* tp_call */
0, /* tp_str */
PyObject_GenericGetAttr, /* tp_getattro */
Expand Down
Loading