From c6b09ebe58a968a85c725c3c01a1d6bca16fada0 Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Mon, 1 Sep 2008 15:10:14 +0000 Subject: [PATCH] #3712: The memoryview object had a reference leak and didn't support cyclic garbage collection. Reviewed by Benjamin Peterson. --- Lib/test/test_memoryview.py | 32 ++++++++++++ Misc/NEWS | 3 ++ Objects/memoryobject.c | 100 ++++++++++++++++++++++++------------ 3 files changed, 101 insertions(+), 34 deletions(-) diff --git a/Lib/test/test_memoryview.py b/Lib/test/test_memoryview.py index dca48b1b5b2..0f6fc5534a3 100644 --- a/Lib/test/test_memoryview.py +++ b/Lib/test/test_memoryview.py @@ -6,6 +6,8 @@ XXX We need more tests! Some tests are in test_bytes import unittest import test.support import sys +import gc +import weakref class CommonMemoryTests: @@ -157,6 +159,36 @@ class CommonMemoryTests: m = self.check_attributes_with_type(bytearray) self.assertEquals(m.readonly, False) + def test_getbuffer(self): + # Test PyObject_GetBuffer() on a memoryview object. + b = self.base_object + oldrefcount = sys.getrefcount(b) + m = self._view(b) + oldviewrefcount = sys.getrefcount(m) + s = str(m, "utf-8") + self._check_contents(b, s.encode("utf-8")) + self.assertEquals(sys.getrefcount(m), oldviewrefcount) + m = None + self.assertEquals(sys.getrefcount(b), oldrefcount) + + def test_gc(self): + class MyBytes(bytes): + pass + class MyObject: + pass + + # Create a reference cycle through a memoryview object + b = MyBytes(b'abc') + m = self._view(b) + o = MyObject() + b.m = m + b.o = o + wr = weakref.ref(o) + b = m = o = None + # The cycle must be broken + gc.collect() + self.assert_(wr() is None, wr()) + class MemoryviewTest(unittest.TestCase, CommonMemoryTests): diff --git a/Misc/NEWS b/Misc/NEWS index 78a89ec7058..2f1f58a3083 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -12,6 +12,9 @@ What's New in Python 3.0 release candidate 1 Core and Builtins ----------------- +- Issue #3712: The memoryview object had a reference leak and didn't support + cyclic garbage collection. + - Issue #3668: Fix a memory leak with the "s*" argument parser in PyArg_ParseTuple and friends, which occurred when the argument for "s*" was correctly parsed but parsing of subsequent arguments failed. diff --git a/Objects/memoryobject.c b/Objects/memoryobject.c index bd5820f0f08..fbb1af65d6b 100644 --- a/Objects/memoryobject.c +++ b/Objects/memoryobject.c @@ -3,24 +3,34 @@ #include "Python.h" +static void +dup_buffer(Py_buffer *dest, Py_buffer *src) +{ + *dest = *src; + if (src->shape == &(src->len)) + dest->shape = &(dest->len); + if (src->strides == &(src->itemsize)) + dest->strides = &(dest->itemsize); +} + static int memory_getbuf(PyMemoryViewObject *self, Py_buffer *view, int flags) { - if (view != NULL) { - if (self->view.obj) - Py_INCREF(self->view.obj); - *view = self->view; - } - if (self->view.obj == NULL) - return 0; - return self->view.obj->ob_type->tp_as_buffer->bf_getbuffer( - self->view.obj, NULL, PyBUF_FULL); + int res = 0; + /* XXX for whatever reason fixing the flags seems necessary */ + if (self->view.readonly) + flags &= ~PyBUF_WRITABLE; + if (self->view.obj != NULL) + res = PyObject_GetBuffer(self->view.obj, view, flags); + if (view) + dup_buffer(view, &self->view); + return res; } static void memory_releasebuf(PyMemoryViewObject *self, Py_buffer *view) { - PyBuffer_Release(&self->view); + PyBuffer_Release(view); } PyDoc_STRVAR(memory_doc, @@ -33,18 +43,15 @@ PyMemoryView_FromBuffer(Py_buffer *info) { PyMemoryViewObject *mview; - mview = (PyMemoryViewObject *)PyObject_New(PyMemoryViewObject, - &PyMemoryView_Type); - if (mview == NULL) return NULL; + mview = (PyMemoryViewObject *) + PyObject_GC_New(PyMemoryViewObject, &PyMemoryView_Type); + if (mview == NULL) + return NULL; mview->base = NULL; - /* XXX there should be an API to duplicate a buffer object */ - mview->view = *info; - if (info->shape == &(info->len)) - mview->view.shape = &(mview->view.len); - if (info->strides == &(info->itemsize)) - mview->view.strides = &(mview->view.itemsize); + dup_buffer(&mview->view, info); /* NOTE: mview->view.obj should already have been incref'ed as part of PyBuffer_FillInfo(). */ + _PyObject_GC_TRACK(mview); return (PyObject *)mview; } @@ -60,9 +67,10 @@ PyMemoryView_FromObject(PyObject *base) return NULL; } - mview = (PyMemoryViewObject *)PyObject_New(PyMemoryViewObject, - &PyMemoryView_Type); - if (mview == NULL) return NULL; + mview = (PyMemoryViewObject *) + PyObject_GC_New(PyMemoryViewObject, &PyMemoryView_Type); + if (mview == NULL) + return NULL; mview->base = NULL; if (PyObject_GetBuffer(base, &(mview->view), PyBUF_FULL_RO) < 0) { @@ -72,6 +80,7 @@ PyMemoryView_FromObject(PyObject *base) mview->base = base; Py_INCREF(base); + _PyObject_GC_TRACK(mview); return (PyObject *)mview; } @@ -233,8 +242,9 @@ PyMemoryView_GetContiguous(PyObject *obj, int buffertype, char fort) return NULL; } - mem = PyObject_New(PyMemoryViewObject, &PyMemoryView_Type); - if (mem == NULL) return NULL; + mem = PyObject_GC_New(PyMemoryViewObject, &PyMemoryView_Type); + if (mem == NULL) + return NULL; view = &mem->view; flags = PyBUF_FULL_RO; @@ -245,7 +255,7 @@ PyMemoryView_GetContiguous(PyObject *obj, int buffertype, char fort) } if (PyObject_GetBuffer(obj, view, flags) != 0) { - PyObject_DEL(mem); + Py_DECREF(mem); return NULL; } @@ -253,11 +263,12 @@ PyMemoryView_GetContiguous(PyObject *obj, int buffertype, char fort) /* no copy needed */ Py_INCREF(obj); mem->base = obj; + _PyObject_GC_TRACK(mem); return (PyObject *)mem; } /* otherwise a copy is needed */ if (buffertype == PyBUF_WRITE) { - PyObject_DEL(mem); + Py_DECREF(mem); PyErr_SetString(PyExc_BufferError, "writable contiguous buffer requested " "for a non-contiguousobject."); @@ -265,7 +276,7 @@ PyMemoryView_GetContiguous(PyObject *obj, int buffertype, char fort) } bytes = PyBytes_FromStringAndSize(NULL, view->len); if (bytes == NULL) { - PyBuffer_Release(view); + Py_DECREF(mem); return NULL; } dest = PyBytes_AS_STRING(bytes); @@ -280,7 +291,7 @@ PyMemoryView_GetContiguous(PyObject *obj, int buffertype, char fort) else { if (_indirect_copy_nd(dest, view, fort) < 0) { Py_DECREF(bytes); - PyBuffer_Release(view); + Py_DECREF(mem); return NULL; } } @@ -290,15 +301,16 @@ PyMemoryView_GetContiguous(PyObject *obj, int buffertype, char fort) mem->base = PyTuple_Pack(2, obj, bytes); Py_DECREF(bytes); if (mem->base == NULL) { - PyBuffer_Release(view); + Py_DECREF(mem); return NULL; } } else { - PyBuffer_Release(view); + PyBuffer_Release(view); /* XXX ? */ /* steal the reference */ mem->base = bytes; } + _PyObject_GC_TRACK(mem); return (PyObject *)mem; } @@ -444,6 +456,7 @@ static PyMethodDef memory_methods[] = { static void memory_dealloc(PyMemoryViewObject *self) { + _PyObject_GC_UNTRACK(self); if (self->view.obj != NULL) { if (self->base && PyTuple_Check(self->base)) { /* Special case when first element is generic object @@ -468,7 +481,7 @@ memory_dealloc(PyMemoryViewObject *self) } Py_CLEAR(self->base); } - PyObject_DEL(self); + PyObject_GC_Del(self); } static PyObject * @@ -749,6 +762,25 @@ _notimpl: } +static int +memory_traverse(PyMemoryViewObject *self, visitproc visit, void *arg) +{ + if (self->base != NULL) + Py_VISIT(self->base); + if (self->view.obj != NULL) + Py_VISIT(self->view.obj); + return 0; +} + +static int +memory_clear(PyMemoryViewObject *self) +{ + Py_CLEAR(self->base); + PyBuffer_Release(&self->view); + return 0; +} + + /* As mapping */ static PyMappingMethods memory_as_mapping = { (lenfunc)memory_length, /*mp_length*/ @@ -785,10 +817,10 @@ PyTypeObject PyMemoryView_Type = { PyObject_GenericGetAttr, /* tp_getattro */ 0, /* tp_setattro */ &memory_as_buffer, /* tp_as_buffer */ - Py_TPFLAGS_DEFAULT, /* tp_flags */ + Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC,/* tp_flags */ memory_doc, /* tp_doc */ - 0, /* tp_traverse */ - 0, /* tp_clear */ + (traverseproc)memory_traverse, /* tp_traverse */ + (inquiry)memory_clear, /* tp_clear */ memory_richcompare, /* tp_richcompare */ 0, /* tp_weaklistoffset */ 0, /* tp_iter */