classification
Title: PyObject_CheckReadBuffer crashes on memoryview object
Type: Stage:
Components: Versions:
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, benjamin.peterson, pitrou, rupole
Priority: critical Keywords: needs review, patch

Created on 2008-09-23 13:54 by rupole, last changed 2008-09-26 21:49 by benjamin.peterson. This issue is now closed.

Files
File name Uploaded Description Edit
no_null_view.patch benjamin.peterson, 2008-09-23 21:35
Messages (9)
msg73640 - (view) Author: Roger Upole (rupole) Date: 2008-09-23 13:54
Sample code:
	PyObject *b=PyBytes_FromString("eh ?????");
	PyObject *mv=PyMemoryView_FromObject(b);
	PyObject_CheckReadBuffer(mv);

From following the chain of calls in PyObject_CheckReadBuffer,
a few things are unclear.

It calls bf_getbuffer with a NULL Py_Buffer pointer, although the PEP
explicitely states that is should never be NULL.

PyBuffer_FillInfo immediately returns success if the view pointer is
NULL.  I'm guessing this is to just determine if the operation could be
completed, but it returns before any checks are done.

It then attempts to release a hardcoded NULL Py_buffer pointer
which of course crashes.
msg73642 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-09-23 14:34
And the following statement crashes the interpreter:

>>> compile(memoryview(b"text"), "name", "exec")
msg73669 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-09-23 21:35
Attaching patch and test.
msg73776 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-09-25 11:39
The test would be better in test_memoryview rather than in test_builtin.
msg73810 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-09-25 20:30
On Thu, Sep 25, 2008 at 6:39 AM, Antoine Pitrou <report@bugs.python.org> wrote:
>
> Antoine Pitrou <pitrou@free.fr> added the comment:
>
> The test would be better in test_memoryview rather than in test_builtin.

I disagree. The test I added is really about compile, and its failure
was really just a side effect of the buffer bug.
msg73822 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-09-25 23:07
> I disagree. The test I added is really about compile, and its failure
> was really just a side effect of the buffer bug.

Well, I won't fight over it, but it's really meant to check an aspect of
memoryview behaviour - otherwise why would it be part of your patch ? -,
and as such it should be in test_memoryview (and if it's meant to check
compile() then it should be in some hypothetical test_compile :-)).

In general I find it annoying that many tests are dumped into "generic"
test files like test_builtin, rather than in the test file appropriate
for the functionality being tested.

That said, I haven't looked at the rest of the patch yet, sorry! (back
from holidays)
msg73863 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-09-26 16:38
The rest of the patch is fine with me (I assume you've run all the unit
tests :-)).
msg73890 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-09-26 21:24
I'm actually having a hard time trying to get memoryview to use
PyObject_CheckReadBuffer. Eventually, test should be written in C, but
for now are you ok with the compile test?
msg73896 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-09-26 21:49
Fixed in r66630.
History
Date User Action Args
2008-09-26 21:49:39benjamin.petersonsetstatus: open -> closed
resolution: fixed
messages: + msg73896
2008-09-26 21:24:52benjamin.petersonsetmessages: + msg73890
2008-09-26 16:38:03pitrousetmessages: + msg73863
2008-09-25 23:07:20pitrousetmessages: + msg73822
2008-09-25 20:30:25benjamin.petersonsetmessages: + msg73810
2008-09-25 11:39:42pitrousetnosy: + pitrou
messages: + msg73776
2008-09-23 21:35:38benjamin.petersonsetkeywords: + needs review, patch
files: + no_null_view.patch
messages: + msg73669
nosy: + benjamin.peterson
2008-09-23 14:34:30amaury.forgeotdarcsetpriority: critical
nosy: + amaury.forgeotdarc
messages: + msg73642
2008-09-23 13:54:49rupolecreate