gh-77894: Fix a crash when the GC breaks a loop containing a memoryview

Now a memoryview object can only be cleared if there are no buffers
that refer it.

Author: serhiy-storchaka

Lib/test/pickletester.py

@@ -2323,8 +2323,6 @@ def test_picklebuffer_error(self):
                     'PickleBuffer can only be pickled with protocol >= 5')
 
     def test_non_continuous_buffer(self):
-        if self.pickler is pickle._Pickler:
-            self.skipTest('CRASHES (see gh-122306)')
         for proto in protocols[5:]:
             with self.subTest(proto=proto):
                 pb = pickle.PickleBuffer(memoryview(b"foobar")[::2])
@@ -4399,10 +4397,6 @@ def test_load_from_and_dump_to_file(self):
         unpickled = self.load(stream)
         self.assertEqual(unpickled, data)
 
-    def test_highest_protocol(self):
-        # Of course this needs to be changed when HIGHEST_PROTOCOL changes.
-        self.assertEqual(pickle.HIGHEST_PROTOCOL, 5)
-
     def test_callapi(self):
         f = io.BytesIO()
         # With and without keyword arguments

Lib/test/test_memoryview.py

@@ -18,6 +18,10 @@
 from test.support import import_helper
 
 
+class MyObject:
+    pass
+
+
 class AbstractMemoryTests:
     source_bytes = b"abcdef"
 
@@ -228,8 +232,6 @@ def __init__(self, base):
                     self.m = memoryview(base)
             class MySource(tp):
                 pass
-            class MyObject:
-                pass
 
             # Create a reference cycle through a memoryview object.
             # This exercises mbuf_clear().
@@ -656,5 +658,26 @@ def __bool__(self):
             m[0] = MyBool()
         self.assertEqual(ba[:8], b'\0'*8)
 
+    def test_buffer_reference_loop(self):
+        m = memoryview(b'abc').__buffer__(0)
+        o = MyObject()
+        o.m = m
+        o.o = o
+        wr = weakref.ref(o)
+        del m, o
+        gc.collect()
+        self.assertIsNone(wr())
+
+    def test_picklebuffer_reference_loop(self):
+        pb = pickle.PickleBuffer(memoryview(b'abc'))
+        o = MyObject()
+        o.pb = pb
+        o.o = o
+        wr = weakref.ref(o)
+        del pb, o
+        gc.collect()
+        self.assertIsNone(wr())
+
+
 if __name__ == "__main__":
     unittest.main()

Lib/test/test_pickle.py

@@ -2,6 +2,7 @@
                             NAME_MAPPING, REVERSE_NAME_MAPPING)
 import builtins
 import pickle
+import gc
 import io
 import collections
 import struct
@@ -351,7 +352,6 @@ def setUp(self):
             )
 
         def test_have_gc(self):
-            import gc
             for tp in self._types:
                 with self.subTest(tp=tp):
                     self.assertTrue(gc.is_tracked(tp))
@@ -615,6 +615,24 @@ def test_multiprocessing_exceptions(self):
                                  ('multiprocessing.context', name))
 
 
+class MiscTests(unittest.TestCase):
+    def test_highest_protocol(self):
+        # Of course this needs to be changed when HIGHEST_PROTOCOL changes.
+        self.assertEqual(pickle.HIGHEST_PROTOCOL, 5)
+
+    def test_reference_loop(self):
+        try:
+            pb = pickle.PickleBuffer(memoryview(b"foobar"))
+            a = []
+            a.append(pb)
+            a.append(a)
+        finally:
+            # Should not crash.
+            del a, pb
+            gc.collect()
+            gc.collect()
+
+
 def load_tests(loader, tests, pattern):
     tests.addTest(doctest.DocTestSuite())
     return tests

Misc/NEWS.d/next/Core_and_Builtins/2024-09-10-13-27-16.gh-issue-77894.ZC-Olu.rst

@@ -0,0 +1,4 @@
+Fix possible crash in the garbage collector when it tries to break a
+reference loop containing a :class:`memoryview` object. Now a
+:class:`!memoryview` object can only be cleared if there are no buffers that
+refer it.

Objects/memoryobject.c

@@ -109,8 +109,6 @@ mbuf_release(_PyManagedBufferObject *self)
     if (self->flags&_Py_MANAGED_BUFFER_RELEASED)
         return;
 
-    /* NOTE: at this point self->exports can still be > 0 if this function
-       is called from mbuf_clear() to break up a reference cycle. */
     self->flags |= _Py_MANAGED_BUFFER_RELEASED;
 
     /* PyBuffer_Release() decrements master->obj and sets it to NULL. */
@@ -1096,32 +1094,19 @@ PyBuffer_ToContiguous(void *buf, const Py_buffer *src, Py_ssize_t len, char orde
 /* Inform the managed buffer that this particular memoryview will not access
    the underlying buffer again. If no other memoryviews are registered with
    the managed buffer, the underlying buffer is released instantly and
-   marked as inaccessible for both the memoryview and the managed buffer.
-
-   This function fails if the memoryview itself has exported buffers. */
-static int
+   marked as inaccessible for both the memoryview and the managed buffer. */
+static void
 _memory_release(PyMemoryViewObject *self)
 {
+    assert(self->exports == 0);
     if (self->flags & _Py_MEMORYVIEW_RELEASED)
-        return 0;
+        return;
 
-    if (self->exports == 0) {
-        self->flags |= _Py_MEMORYVIEW_RELEASED;
-        assert(self->mbuf->exports > 0);
-        if (--self->mbuf->exports == 0)
-            mbuf_release(self->mbuf);
-        return 0;
+    self->flags |= _Py_MEMORYVIEW_RELEASED;
+    assert(self->mbuf->exports > 0);
+    if (--self->mbuf->exports == 0) {
+        mbuf_release(self->mbuf);
     }
-    if (self->exports > 0) {
-        PyErr_Format(PyExc_BufferError,
-            "memoryview has %zd exported buffer%s", self->exports,
-            self->exports==1 ? "" : "s");
-        return -1;
-    }
-
-    PyErr_SetString(PyExc_SystemError,
-                    "_memory_release(): negative export count");
-    return -1;
 }
 
 /*[clinic input]
@@ -1134,9 +1119,21 @@ static PyObject *
 memoryview_release_impl(PyMemoryViewObject *self)
 /*[clinic end generated code: output=d0b7e3ba95b7fcb9 input=bc71d1d51f4a52f0]*/
 {
-    if (_memory_release(self) < 0)
+    if (self->exports == 0) {
+        _memory_release(self);
+        Py_RETURN_NONE;
+    }
+
+    if (self->exports > 0) {
+        PyErr_Format(PyExc_BufferError,
+            "memoryview has %zd exported buffer%s", self->exports,
+            self->exports==1 ? "" : "s");
         return NULL;
-    Py_RETURN_NONE;
+    }
+
+    PyErr_SetString(PyExc_SystemError,
+                    "memoryview: negative export count");
+    return NULL;
 }
 
 static void
@@ -1145,7 +1142,7 @@ memory_dealloc(PyObject *_self)
     PyMemoryViewObject *self = (PyMemoryViewObject *)_self;
     assert(self->exports == 0);
     _PyObject_GC_UNTRACK(self);
-    (void)_memory_release(self);
+    _memory_release(self);
     Py_CLEAR(self->mbuf);
     if (self->weakreflist != NULL)
         PyObject_ClearWeakRefs((PyObject *) self);
@@ -1164,8 +1161,10 @@ static int
 memory_clear(PyObject *_self)
 {
     PyMemoryViewObject *self = (PyMemoryViewObject *)_self;
-    (void)_memory_release(self);
-    Py_CLEAR(self->mbuf);
+    if (self->exports == 0) {
+        _memory_release(self);
+        Py_CLEAR(self->mbuf);
+    }
     return 0;
 }