New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PyMemoryView_FromBuffer() behavior change (possible regression) #60025
Comments
People are using PyMemoryView_FromBuffer() to create and return The docs should perhaps warn against this. |
This may be a bigger problem (grep for image_surface_get_data): http://lists.cairographics.org/archives/cairo/2011-December/022563.html The previous semantics for PyMemoryView_FromBuffer(view) were:
The new semantics for PyMemoryView_FromBuffer(view) are:
PyManaged_Buffer is designed to expect buffers from exporters, |
Having been bitten by an indirect buffer bug in 2.7, I decided to write some tests for 3.3. I added an objview() function to _testbuffer module that creates an indirect view for nested tuples. I have not written unit tests yet, so I'll attach a demo script test_indirect.py. I hope this test case will be helpful in figuring out the right semantics for PyMemoryView_FromBuffer(). My feeling is that memoryview should be able to store shape/strides/suboffsets arrays in its own structure. I also think the only way to achieve that is to make it a PyVarObject and allocate up to sizeof(Py_ssize_t)*ndim*3 bytes after the "view" substructure to store copies of shape/strides/suboffsets arrays. |
PyMemoryViewObject already is a PyVarObject with its own shape, strides The decision to store *exactly* the buffer that is obtained from the Your test case seems to pass here. :) Was it supposed to crash? |
No, I worked real hard to make it pass. :-) I think it would crash 2.7 and 3.2, but I have not tried. I also suspect it leaks memory. Do you think this is something that we should include in the test suit. If so, I will probably open a separate issue and transform test_indirect.py into a patch for test_buffer. |
You are right. I was mistakenly looking at 3.2.3 sources. It looks like there are a lot of undocumented changes here. |
On second thought, it's impossible to crash a memoryview generated The reason is this: All new memoryviews are created through the mbuf_add* Now PyMemoryView_FromBuffer() returns and the managed buffer itself Chained memoryviews are generated from existing memoryviews, and mbuf_add_incomplete_view(mbuf, NULL, ndim) does not access shape, strides If info->format points to a stack address, it would crash both the I've never considered this use of PyMemoryView_FromBuffer(info) legitimate, So the topic is reduced to:
|
I've updated test_indirect.py to print all memoryview fields and cleaned up the _testbuffer patch a little. Note this code that is needed to prevent a memory leak: + /* PyMemoryView_FromBuffer ignores info.obj. Add it explicitely. */ Shouldn't PyMemoryView_FromBuffer() copy non-NULL obj from Py_buffer? |
On Thu, Aug 30, 2012 at 1:22 PM, Stefan Krah <report@bugs.python.org> wrote:
+1 to both ideas. |
Alexander, your test case is brilliant and could eventually go into Here's a patch that restores the info.obj cleanup facility. I intentionally did not add copying the format in order to keep the Apparently PyMemoryView_FromBuffer() is used for low level tinkering and Otherwise people using the cleanup facility will have reference leaks. However, I don't like the interface at all: Passing a pointer to a struct With the ManagedBuffer, we could now write a new function that returns What do you think? Should this go into 3.3.0? The changes to memoryobject.c |
On Fri, Aug 31, 2012 at 2:34 PM, Stefan Krah <report@bugs.python.org> wrote:
I am still getting up to speed with all the changes that went in since |
Alexander Belopolsky <report@bugs.python.org> wrote:
Now I'm puzzled: I thought your goal was to preserve the implicit cleanup Without the patch it's OK to call PyBuffer_Release(info) after |
On Fri, Aug 31, 2012 at 3:12 PM, Stefan Krah <report@bugs.python.org> wrote:
The issue that I raised in msg169472 above was that |
Here is what I think the test case should look like (untested): static PyObject *
memoryview_from_buffer_cleanup(PyObject *self, PyObject *noargs)
{
PyObject *b, *view = NULL;
Py_buffer info;
Py_ssize_t shape[3] = {2, 2, 3};
Py_ssize_t strides[3] = {6, 3, 1};
const char *cp = "abcdefghijkl"; b = PyBytes_FromString(cp);
if (b == NULL)
return NULL;
view = PyMemoryView_FromBuffer(&info);
/* release resources allocated for Py_buffer struct
before it goes out of scope */
PyBuffer_Release(&info);
done:
Py_DECREF(b);
return view;
} |
In general, the number of calls to PyObject_GetBuffer() must be equal to Now, in my test case I omitted the call to PyObject_GetBuffer() *with* No matter how you turn it, if info.obj != NULL you need some internal So you need to know the exporter really well. My strategy here is this: I want to keep backwards compatibility A proper function that returns a memoryview with automatic cleanup Flags: PyMemoryView_FromBufferWithCleanup(info, flags)
{
/* info should never have been obtained by a call to PyObject_GetBuffer().
Hence it should never be released. */
assert(info.obj == NULL);
mbuf = mbuf_alloc();
mbuf->master = *info;
mbuf->flags |= flags;
return new memoryview
} mbuf_dealloc(self)
{
if (self->flags&PyManagedBuffer_FreeBuf)
PyMem_Free(self->master.buf);
...
} This is much more flexible than the existing function and does not suffer |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: