From 4e7d5c25bc1ec0d56c29f2d04cb4278b4e9c321e Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 10 Sep 2024 10:30:04 +0300 Subject: [PATCH 1/2] 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. --- Lib/test/pickletester.py | 2 - Lib/test/test_memoryview.py | 27 ++++++++- ...4-09-10-13-27-16.gh-issue-77894.ZC-Olu.rst | 4 ++ Objects/memoryobject.c | 55 +++++++++---------- 4 files changed, 56 insertions(+), 32 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-09-10-13-27-16.gh-issue-77894.ZC-Olu.rst diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index c7dbd9978941de..7cd44ac8781b59 100644 --- a/Lib/test/pickletester.py +++ b/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]) diff --git a/Lib/test/test_memoryview.py b/Lib/test/test_memoryview.py index 0eb2a367603cfc..2d4bf5f1408df8 100644 --- a/Lib/test/test_memoryview.py +++ b/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() 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 index 00000000000000..a714033dd296b9 --- /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 498a37c1a3d869..a2472d4873641d 100644 --- 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; } From 078da69acbc7eef542fae294136c12706c4f8718 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 10 Sep 2024 15:10:36 +0300 Subject: [PATCH 2/2] Fix the pickle test. --- Lib/test/pickletester.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index 7cd44ac8781b59..334d4dfebdf893 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -2326,7 +2326,7 @@ def test_non_continuous_buffer(self): 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):