memoryview to freed memory can cause segfault

[sbt]

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']

Linked PRs

• gh-60198: Prevent memoryview pointing to freed heap memory #105290

[sbt]

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.

[BreamoreBoy]

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

[skrah]

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

[vadmium]

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]

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]

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]

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.

[ismail]

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

[CAM-Gerlach]

Can confirm I can repro this via the above for 3.11 and a recent-ish checkout of Python 3.12.0a3+ (built in release config).

[gpshead]

"Given that you need to be able to execute arbitary Python code in the first place, this exploit won’t be useful in most settings." is the most important line from that blog post.

I don't consider this any more a security issue than any use of pickle or marshal already is.

[oconnor663]

As a fourth option to add to @vadmium's three options above, could it also be possible to replace the raw self->buffer allocation with an internal bytearray? In that case, if the caller smuggles out a memoryview like we're talking about here, the result would be that the BufferedReader is in a bad state where any resizing operations on its internal bytearray will fail. As long as those failures get propagated to the caller correctly and don't lead to any other memory corruption, then all a pathological caller can do is cause more exceptions for themselves.

[vstinner]

cc @SethMichaelLarson

[serhiy-storchaka]

Idea 1 has the same problem as idea 2. If the memoryview were kept alive by a reference cycle, release() will fail. It will leave us in the same state, except that the user code maybe has a chance to report an error before crash.

The problem in the buffered reader can be solved by using bytearray as a buffer (the code will be complicated by necessary to support multiple buffers if they are not yet released). But PyUnicode_Decode() is more hopeless case. It takes a pointer to raw buffer and have no control on its lifespan. The only reliable way is to copy it to a nyw bytes or bytearray object, but it would be grossy inefficient.