classification
Title: Python 3.4.3 regression in ctypes.frombuffer()
Type: crash Stage: resolved
Components: ctypes Versions: Python 3.6, Python 3.4, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: JakeMont, eryksun, jnoller, martin.panter, python-dev, r.david.murray, sbt, serhiy.storchaka, skrah, vstinner, zach.ware
Priority: high Keywords: 3.4regression, patch

Created on 2015-10-28 18:06 by JakeMont, last changed 2015-11-16 16:46 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
pythoncrash.py JakeMont, 2015-10-28 18:06 This is python code that will trigger the crash
ctypes_crash.py eryksun, 2015-10-29 04:34 Trigger a crash, isolated to ctypes from_buffer.
ctypes_from_buffer_2.patch eryksun, 2015-10-29 07:17 Use PyMemoryView_FromObject instead of PyMemoryView_FromBuffer. review
ctypes_from_buffer_3.patch martin.panter, 2015-11-11 04:31 review
ctypes_from_buffer_4.patch martin.panter, 2015-11-13 00:07 review
Messages (22)
msg253613 - (view) Author: Jake Montgomery (JakeMont) Date: 2015-10-28 18:06
We are seeing a consistent "invalid memory access" crash in Python3.4.3. 

*Test setup* 
This is occurring on Ubuntu 14.04.1 LTS. It is occurring on multiple unrelated installs, but has not been tested with any other operating system. It occurs using the "Python 3.4.3 (default, Oct 14 2015, 20:28:29)" that is installed using apt-get. It does *not* appear to occur using Python 3.4.0. Other versions of python were not tested. 

*Reproducing*
I was able to reduce the code needed to a pretty minimal python program, which is attached. Running this as "pyton3 ./pythoncrash.py" will trigger the crash, and a core dump if enabled. 

If run under python3.4.3-dbg The error is:

    python3.4-dbg: ../Objects/memoryobject.c:115: mbuf_dealloc: Assertion `self->exports == 0' failed.

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*
I suspect that the key is the multiprocessing.Value object, and its destruction. The other "import" statements may just be happenstance needed to create the correct memory conditions for the crash. I was able to start "stubbing out" the actual code in those imported packages, and the crash continued as long as the number of functions, classes, and class dependencies were maintained. But that became too tedious. In earlier versions of the test code, adding or removing significant numbers of function or classes could make the crash not manifest. 

*Stack trace*
Running under python3.4.3-dbg, gdb gives the following stack trace:

#0  0x00007fc0425b4cc9 in __GI_raise (sig=sig@entry=6)
    at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007fc0425b80d8 in __GI_abort () at abort.c:89
#2  0x00007fc0425adb86 in __assert_fail_base (
    fmt=0x7fc0426fe830 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
    assertion=assertion@entry=0x707199 "self->exports == 0",
    file=file@entry=0x70715a "../Objects/memoryobject.c", line=line@entry=115,
    function=function@entry=0x708923 <__PRETTY_FUNCTION__.10355> "mbuf_dealloc") at assert.c:92
#3  0x00007fc0425adc32 in __GI___assert_fail (assertion=0x707199 "self->exports == 0",
    file=0x70715a "../Objects/memoryobject.c", line=115,
    function=0x708923 <__PRETTY_FUNCTION__.10355> "mbuf_dealloc") at assert.c:101
#4  0x00000000004beb92 in mbuf_dealloc (self=0x7fc042068058) at ../Objects/memoryobject.c:115
#5  0x00000000004cd67b in _Py_Dealloc (op=<managedbuffer at remote 0x7fc042068058>)
    at ../Objects/object.c:1749
#6  0x00000000004c0ec6 in memory_clear (self=0x7fc042026bf8) at ../Objects/memoryobject.c:1079
#7  0x00000000004409c3 in delete_garbage (collectable=0x7fff21065180,
    old=0x9a4f40 <generations+64>) at ../Modules/gcmodule.c:866
#8  0x0000000000440f56 in collect (generation=2, n_collected=0x7fff210651e8,
    n_uncollectable=0x7fff210651f0, nofail=0) at ../Modules/gcmodule.c:1032
#9  0x000000000044144e in collect_with_callback (generation=2) at ../Modules/gcmodule.c:1140
#10 0x00000000004421f2 in PyGC_Collect () at ../Modules/gcmodule.c:1616
#11 0x00000000004226a0 in Py_Finalize () at ../Python/pythonrun.c:607
#12 0x0000000000428f38 in Py_Exit (sts=0) at ../Python/pythonrun.c:2713
#13 0x0000000000426077 in handle_system_exit () at ../Python/pythonrun.c:1812
#14 0x00000000004260a2 in PyErr_PrintEx (set_sys_last_vars=1) at ../Python/pythonrun.c:1822
#15 0x0000000000425cef in PyErr_Print () at ../Python/pythonrun.c:1718
#16 0x00000000004255c4 in PyRun_SimpleFileExFlags (fp=0x29824c0,
    filename=0x7fc042442b68 "./pythoncrash.py", closeit=1, flags=0x7fff21065520)
    at ../Python/pythonrun.c:1611
#17 0x0000000000424418 in PyRun_AnyFileExFlags (fp=0x29824c0,
    filename=0x7fc042442b68 "./pythoncrash.py", closeit=1, flags=0x7fff21065520)
    at ../Python/pythonrun.c:1292
#18 0x000000000043e9fb in run_file (fp=0x29824c0, filename=0x28b62f0 L"./pythoncrash.py",
    p_cf=0x7fff21065520) at ../Modules/main.c:319
#19 0x000000000043f781 in Py_Main (argc=2, argv=0x28b5020) at ../Modules/main.c:751
#20 0x000000000041e6d6 in main (argc=2, argv=0x7fff21065798) at ../Modules/python.c:69

The full core dump is available, if it would help.
msg253616 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-10-28 18:50
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.
msg253617 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-10-28 19:16
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.
Abort trap: 6
msg253639 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-10-28 22:47
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.
msg253646 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-10-29 00:29
I traced this down to part of revision 1da9630e9b7f (Issue #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.
msg253682 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-10-29 16:00
We should ultimately do something like I suggested in msg235256.

Everything else is a hack (N.B.: the issues that 1da9630e9b7f
tried to fix were also hacks, so it isn't really the fault of
that commit).
msg253702 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-10-29 23:37
Before the Issue 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.
msg253749 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-10-30 17:39
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):

    list->memoryview->ManagedBuffer->memoryview->ManagedBuffer->bytearray

Using PyMemoryView_FromObject or hypothetical PyMemoryView_FromObjectEx we could get shorter chain:

    list->memoryview->ManagedBuffer->bytearray

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.
msg253756 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-10-30 21:01
If memoryview B is created from memoryview A, then B must be
registered with the same ManagedBuffer as A, otherwise the
whole scheme breaks down (PyMemoryView_FromObject() performs
this check).


PyMemoryView_FromBuffer() is really a legacy function that
should be only used for creating temporary views within a
function.  As far as I know, it has usually been used that
way from the start (since 3.0).
msg253758 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-10-30 21:19
Per the docs the readonly flag must be consistent for all consumers,
so checking for writability after getting the view should be okay in
principle.
msg253776 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-10-31 05:48
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 Issue 15821, but I don’t know enough details or history to be sure.
msg253789 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-10-31 14:28
The current hack isn't necessary. I didn't review eryksun's patch
in depth, but on the surface it looks good.

I don't think the other issues you mentioned are closely related:
The cause here is that the obj fields of both the second memoryview
and the second mbuf are manually set to the address of the first
memoryview, which plays havoc with the rather complex deallocation
scheme.

I've opened #25524 in order to make it easier to create views
with detailed request flags.



As a practical matter, I think we should just revert the ctypes
change for 3.4 and 3.5 (after verifying a second time that
PyObject_AsWriteBuffer() is exactly the same as in 3.4.0).


Then we use the new function from #25524 for 3.6, which should
alleviate concerns about writablility (not all extension writers
read the docs:) etc.
msg253801 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-10-31 17:56
I rather inclined to commit eryksun's patch.
msg253950 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-02 23:41
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 Issue 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.
msg254472 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-11 04:31
I propose this patch, based on Eryksun’s patch (thanks Eryksun). New changes:

* Added test case
* Applied Serhiy’s review suggestion
* Changed the new BufferError exceptions back to TypeError to match previous behaviour and test cases
msg254473 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-11-11 05:48
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?
msg254577 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-13 00:07
New patch with Fortran array test, and read-only and reversed memoryview() tests.
msg254593 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-11-13 09:40
Great! The patch LGTM. I left only two minor suggestions on Rietveld.
msg254596 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-11-13 09:59
The change 1da9630e9b7f was first published in Python 3.4.3.
msg254628 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-11-13 22:20
New changeset 9b23f7a94ba9 by Martin Panter in branch '3.4':
Issue #25498: Fix GC crash due to ctypes objects wrapping a memoryview
https://hg.python.org/cpython/rev/9b23f7a94ba9

New changeset 7445de785670 by Martin Panter in branch '3.5':
Issue #25498: Merge ctypes crash fix from 3.4 into 3.5
https://hg.python.org/cpython/rev/7445de785670

New changeset deb0fb601191 by Martin Panter in branch '3.5':
Issue #25498: Update error message for 3.5
https://hg.python.org/cpython/rev/deb0fb601191

New changeset f3dba13c22e5 by Martin Panter in branch 'default':
Issue #25498: Merge ctypes crash fix from 3.5
https://hg.python.org/cpython/rev/f3dba13c22e5

New changeset 08c56962c24c by Martin Panter in branch 'default':
Issue #25498: Add NEWS entry for 3.6
https://hg.python.org/cpython/rev/08c56962c24c
msg254633 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-13 23:07
Thanks to the various people that helped report and solve this.
msg254740 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-11-16 16:46
New changeset b04ae5392cf2 by Serhiy Storchaka in branch '3.4':
Issue #25498: Fixed contributors name.
https://hg.python.org/cpython/rev/b04ae5392cf2

New changeset 3738b0ca1356 by Serhiy Storchaka in branch '3.5':
Issue #25498: Fixed contributors name.
https://hg.python.org/cpython/rev/3738b0ca1356

New changeset 559ff2852cf2 by Serhiy Storchaka in branch 'default':
Issue #25498: Fixed contributors name.
https://hg.python.org/cpython/rev/559ff2852cf2
History
Date User Action Args
2015-11-16 16:46:34python-devsetmessages: + msg254740
2015-11-13 23:07:40martin.pantersetstatus: open -> closed
resolution: fixed
messages: + msg254633

stage: commit review -> resolved
2015-11-13 22:20:14python-devsetnosy: + python-dev
messages: + msg254628
2015-11-13 09:59:39vstinnersetnosy: + vstinner

messages: + msg254596
title: Python 3.4.3 core dump with simple sample code -> Python 3.4.3 regression in ctypes.frombuffer()
2015-11-13 09:40:56serhiy.storchakasetmessages: + msg254593
stage: patch review -> commit review
2015-11-13 00:07:17martin.pantersetfiles: + ctypes_from_buffer_4.patch

messages: + msg254577
2015-11-11 05:48:12serhiy.storchakasetmessages: + msg254473
2015-11-11 04:31:02martin.pantersetfiles: + ctypes_from_buffer_3.patch
nosy: + eryksun
messages: + msg254472

2015-11-04 10:13:19serhiy.storchakasetassignee: serhiy.storchaka ->
2015-11-02 23:41:04martin.pantersetmessages: + msg253950
2015-10-31 17:56:47serhiy.storchakasetmessages: + msg253801
2015-10-31 14:28:08skrahsetmessages: + msg253789
2015-10-31 05:48:17martin.pantersetmessages: + msg253776
2015-10-30 21:19:22skrahsetmessages: + msg253758
2015-10-30 21:01:47skrahsetmessages: + msg253756
2015-10-30 17:39:59serhiy.storchakasetmessages: + msg253749
2015-10-29 23:37:58martin.pantersetmessages: + msg253702
2015-10-29 16:00:11skrahsetnosy: + skrah
messages: + msg253682
2015-10-29 07:48:54serhiy.storchakasetpriority: normal -> high
assignee: serhiy.storchaka
stage: needs patch -> patch review
2015-10-29 07:17:38eryksunsetfiles: + ctypes_from_buffer_2.patch
2015-10-29 05:25:37eryksunsetfiles: - ctypes_from_buffer_1.patch
2015-10-29 04:39:10eryksunsetfiles: + ctypes_from_buffer_1.patch
keywords: + patch
2015-10-29 04:34:15eryksunsetfiles: + ctypes_crash.py
2015-10-29 00:29:35martin.pantersetnosy: + serhiy.storchaka
messages: + msg253646

components: + ctypes, - Interpreter Core
keywords: + 3.4regression
2015-10-28 22:47:34martin.pantersetnosy: + martin.panter
messages: + msg253639
2015-10-28 19:16:47zach.waresetversions: + Python 3.5, Python 3.6
nosy: + zach.ware, jnoller, sbt

messages: + msg253617

type: crash
stage: needs patch
2015-10-28 18:50:53r.david.murraysetnosy: + r.david.murray
messages: + msg253616
2015-10-28 18:06:12JakeMontcreate