ENH: review return values for PyArray_DescrNew by mattip · Pull Request #20960 · numpy/numpy

mattip · 2022-02-01T11:51:11Z

Fixes #19038

Review all the places PyArray_DescrNew and first derivative functions are used, and make sure to check the return value. Functions grepped for:

PyArray_DescrNew (API function)
PyArray_DescrNewFromType (API function)
PyArray_DescrNewByteOrder (API function)
PyArray_DESCR_REPLACE (macro)
ensure_dtype_nbo (helper function)
_descriptor_from_pep3118_format (helper function)
arraydescr_newbyteorder (helper function)

An alternative would be to use exit(1) where malloc fails in PyArray_DescrNew since allocating a new descriptor should only fail on out-of-memory, and there is no real way to recover from that. Here are a few recommendations for using exit(1) (thanks @rgommers):

https://titanwolf.org/Network/Articles/Article?AID=55f68680-740b-4f0b-ad05-a991894108bb.

It can be concluded that for applications above 32 bits, the "out of memory" error handler is useless
https://eli.thegreenplace.net/2009/10/30/handling-out-of-memory-conditions-in-c

The abort policy is simple and familiar: when no memory is available, print a polite error message and exit (abort) the application. This is the most commonly used policy - most command-line tools and desktop applications use it.
As a matter of fact, this policy is so common that most Unix programs use a gnulib library function xmalloc instead of malloc

mattip · 2022-02-01T11:53:13Z

    }
-    descr = PyArray_DescrNewFromType(NPY_FLOAT64);
-    return PyArray_FromString(string, -1, descr, -1, " ");
+    return PyArray_FromString(string, -1, NULL, -1, " ");


Allow PyArray_FromString to allocate the descriptor:

numpy/numpy/core/src/multiarray/ctors.c

Lines 3743 to 3746 in c65bc21

if (dtype == NULL) {

dtype=PyArray_DescrFromType(NPY_DEFAULT_TYPE);

if (dtype == NULL) {

return NULL;

seberg

Thanks! Tricky stuff (although meaningless probability for running into)... a few suggestions that should be good, and 2 places where suggesting didn't do the trick. Let me know if you prefer me applying those changes.

seberg · 2022-02-01T16:56:21Z

@@ -1382,7 +1382,7 @@ PyArray_DescrNewFromType(int type_num)

    old = PyArray_DescrFromType(type_num);
    new = PyArray_DescrNew(old);


This doesn't work PyArray_DescrNew does not accept NULL

seberg · 2022-02-01T17:25:10Z

I don't mind accepting NULL in for PyArray_NewFromDescr (passing through the error). I generally prefer if the caller sanitizes, but it seems a good exception to me here.

Co-authored-by: Sebastian Berg <[email protected]>

mattip · 2022-02-01T23:06:30Z

PyArray_NewFromDescr is an API function, so it is on us to check the inputs.

mattip · 2022-02-01T23:19:38Z

+    if (descr == NULL) {
+#ifdef __GNUC__
+#pragma GCC diagnostic pop
+#endif


Not sure why I had to work so hard to do this check. The function is NUMPY_API, so it is on us to sanitize input. Why does gcc think descr can never be NULL?

I don't agree that API automatically means we have to sanitize, I do not think the Python C-API does this (EDIT: to be clear, does this always, not often, I think also for Python it is pretty common). But it is fairly common for us to do, and in this case I think the pattern "relying" on it (at least theoretically) is common enough to do it.

The generated header include:

NPY_NO_EXPORT NPY_STEALS_REF_TO_ARG(2) NPY_GCC_NONNULL(1) NPY_GCC_NONNULL(2) PyObject * PyArray_NewFromDescr \ (PyTypeObject *, PyArray_Descr *, int, npy_intp const *, npy_intp const *, void *, int, PyObject *);

So we mark this as NONNULL explicitly, actually. This is done here:

numpy/numpy/core/code_generators/numpy_api.py

Line 140 in 855b5ed

'PyArray_NewFromDescr': (94, StealRef(2), NonNull([1, 2])),

Maybe we can get away with not sanitizing inputs (I think best practices is always to be liberal with input and strict with output), but using NonNull on API interfaces seems to me to be an anti-pattern. I will open an issue.

Done in #20971

No idea about the merit of having this in the public API. I think we should just change it to NonNull([1]) for the sake of this PR, though?

If we do we can only put it into 1.23

Hmmm, isn't that fully ABI/API compatible, since it is just an attribute (and relaxing an attribute at that)?

Hmm, lets discuss tomorrow or so. I thought we may not have to put any error at all (anywhere), because passing in NULL should only happen if the NULL exists due to an error being already in-progress. And if that is not true, Python will set a SystemError...

Let's leave that for #20971. For this PR, I ~~pushed the check one level down.~~ removed the check

I removed the check.

Co-authored-by: Sebastian Berg <[email protected]>

The dealloc is now part of the Py_DECREF(ret) and handled there. Doing it here would decref it twice.

It is probably not good to call PyObject_GetIter() if dtype is NULL and an error is already in progress... (If we check for it, lets try to do it right.)

seberg

I pushed a small fixup and removed all custom error messages (to be clear, I am OK with re-adding them).

So effectively, this fixes up a bit of shady error checks, and otherwise now semi-officially allows calling a few/most(?) array-creation functions with a NULL dtype. These functions will then return NULL immediately while assuming that an error is already set.

`DescrNewFromType` cannot fail in most cases, but if it does, DescrNew does not accept NULL as input.

charris · 2022-02-02T20:47:15Z

Thanks Matti, thanks Sebastian.

* ENH: review return value from PyArray_DescrNew* calls * BUG: remove unused variable * BUG: typo * Update numpy/core/src/multiarray/methods.c Co-authored-by: Sebastian Berg <[email protected]> * Update numpy/core/src/multiarray/methods.c Co-authored-by: Sebastian Berg <[email protected]> * Update numpy/core/src/multiarray/getset.c Co-authored-by: Sebastian Berg <[email protected]> * Update numpy/core/src/multiarray/methods.c Co-authored-by: Sebastian Berg <[email protected]> * fixes from review * Update numpy/core/src/umath/ufunc_type_resolution.c Co-authored-by: Sebastian Berg <[email protected]> * move check to internal function * remove check * Remove unnecessary dealloc The dealloc is now part of the Py_DECREF(ret) and handled there. Doing it here would decref it twice. * MAINT: Remove custom error message (and small cleanup) It is probably not good to call PyObject_GetIter() if dtype is NULL and an error is already in progress... (If we check for it, lets try to do it right.) * Fixup DescrNewFromType `DescrNewFromType` cannot fail in most cases, but if it does, DescrNew does not accept NULL as input. Co-authored-by: Sebastian Berg <[email protected]>

ENH: review return values for PyArray_DescrNew (#20960)

charris · 2022-02-03T17:14:24Z

I notice that numpy.sort is also part of the CVE, did we fix that?

seberg · 2022-02-03T17:20:53Z

yes, it was.

* ENH: review return value from PyArray_DescrNew* calls * BUG: remove unused variable * BUG: typo * Update numpy/core/src/multiarray/methods.c Co-authored-by: Sebastian Berg <[email protected]> * Update numpy/core/src/multiarray/methods.c Co-authored-by: Sebastian Berg <[email protected]> * Update numpy/core/src/multiarray/getset.c Co-authored-by: Sebastian Berg <[email protected]> * Update numpy/core/src/multiarray/methods.c Co-authored-by: Sebastian Berg <[email protected]> * fixes from review * Update numpy/core/src/umath/ufunc_type_resolution.c Co-authored-by: Sebastian Berg <[email protected]> * move check to internal function * remove check * Remove unnecessary dealloc The dealloc is now part of the Py_DECREF(ret) and handled there. Doing it here would decref it twice. * MAINT: Remove custom error message (and small cleanup) It is probably not good to call PyObject_GetIter() if dtype is NULL and an error is already in progress... (If we check for it, lets try to do it right.) * Fixup DescrNewFromType `DescrNewFromType` cannot fail in most cases, but if it does, DescrNew does not accept NULL as input. Co-authored-by: Sebastian Berg <[email protected]>

ENH: review return value from PyArray_DescrNew* calls

039612d

mattip commented Feb 1, 2022

View reviewed changes

Comment thread numpy/core/src/multiarray/ctors.c

mattip changed the title ~~Retval~~ ENH: review return values for PyArray_DescrNew Feb 1, 2022

mattip added 01 - Enhancement 09 - Backport-Candidate PRs tagged should be backported labels Feb 1, 2022

mattip added 2 commits February 1, 2022 13:57

BUG: remove unused variable

a298ab7

BUG: typo

d861f9e

seberg reviewed Feb 1, 2022

View reviewed changes

mattip and others added 2 commits February 1, 2022 20:24

Update numpy/core/src/multiarray/methods.c

ead5d09

Co-authored-by: Sebastian Berg <[email protected]>

Update numpy/core/src/multiarray/methods.c

a390b57

Co-authored-by: Sebastian Berg <[email protected]>

mattip mentioned this pull request Feb 1, 2022

Missing return-value validation of the function PyArray_DescrNew #19038

Closed

mattip and others added 2 commits February 2, 2022 00:44

Update numpy/core/src/multiarray/getset.c

ca870df

Co-authored-by: Sebastian Berg <[email protected]>

Update numpy/core/src/multiarray/methods.c

b3a8da7

Co-authored-by: Sebastian Berg <[email protected]>

fixes from review

6d96ec3

mattip commented Feb 1, 2022

View reviewed changes

mattip and others added 5 commits February 2, 2022 01:21

Update numpy/core/src/umath/ufunc_type_resolution.c

e2809ef

Co-authored-by: Sebastian Berg <[email protected]>

move check to internal function

7bbf483

remove check

2b884ab

Remove unnecessary dealloc

d18d273

The dealloc is now part of the Py_DECREF(ret) and handled there. Doing it here would decref it twice.

MAINT: Remove custom error message (and small cleanup)

fc16115

It is probably not good to call PyObject_GetIter() if dtype is NULL and an error is already in progress... (If we check for it, lets try to do it right.)

seberg approved these changes Feb 2, 2022

View reviewed changes

Fixup DescrNewFromType

3384325

`DescrNewFromType` cannot fail in most cases, but if it does, DescrNew does not accept NULL as input.

mattip mentioned this pull request Feb 2, 2022

MAINT: refactor NonNull in API functions #20975

Merged

charris merged commit ac87f07 into numpy:main Feb 2, 2022

charris mentioned this pull request Feb 2, 2022

ENH: review return values for PyArray_DescrNew #20984

Merged

charris removed the 09 - Backport-Candidate PRs tagged should be backported label Feb 2, 2022

charris added a commit that referenced this pull request Feb 3, 2022

Merge pull request #20984 from charris/backport-20960

c12001c

ENH: review return values for PyArray_DescrNew (#20960)

mattip deleted the retval branch December 27, 2022 16:49

	if (dtype == NULL) {
	dtype=PyArray_DescrFromType(NPY_DEFAULT_TYPE);
	if (dtype == NULL) {
	return NULL;

		@@ -1382,7 +1382,7 @@ PyArray_DescrNewFromType(int type_num)

		old = PyArray_DescrFromType(type_num);
		new = PyArray_DescrNew(old);

Conversation

mattip commented Feb 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seberg commented Feb 1, 2022

Uh oh!

mattip commented Feb 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

charris commented Feb 2, 2022

Uh oh!

charris commented Feb 3, 2022

Uh oh!

seberg commented Feb 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seberg Feb 1, 2022 •

edited

Loading

mattip Feb 2, 2022 •

edited

Loading