classification
Title: memoryview to freed memory can cause segfault
Type: crash Stage: patch review
Components: IO, Unicode Versions: Python 3.8, Python 3.7, Python 3.6, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, abacabadabacaba, christian.heimes, ezio.melotti, gregory.p.smith, jcea, martin.panter, methane, sbt, skrah, taralx, vstinner
Priority: normal Keywords: patch

Created on 2012-09-20 20:37 by sbt, last changed 2019-04-08 13:02 by methane.

Files
File name Uploaded Description Edit
release-view.patch martin.panter, 2016-04-18 13:40 review
Messages (7)
msg170846 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-09-20 20:37
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 #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.
msg223224 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-07-16 15:43
Can someone follow this up noting it refers to #15903.
msg223406 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-07-18 11:51
We deal with it when we have time.  IMO there is little value in
bumping up issues this way.
msg253774 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-10-31 05:30
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.
msg253796 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-10-31 15:05
> 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.
msg263245 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-12 13:12
I recently created Issue 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.
msg263664 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-18 13:40
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.
History
Date User Action Args
2019-04-08 13:02:28methanesetnosy: + methane
2018-10-08 17:39:51taralxsetnosy: + taralx
2018-09-28 17:51:49gregory.p.smithsetnosy: + gregory.p.smith

versions: + Python 3.7, Python 3.8, - Python 3.5
2016-10-13 20:31:18abacabadabacabasetnosy: + abacabadabacaba
2016-04-18 13:43:40martin.panterlinkissue26720 superseder
2016-04-18 13:40:10martin.pantersetfiles: + release-view.patch

components: + Unicode

keywords: + patch
nosy: + vstinner, ezio.melotti
messages: + msg263664
stage: patch review
2016-04-12 13:33:34BreamoreBoysetnosy: - BreamoreBoy
2016-04-12 13:12:46martin.pantersetmessages: + msg263245
versions: + Python 2.7, Python 3.5, Python 3.6, - Python 3.4
2015-10-31 15:05:22skrahsetnosy: + skrah
messages: + msg253796
2015-10-31 05:30:26martin.pantersetnosy: + martin.panter
messages: + msg253774
components: + IO
2014-10-14 23:53:18Arfreversetnosy: + Arfrever
2014-10-14 18:02:29skrahsetnosy: - skrah
2014-07-18 11:51:56skrahsetmessages: + msg223406
2014-07-16 15:43:28BreamoreBoysetnosy: + BreamoreBoy
messages: + msg223224
2013-01-15 12:35:43christian.heimessetnosy: + christian.heimes
2013-01-15 09:09:07jceasetnosy: + jcea
2012-09-20 20:37:21sbtcreate