Commits

Lenard Lindstrom  committed 1750c20

Fix reference counting problems with array interface

When the PgObject_GetView C api function in base.c acquired a view for an object
exporting a Python level array interface, a dict object, a reference to the dict
was maintained within the returned Pg_buffer C struct. Due to reference counting
problems, when the Pg_buffer was released with PgBuffer_Release, the dict was
left with a reference count of one too low.

Since in principle a reference is kept on an exporting object until its buffer
is released, it is unnecessary to maintain a reference to the dict or
cobject/capsule that describes the exporting object's buffer. Additionally, it
is unnecessary for an array struct cobject/capsule to keep a reference to the
exporting object. This is the case in NumPy, the definitive use case of the
array interface. This changeset removes the redundant references from array
interface support in base.c. It also corrects the above bug.

  • Participants
  • Parent commits 1092594

Comments (0)

Files changed (2)

 /* Extended array struct */
 typedef struct capsule_interface_s {
     PyArrayInterface inter;
-    PyObject *parent;
     Py_intptr_t imem[1];
 } CapsuleInterface;
 
 /* Py_buffer internal data for an array interface/struct */
 typedef struct view_internals_s {
-    PyObject* cobj;
     char format[4];      /* make 4 byte word sized */
     Py_ssize_t imem[1];
 } ViewInternals;
                                PyObject*,
                                PyObject*,
                                PyObject*,
-                               PyObject*,
                                PyObject*);
 static int _pyinttuple_as_ssize_arr (PyObject*, Py_ssize_t*);
 static int _pytypestr_as_format (PyObject*, char*, Py_ssize_t*);
 static char _as_arrayinter_byteorder (Py_buffer*);
 static int _as_arrayinter_flags (Py_buffer*);
 static CapsuleInterface* _new_capsuleinterface (Py_buffer*);
-static void _free_capsuleinterface (void*);
 #if PY3
-static void _capsule_free_capsuleinterface (PyObject*);
+static void _capsule_PyMem_Free (PyObject*);
 #endif
 static PyObject* _shape_as_tuple (PyArrayInterface*);
 static PyObject* _typekind_as_str (PyArrayInterface*);
         return 0;
     }
 #if PY3
-    capsule = PyCapsule_New (cinter_p, 0, _capsule_free_capsuleinterface);
+    capsule = PyCapsule_New (cinter_p, 0, _capsule_PyMem_Free);
 #else
-    capsule = PyCObject_FromVoidPtr (cinter_p, _free_capsuleinterface);
+    capsule = PyCObject_FromVoidPtr (cinter_p, PyMem_Free);
 #endif
     if (!capsule) {
-        _free_capsuleinterface ((void*)cinter_p);
+        PyMem_Free (cinter_p);
         return 0;
     }
     return capsule;
     }
     cinter_p->inter.data = view_p->buf;
     cinter_p->inter.descr = 0;
-    cinter_p->parent = view_p->obj;
-    Py_XINCREF (cinter_p->parent);
     return cinter_p;
 }
 
-static void
-_free_capsuleinterface (void *p)
-{
-    CapsuleInterface *cinter_p = (CapsuleInterface *)p;
-
-    Py_XDECREF (cinter_p->parent);
-    PyMem_Free (p);
-}
-
 #if PY3
 static void
-_capsule_free_capsuleinterface (PyObject *capsule)
+_capsule_PyMem_Free (PyObject *capsule)
 {
-    _free_capsuleinterface (PyCapsule_GetPointer (capsule, 0));
+    PyMem_Free (PyCapsule_GetPointer (capsule, 0));
 }
 #endif
 
 #endif
     if (!success && GetArrayStruct (obj, &cobj, &inter_p) == 0) {
         if (PgArrayStruct_AsBuffer (pg_view_p, cobj, inter_p, flags)) {
+            Py_DECREF (cobj);
             return -1;
         }
         Py_INCREF (obj);
         view_p->obj = obj;
+        Py_DECREF (cobj);
         success = 1;
     }
     else if (!success) {
 
     if (!success && GetArrayInterface (&dict, obj) == 0) {
         if (PgDict_AsBuffer (pg_view_p, dict, flags)) {
+            Py_DECREF (dict);
             return -1;
         }
         Py_INCREF (obj);
         view_p->obj = obj;
+        Py_DECREF (dict);
         success = 1;
     }
     else if (!success) {
 {
     /* This is deliberately made safe for use on an unitialized *view_p */
     if (view_p->internal) {
-        Py_XDECREF (((ViewInternals*)view_p->internal)->cobj);
         PyMem_Free (view_p->internal);
         view_p->internal = 0;
     }
-    if (view_p->obj) {
-        Py_DECREF (view_p->obj);
-        view_p->obj = 0;
-    }
+    _release_buffer_generic (view_p);
 }
 
 static int
     if (_arraystruct_as_buffer ((Py_buffer*)pg_view_p,
                                 cobj, inter_p, flags)) {
         PgBuffer_Release (pg_view_p);
-        Py_DECREF (cobj);
         return -1;
     }
     return 0;
         PyErr_NoMemory ();
         return -1;
     }
-    internal_p->cobj = 0;
     view_p->internal = internal_p;
     if (PyBUF_HAS_FLAG (flags, PyBUF_FORMAT)) {
         if (_arraystruct_to_format(internal_p->format,
     for (i = 0; i < inter_p->nd; ++i) {
         view_p->len *= (Py_ssize_t)inter_p->shape[i];
     }
-    internal_p->cobj = cobj;
     return 0;
 }
 
         return -1;
     }
     pg_view_p->release_buffer = _release_buffer_array;
-    if (_pyvalues_as_buffer ((Py_buffer*)pg_view_p, flags, dict,
+    if (_pyvalues_as_buffer ((Py_buffer*)pg_view_p, flags,
                              pytypestr, pyshape, pydata, pystrides)) {
         PgBuffer_Release (pg_view_p);
         return -1;
 }
 
 static int
-_pyvalues_as_buffer(Py_buffer* view_p, int flags, PyObject* obj,
+_pyvalues_as_buffer(Py_buffer* view_p, int flags,
                     PyObject* pytypestr,
                     PyObject* pyshape,
                     PyObject* pydata,
         PyErr_NoMemory ();
         return -1;
     }
-    internal_p->cobj = 0;
     view_p->internal = internal_p;
     view_p->format = internal_p->format;
     view_p->shape = internal_p->imem;
     if (!PyBUF_HAS_FLAG (flags, PyBUF_ND)) {
         view_p->ndim = 0;
     }
-    Py_XINCREF (obj);
-    view_p->obj = obj;
     return 0;
 }
 

File test/base_test.py

             v = BufferProxy(o)
             self.assertSame(v, o)
 
+        # Is the dict received from an exporting object properly released?
+        # The dict should be freed before PgObject_GetBuffer returns.
+        # When the BufferProxy v's length property is referenced, v calls
+        # PgObject_GetBuffer, which in turn references Exporter2 o's
+        # __array_interface__ property. The Exporter2 instance o returns a
+        # dict subclass for which it keeps both a regular reference and a
+        # weak reference. The regular reference should be the only
+        # remaining reference when PgObject_GetBuffer returns. This is
+        # verified by first checking the weak reference both before and
+        # after the regular reference held by o is removed.
+
+        import weakref, gc
+
+        class NoDictError(RuntimeError):
+            pass
+
+        class WRDict(dict):
+            """Weak referenceable dict"""
+            pass
+
+        class Exporter2(Exporter):
+            def get__array_interface__2(self):
+                self.d = WRDict(Exporter.get__array_interface__(self))
+                self.dict_ref = weakref.ref(self.d)
+                return self.d
+            __array_interface__ = property(get__array_interface__2)
+            def free_dict(self):
+                self.d = None
+            def is_dict_alive(self):
+                try:
+                    return self.dict_ref() is not None
+                except AttributeError:
+                    raise NoDictError("__array_interface__ is unread")
+
+        o = Exporter2((2, 4), 'u', 4)
+        v = BufferProxy(o)
+        self.assertRaises(NoDictError, o.is_dict_alive)
+        length = v.length
+        self.assertTrue(o.is_dict_alive())
+        o.free_dict()
+        gc.collect()
+        self.assertFalse(o.is_dict_alive())
+
     def test_GetView_array_struct(self):
         from pygame.bufferproxy import BufferProxy
 
             v = BufferProxy(o)
             self.assertSame(v, o)
 
+        # Check returned cobject/capsule reference count
+        try:
+            from sys import getrefcount
+        except ImportError:
+            # PyPy: no reference counting
+            pass
+        else:
+            o = Exporter(shape, typechar, itemsize)
+            self.assertEqual(getrefcount(o.__array_struct__), 1)
+ 
     if (pygame.HAVE_NEWBUF):
         def test_newbuf(self):
             self.NEWBUF_test_newbuf()