Skip to content

gh-142663: Fix use-after-free in memoryview comparison #143305

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
superboy-zjc wants to merge 4 commits into
base: main
Choose a base branch
from
Open

gh-142663: Fix use-after-free in memoryview comparison #143305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
edd566c
gh-142663: Fix use-after-free in memoryview comparison
superboy-zjc Dec 31, 2025
06846a9
gh-142663: resolving comments by refactoring unit tests and cache typ…
superboy-zjc Jan 3, 2026
0694b20
Merge branch 'main' of https://github.com/python/cpython into issue-1…
superboy-zjc Jan 3, 2026
8a90934
Merge branch 'main' into issue-142663
serhiy-storchaka Jul 13, 2026
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
26 changes: 26 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 @@ -228,6 +228,32 @@ def test_compare(self):
self.assertRaises(TypeError, lambda: m >= c)
self.assertRaises(TypeError, lambda: c > m)

def test_compare_1d_concurrent_mutation(self):
# Prevent crashes during a mixed format 1-D comparison loop.
# Regression test for https://github.com/python/cpython/issues/142663.
src1 = array.array("d", [1.0, 2.0])
src2 = array.array("l", [1, 2])
mv1, mv2 = memoryview(src1), memoryview(src2)
self.do_test_compare_concurrent_mutation(src1, mv1, mv2)

def test_compare_2d_concurrent_mutation(self):
# Prevent crashes during a mixed format 2-D comparison loop.
# Regression test for https://github.com/python/cpython/issues/142663.
src1 = array.array("d", [1.0, 2.0])
src2 = array.array("l", [1, 2])
mv1 = memoryview(src1).cast("B").cast("d", shape=(1, 2))
mv2 = memoryview(src2).cast("B").cast("l", shape=(1, 2))
self.do_test_compare_concurrent_mutation(src1, mv1, mv2)

def do_test_compare_concurrent_mutation(self, src1, mv1, mv2):
class S(struct.Struct):
def unpack_from(self, buf, /, offset=0):
mv1.release()
src1.append(3.14)
return (1,)
with support.swap_attr(struct, "Struct", S):

@sobolevn sobolevn Jan 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line puzzles me a little bit. When you swap classes in the stdlib, you can get unexpected results. Can we replicate this without monkeypatching?

@superboy-zjc superboy-zjc Jan 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @sobolevn I doubt the possibility of replicating this without monkeypatching. The issue stems from, during a mixed format buffer comparison, memory_richcompare function calls a re-entrant struct.Struct.unpack_from that is loaded by a PyImport_ImportModuleAttrString("struct", "Struct") call in the struct_get_unpacker. Is there anyway you would recommend to work around it ?

/* Return a new unpacker for the given format. */
static struct unpacker *
struct_get_unpacker(const char *fmt, Py_ssize_t itemsize)
{
    ...

    // struct.Struct Importation
    Struct = PyImport_ImportModuleAttrString("struct", "Struct");
    if (Struct == NULL)
        return NULL;

    x = unpacker_new();

    ...

    x->unpack_from = PyObject_GetAttrString(structobj, "unpack_from");
    if (x->unpack_from == NULL)
        goto error;
    ...

}

@sobolevn sobolevn Jan 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are countless potential issues if we replace stdlib types with something else. Let's please first discuss in the issue whether this is even a valid bug report.

@picnixz picnixz Jan 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I did consider it as a crash because we also fixed similar occurrences elsewhere. If we just had an exception it would have been ok, but here we have a crash. We do not want this.

@superboy-zjc superboy-zjc Jan 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see there are many tests having monkey patch. Why does this case make difference? Is there any update regarding this pr or should we @ more core dev to take a look?

@serhiy-storchaka serhiy-storchaka Feb 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

struct_get_unpacker looks inefficient. We may add a cache for struct.Struct in future. Or we can cache the bound unpack_from() methods. Anyway, monkey-patching struct.Struct will either not have any effect or will affect all following code. I suspect that after I wrote this, someone might take it as a challenge.

I do not see a good solution for this. We might run the tests in a subprocess to reduce affect on other code, but this is too much hassle (and this is not 100% future proof). Perhaps it would be safer to not add a test.

@serhiy-storchaka serhiy-storchaka Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Drop the tests, future changes can make it unsafe. Add a simple reproducer script in the PR or in the issue, so we can test the fix manually.

self.assertRaises(BufferError, mv1.__eq__, mv2)

def check_attributes_with_type(self, tp):
m = self._view(tp(self._source))
self.assertEqual(m.format, self.format)
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2025-12-30-22-12-27.gh-issue-142663.gq7iIf.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix use-after-free crashes when a :class:`memoryview` is mutated
during a comparison with another object.
15 changes: 14 additions & 1 deletion Objects/memoryobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3243,7 +3243,8 @@ memory_richcompare(PyObject *v, PyObject *w, int op)
}
}

if (PyMemoryView_Check(w)) {
int w_is_mv = PyMemoryView_Check(w);
if (w_is_mv) {
if (BASE_INACCESSIBLE(w)) {
equal = (v == w);
goto result;
Expand Down Expand Up @@ -3286,6 +3287,13 @@ memory_richcompare(PyObject *v, PyObject *w, int op)
goto result;
}
}
/* Prevent memoryview object from being released and its underlying buffer
reshaped during a mixed format comparison loop. */
// See https://github.com/python/cpython/issues/142663.
Comment on lines +3290 to +3292

@serhiy-storchaka serhiy-storchaka Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do not mix different types of comments, especially in such proximity. Drop the URL, the NEWS/PR already carry the issue link.

((PyMemoryViewObject *)v)->exports++;

@serhiy-storchaka serhiy-storchaka Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All other modifications of exports use FT_ATOMIC_ADD_SSIZE. Please use it here and in other 3 sites.

Suggested change
((PyMemoryViewObject *)v)->exports++;
FT_ATOMIC_ADD_SSIZE(((PyMemoryViewObject *)v)->exports, 1);

if (w_is_mv) {
((PyMemoryViewObject *)w)->exports++;
}

if (vv->ndim == 0) {
equal = unpack_cmp(vv->buf, ww->buf,
Expand All @@ -3304,6 +3312,11 @@ memory_richcompare(PyObject *v, PyObject *w, int op)
vfmt, unpack_v, unpack_w);
}

((PyMemoryViewObject *)v)->exports--;
if (w_is_mv) {
((PyMemoryViewObject *)w)->exports--;
}

result:
if (equal < 0) {
if (equal == MV_COMPARE_NOT_IMPL)
Expand Down
Loading