Problems with Py_buffer management in memoryobject.c (and elsewhere?)

[rupole]

BPO	10181
Nosy	@loewis, @pfmoore, @mdickinson, @ncoghlan, @pitrou, @kristjanvalur, @vstinner, @pv, @skrah, @akheron
Files	pybuffer.diff buffer-interface-clarify.patch: Clarifying the behavior of the buffer interface (for discussion, probably shouldn't be applied as-is) bbe70ca4e0e5.diff: PyManagedBuffer implementation 718801740bde.diff: PyManagedBuffer implementation Rename README to README.rst and enhance formatting #2 78fb1181787a.diff: Add Modules/_testbuffer.c and Lib/test/test_buffer.py 70a8ccd53ade.diff: Fixed suboffsets representation in _testbuffer.c 51cedae5acfc.diff: PyManagedBuffer, native format, complete 1D slicing, casts 4492afe05a07.diff: Handle objects with an index() method. 8dd9f0ea4876.diff a5c4a96dc981.diff issue10181-news.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2012-03-01.08:32:50.822>
created_at = <Date 2010-10-23.18:22:37.367>
labels = ['interpreter-core', 'type-crash']
title = 'Problems with Py_buffer management in memoryobject.c (and elsewhere?)'
updated_at = <Date 2012-04-09.08:15:56.593>
user = 'https://bugs.python.org/rupole'

bugs.python.org fields:

activity = <Date 2012-04-09.08:15:56.593>
actor = 'joshtriplett'
assignee = 'none'
closed = True
closed_date = <Date 2012-03-01.08:32:50.822>
closer = 'skrah'
components = ['Interpreter Core']
creation = <Date 2010-10-23.18:22:37.367>
creator = 'rupole'
dependencies = []
files = ['20282', '20756', '22564', '22577', '22873', '22909', '23119', '23185', '24323', '24381', '24629']
hgrepos = ['36']
issue_num = 10181
keywords = ['patch']
message_count = 147.0
messages = ['119460', '120267', '120268', '120270', '120280', '120281', '120297', '120318', '120319', '120321', '120322', '125296', '125297', '125327', '125328', '125332', '125418', '125424', '125431', '125433', '125462', '125499', '125502', '125507', '125531', '125582', '125618', '125630', '125631', '125635', '125640', '125641', '128475', '128477', '128491', '128495', '128499', '128503', '128505', '128506', '128508', '128514', '128516', '128517', '128518', '128525', '128526', '128527', '128528', '128537', '128538', '128539', '128540', '128542', '128544', '128546', '128551', '128878', '139162', '139172', '139173', '139256', '139257', '139266', '139314', '139316', '139317', '139320', '139323', '139325', '139341', '139747', '139748', '139751', '139758', '139760', '139762', '139764', '139766', '139767', '139775', '139826', '139834', '139846', '139856', '139859', '139861', '139865', '139866', '139868', '139870', '139871', '139872', '139873', '139874', '139875', '139877', '139878', '139883', '139919', '139920', '139921', '139932', '139933', '139935', '139936', '141858', '142191', '143727', '143730', '144234', '151360', '151445', '151447', '151537', '151538', '152086', '152090', '152091', '152124', '152145', '152161', '152173', '152174', '152175', '152188', '152196', '152197', '152206', '152208', '152223', '152245', '152404', '153870', '154138', '154141', '154240', '154323', '154327', '154616', '154651', '154665', '154680', '154764', '154937', '157834', '157835']
nosy_count = 15.0
nosy_names = ['loewis', 'teoliphant', 'paul.moore', 'mark.dickinson', 'ncoghlan', 'rupole', 'pitrou', 'kristjan.jonsson', 'vstinner', 'joshtriplett', 'pv', 'skrah', 'python-dev', 'jcon', 'petri.lehtinen']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue10181'
versions = ['Python 3.3']

[rupole]

There are a number of places in memoryobject.c where get_shape0 is used without the return value being checked. If it fails, this leads to hanging exceptions and crashes.

[pitrou]

Yes, there are probably many holes like this. I've done what I could to make the simple cases (builtin types) to work, but the spec is rotten from the start. Blame the numpy people, sorry.

[ncoghlan]

As far as I know, PEP-3118 serves its purpose of allowing extension modules like numpy and PIL to share data without needing to copy it around all the time. It's just that memoryview wasn't really part of that purpose (since all the affected third party libraries have their own objects for looking at memory), and it shows :P

Our near term goal should be to get memoryview in a good place for handling 1D contiguous data and throw exceptions (rather than crashing) for everything else. I think Antoine has achieved the former, but it sounds like there is still some work to do on the latter.

[pitrou]

As far as I know, PEP-3118 serves its purpose of allowing extension
modules like numpy and PIL to share data without needing to copy it
around all the time. It's just that memoryview wasn't really part of
that purpose (since all the affected third party libraries have their
own objects for looking at memory), and it shows :P

The deeper issue is that there are no ownership or lifetime rules for
dynamically allocated fields in Py_buffer (such as strides and shape),
which itself isn't a PyObject. For very simple objects (such as bytes or
bytearray) it isn't really a problem, but it is in more complicated
situations; and there's no possible solution without a clear spec..

(and I'm not even talking of the issue of making slices of memoryviews,
since the buffer API itself doesn't handle slicing, which means the
memoryview object has to modify its inner Py_buffer...)

[ncoghlan]

Read the "Releasing the buffer" section in PEP-3118 again. Unless I'm misunderstanding you completely, the rules you're asking for are already in place: those fields are entirely the responsibility of the exporting object, and it needs to ensure they remain valid until the buffer is released.

Now, it may be that we haven't *implemented* this correctly or consistently in the builtin objects, in which case we should do something about it. But the rules are there.

[pitrou]

Read the "Releasing the buffer" section in PEP-3118 again. Unless I'm
misunderstanding you completely, the rules you're asking for are
already in place: those fields are entirely the responsibility of the
exporting object, and it needs to ensure they remain valid until the
buffer is released.

In practice, though, we copy Py_buffer structs around and there's no way
for the original object to know about that. That's the fundamental
difference with a PyObject where you would only increase a refcount
instead of copying the structure's contents.

[rupole]

The culprit wrt copying Py_buffer structs seems mainly to be dup_buffer, which is called in memory_getbuf. This seems unnecessary in the case where there's an underlying object and it has returned the view thru its own tp_as_buffer. The underlying object at that point is solely responsible for releasing the buffer, so memory_getbuf shouldn't mess with it at all. In the case where there is no underlying object (mainly thru PyMemoryView_FromBuffer), it probably should allocate any memory in the view in such a way that it can be freed in memory_releasebuf when the view->obj is NULL.

[rupole]

While on the subject, the docs for PyMemoryView_FromBuffer state that the resulting memoryview takes ownership of the Py_buffer struct and is responsible for freeing any associated memory. It does not do so, which is not surprising. The absence of a standard for allocation makes it absolutely impossible for it to do so safely.

[ncoghlan]

Hmm, if we're ever creating a new copy of a Py_buffer without calling GetBuffer again on the original object, then *that* is a bug (exactly comparable to copying a PyObject pointer without incrementing the reference count - it's OK if you can guarantee the lifecycle of your borrowed pointer is shorter than the lifecycle of the original, but disallowed otherwise).

This still doesn't sound like a problem with the spec to me, it sounds like a problem with the implementation strategy that was originally used when integrating the spec into the interpeter core (which I readily agree received far less attention than the spec itself did).

It really sounds like we should be creating a _Py_ManagedBuffer internal object, with each instance containing a PyObject* and a Py_buffer instance. We don't need to alter PEP-3118 to do that - such a change is entirely consumer side, so the objects providing the buffers don't need to care how the lifecycle of the Py_buffer struct is managed.

When we need another reference to the buffer, we can then just increment the refcount of the _Py_ManagedBuffer instance rather than having to call GetBuffer again on the original object.

The contents of Py_buffer objects that have been passed to GetBuffer really shouldn't be getting modified at all - for those cases, we should maintain *two* Py_buffer structs, one (unmodified) with the original results of the GetBuffer call, and a second owned by the caller (for manipulation).

[ncoghlan]

It doesn't help that neither PEP-3118 nor the Py_buffer docs mention the "obj" member of the Py_buffer struct that refers back to the original object providing the buffer - that's fairly fundamental to understanding how PyBuffer_Release and PyMemoryView_FromBuffer can work even in theory.

(Given that, an additional _PyManagedBuffer object shouldn't be necessary - MemoryView just needs to call ReleaseBuffer and GetBuffer at the appropriate times)

[ncoghlan]

So the overall to-do list here is sounding like:

1. Document "Py_buffer->obj" appropriately in both the C API documentation and in PEP-3118

2. Ensure GetBuffer/ReleaseBuffer are used as the moral equivalent of INCREF/DECREF for Py_buffer objects.

3. Check builtin types correctly manage the lifecycle of all elements of their exported Py_buffer structs

4. Check return values and throw exceptions as noted in the original post