Skip to content

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

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.

Sign up for GitHub

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

Open
furkanonder wants to merge 11 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
426d495
Prevent memoryview points to freed heap memory.
furkanonder Jun 4, 2023
d106d12
Merge branch 'main' into issue-60198
furkanonder Jun 4, 2023
3a5a604
Merge branch 'main' into issue-60198
furkanonder Jun 5, 2023
27cd033
Merge branch 'main' into issue-60198
furkanonder Jun 6, 2023
bb29de1
Merge branch 'main' into issue-60198
furkanonder Jul 22, 2023
1809a2a
Don't reduce ref count for memobj in _bufferedwriter_raw_write
furkanonder Jul 22, 2023
7b2cdda
Use _PyErr_ChainExceptions1 instead of _PyErr_ChainExceptions
furkanonder Jul 22, 2023
83a4bf4
📜🤖 Added by blurb_it.
blurb-it[bot] Jul 23, 2023
ad03e4a
Mention Co-Author in the News entry
furkanonder Jul 23, 2023
c837064
Merge branch 'main' into issue-60198
furkanonder Jul 23, 2023
7a81f60
Add test case to reproduce issue
furkanonder Jul 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions Lib/test/test_memoryview.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,5 +656,30 @@ def __bool__(self):
m[0] = MyBool()
self.assertEqual(ba[:8], b'\0'*8)


def test_gh60198(self):
global view

class File(io.RawIOBase):
def readinto(self, buf):
global view
view = buf

def readable(self):
return True

f = io.BufferedReader(File())
# get view of buffer used by BufferedReader
f.read(1)
# deallocate buffer
del f

with self.assertRaises(ValueError):
view = view.cast('P')
L = [None] * len(view)
# overwrite first item with NULL
view[0] = 0
print(L[0])

if __name__ == "__main__":
unittest.main()
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Core and Builtins/2023-07-23-07-43-57.gh-issue-60198.fysv2O.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Prevent :class:`memoryview` objects from pointing to free heap memory. Patch by Martin Panter and Furkan Onder.
22 changes: 20 additions & 2 deletions Modules/_io/bufferedio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1545,7 +1545,8 @@ _bufferedreader_raw_read(buffered *self, char *start, Py_ssize_t len)
Py_buffer buf;
PyObject *memobj, *res;
Py_ssize_t n;
/* NOTE: the buffer needn't be released as its object is NULL. */
PyObject *release_res;
/* The buffer will be released when raw.readinto() returns. */
if (PyBuffer_FillInfo(&buf, NULL, start, len, 0, PyBUF_CONTIG) == -1)
return -1;
memobj = PyMemoryView_FromBuffer(&buf);
Expand All @@ -1559,7 +1560,15 @@ _bufferedreader_raw_read(buffered *self, char *start, Py_ssize_t len)
do {
res = PyObject_CallMethodOneArg(self->raw, &_Py_ID(readinto), memobj);
} while (res == NULL && _PyIO_trap_eintr());
PyObject *exc = PyErr_GetRaisedException();
release_res = PyObject_CallMethod(memobj, "release", NULL);
_PyErr_ChainExceptions1(exc);
Py_DECREF(memobj);
if (release_res == NULL) {
Py_XDECREF(res);
return -1;
}
Py_DECREF(release_res);
Comment on lines +1563 to +1571
Copy link
Contributor

@ambv ambv Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. The performance impact should be minimal as it's only a cleanup method call after the buffer's been entirely read.

However, since mv.release() can potentially raise, this is a change in behavior so we have to decide whether the risk of existing code breakage is worth backporting the patch to older releases.

I see @gpshead decided this isn't a security issue so we won't be bringing it to 3.8, 3.9, 3.10. Question is if we want it in 3.11 and 3.12.

if (res == NULL)
return -1;
if (res == Py_None) {
Expand Down Expand Up @@ -1904,7 +1913,8 @@ _bufferedwriter_raw_write(buffered *self, char *start, Py_ssize_t len)
PyObject *memobj, *res;
Py_ssize_t n;
int errnum;
/* NOTE: the buffer needn't be released as its object is NULL. */
PyObject *release_res;
/* The buffer will be released when raw.write() returns. */
if (PyBuffer_FillInfo(&buf, NULL, start, len, 1, PyBUF_CONTIG_RO) == -1)
return -1;
memobj = PyMemoryView_FromBuffer(&buf);
Expand All @@ -1920,6 +1930,14 @@ _bufferedwriter_raw_write(buffered *self, char *start, Py_ssize_t len)
res = PyObject_CallMethodOneArg(self->raw, &_Py_ID(write), memobj);
errnum = errno;
} while (res == NULL && _PyIO_trap_eintr());
PyObject *exc = PyErr_GetRaisedException();
release_res = PyObject_CallMethod(memobj, "release", NULL);
_PyErr_ChainExceptions1(exc);
if (release_res == NULL) {
Py_XDECREF(res);
return -1;
}
Py_DECREF(release_res);
Py_DECREF(memobj);
if (res == NULL)
return -1;
Expand Down
9 changes: 9 additions & 0 deletions Objects/unicodeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3190,6 +3190,7 @@ PyUnicode_Decode(const char *s,
PyObject *buffer = NULL, *unicode;
Py_buffer info;
char buflower[11]; /* strlen("iso-8859-1\0") == 11, longest shortcut */
PyObject *res;

if (unicode_check_encoding_errors(encoding, errors) < 0) {
return NULL;
Expand Down Expand Up @@ -3252,6 +3253,14 @@ PyUnicode_Decode(const char *s,
if (buffer == NULL)
goto onError;
unicode = _PyCodec_DecodeText(buffer, encoding, errors);
PyObject *exc = PyErr_GetRaisedException();
res = PyObject_CallMethod(buffer, "release", NULL);
_PyErr_ChainExceptions1(exc);
if (res == NULL) {
Py_XDECREF(unicode);
goto onError;
}
Py_DECREF(res);
if (unicode == NULL)
goto onError;
if (!PyUnicode_Check(unicode)) {
Expand Down