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
Python 3.4.3 regression in ctypes.frombuffer() #69684
Comments
We are seeing a consistent "invalid memory access" crash in Python3.4.3. *Test setup* *Reproducing* If run under python3.4.3-dbg The error is:
In some versions of this code, it had to be run twice, since it would only occur when the pyc files were already created. The attached version does not appear to have this behavior, but run it twice, at least, if you do not see the crash. The code supplied is "minimal", in that removing any line will cause the crash not to occur. *More clues* *Stack trace* #0 0x00007fc0425b4cc9 in __GI_raise (sig=sig@entry=6) The full core dump is available, if it would help. |
The program does not crash for me on gentoo running python 3.4 tip (--with-pydebug). It does crash running gentoo's stock 3.4.3. So, perhaps this bug has already been fixed. |
I get the crash on OSX with recent builds of 3.4 and 3.6: Assertion failed: (self->exports == 0), function mbuf_dealloc, file Objects/memoryobject.c, line 115. |
Crashes on Linux x86-64 for me with and without --with-pydebug, with the default 3.6 branch, 3.5.0, and 3.4 tip. No extra debugging output though in the --with-pydebug cases though. |
I traced this down to part of revision 1da9630e9b7f (Issue bpo-22896). If I revert the changes to CDataType_from_buffer() at <https://hg.python.org/cpython/rev/1da9630e9b7f/#l4.3\>, the crash no longer happens. I suspect the offending code is this, that is trying to stash the original buffer protocol supporting object into a memory view: mv = PyMemoryView_FromBuffer(&buffer);
if (mv == NULL) {
PyBuffer_Release(&buffer);
return NULL;
}
/* Hack the memoryview so that it will release the buffer. */
((PyMemoryViewObject *)mv)->mbuf->master.obj = buffer.obj;
((PyMemoryViewObject *)mv)->view.obj = buffer.obj;
//~ Py_INCREF(buffer.obj);
if (-1 == KeepRef((CDataObject *)result, -1, mv))
result = NULL; If I enable my INCREF() line it also stops the crash, but I guess at the expense of a memory leak. |
We should ultimately do something like I suggested in msg235256. Everything else is a hack (N.B.: the issues that 1da9630e9b7f |
Before the bpo-22896 changes, PyObject_AsWriteBuffer() was used, which requests a buffer with the PyBUF_WRITABLE flag. Currently we use “w*” argument parsing format, which also uses PyBUF_WRITABLE. (Incidentally, I suspect the C-contiguity check is redundant for “w*”; non-contiguous buffers trigger the “read-write” error instead.) Now Eryksun’s patch changes to PyMemoryView_FromObject(), which requests a buffer with the PyBUF_FULL_RO flag (the most liberal), and only then checks for writability and contiguity. Could this be a problem for some kind of object that returns different buffers depending on the request flags? I agree that the existing buffer and memory view APIs don’t seem to be very practical and well understood. PyMemoryView_FromBuffer() is also used in the “io” module. It doesn’t even try to save a reference to the underlying buffers, meaning it is possible for Python code to write into freed memory by saving the memory view object passed to the readinto() method. |
Before committing any solution we first should have understood the cause of the crash. The peculiarity of example ctypes_crash.py is that the argument of ctypes.c_char.from_buffer is a memoryview. With current code we have following chain (the list creates a reference loop with itself):
Using PyMemoryView_FromObject or hypothetical PyMemoryView_FromObjectEx we could get shorter chain:
May be the cause not in current hack, but in memoryview that can't survive with breaking long chain? Or may be we just are lucky in latter case? I share Martin's doubts about writability in Eryksun’s patch. |
If memoryview B is created from memoryview A, then B must be PyMemoryView_FromBuffer() is really a legacy function that |
Per the docs the readonly flag must be consistent for all consumers, |
Okay, so <https://docs.python.org/dev/c-api/buffer.html#c.PyBUF_WRITABLE\> says writability must be consistent. As far as I can see, there is no similar requirement for contiguity. So in theory PyBUF_FULL_RO could produce a discontiguous buffer when PyBUF_WRITABLE would have produced a contiguous one. But I guess this is rather unlikely, so Eryksun’s approach might be good enough. I am starting to wonder why the current memory view hack should even be necessary. It seems like a memory view created by PyMemoryView_FromBuffer() will eventually do the equivalent of PyBuffer_Release(), except that it leaks a reference to the underlying Python object. This was also brought up at <https://bugs.python.org/issue15903#msg170658\>, but the question is not resolved that I can see. Possibly also related is bpo-15821, but I don’t know enough details or history to be sure. |
The current hack isn't necessary. I didn't review eryksun's patch I don't think the other issues you mentioned are closely related: I've opened bpo-25524 in order to make it easier to create views As a practical matter, I think we should just revert the ctypes Then we use the new function from bpo-25524 for 3.6, which should |
I rather inclined to commit eryksun's patch. |
I tend to agree with Serhiy about Eryksun’s patch. I understand it solves the current problem, and continues to protect against this scenario: >>> b = bytearray(b"a")
>>> c = ctypes.c_char.from_buffer(b)
>>> b *= 1000
BufferError: Existing exports of data: object cannot be re-sized PyObject_AsWriteBuffer() and PyBuffer_Release() did get changed in 3.4, also by bpo-22896. Reviewing the history (mainly <https://hg.python.org/cpython/diff/1da9630e9b7f/Objects/abstract.c\>), the only behaviour change I see is that it now clears view->obj to NULL, via the new PyBuffer_Release() call, before calling DECREF on the object. |
I propose this patch, based on Eryksun’s patch (thanks Eryksun). New changes:
|
Added new comments on Rietveld. Would be nice to add tests for read-only and non-contiguous buffers. eryksun, what is your real name? What we have to add in the Misc/ACKS file? |
New patch with Fortran array test, and read-only and reversed memoryview() tests. |
Great! The patch LGTM. I left only two minor suggestions on Rietveld. |
The change 1da9630e9b7f was first published in Python 3.4.3. |
New changeset 9b23f7a94ba9 by Martin Panter in branch '3.4': New changeset 7445de785670 by Martin Panter in branch '3.5': New changeset deb0fb601191 by Martin Panter in branch '3.5': New changeset f3dba13c22e5 by Martin Panter in branch 'default': New changeset 08c56962c24c by Martin Panter in branch 'default': |
Thanks to the various people that helped report and solve this. |
New changeset b04ae5392cf2 by Serhiy Storchaka in branch '3.4': New changeset 3738b0ca1356 by Serhiy Storchaka in branch '3.5': New changeset 559ff2852cf2 by Serhiy Storchaka in branch 'default': |
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: