Skip to content

Don't use PyObject_As*Buffer() functions #67085

Closed
Closed
Don't use PyObject_As*Buffer() functions#67085
Assignees
serhiy-storchaka
Labels
extension-modulesC modules in the Modules dirinterpreter-core(Objects, Python, Grammar, and Parser dirs)performancePerformance or resource usage

Description

@serhiy-storchaka
serhiy-storchaka
opened on Nov 18, 2014
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']

    Activity

    1. serhiy-storchaka commented on Nov 18, 2014

      @serhiy-storchaka
      serhiy-storchaka
      on Nov 18, 2014
      MemberAuthor

      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.

    2. serhiy-storchaka
      self-assigned this
      on Nov 18, 2014
    3. serhiy-storchaka
      added
      extension-modulesC modules in the Modules dir
      interpreter-core(Objects, Python, Grammar, and Parser dirs)
      performancePerformance or resource usage
      on Nov 18, 2014

    4. serhiy-storchaka commented on Nov 25, 2014

      @serhiy-storchaka
      serhiy-storchaka
      on Nov 25, 2014
      MemberAuthor

      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?).

    5. serhiy-storchaka commented on Dec 19, 2014

      @serhiy-storchaka
      serhiy-storchaka
      on Dec 19, 2014
      MemberAuthor

      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?

    6. pitrou commented on Dec 19, 2014

      @pitrou
      pitrou
      on Dec 19, 2014
      Member

      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.

    7. serhiy-storchaka commented on Dec 19, 2014

      @serhiy-storchaka
      serhiy-storchaka
      on Dec 19, 2014
      MemberAuthor

      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.

    8. skrah commented on Dec 19, 2014

      @skrah
      skrahmannequin
      on Dec 19, 2014
      Mannequin

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

    9. serhiy-storchaka commented on Dec 20, 2014

      @serhiy-storchaka
      serhiy-storchaka
      on Dec 20, 2014
      MemberAuthor

      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().

    10. serhiy-storchaka commented on Feb 1, 2015

      @serhiy-storchaka
      serhiy-storchaka
      on Feb 1, 2015
      MemberAuthor

      Could you please look at the patch Stefan?

    11. skrah commented on Feb 1, 2015

      @skrah
      skrahmannequin
      on Feb 1, 2015
      Mannequin

      [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.

    12. vadmium commented on Feb 1, 2015

      @vadmium
      vadmium
      on Feb 1, 2015
      Member

      _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.

    13. serhiy-storchaka commented on Feb 1, 2015

      @serhiy-storchaka
      serhiy-storchaka
      on Feb 1, 2015
      MemberAuthor

      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?

    14. serhiy-storchaka commented on Feb 1, 2015

      @serhiy-storchaka
      serhiy-storchaka
      on Feb 1, 2015
      MemberAuthor

      Oh, Martin expressed the same thing better.

    15. skrah commented on Feb 2, 2015

      @skrah
      skrahmannequin
      on Feb 2, 2015
      Mannequin

      Thanks. No, I don't think there's an official way to accomplish that,
      but let's create one. How about a new function that takes the buffer
      request flags:

          PyMemoryView_FromObjectEx(exporter, PyBUF_SIMPLE|PyBUF_WRITABLE)

      If we can spare a new format code, this could be called directly in
      PyArg_ParseTuple(), which would give back the memoryview.

      Otherwise, you get the exporter from PyArg_ParseTuple() and call
      PyMemoryView_FromObjectEx() manually.

      If I'm not mistaken, this would save us the intermediate buffer on the
      stack, and it's more readable.

    16. serhiy-storchaka commented on Feb 2, 2015

      @serhiy-storchaka
      serhiy-storchaka
      on Feb 2, 2015
      MemberAuthor

      In any case we need a hack in 3.4. Let open new issue for adding PyMemoryView_FromObjectEx() or like.

    17. skrah commented on Feb 2, 2015

      @skrah
      skrahmannequin
      on Feb 2, 2015
      Mannequin

      Nice patch. I've found one issue (see Rietveld). I'm not sure
      about 3.4 (the patch contains minor refactorings), but otherwise
      I'd say go ahead with it.

    18. python-dev commented on Feb 3, 2015

      @python-dev
      python-devmannequin
      on Feb 3, 2015
      Mannequin

      New changeset 1da9630e9b7f by Serhiy Storchaka in branch '3.4':
      Issue bpo-22896: Avoid to use PyObject_AsCharBuffer(), PyObject_AsReadBuffer()
      https://hg.python.org/cpython/rev/1da9630e9b7f

      New changeset 2e684ce772de by Serhiy Storchaka in branch 'default':
      Issue bpo-22896: Avoid to use PyObject_AsCharBuffer(), PyObject_AsReadBuffer()
      https://hg.python.org/cpython/rev/2e684ce772de

      New changeset 0024537a4687 by Serhiy Storchaka in branch 'default':
      Issue bpo-22896: Fixed using _getbuffer() in recently added _PyBytes_Format().
      https://hg.python.org/cpython/rev/0024537a4687

    19. serhiy-storchaka commented on Feb 3, 2015

      @serhiy-storchaka
      serhiy-storchaka
      on Feb 3, 2015
      MemberAuthor

      Thanks Antoine and Stefan for your reviews.

    20. serhiy-storchaka
      closed this as completedon Feb 3, 2015

    21. vadmium commented on Oct 29, 2015

      @vadmium
      vadmium
      on Oct 29, 2015
      Member

      Please see bpo-25498 for a crash possibly caused by the memoryview hack in CDataType_from_buffer().

    22. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    Labels

    extension-modulesC modules in the Modules dirinterpreter-core(Objects, Python, Grammar, and Parser dirs)performancePerformance or resource usage

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions