Skip to content
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

memoryview to freed memory can cause segfault #60198

Open
sbt mannequin opened this issue Sep 20, 2012 · 8 comments
Open

memoryview to freed memory can cause segfault #60198

sbt mannequin opened this issue Sep 20, 2012 · 8 comments
Labels
3.7 3.8 expert-IO expert-unicode type-crash

Comments

@sbt
Copy link
Mannequin

@sbt sbt mannequin commented Sep 20, 2012

BPO 15994
Nosy @gpshead, @jcea, @vstinner, @tiran, @ezio-melotti, @methane, @skrah, @vadmium, @taralx
Files
  • release-view.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 = None
    closed_at = None
    created_at = <Date 2012-09-20.20:37:21.249>
    labels = ['3.8', 'expert-unicode', '3.7', 'expert-IO', 'type-crash']
    title = 'memoryview to freed memory can cause segfault'
    updated_at = <Date 2019-04-08.13:02:28.778>
    user = 'https://bugs.python.org/sbt'

    bugs.python.org fields:

    activity = <Date 2019-04-08.13:02:28.778>
    actor = 'methane'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Unicode', 'IO']
    creation = <Date 2012-09-20.20:37:21.249>
    creator = 'sbt'
    dependencies = []
    files = ['42503']
    hgrepos = []
    issue_num = 15994
    keywords = ['patch']
    message_count = 7.0
    messages = ['170846', '223224', '223406', '253774', '253796', '263245', '263664']
    nosy_count = 12.0
    nosy_names = ['gregory.p.smith', 'jcea', 'vstinner', 'christian.heimes', 'ezio.melotti', 'Arfrever', 'methane', 'skrah', 'abacabadabacaba', 'sbt', 'martin.panter', 'taralx']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue15994'
    versions = ['Python 2.7', 'Python 3.6', 'Python 3.7', 'Python 3.8']

    @sbt
    Copy link
    Mannequin Author

    @sbt sbt mannequin commented Sep 20, 2012

    A memoryview which does not own a reference to its base object can point to freed or reallocated memory. For instance the following segfaults for me on Windows and Linux.

    import io
    
    class File(io.RawIOBase):
        def readinto(self, buf):
            global view
            view = buf
        def readable(self):
            return True
    
    f = io.BufferedReader(File())
    f.read(1)                       # get view of buffer used by BufferedReader
    del f                           # deallocate buffer
    view = view.cast('P')
    L = [None] * len(view)          # create list whose array has same size
                                    # (this will probably coincide with view)
    view[0] = 0                     # overwrite first item with NULL
    print(L[0])                     # segfault: dereferencing NULL

    I realize there are easier ways to make Python segfault, so maybe this should not be considered a serious issue. But I think there should be some way of guaranteeing that a memoryview will not try to access memory which has already been freed.

    In bpo-15903 skrah proposed exposing memory_release() as PyBuffer_Release(). However, I don't think that would necessarily invalidate all exports of the buffer.

    Alternatively, one could incref the buffered reader object and set mview->mbuf->obj to it. Maybe we could have

        PyMemoryView_FromMemoryEx(char *mem, Py_ssize_t size, int flags, PyObject *obj)

    which guarantees that if obj is non-NULL then it will not be garbage collected before the memoryview. This should *not* expose obj as an attribute of the memoryview.

    @sbt sbt mannequin added the type-crash label Sep 20, 2012
    @BreamoreBoy
    Copy link
    Mannequin

    @BreamoreBoy BreamoreBoy mannequin commented Jul 16, 2014

    Can someone follow this up noting it refers to bpo-15903.

    @skrah
    Copy link
    Mannequin

    @skrah skrah mannequin commented Jul 18, 2014

    We deal with it when we have time. IMO there is little value in
    bumping up issues this way.

    @vadmium
    Copy link
    Member

    @vadmium vadmium commented Oct 31, 2015

    Saving a reference to the overall buffered reader is probably not good enough. I think the buffer is freed when close() is called, so a reference to this internal buffer would need to be saved instead (or as well). Unless we stop having close() free the buffer.

    @skrah
    Copy link
    Mannequin

    @skrah skrah mannequin commented Oct 31, 2015

    But I think there should be some way of guaranteeing that a memoryview will not try to access memory which has already been freed.

    Unless the "buffered *self" parameter in _buffered_raw_read() is
    made a full PEP-3118 exporter, I'm not sure how it would be possible
    to prevent all examples of this kind.

    @vadmium
    Copy link
    Member

    @vadmium vadmium commented Apr 12, 2016

    I recently created bpo-26720 about a similar situation with BufferedWriter. However I am starting to believe that the problem cannot be solved the way I originally wanted. Instead, the best solution there would be similar what I would suggest here: we need to avoid the user accessing the memoryview after readinto() or write() has returned. Some ideas, not all perfect:

    1. Call memoryview.release() after the method returns. This would be simple and practical, but could be cheated by making a second memoryview of the original. Also, memoryview.release() is not guaranteed to succeed; there is code that raises BufferError("memoryview has exported buffers").

    2. Raise an exception (BufferError or RuntimeError?) if readinto() or write() returns and the memoryview(s) are not all released. This doesn’t prevent a determined programmer from overwriting memory or losing written data, but it might let them know about the problem when it happens.

    3. Try to force the memoryview.release() state on all views into the original memory. Is this practical? I guess this would be a bit inconsistent if some other thread was in the middle of a system call using the buffer at the time that we want to do the release.

    @vadmium
    Copy link
    Member

    @vadmium vadmium commented Apr 18, 2016

    I think idea 2 (error if memoryview not released) would not work. It would rely on garbage collection, which is not always immediate. E.g. if the memoryview were kept alive by a reference cycle, it may not immediately be released. I don’t think idea 3 is worthwhile, because the memoryview API does not seem designed for this use case, and it would not be 100% consistent anyway.

    So that leaves my first idea, to call view.release() when readinto(), etc, return. This should avoid the crash and data loss problems in some common cases. It is implemented in release-view.patch, along with some notes.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @ismail
    Copy link

    @ismail ismail commented May 15, 2022

    Just stumbled upon https://pwn.win/2022/05/11/python-buffered-reader.html which is an “exploit” for this //cc @vstinner

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 3.8 expert-IO expert-unicode type-crash
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants