classification
Title: MemoryView memory_getbuf causes segfaults, double call to tp_releasebuffer
Type: crash Stage: resolved
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: duplicate
Dependencies: Superseder: Problems with Py_buffer management in memoryobject.c (and elsewhere?)
View: 10181
Assigned To: Nosy List: mark.dickinson, meador.inge, pitrou, pv, python-dev, skrah, vstinner
Priority: normal Keywords:

Created on 2009-12-03 22:40 by pv, last changed 2012-02-25 11:25 by python-dev. This issue is now closed.

Messages (11)
msg95952 - (view) Author: Pauli Virtanen (pv) * Date: 2009-12-03 22:40
The following code causes a segmentation fault (or glibc error, or other
problems):

>>> x = someobject()
>>> y = memoryview(x)
>>> z = memoryview(y)

The problem is that someobject.bf_releasebuffer will be called two times
with an identical Py_buffer structure. 

This can be seen in memoryobject.c:

static int
memory_getbuf(PyMemoryViewObject *self, Py_buffer *view, int flags)
{
    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;
}

At the end of the call, view and self->view contain identical data
because of the call to dup_buffer.

static void
memory_releasebuf(PyMemoryViewObject *self, Py_buffer *view)
{
    PyBuffer_Release(view);
}

But when the outer memoryview is destroyed, memory_releasebuf calls
PyBuffer_Release for the original object once.

And when the inner memoryview is destroyed, PyBuffer_Release is called
by memory_dealloc the second time. Both calls supply an identical
Py_buffer structure.

Now, if the original object's bf_getbuffer and bf_releasebuffer allocate
some memory dynamically, this will likely cause a double-free of memory,
usually leading to a segmentation fault.

There is no feasible way the bf_releasebuffer can keep track of how many
calls to it have been made. So probably the code in memory_getbuf is
wrong -- at least the dup_buffer function looks wrong.
msg95961 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-12-04 13:29
Why do you say that:

> There is no feasible way the bf_releasebuffer can keep track of how many
> calls to it have been made.

Because that's exactly what e.g. bytearray objects do (see the
ob_exports field in Objects/bytearrayobject.c).
msg95963 - (view) Author: Pauli Virtanen (pv) * Date: 2009-12-04 13:56
> Why do you say that:
> 
> > There is no feasible way the bf_releasebuffer can keep track of how 
> > many calls to it have been made.

I was probably thinking about allocating new temporary arrays for
strides etc. on each *_getbuffer -- if that's done, then manually
keeping track of all the allocated memory seems like a waste of effort
(ie. not feasible).

But yes, if memory allocated for entries in Py_buffer is shared between
all exported buffer views, that sounds better -- for some reason I
didn't think about that... So we'll do it like this in Numpy then.

But still, I take it that the way it currently works is not the intended
behavior? The segmentation faults caused by this came as a bit of a
surprise to me, as the assumption about paired *_getbuffer and
*_releasebuffer calls is very natural.
msg95964 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-12-04 14:05
> I was probably thinking about allocating new temporary arrays for
> strides etc. on each *_getbuffer -- if that's done, then manually
> keeping track of all the allocated memory seems like a waste of effort
> (ie. not feasible).

Yes, I know it looks very painful to do so. I am not responsible for the
Py_buffer / memorview design, however. Travis Oliphant is, and I hear
he's a member of the Numpy community: you might want to ask him for
advice.

(the general problem is that managing Py_buffers can entail memory
allocations, but Py_buffer is not a PyObject and therefore you can't
take advantage of Python's general object management facilities)

> But still, I take it that the way it currently works is not the intended
> behavior? The segmentation faults caused by this came as a bit of a
> surprise to me, as the assumption about paired *_getbuffer and
> *_releasebuffer calls is very natural.

Well, those calls still /are/ paired, aren't they?
There is one odd thing which you must be careful about, it is that
*_getbuffer can be called with a NULL Py_buffer pointer.
msg95965 - (view) Author: Pauli Virtanen (pv) * Date: 2009-12-04 14:24
I think this is an implementation issue in MemoryView rather than an
issue with the buffer interface. PEP 3118 states, "This same bufferinfo
structure must be passed to bf_releasebuffer (if available) when the
consumer is done with the memory." -- this is not guaranteed by the
current MemoryView implementation.

The calls are not paired: the *_getbuf calls fill in structures with
data "view1", and "view2". The *_releasebuf calls receive structures
with data "view1", and "view1". The data filled in the second getbuf
call ("view2") is never passed back to *_releasebuf, as it is
overwritten with "view1" data by dup_buffer.

To work around this, *_releasebuf must be written so that it does not
use the "view" pointer passed to it -- the data structure may have been
shallow copied and any memory pointers in it may have already been freed.

I can try to cook up a patch fixing this.
msg95967 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-12-04 14:39
> To work around this, *_releasebuf must be written so that it does not
> use the "view" pointer passed to it -- the data structure may have been
> shallow copied and any memory pointers in it may have already been freed.
> 
> I can try to cook up a patch fixing this.

If you can do it without breaking the current unit tests
(Lib/test/test_memoryview.py) this would be nice indeed.
msg102277 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2010-04-03 15:38
> I can try to cook up a patch fixing this.

Did you wrote something? I don't see any patch attached to this issue :-/
msg141861 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2011-08-10 12:43
This should be fixed in features/pep-3118.
msg147971 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2011-11-19 21:10
My last comment could be misinterpreted: This *is* actually
already fixed in features/pep-3118.
msg152992 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-02-09 22:31
Closing, since it's fixed in #10181.
msg154239 - (view) Author: Roundup Robot (python-dev) Date: 2012-02-25 11:25
New changeset 3f9b3b6f7ff0 by Stefan Krah in branch 'default':
- Issue #10181: New memoryview implementation fixes multiple ownership
http://hg.python.org/cpython/rev/3f9b3b6f7ff0
History
Date User Action Args
2012-02-25 11:25:29python-devsetnosy: + python-dev
messages: + msg154239
2012-02-09 22:31:39skrahsetstatus: open -> closed
superseder: Problems with Py_buffer management in memoryobject.c (and elsewhere?)
messages: + msg152992

dependencies: - Problems with Py_buffer management in memoryobject.c (and elsewhere?)
resolution: duplicate
stage: needs patch -> resolved
2011-11-19 21:10:37skrahsetmessages: + msg147971
versions: + Python 3.3, - Python 3.1
2011-11-19 16:30:27mark.dickinsonsetassignee: mark.dickinson ->
2011-08-19 01:47:48meador.ingesetstage: needs patch
2011-08-10 12:43:05skrahsetnosy: + skrah
dependencies: + Problems with Py_buffer management in memoryobject.c (and elsewhere?)
messages: + msg141861
2011-01-04 02:19:07pitrousetassignee: mark.dickinson

nosy: + mark.dickinson
2010-04-11 18:38:40meador.ingesetnosy: + meador.inge
2010-04-03 15:38:58vstinnersetnosy: + vstinner
messages: + msg102277
2009-12-04 14:39:26pitrousetmessages: + msg95967
2009-12-04 14:24:50pvsetmessages: + msg95965
2009-12-04 14:05:03pitrousetmessages: + msg95964
2009-12-04 13:56:40pvsetmessages: + msg95963
2009-12-04 13:29:34pitrousetnosy: + pitrou
messages: + msg95961
2009-12-03 22:40:42pvsettype: crash
2009-12-03 22:40:20pvcreate