Don't use PyObject_As*Buffer() functions

[serhiy-storchaka]

BPO	22896
Nosy	@abalkin, @pitrou, @larryhastings, @skrah, @vadmium, @serhiy-storchaka
Files	buffers.diff buffers_2.patch buffers_3.patch

^{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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2015-02-03.07:39:05.343>
created_at = <Date 2014-11-18.14:02:55.298>
labels = ['extension-modules', 'interpreter-core', 'performance']
title = "Don't use PyObject_As*Buffer() functions"
updated_at = <Date 2015-10-29.00:32:09.558>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2015-10-29.00:32:09.558>
actor = 'martin.panter'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2015-02-03.07:39:05.343>
closer = 'serhiy.storchaka'
components = ['Extension Modules', 'Interpreter Core']
creation = <Date 2014-11-18.14:02:55.298>
creator = 'serhiy.storchaka'
dependencies = []
files = ['37272', '37507', '37514']
hgrepos = []
issue_num = 22896
keywords = ['patch', 'needs review']
message_count = 18.0
messages = ['231323', '231642', '232941', '232944', '232946', '232947', '232968', '235197', '235205', '235210', '235211', '235212', '235256', '235257', '235274', '235305', '235318', '253647']
nosy_count = 8.0
nosy_names = ['belopolsky', 'pitrou', 'larry', 'Arfrever', 'skrah', 'python-dev', 'martin.panter', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue22896'
versions = ['Python 3.4', 'Python 3.5']

[serhiy-storchaka]

PyObject_AsCharBuffer(), PyObject_AsReadBuffer() and PyObject_AsWriteBuffer() release the buffer right after acquiring and return a pointer to released buffer. This is not safe and could cause issues sooner or later. These functions shouldn't be used in the stdlib at all.

[serhiy-storchaka]

Here is a patch which replaces deprecated functions with PyObject_GetBuffer() and like. It also introduce _PyBuffer_Converter for using in PyArgs_Parse* and clinic converter simple_buffer_converter which unlike to Py_buffer_converter ("y*") does not not force C contiguousity (is it correct?).

[serhiy-storchaka]

Updated patch addresses Antoine's comments.

I still hesitate about C-contiguousity. Looks as all buffers created in the stdlib are C-contiguous, so we can't test non-contiguous buffers. Shouldn't PyObject_AsCharBuffer (or even PyObject_AsReadBuffer and_PyBuffer_Converter) accept only C-contiguous buffers? Who are buffer protocol experts?

[pitrou]

Shouldn't PyObject_AsCharBuffer (or even PyObject_AsReadBuffer and_PyBuffer_Converter) accept only C-contiguous buffers?

PyBUF_SIMPLE enforces contiguity. See https://www.python.org/dev/peps/pep-3118/#access-flags and https://docs.python.org/3/c-api/buffer.html#c.Py_buffer.len

Also Stefan's post at http://mail.scipy.org/pipermail/numpy-discussion/2011-August/058189.html

Perhaps Stefan can confirm.

[serhiy-storchaka]

Ah, there is a way to create non-contiguous buffers.

>>> b = bytes(range(16))
>>> m = memoryview(b)
>>> m[::2].c_contiguous
False

PyBUF_SIMPLE enforces contiguity.

Then contiguousity check in getbuffer() in Python/getargs.c is redundant. And in most cases the use of _PyBuffer_Converter() and PyObject_GetBuffer() could be replaced by the use of "y*" and "w*" format units.

[skrah]

Yes, a PyBUF_SIMPLE request implies c-contiguous, so it's ok.

[serhiy-storchaka]

Here is simplified patch. _PyBuffer_Converter() and "simple_buffer" converter are gone. Fixed few leaks found by Martin. Fixed potential crash in ctypes. Fixed minor bugs and cleaned up ctypes tests for from_buffer().

[serhiy-storchaka]

Could you please look at the patch Stefan?

[skrah]

[Slow internet connection, can't use Rietveld.]

CDataType_from_buffer():

I'm not that familiar with ctypes. What is the high level goal here?
Allocate a chunk of memory, wrap it in a memoryview and have the
memoryview release that memory when its refcount is 0?

If so, I think we desperately need direct support for that in
memoryview.

[vadmium]

_CData.from_buffer() is meant to take a writable buffer, and create a “ctypes” object that shares the same memory. So it should not release the buffer until that “ctypes” object is no longer needed.

However I don’t know the insides of memoryview() objects that well so I can’t say if the hack-the-memoryview code is correct or not.

[serhiy-storchaka]

from_buffer() uses a memory buffer of other object. It keeps a reference to the object to prevent deallocation of memory when there will be no more external references. But this doesn't prevent from reallocating of memory of living object (e.g. for resizing of bytearray). So we need to grab the buffer (with PyObject_GetBuffer) in from_buffer() and free it (with PyBuffer_Release) when it is no longer needed. menoryview can do this but needs a hack, because a memoryview created by PyMemoryView_FromBuffer() doesn't release the buffer. May be there is more official way?