API,BUG: Fix scalar handling in array-interface allowing NULL pointers #29338
Conversation
| goto fail; | ||
| } | ||
| if (PyArray_SETITEM(ret, PyArray_DATA(ret), origin) < 0) { | ||
| if (PyArray_Pack(PyArray_DESCR(ret), PyArray_DATA(ret), origin) < 0) { |
There was a problem hiding this comment.
This probably doesn't matter in practice, but could matter for new dtypes in principle. It's just the right way now.
| } | ||
| else if (result == 1) { | ||
| Py_DECREF(iface); | ||
| Py_DECREF(attr); |
There was a problem hiding this comment.
This is a plain bug-fix, it could (and maybe should) be backported. In this branch attr is explicitly still NULL, since result == 0.
There was a problem hiding this comment.
I agree. This must have been a bug from the GetItemRef migration. Care to make a backport PR for this?
This fixes the scalar handling in the array-interface and also a bug in the shape handling error path. In theory it breaks code that used NULL to trigger the scalar path, but this path was: 1. Completely undocumented, and 2. correctly triggered by *ommitting* the data field instead. I didn't remove/deprecate the scalar path in this PR, maybe we should. But, I do think we can ignore that theoretical use-case since it is nonsensical. Signed-off-by: Sebastian Berg <[email protected]>
seberg
force-pushed
the
array-interface-null
branch
from
9ec8b22 to
10af699
Compare
| } | ||
| else if (result == 1) { | ||
| Py_DECREF(iface); | ||
| Py_DECREF(attr); |
There was a problem hiding this comment.
I agree. This must have been a bug from the GetItemRef migration. Care to make a backport PR for this?
|
OK, one approval seems good enough. If anyone thinks that this weird NULL scalar path is actually used out there, let me know and I am happy to revert if needed. |
numpy#29338) This fixes the scalar handling in the array-interface and also a bug in the shape handling error path. In theory it breaks code that used NULL to trigger the scalar path, but this path was: 1. Completely undocumented, and 2. correctly triggered by *ommitting* the data field instead. I didn't remove/deprecate the scalar path in this PR, maybe we should. But, I do think we can ignore that theoretical use-case since it is nonsensical. Signed-off-by: Sebastian Berg <[email protected]>
2 participants
This fixes the scalar handling in the array-interface and also a bug in the shape handling error path.
In theory it breaks code that used NULL to trigger the scalar path, but this path was:
I didn't remove/deprecate the scalar path in this PR, maybe we should. But, I do think we can ignore that theoretical use-case since it is nonsensical.
Closes gh-26037