Repository: numpy/numpy
Pull request: BUG: pickle the content of a scalar containing objects, not the address
Fixes gh-13593

Scalars with a void dtype that contains objects were not pickled properly. Add a test and fix by checking the `NPY_LIST_PICKLE` flag of the dtype.

Problem statement:
Segfault after loading pickled void objects
The title is self sufficient. I'm getting segmentation faults after unpickling void numpy objects. That's quite easy to reproduce.

### Reproducing code example:

To reproduce the bug two steps a needed. At first dump the void object:
```python
import pickle
import numpy as np

dt = np.dtype([('name', 'O')])
a = np.empty(1, dtype=dt)
a['name'] = 'eman'

with open('b.pickle', 'wb') as file:
    pickle.dump(a[0], file)
```

Then try to load and print the pickled object from *another* interpreter:
```python
import pickle
print(pickle.load(open('b.pickle', 'rb')))
# Segmentation fault here
```

### Error message:
I'm typing the GDB traceback here.

```
Starting program: /home/balodja/tmp/tmpenv/bin/python a.py
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7ffff4df8700 (LWP 19796)]
[New Thread 0x7ffff45f7700 (LWP 19797)]
[New Thread 0x7ffff1df6700 (LWP 19798)]

Thread 1 "python" received signal SIGSEGV, Segmentation fault.
0x00007ffff6fdfd78 in PyArray_Item_INCREF (data=0xc57570 "\340\065\a\034S\177", descr=0x7fffec232150) at numpy/core/src/multiarray/refcount.c:39
39      numpy/core/src/multiarray/refcount.c: No such file or directory.
```

### Numpy/Python version information:

```
>>> import sys, numpy; print(numpy.__version__, sys.version)
1.16.3 3.7.3rc1 (default, Mar 13 2019, 11:01:15) 
[GCC 8.3.0]
```

Hints from issue discussion:
This only happens when the void type contains objects, right? Looks like we blindly serialize the raw pointer values...
I think the bug is actually in the void *scalar* pickling. The array correctly checks that it cannot do a simple pickle operation, but it will create a list of void objects instead. And those fail the pickling.
Hmm, no, that was wrong I think. Maybe I am missing something earlier on, but it seems to me it should be converting to a list, which should be converting the items to tuples, which I guess is should be fine. Right now I am not quite sure why it even calls into `PyArray_Item_INCREF`, but have to dig some more later probably.

EDIT: Oh man, I am silly, missing that the issue was about void type all along...
Pushing this off to 1.17.2 (if it happens).
What do we want to do about this?
Calling pickle on `a[0]` currently encodes the object's pointer as bytes:
```
>>> a = np.array([(bytearray(b'abcdef'),)], dtype=[('name', 'O')])
>>> a[0].__reduce__()
(<built-in function scalar>, (dtype([('name', 'O')]), b'\xf0qq\xf0\xff\x7f\x00\x00'))
```
The code in `gentype_reduce` to pickle a scalar *does* check for an object dtype, but *not* for a dtype containing an object. The parallel code in `array_reduce` will check that the dtype has the NPY_LIST_PICKLE flag and build a proper value for the pickle.

So step 1 is to add code to check the dtype's flags for NPY_LIST_PICKLE, and if so create something similar to what `array_reduce` produces:
```
>>> r = a[0].__reduce__()
>>> r
(<built-in function scalar>, (dtype([('name', 'O')]), (bytearray(b'abcdef'),)))
```

However, that brings us to step 2 which is to load this pickle, we need to reconstitute it:
```
>> r[0](*r[1])
TypeError: initializing object must be a string
```
So `scalar` (which internally is `array_scalar`) also needs to learn to handle void dtypes.

Commit message: BUG: pickle the content of a scalar containing objects, not the address

Patch to review:
diff --git a/numpy/core/src/multiarray/multiarraymodule.c b/numpy/core/src/multiarray/multiarraymodule.c
index 9169814c24fc..f4ab80be9ed3 100644
--- a/numpy/core/src/multiarray/multiarraymodule.c
+++ b/numpy/core/src/multiarray/multiarraymodule.c
@@ -1891,8 +1891,16 @@ array_scalar(PyObject *NPY_UNUSED(ignored), PyObject *args, PyObject *kwds)
                 &PyArrayDescr_Type, &typecode, &obj)) {
         return NULL;
     }
+    if (PyDataType_FLAGCHK(typecode, NPY_LIST_PICKLE)) {
+        if (!PySequence_Check(obj)) {
+            PyErr_SetString(PyExc_TypeError,
+                            "scalar pickle not returning sequence");
+            return NULL;
+        }
+        dptr = &obj;
+    }
 
-    if (PyDataType_FLAGCHK(typecode, NPY_ITEM_IS_POINTER)) {
+    else if (PyDataType_FLAGCHK(typecode, NPY_ITEM_IS_POINTER)) {
         if (obj == NULL) {
             obj = Py_None;
         }
diff --git a/numpy/core/src/multiarray/scalartypes.c.src b/numpy/core/src/multiarray/scalartypes.c.src
index 32d712e0c9d6..3310bcd29e80 100644
--- a/numpy/core/src/multiarray/scalartypes.c.src
+++ b/numpy/core/src/multiarray/scalartypes.c.src
@@ -1887,6 +1887,22 @@ gentype_reduce(PyObject *self, PyObject *NPY_UNUSED(args))
         mod = ((PyObjectScalarObject *)self)->obval;
         PyTuple_SET_ITEM(ret, 1, Py_BuildValue("NO", obj, mod));
     }
+    else if (obj && PyDataType_FLAGCHK((PyArray_Descr *)obj, NPY_LIST_PICKLE)) {
+        /* a structured dtype with an object in a field */
+        PyObject *arr = PyArray_FromScalar(self, NULL);
+        if (arr == NULL) {
+            return NULL;
+        }
+        PyArrayIterObject *it = (PyArrayIterObject *)PyArray_IterNew(arr);
+        if (it == NULL) {
+            return NULL;
+        }
+        mod = ((PyArray_Descr *)obj)->f->getitem(it->dataptr, arr);
+        Py_DECREF(it);
+        Py_DECREF(arr);
+        /* XXX N does not incref, O does. What do we want? */
+        PyTuple_SET_ITEM(ret, 1, Py_BuildValue("NO", obj, mod));
+    }
     else {
 #ifndef Py_UNICODE_WIDE
         /*
diff --git a/numpy/core/tests/test_records.py b/numpy/core/tests/test_records.py
index c1b794145d19..794f6b9e8cdb 100644
--- a/numpy/core/tests/test_records.py
+++ b/numpy/core/tests/test_records.py
@@ -15,7 +15,7 @@
 from numpy.compat import Path
 from numpy.testing import (
     assert_, assert_equal, assert_array_equal, assert_array_almost_equal,
-    assert_raises, temppath
+    assert_raises, temppath,
     )
 from numpy.compat import pickle
 
@@ -416,6 +416,21 @@ def test_pickle_3(self):
             assert_(pa.flags.writeable)
             assert_(pa.flags.aligned)
 
+    def test_pickle_void(self):
+        # issue gh-13593
+        dt = np.dtype([('obj', 'O')])
+        a = np.empty(1, dtype=dt)
+        data = (bytearray(b'eman'),)
+        a['obj'] = data
+        state = a[0].__reduce__()
+        # If the data is bytes, it will pickle the address of the content
+        # which cannot be saved/restored
+        assert not isinstance(state[1][1], bytes)
+        b = state[0](*state[1])
+        assert b[0] == (bytearray(b'eman'),)
+        # got a fresh version, not the original pointers rebuilt
+        assert b[0] is not data
+
     def test_objview_record(self):
         # https://github.com/numpy/numpy/issues/2599
         dt = np.dtype([('foo', 'i8'), ('bar', 'O')])


Code review comments:
diff_hunk: @@ -1887,6 +1887,22 @@ gentype_reduce(PyObject *self, PyObject *NPY_UNUSED(args))
         mod = ((PyObjectScalarObject *)self)->obval;
         PyTuple_SET_ITEM(ret, 1, Py_BuildValue("NO", obj, mod));
     }
+    else if (obj && PyDataType_FLAGCHK((PyArray_Descr *)obj, NPY_LIST_PICKLE)) {
+        /* a structured dtype with an object in a field */
+        PyObject *arr = PyArray_FromScalar(self, NULL);
+        if (arr == NULL) {
+            return NULL;
+        }
+        PyArrayIterObject *it = (PyArrayIterObject *)PyArray_IterNew(arr);
+        if (it == NULL) {
+            return NULL;
+        }
+        mod = ((PyArray_Descr *)obj)->f->getitem(it->dataptr, arr);
+        Py_DECREF(it);
+        Py_DECREF(arr);
+        /* XXX N does not incref, O does. What do we want? */
+        PyTuple_SET_ITEM(ret, 1, Py_BuildValue("NO", obj, mod));
+    }
path: numpy/core/src/multiarray/scalartypes.c.src
text: @author:
Does this work properly or did I miss a refcount in the call to `Py_BuildValue` by using `N` ? I copied what is done below.

@user1:
N is correct, `obj` is a new reference that you want to destroy.

Having said that, you ought to check the return value of `Py_BuildValue` in case it runs out of memory, and propagate the error.

@author:
ok, will modify it above as well

diff_hunk: @@ -1887,6 +1887,22 @@ gentype_reduce(PyObject *self, PyObject *NPY_UNUSED(args))
         mod = ((PyObjectScalarObject *)self)->obval;
         PyTuple_SET_ITEM(ret, 1, Py_BuildValue("NO", obj, mod));
     }
+    else if (obj && PyDataType_FLAGCHK((PyArray_Descr *)obj, NPY_LIST_PICKLE)) {
+        /* a structured dtype with an object in a field */
+        PyObject *arr = PyArray_FromScalar(self, NULL);
+        if (arr == NULL) {
+            return NULL;
+        }
+        PyArrayIterObject *it = (PyArrayIterObject *)PyArray_IterNew(arr);
+        if (it == NULL) {
+            return NULL;
+        }
+        mod = ((PyArray_Descr *)obj)->f->getitem(it->dataptr, arr);
path: numpy/core/src/multiarray/scalartypes.c.src
text: @user1:
Rather than reusing the unrelated `mod` (module, line 1875), can we rename this to something else? `scalar`? `val`?

@user1:
A comment saying what python type this is would be handy too

@author:
ok

diff_hunk: @@ -416,6 +416,21 @@ def test_pickle_3(self):
             assert_(pa.flags.writeable)
             assert_(pa.flags.aligned)
 
+    def test_pickle_void(self):
+        # issue gh-13593
+        dt = np.dtype([('obj', 'O')])
path: numpy/core/tests/test_records.py
text: @user1:
Can you give this a second field too, just to be sure?

@author:
ok

diff_hunk: @@ -416,6 +416,21 @@ def test_pickle_3(self):
             assert_(pa.flags.writeable)
             assert_(pa.flags.aligned)
 
+    def test_pickle_void(self):
+        # issue gh-13593
+        dt = np.dtype([('obj', 'O')])
+        a = np.empty(1, dtype=dt)
+        data = (bytearray(b'eman'),)
+        a['obj'] = data
+        state = a[0].__reduce__()
+        # If the data is bytes, it will pickle the address of the content
+        # which cannot be saved/restored
+        assert not isinstance(state[1][1], bytes)
path: numpy/core/tests/test_records.py
text: @user1:
I don't quite understand the point you're making here.

@author:
`state` is now `(<built-in function scalar>, (dtype([('obj', 'O')]), (bytearray(b'eman'),)))`. Before it was `(<built-in function scalar>, (dtype([('obj', 'O')]), b'xxxxxxx'))` where `xxxxxxxx` is the address of data. The result of `__reduce__` is passed to `pickle` for storage.

I tried to say that we need to make sure `a[0]` is not stored as a pointer (the address of `data`, rather it should be `a[0]` itself

@user1:
I think then this test would be clearer as
```python
ctor, args = a[0].__reduce__()
# check the contructor is what we expect before interpreting the arguments
assert ctor is np.core.multiarray.scalar
dtype, obj = args
# make sure we did not pickle the address
assert not isinstance(obj, bytes)
```


@author:
adopted