Skip to content

BUG: fix improper use of C-API #12524

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 7 commits into from
Dec 14, 2018
Merged

BUG: fix improper use of C-API #12524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e64354e
MAINT: call PySequence_Fast() before any other PySequence_Fast* funcs
mattip Dec 9, 2018
1c7a2f9
BUG: call PyType_Ready before using type
mattip Dec 10, 2018
a3be55e
MAINT: comment, fix from review
mattip Dec 10, 2018
508c0f4
STY: bracket if blocks with braces
mattip Dec 12, 2018
7dcee00
DOC: rearrange comment sequence from review
mattip Dec 13, 2018
b513156
STY: add space after "if"
mattip Dec 13, 2018
ff32c7f
MAINT: unify error messages
mattip Dec 13, 2018
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
15 changes: 13 additions & 2 deletions numpy/core/src/common/ufunc_override.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,19 @@ PyUFuncOverride_GetOutObjects(PyObject *kwds, PyObject **out_kwd_obj, PyObject *
return 0;
}
if (PyTuple_CheckExact(*out_kwd_obj)) {
*out_objs = PySequence_Fast_ITEMS(*out_kwd_obj);
return PySequence_Fast_GET_SIZE(*out_kwd_obj);
/*
* The C-API recommends calling PySequence_Fast before any of the other
* PySequence_Fast* functions. This is required for PyPy
*/
PyObject *seq = PySequence_Fast(*out_kwd_obj, "Could not convert object to sequence");
int ret;
if (seq == NULL) {
return -1;
}
*out_objs = PySequence_Fast_ITEMS(seq);
ret = PySequence_Fast_GET_SIZE(seq);
Py_SETREF(*out_kwd_obj, seq);
Copy link
Member

Choose a reason for hiding this comment

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

The comment above says that *out_kwd_obj is a borrowed reference, but as a result of PySequence_Fast, it's now a new reference, so the caller needs to decref it.

Copy link
Member

Choose a reason for hiding this comment

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

Similarly, Py_SETREF is incorrect here - it's not safe to decref *out_kwd_obj, as you don't own it. Simply

*out_kwd_obj = seq

is what you want here

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. But now seq may not be a borrowed ref. I changed the interface so that *out_kwd_obj is never a borrowed ref.

Copy link
Member Author

Choose a reason for hiding this comment

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

The fact that tests pass indicates there may be a leak of kwds['out'] somewhere.

Copy link
Member

Choose a reason for hiding this comment

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

Tests pass because you were swapping a refcount between seq and the original out_kwd_obj. In CPython, those objects happen to be the same, so you got away with it.

return ret;
}
else {
*out_objs = out_kwd_obj;
Expand Down
8 changes: 7 additions & 1 deletion numpy/core/src/multiarray/common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,12 +440,18 @@ PyArray_DTypeFromObjectHelper(PyObject *obj, int maxdims,
return 0;
}

/* Recursive case, first check the sequence contains only one type */
/*
* The C-API recommends calling PySequence_Fast before any of the other
* PySequence_Fast* functions. This is required for PyPy
*/
Copy link
Member

Choose a reason for hiding this comment

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

I think you might want to move this comment and the PySequence_Fast call to before the /* Recursive case ... */ comment

Copy link
Member Author

Choose a reason for hiding this comment

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

done

seq = PySequence_Fast(obj, "Could not convert object to sequence");
if (seq == NULL) {
goto fail;
}

/* Recursive case, first check the sequence contains only one type */
size = PySequence_Fast_GET_SIZE(seq);
/* objects is borrowed, do not release seq */
objects = PySequence_Fast_ITEMS(seq);
common_type = size > 0 ? Py_TYPE(objects[0]) : NULL;
for (i = 1; i < size; ++i) {
Expand Down
9 changes: 8 additions & 1 deletion numpy/core/src/multiarray/methods.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1003,19 +1003,26 @@ any_array_ufunc_overrides(PyObject *args, PyObject *kwds)
int i;
int nin, nout;
PyObject *out_kwd_obj;
PyObject *fast;
PyObject **in_objs, **out_objs;

/* check inputs */
nin = PyTuple_Size(args);
if (nin < 0) {
return -1;
}
in_objs = PySequence_Fast_ITEMS(args);
fast = PySequence_Fast(args, "Could not convert object to sequence");
if (fast == NULL) {
return -1;
}
in_objs = PySequence_Fast_ITEMS(fast);
for (i = 0; i < nin; ++i) {
if (PyUFunc_HasOverride(in_objs[i])) {
Py_DECREF(fast);
return 1;
}
}
Py_DECREF(fast);
/* check outputs, if any */
nout = PyUFuncOverride_GetOutObjects(kwds, &out_kwd_obj, &out_objs);
if (nout < 0) {
Expand Down
31 changes: 18 additions & 13 deletions numpy/core/src/multiarray/multiarraymodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -982,7 +982,7 @@ PyArray_MatrixProduct2(PyObject *op1, PyObject *op2, PyArrayObject* out)
for (i = 0; i < PyArray_NDIM(ap2) - 2; i++) {
dimensions[j++] = PyArray_DIMS(ap2)[i];
}
if(PyArray_NDIM(ap2) > 1) {
if (PyArray_NDIM(ap2) > 1) {
dimensions[j++] = PyArray_DIMS(ap2)[PyArray_NDIM(ap2)-1];
}

Expand Down Expand Up @@ -1318,7 +1318,7 @@ PyArray_Correlate2(PyObject *op1, PyObject *op2, int mode)
*/
if (inverted) {
st = _pyarray_revert(ret);
if(st) {
if (st) {
goto clean_ret;
}
}
Expand Down Expand Up @@ -1365,7 +1365,7 @@ PyArray_Correlate(PyObject *op1, PyObject *op2, int mode)
}

ret = _pyarray_correlate(ap1, ap2, typenum, mode, &unused);
if(ret == NULL) {
if (ret == NULL) {
goto fail;
}
Py_DECREF(ap1);
Expand Down Expand Up @@ -1654,7 +1654,7 @@ _array_fromobject(PyObject *NPY_UNUSED(ignored), PyObject *args, PyObject *kws)
}

full_path:
if(!PyArg_ParseTupleAndKeywords(args, kws, "O|O&O&O&O&i:array", kwd,
if (!PyArg_ParseTupleAndKeywords(args, kws, "O|O&O&O&O&i:array", kwd,
&op,
PyArray_DescrConverter2, &type,
PyArray_BoolConverter, &copy,
Expand Down Expand Up @@ -2489,7 +2489,7 @@ einsum_sub_op_from_lists(PyObject *args,
"operand and a subscripts list to einsum");
return -1;
}
else if(nop >= NPY_MAXARGS) {
else if (nop >= NPY_MAXARGS) {
PyErr_SetString(PyExc_ValueError, "too many operands");
return -1;
}
Expand Down Expand Up @@ -2724,7 +2724,7 @@ array_arange(PyObject *NPY_UNUSED(ignored), PyObject *args, PyObject *kws) {
static char *kwd[]= {"start", "stop", "step", "dtype", NULL};
PyArray_Descr *typecode = NULL;

if(!PyArg_ParseTupleAndKeywords(args, kws, "O|OOO&:arange", kwd,
if (!PyArg_ParseTupleAndKeywords(args, kws, "O|OOO&:arange", kwd,
&o_start,
&o_stop,
&o_step,
Expand Down Expand Up @@ -2762,7 +2762,7 @@ array__get_ndarray_c_version(PyObject *NPY_UNUSED(dummy), PyObject *args, PyObje
{
static char *kwlist[] = {NULL};

if(!PyArg_ParseTupleAndKeywords(args, kwds, "", kwlist )) {
if (!PyArg_ParseTupleAndKeywords(args, kwds, "", kwlist )) {
return NULL;
}
return PyInt_FromLong( (long) PyArray_GetNDArrayCVersion() );
Expand Down Expand Up @@ -2835,7 +2835,7 @@ array_set_string_function(PyObject *NPY_UNUSED(self), PyObject *args,
int repr = 1;
static char *kwlist[] = {"f", "repr", NULL};

if(!PyArg_ParseTupleAndKeywords(args, kwds, "|Oi:set_string_function", kwlist, &op, &repr)) {
if (!PyArg_ParseTupleAndKeywords(args, kwds, "|Oi:set_string_function", kwlist, &op, &repr)) {
return NULL;
}
/* reset the array_repr function to built-in */
Expand Down Expand Up @@ -3145,7 +3145,7 @@ array_promote_types(PyObject *NPY_UNUSED(dummy), PyObject *args)
PyArray_Descr *d1 = NULL;
PyArray_Descr *d2 = NULL;
PyObject *ret = NULL;
if(!PyArg_ParseTuple(args, "O&O&:promote_types",
if (!PyArg_ParseTuple(args, "O&O&:promote_types",
PyArray_DescrConverter2, &d1, PyArray_DescrConverter2, &d2)) {
goto finish;
}
Expand All @@ -3171,7 +3171,7 @@ array_min_scalar_type(PyObject *NPY_UNUSED(dummy), PyObject *args)
PyArrayObject *array;
PyObject *ret = NULL;

if(!PyArg_ParseTuple(args, "O:min_scalar_type", &array_in)) {
if (!PyArg_ParseTuple(args, "O:min_scalar_type", &array_in)) {
return NULL;
}

Expand Down Expand Up @@ -3248,7 +3248,7 @@ array_datetime_data(PyObject *NPY_UNUSED(dummy), PyObject *args)
PyArray_Descr *dtype;
PyArray_DatetimeMetaData *meta;

if(!PyArg_ParseTuple(args, "O&:datetime_data",
if (!PyArg_ParseTuple(args, "O&:datetime_data",
PyArray_DescrConverter, &dtype)) {
return NULL;
}
Expand All @@ -3267,7 +3267,7 @@ new_buffer(PyObject *NPY_UNUSED(dummy), PyObject *args)
{
int size;

if(!PyArg_ParseTuple(args, "i:buffer", &size)) {
if (!PyArg_ParseTuple(args, "i:buffer", &size)) {
return NULL;
}
return PyBuffer_New(size);
Expand Down Expand Up @@ -4570,6 +4570,10 @@ PyMODINIT_FUNC init_multiarray_umath(void) {
*/
PyArray_Type.tp_hash = PyObject_HashNotImplemented;

if (PyType_Ready(&PyUFunc_Type) < 0) {
goto err;
}

/* Load the ufunc operators into the array module's namespace */
if (InitOperators(d) < 0) {
goto err;
Expand All @@ -4580,8 +4584,9 @@ PyMODINIT_FUNC init_multiarray_umath(void) {
}
initialize_casting_tables();
initialize_numeric_types();
if(initscalarmath(m) < 0)
if (initscalarmath(m) < 0) {
goto err;
}

if (PyType_Ready(&PyArray_Type) < 0) {
goto err;
Expand Down
4 changes: 0 additions & 4 deletions numpy/core/src/umath/umathmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,10 +268,6 @@ int initumath(PyObject *m)
UFUNC_FLOATING_POINT_SUPPORT = 0;
#endif

/* Initialize the types */
if (PyType_Ready(&PyUFunc_Type) < 0)
return -1;

/* Add some symbolic constants to the module */
d = PyModule_GetDict(m);

Expand Down