[3.13] gh-77894: Fix a crash when the GC breaks a loop containing a memoryview (GH...

gh-77894: Fix a crash when the GC breaks a loop containing a memoryview (GH-123898)

Now a memoryview object can only be cleared if there are no buffers
that refer it.
(cherry picked from commit a1dbf2ea69acc6ccee6292709af1dadd55c068be)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py
index 251ed0c2bc3edff769ca591526a3c22218249cf9..7321546a7477ac231a463416383b070c5e6fc8ba 100644 (file)
--- a/Lib/test/pickletester.py
+++ b/Lib/test/pickletester.py
@@ -2017,12 +2017,10 @@ class AbstractPicklingErrorTests:
                     '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])
-                with self.assertRaises(pickle.PicklingError):
+                with self.assertRaises((pickle.PicklingError, BufferError)):
                     self.dumps(pb, proto)
 
     def test_buffer_callback_error(self):

diff --git a/Lib/test/test_memoryview.py b/Lib/test/test_memoryview.py
index 0eb2a367603cfc9c59645296767039169a588361..2d4bf5f1408df8b83ce84cc39b475873effdd67e 100644 (file)
--- a/Lib/test/test_memoryview.py
+++ b/Lib/test/test_memoryview.py
@@ -18,6 +18,10 @@ import struct
 from test.support import import_helper
 
 
+class MyObject:
+    pass
+
+
 class AbstractMemoryTests:
     source_bytes = b"abcdef"
 
@@ -228,8 +232,6 @@ class AbstractMemoryTests:
                     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 @@ class OtherTest(unittest.TestCase):
             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()

diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-09-10-13-27-16.gh-issue-77894.ZC-Olu.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-09-10-13-27-16.gh-issue-77894.ZC-Olu.rst
new file mode 100644 (file)
index 0000000..a714033
--- /dev/null
+++ b/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.

diff --git a/Objects/memoryobject.c b/Objects/memoryobject.c
index 226bd6defdec5a4ea62a7458f0f64a3ce2f71ee1..4f87236111792757d63f3b4aca2d407c4443e898 100644 (file)
--- a/Objects/memoryobject.c
+++ b/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;
 }