Skip to content

gh-142665: fix UAF when accessing a memoryview concurrently mutates the underlying buffer #143324

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
picnixz wants to merge 1 commit into
base: main
Choose a base branch
from
+13 −3
Open

gh-142665: fix UAF when accessing a memoryview concurrently mutates the underlying buffer #143324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 6 additions & 2 deletions Lib/test/test_memoryview.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,13 +685,17 @@ def __bool__(self):
with self.assertRaises(ValueError):
m[MyIndex()]

# Other exceptions can be raised when working on a released buffer.
# See https://github.com/python/cpython/issues/142665.
ba = None
m = memoryview(bytearray(b'\xff'*size))
self.assertEqual(list(m[:MyIndex()]), [255] * 4)
with self.assertRaises(BufferError):
m[:MyIndex()]

ba = None
m = memoryview(bytearray(b'\xff'*size))
self.assertEqual(list(m[MyIndex():8]), [255] * 4)
with self.assertRaises(BufferError):
m[MyIndex():8]

ba = None
m = memoryview(bytearray(b'\xff'*size)).cast('B', (64, 2))
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2026-01-01-12-22-35.gh-issue-142665.7YCUBF.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix use-after-free crashes when slicing a :class:`memoryview` or
accessing the elements of a sliced view concurrently mutates the
underlying buffer. Patch by Bénédikt Tran.
5 changes: 4 additions & 1 deletion Objects/memoryobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2623,7 +2623,10 @@ memory_subscript(PyObject *_self, PyObject *key)
if (sliced == NULL)
return NULL;

if (init_slice(&sliced->view, key, 0) < 0) {
self->exports++;
Copy link
Member

@vstinner vstinner Jan 5, 2026

Choose a reason for hiding this comment

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

Can you add a comment explaining the purpose of the exports++? Add a reference to the GitHub issue (gh-142665:).

Copy link
Member Author

@picnixz picnixz Jan 5, 2026

Choose a reason for hiding this comment

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

Ah, I think I wasn't consistent in my reviews or in my commits. I thought the commit history would have been enough for that. But sure I can a comment

int rc = init_slice(&sliced->view, key, 0);
self->exports--;
Copy link
Member

@serhiy-storchaka serhiy-storchaka Jan 12, 2026

Choose a reason for hiding this comment

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

If we go this way, should not we call init_len() and init_flags() while export is incremented?

Also, we can have a similar issue with memory_item_multi().

Copy link
Member

@serhiy-storchaka serhiy-storchaka Jan 12, 2026

Choose a reason for hiding this comment

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

Also, memory_ass_sub() can have the same issue.

Copy link
Member Author

@picnixz picnixz Jan 12, 2026

Choose a reason for hiding this comment

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

For ass_sub, there is:

        /* rvalue must be an exporter */
        if (PyObject_GetBuffer(value, &src, PyBUF_FULL_RO) < 0)
            return ret;

so it's already protected I think

Copy link
Member Author

@picnixz picnixz Jan 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

But I wasn't aware of memory_item_multi which uses ptr_from_tuple and this looks like vulnerable.

Copy link
Member Author

@picnixz picnixz Jan 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

If we go this way, should not we call init_len() and init_flags() while export is incremented?

Strictly speaking no because those functions don't call any Python code, unless there are some invariants I'm not aware of

if (rc < 0) {
Py_DECREF(sliced);
return NULL;
}
Expand Down
Loading