BUG: do not memcpy ptr to freed object #8862

mattip · 2017-03-28T13:54:12Z

Running scipy tests with pypy 5.7.0 crashed in these functions when called with array([None]). The returned block of memory holds a pointer-to-Pyobject, but that object has been decref'ed and the pypy garbage collector is free to release the memory. Note that on CPython, small ints are immortal so this does not show up on that interpreter

This does cause permanent allocation of a static variable, but clears the segfault.

I left the decref commented out to make the change more clear, if the idea is acceptable I will amend the pull request to remove the commented out code

eric-wieser · 2017-03-28T14:25:08Z

numpy/core/src/multiarray/convert_datatype.c

@@ -1908,6 +1908,8 @@ _check_object_rec(PyArray_Descr *descr)
    return 0;
 }

+static PyObject * global_zero = NULL;


This could go within the function, if its not used elsewhere. Ditto for one.

juliantaylor · 2017-03-28T14:26:22Z

can you give more context on how scipy crashes?

The way scipy uses these functions is looks wrong and this doesn't really fix it properly, it just avoids it by adding refcounts to the object.
What scipy seems to actually need is a Fill_With_Zero and Is_Zero function

juliantaylor · 2017-03-28T14:32:08Z

numpy/core/src/multiarray/convert_datatype.c

+        global_zero = PyInt_FromLong((long) 0);
+        if (global_zero == NULL)
+        {
+            PyErr_SetNone(PyExc_MemoryError);


the python function should already set the correct exception, so no special handling is necessary
Also our if brace style is:

if (cond) { }

eric-wieser · 2017-03-28T14:34:17Z

numpy/core/src/multiarray/convert_datatype.c

+        global_zero = PyInt_FromLong((long) 0);
+        if (global_zero == NULL)
+        {
+            PyErr_SetNone(PyExc_MemoryError);


Isn't it better just to pass on the internal error here?

eric-wieser · 2017-03-28T14:46:59Z

The way scipy uses these functions is looks wrong

I don't think these functions can be used correctly on object arrays right now anyway - they always return a pointer to an object that likely has refcount = 0. As @mattip mentions, we can get away with it on CPython because the object is cached.

On PyPy, there's nothing useful you can ever do with this pointer - PyArray_Zero and PyArray_One both return pointers to discarded objects, so you can't even tell the difference between them!

This isn't the right fix though - if you look in multiarray/iterators.c at the only place we use PyArray_Zero, you'll see we Py_DECREF the return value as cleanup. So PyArray_Zero should never have decref'd in the first place.

mattip · 2017-03-28T19:37:49Z

@juliantaylor I found this in scipy.signal.medfilt(None), which segfaulted after calling PyArray_Zero in PyArray_OrderFilterND found in sigtoolsmodule.c.

Once I removed the DECREF's, I could successfully complete all but one of the scipy tests with PyPy (disabling test_kde_2d from test_kdeoth.py, it eats all available memory)

mattip · 2017-03-28T19:48:38Z

@eric-wieser What would you suggest instead of removing the Py_DECREFs?

eric-wieser · 2017-03-28T19:56:31Z

@eric-wieser What would you suggest instead of removing the Py_DECREFs?

I'm claiming that there's no need to have the static global, but that removing those is the right call anyway. I think the only sane calling convention of PyArray_Zero might be to return a new reference each time, and require the caller to clean up when using object arrays.

I'm struggling to tell if this is what multiarray/iterators.c is doing or not.

mattip · 2017-03-28T20:24:11Z

IMO multiarray/iterators.c is not calling Py_DECREF but simply free(). The code has a hidden assumption that the object returned from PyArray_Zero and PyArray_One will be within the range of
NSMALLNEGINTS to NSMALLPOSINTS (as used in PyInt_FromLong), and so will be immortal.

Note that AFAICT, the code using PyArray_* in multiarray/iterators.c holds the GIL for the entire time the function runs, so the PyPy GC does not interfere with the (ab)use of the object pointer. However I still claim the Py_DECREF before this pull request is problematic, and sometime someone will write code that proves that :)

eric-wieser · 2017-03-28T21:03:35Z

IMO multiarray/iterators.c is not calling Py_DECREF but simply free().

This is the part that looked suspect to me. iter->constant contains the result of PyArray_Zero

static void neighiter_dealloc(PyArrayNeighborhoodIterObject* iter)
{
    if (iter->mode == NPY_NEIGHBORHOOD_ITER_CONSTANT_PADDING) {
        if (PyArray_ISOBJECT(iter->_internal_iter->ao)) {
            Py_DECREF(*(PyObject**)iter->constant);
        }
    }
    PyDataMem_FREE(iter->constant);
    ...
}

EDIT: Ah, that's not a path that's taken with PyArray_Zero. So yes, this fix is alright.

njsmith · 2017-03-28T23:53:05Z

The PyArray_Zero and PyArray_One APIs look unsalvagable to me. Even if they did return a new reference in the buffer, realistic use cases for these things require making copies of that buffer, and then you're screwed again. It seems clear that when the API was designed they forgot about the existence of non-POD dtypes. I did a quick skim of numpy and scipy and every use looks broken to me.

The approach taken in this PR looks... horrible and will have to be ripped out sooner or later when we rework dtypes. (In particular it hard-codes the assumption that object dtypes are the only kind of non-POD dtype, which is already something that can be violated by user-defined dtypes but probably isn't the only place where this is hardcoded.) But as a short-term hack I don't see any other option.

Going forward IMO we need to deprecate PyArray_Zero and PyArray_One and replace them with an API that makes sense.

eric-wieser · 2017-03-29T00:12:57Z

realistic use cases for these things require making copies of that buffer, and then you're screwed again

Why are you screwed? As long as you Py_INCREF after making that copy and before doing anything with it, you're fine, right?

I did a quick skim of numpy and scipy and every use looks broken to me.

When combined with this patch, numpy looks alright to me, in the one place we use it. That doesn't mean this is a good api, but it does look like a usable one.

eric-wieser · 2017-03-29T00:31:31Z

numpy/core/src/multiarray/convert_datatype.c

-        memcpy(zeroval, &obj, sizeof(PyObject *));
-        Py_DECREF(obj);
+        memcpy(zeroval, &global_zero, sizeof(PyObject *));
+        /*Py_DECREF(obj);*/


What happens without this special case?

njsmith · 2017-03-29T01:10:57Z

@eric-wieser: OK, I spent a while working out how the neighborhood iterator works, and I guess you're right inasmuch as the neighborhood iterator also has a kinda broken API for similar reasons :-). It's not at all clear to me how to make the neighborhood iterator work properly for arbitrary custom dtypes, and if you search google for neighborhood iterator examples then this code is the first thing that comes up, and it's totally wrong for object arrays because of how it screws up the refcounts: http://numpy-discussion.10968.n7.nabble.com/Example-Usage-of-Neighborhood-Iterator-in-Cython-td8757.html

njsmith · 2017-03-29T01:12:53Z

And yeah, technically I guess you can hard-code bits of the object dtype's internal semantics by checking for it and putting in manual INCREF calls but 😭.

eric-wieser · 2017-03-29T01:23:37Z

the neighborhood iterator also has a kinda broken API for similar reasons :-)

Can you elaborate on these? Looks pretty reasonable to me

you can hard-code bits of the object dtype's internal semantics ... but 😭.

Do we even need the special case here? Doesn't setitem handle everything we care about?

mattip · 2017-03-29T20:12:52Z

I updated the pull requests according to the review comments, and tried to summarize the discussion into a warning in the code

eric-wieser · 2017-03-29T20:50:09Z

numpy/core/src/multiarray/convert_datatype.c

@@ -1916,7 +1916,7 @@ PyArray_Zero(PyArrayObject *arr)
 {
    char *zeroval;
    int ret, storeflags;
-    PyObject *obj;
+    static PyObject * zero_obj = NULL;


nitpick: I think I'd prefer the static variable to be first

eric-wieser · 2017-03-29T20:52:37Z

numpy/core/src/multiarray/convert_datatype.c

+        if (zero_obj == NULL) {
+            return NULL;
+        }
+    }
    if (PyArray_ISOBJECT(arr)) {


Same question as before - what goes wrong if we remove this special case?

we ignore the fact that we could not allocate a PyIntOjbect? That seems dangerous, and bad practice. OTOH, if this even occurs we have bigger problems than just this allocation ...

That comment was directed at if (PyArray_ISOBJECT(arr)) {, not the lines above it. What's special about OBJECT_setitem that means we don't want to call it below?

I'm not sure I follow. AFAIK PyArray_DESCR(arr)->f->setitem(one_obj, oneval, arr) will assume oneval is a valid pointer to a block of memory large enough to hold sizeof(PyIntObject), but it is actually a void* NULL at this point, with no valid content. So setitem will effectively call memcpy(oneval, one_obj, sizeof(PyIntObject) which will attempt to copy around 32 bytes into a NULL pointer.

[zero]val is a valid pointer to a block of memory large enough to hold sizeof(PyIntObject), but it is actually a void* NULL

citation needed. I see a zeroval = PyDataMem_NEW(PyArray_DESCR(arr)->elsize); on one of the lines above. Obviously it's not NULL, else the memcpy here would fail.

My analysis was wrong, yours was more correct. But in the end, we cannot use setitem, when I tried I got a segfault in test_simple_object from test_multiarray.

Theoretically PyArray_DESCR(arr)->f->setitem(one_obj, oneval, arr), which ends up on OBJECT_setitem, should work, although it would call Py_INCREF on the PyObject* so we would need a matching Py_DECREF after the call, who would do it and when?

Unfortunately the current implementation crashes since OBJECT_setitem calls Py_XDECREF on oneval (or zeroval) and since the memory was allocated with malloc via PyDataMem_New it is not zeroed out, so the Py_XDECREF crashes the interpreter. Perhaps this is why the original implementer did not call setitem, in addition to the concern about the extra Py_INCREF needing a matching call somewhere.

njsmith · 2017-03-31T05:16:30Z

(Regarding the more general debate about the brokenness or lack thereof of PyArray_Zero and the neighborhood iterator APIs, after having slept on it a bit: I guess it's technically possible to make this kind of "raw pointer" exposure work so long as everyone is very careful to use the appropriate dtype function pointers etc to work with things, but basically it's very tricky to get right – memory owned by an array has a well-defined life cycle for creation, initialization, copying, assignment, destruction, none of which are trivial. If your API is "here's a pointer to a chunk of memory" then you have to replicate all that stuff in an ad hoc way.)

mattip · 2017-04-03T09:23:13Z

I think the latest version of the pull request is the best I can do at this point. So bottom line - is this merge-able or should I close it?

eric-wieser · 2017-04-03T10:09:37Z

Nitpick about declaration order still stands, but this PR looks like an obvious improvement to me, even if there's still an underlying problem - so I'm +1 on merging this.

juliantaylor · 2017-04-03T18:35:12Z

it is ugly but it shouldn't make the situation worse, I guess it can go in.
thanks.

eric-wieser reviewed Mar 28, 2017

View reviewed changes

juliantaylor reviewed Mar 28, 2017

View reviewed changes

eric-wieser reviewed Mar 28, 2017

View reviewed changes

eric-wieser reviewed Mar 29, 2017

View reviewed changes

BUG: do not memcpy ptr to freed object, use static obj instead

3884872

eric-wieser reviewed Mar 29, 2017

View reviewed changes

charris added the component: numpy._core label Mar 29, 2017

juliantaylor merged commit ad7ee70 into numpy:master Apr 3, 2017

mattip deleted the static_objs branch June 1, 2017 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: do not memcpy ptr to freed object #8862

BUG: do not memcpy ptr to freed object #8862

mattip commented Mar 28, 2017

eric-wieser Mar 28, 2017

juliantaylor commented Mar 28, 2017

juliantaylor Mar 28, 2017

eric-wieser Mar 28, 2017

eric-wieser commented Mar 28, 2017

mattip commented Mar 28, 2017

mattip commented Mar 28, 2017

eric-wieser commented Mar 28, 2017 •

edited

mattip commented Mar 28, 2017

eric-wieser commented Mar 28, 2017

njsmith commented Mar 28, 2017

eric-wieser commented Mar 29, 2017 •

edited

eric-wieser Mar 29, 2017

njsmith commented Mar 29, 2017

njsmith commented Mar 29, 2017

eric-wieser commented Mar 29, 2017

mattip commented Mar 29, 2017

eric-wieser Mar 29, 2017

eric-wieser Mar 29, 2017 •

edited

mattip Mar 29, 2017

eric-wieser Mar 29, 2017 •

edited

mattip Mar 29, 2017

eric-wieser Mar 29, 2017 •

edited

mattip Mar 30, 2017

njsmith commented Mar 31, 2017

mattip commented Apr 3, 2017

eric-wieser commented Apr 3, 2017

juliantaylor commented Apr 3, 2017

BUG: do not memcpy ptr to freed object #8862

BUG: do not memcpy ptr to freed object #8862

Conversation

mattip commented Mar 28, 2017

Choose a reason for hiding this comment

juliantaylor commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser commented Mar 28, 2017

mattip commented Mar 28, 2017

mattip commented Mar 28, 2017

eric-wieser commented Mar 28, 2017 • edited

mattip commented Mar 28, 2017

eric-wieser commented Mar 28, 2017

njsmith commented Mar 28, 2017

eric-wieser commented Mar 29, 2017 • edited

Choose a reason for hiding this comment

njsmith commented Mar 29, 2017

njsmith commented Mar 29, 2017

eric-wieser commented Mar 29, 2017

mattip commented Mar 29, 2017

Choose a reason for hiding this comment

eric-wieser Mar 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Mar 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Mar 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njsmith commented Mar 31, 2017

mattip commented Apr 3, 2017

eric-wieser commented Apr 3, 2017

juliantaylor commented Apr 3, 2017

eric-wieser commented Mar 28, 2017 •

edited

eric-wieser commented Mar 29, 2017 •

edited

eric-wieser Mar 29, 2017 •

edited

eric-wieser Mar 29, 2017 •

edited

eric-wieser Mar 29, 2017 •

edited