From 73ffb29d9446409f75a39297e074c49dfe4d1ef4 Mon Sep 17 00:00:00 2001 From: Ken Date: Thu, 19 May 2022 16:49:22 +0800 Subject: [PATCH 01/11] Fix memoryview bad `__index__` use after free Co-Authored-By: chilaxan <35645806+chilaxan@users.noreply.github.com> --- Lib/test/test_memoryview.py | 21 +++++++++++++++++++++ Objects/memoryobject.c | 18 ++++++++++++++---- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_memoryview.py b/Lib/test/test_memoryview.py index d7e3f0c0effa69..62abd481192207 100644 --- a/Lib/test/test_memoryview.py +++ b/Lib/test/test_memoryview.py @@ -545,6 +545,27 @@ def test_pickle(self): with self.assertRaises(TypeError): pickle.dumps(m, proto) + def test_memoryview_bad_index_uaf(self): + # memoryview Use After Free (memory_ass_sub) see gh-92888 + uaf_backing = bytearray(bytearray.__basicsize__) + uaf_view = memoryview(uaf_backing).cast('n') # ssize_t format + + class weird_index: + def __index__(self): + global memory_backing + uaf_view.release() # release memoryview (UAF) + # free `uaf_backing` memory and allocate a new bytearray into it + memory_backing = uaf_backing.clear() or bytearray() + return 2 # `ob_size` idx + + # by the time this line finishes executing, it writes the max ptr size + # into the `ob_size` slot of `memory_backing` + with self.assertRaisesRegex(ValueError, "operation forbidden on released memoryview object"): + uaf_view[weird_index()] = (2 ** (tuple.__itemsize__ * 8) - 1) // 2 + memory = memoryview(memory_backing) + with self.assertRaisesRegex(IndexError, "index out of bounds"): + memory[id(250) + int.__basicsize__] = 100 + self.assertEqual(250, eval("250")) if __name__ == "__main__": unittest.main() diff --git a/Objects/memoryobject.c b/Objects/memoryobject.c index 8c269168824471..3c512e9976f993 100644 --- a/Objects/memoryobject.c +++ b/Objects/memoryobject.c @@ -1767,7 +1767,7 @@ unpack_single(const char *ptr, const char *fmt) /* Pack a single item. 'fmt' can be any native format character in struct module syntax. */ static int -pack_single(char *ptr, PyObject *item, const char *fmt) +pack_single(PyMemoryViewObject *self, char *ptr, PyObject *item, const char *fmt) { unsigned long long llu; unsigned long lu; @@ -1784,6 +1784,7 @@ pack_single(char *ptr, PyObject *item, const char *fmt) ld = pylong_as_ld(item); if (ld == -1 && PyErr_Occurred()) goto err_occurred; + CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ switch (fmt[0]) { case 'b': if (ld < SCHAR_MIN || ld > SCHAR_MAX) goto err_range; @@ -1804,6 +1805,7 @@ pack_single(char *ptr, PyObject *item, const char *fmt) lu = pylong_as_lu(item); if (lu == (unsigned long)-1 && PyErr_Occurred()) goto err_occurred; + CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ switch (fmt[0]) { case 'B': if (lu > UCHAR_MAX) goto err_range; @@ -1824,12 +1826,14 @@ pack_single(char *ptr, PyObject *item, const char *fmt) lld = pylong_as_lld(item); if (lld == -1 && PyErr_Occurred()) goto err_occurred; + CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ PACK_SINGLE(ptr, lld, long long); break; case 'Q': llu = pylong_as_llu(item); if (llu == (unsigned long long)-1 && PyErr_Occurred()) goto err_occurred; + CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ PACK_SINGLE(ptr, llu, unsigned long long); break; @@ -1838,12 +1842,14 @@ pack_single(char *ptr, PyObject *item, const char *fmt) zd = pylong_as_zd(item); if (zd == -1 && PyErr_Occurred()) goto err_occurred; + CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ PACK_SINGLE(ptr, zd, Py_ssize_t); break; case 'N': zu = pylong_as_zu(item); if (zu == (size_t)-1 && PyErr_Occurred()) goto err_occurred; + CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ PACK_SINGLE(ptr, zu, size_t); break; @@ -1852,6 +1858,7 @@ pack_single(char *ptr, PyObject *item, const char *fmt) d = PyFloat_AsDouble(item); if (d == -1.0 && PyErr_Occurred()) goto err_occurred; + CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ if (fmt[0] == 'f') { PACK_SINGLE(ptr, d, float); } @@ -1865,6 +1872,7 @@ pack_single(char *ptr, PyObject *item, const char *fmt) ld = PyObject_IsTrue(item); if (ld < 0) return -1; /* preserve original error */ + CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ PACK_SINGLE(ptr, ld, _Bool); break; @@ -1882,6 +1890,7 @@ pack_single(char *ptr, PyObject *item, const char *fmt) p = PyLong_AsVoidPtr(item); if (p == NULL && PyErr_Occurred()) goto err_occurred; + CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ PACK_SINGLE(ptr, p, void *); break; @@ -2538,7 +2547,7 @@ memory_ass_sub(PyMemoryViewObject *self, PyObject *key, PyObject *value) if (key == Py_Ellipsis || (PyTuple_Check(key) && PyTuple_GET_SIZE(key)==0)) { ptr = (char *)view->buf; - return pack_single(ptr, value, fmt); + return pack_single(self, ptr, value, fmt); } else { PyErr_SetString(PyExc_TypeError, @@ -2557,10 +2566,11 @@ memory_ass_sub(PyMemoryViewObject *self, PyObject *key, PyObject *value) index = PyNumber_AsSsize_t(key, PyExc_IndexError); if (index == -1 && PyErr_Occurred()) return -1; + CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ ptr = ptr_from_index(view, index); if (ptr == NULL) return -1; - return pack_single(ptr, value, fmt); + return pack_single(self, ptr, value, fmt); } /* one-dimensional: fast path */ if (PySlice_Check(key) && view->ndim == 1) { @@ -2599,7 +2609,7 @@ memory_ass_sub(PyMemoryViewObject *self, PyObject *key, PyObject *value) ptr = ptr_from_tuple(view, key); if (ptr == NULL) return -1; - return pack_single(ptr, value, fmt); + return pack_single(self, ptr, value, fmt); } if (PySlice_Check(key) || is_multislice(key)) { /* Call memory_subscript() to produce a sliced lvalue, then copy From 3581de74e0bb4eb71e629f7ba9ad0f4762c1fc3d Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Thu, 19 May 2022 08:53:08 +0000 Subject: [PATCH 02/11] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst diff --git a/Misc/NEWS.d/next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst b/Misc/NEWS.d/next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst new file mode 100644 index 00000000000000..530ed8cafc94ab --- /dev/null +++ b/Misc/NEWS.d/next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst @@ -0,0 +1 @@ +Fix ``memoryview`` use after free when subscripting an object with bad ``__index__``. This occurs when the backing buffer is released inside ``__index__``. From 43c66d62749b43eb4e1f173959fee1cd75fbd622 Mon Sep 17 00:00:00 2001 From: Ken Date: Thu, 19 May 2022 16:56:47 +0800 Subject: [PATCH 03/11] stylistic nits --- Lib/test/test_memoryview.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_memoryview.py b/Lib/test/test_memoryview.py index 62abd481192207..6a3e43bcdf54a7 100644 --- a/Lib/test/test_memoryview.py +++ b/Lib/test/test_memoryview.py @@ -549,10 +549,11 @@ def test_memoryview_bad_index_uaf(self): # memoryview Use After Free (memory_ass_sub) see gh-92888 uaf_backing = bytearray(bytearray.__basicsize__) uaf_view = memoryview(uaf_backing).cast('n') # ssize_t format - + memory_backing = None + class weird_index: def __index__(self): - global memory_backing + nonlocal memory_backing uaf_view.release() # release memoryview (UAF) # free `uaf_backing` memory and allocate a new bytearray into it memory_backing = uaf_backing.clear() or bytearray() From 2f3c04f8f2f969c92977a00bd8e5dd0a147e32d2 Mon Sep 17 00:00:00 2001 From: Ken Date: Sat, 21 May 2022 21:55:01 +0800 Subject: [PATCH 04/11] Fix whitespace --- Lib/test/test_memoryview.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_memoryview.py b/Lib/test/test_memoryview.py index 6a3e43bcdf54a7..ba6b460385730d 100644 --- a/Lib/test/test_memoryview.py +++ b/Lib/test/test_memoryview.py @@ -550,7 +550,7 @@ def test_memoryview_bad_index_uaf(self): uaf_backing = bytearray(bytearray.__basicsize__) uaf_view = memoryview(uaf_backing).cast('n') # ssize_t format memory_backing = None - + class weird_index: def __index__(self): nonlocal memory_backing From a250b02d74d5e4d642911a421aa8a9c025daec21 Mon Sep 17 00:00:00 2001 From: Ken Date: Mon, 23 May 2022 18:57:00 +0800 Subject: [PATCH 05/11] Use more comprehensive tests by Serhiy Co-Authored-By: Serhiy Storchaka <3659035+serhiy-storchaka@users.noreply.github.com> --- Lib/test/test_memoryview.py | 97 +++++++++++++++---- ...2-05-19-08-53-07.gh-issue-92888.TLtR9W.rst | 2 +- Objects/memoryobject.c | 5 +- 3 files changed, 80 insertions(+), 24 deletions(-) diff --git a/Lib/test/test_memoryview.py b/Lib/test/test_memoryview.py index ba6b460385730d..4399e420b6cc1b 100644 --- a/Lib/test/test_memoryview.py +++ b/Lib/test/test_memoryview.py @@ -545,28 +545,83 @@ def test_pickle(self): with self.assertRaises(TypeError): pickle.dumps(m, proto) - def test_memoryview_bad_index_uaf(self): - # memoryview Use After Free (memory_ass_sub) see gh-92888 - uaf_backing = bytearray(bytearray.__basicsize__) - uaf_view = memoryview(uaf_backing).cast('n') # ssize_t format - memory_backing = None - - class weird_index: + def test_use_released_memory(self): + size = 128 + def release(): + m.release() + nonlocal ba + ba = bytearray(size) + class MyIndex: def __index__(self): - nonlocal memory_backing - uaf_view.release() # release memoryview (UAF) - # free `uaf_backing` memory and allocate a new bytearray into it - memory_backing = uaf_backing.clear() or bytearray() - return 2 # `ob_size` idx - - # by the time this line finishes executing, it writes the max ptr size - # into the `ob_size` slot of `memory_backing` - with self.assertRaisesRegex(ValueError, "operation forbidden on released memoryview object"): - uaf_view[weird_index()] = (2 ** (tuple.__itemsize__ * 8) - 1) // 2 - memory = memoryview(memory_backing) - with self.assertRaisesRegex(IndexError, "index out of bounds"): - memory[id(250) + int.__basicsize__] = 100 - self.assertEqual(250, eval("250")) + release() + return 4 + class MyFloat: + def __float__(self): + release() + return 4.25 + class MyBool: + def __bool__(self): + release() + return True + + ba = None + m = memoryview(bytearray(b'\xff'*size)) + with self.assertRaises(ValueError): + m[MyIndex()] + + ba = None + m = memoryview(bytearray(b'\xff'*size)) + self.assertEqual(list(m[:MyIndex()]), [255] * 4) + + ba = None + m = memoryview(bytearray(b'\xff'*size)) + self.assertEqual(list(m[MyIndex():8]), [255] * 4) + + ba = None + m = memoryview(bytearray(b'\xff'*size)) + with self.assertRaisesRegex(ValueError, "operation forbidden"): + m[MyIndex()] = 42 + self.assertEqual(ba[:8], b'\0'*8) + + ba = None + m = memoryview(bytearray(b'\xff'*size)) + with self.assertRaisesRegex(ValueError, "operation forbidden"): + m[:MyIndex()] = b'spam' + self.assertEqual(ba[:8], b'\0'*8) + + ba = None + m = memoryview(bytearray(b'\xff'*size)) + with self.assertRaisesRegex(ValueError, "operation forbidden"): + m[MyIndex():8] = b'spam' + self.assertEqual(ba[:8], b'\0'*8) + + ba = None + m = memoryview(bytearray(b'\xff'*size)) + with self.assertRaisesRegex(ValueError, "operation forbidden"): + m[0] = MyIndex() + self.assertEqual(ba[:8], b'\0'*8) + + for fmt in 'bhilqnBHILQN': + with self.subTest(fmt=fmt): + ba = None + m = memoryview(bytearray(b'\xff'*size)).cast(fmt) + with self.assertRaisesRegex(ValueError, "operation forbidden"): + m[0] = MyIndex() + self.assertEqual(ba[:8], b'\0'*8) + + for fmt in 'fd': + with self.subTest(fmt=fmt): + ba = None + m = memoryview(bytearray(b'\xff'*size)).cast(fmt) + with self.assertRaisesRegex(ValueError, "operation forbidden"): + m[0] = MyFloat() + self.assertEqual(ba[:8], b'\0'*8) + + ba = None + m = memoryview(bytearray(b'\xff'*size)).cast('?') + with self.assertRaisesRegex(ValueError, "operation forbidden"): + m[0] = MyBool() + self.assertEqual(ba[:8], b'\0'*8) if __name__ == "__main__": unittest.main() diff --git a/Misc/NEWS.d/next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst b/Misc/NEWS.d/next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst index 530ed8cafc94ab..cb93b9be151d8f 100644 --- a/Misc/NEWS.d/next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst +++ b/Misc/NEWS.d/next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst @@ -1 +1 @@ -Fix ``memoryview`` use after free when subscripting an object with bad ``__index__``. This occurs when the backing buffer is released inside ``__index__``. +Fix ``memoryview`` use after free when accessing the backing buffer in certain cases. \ No newline at end of file diff --git a/Objects/memoryobject.c b/Objects/memoryobject.c index 3c512e9976f993..45743fc319ff8c 100644 --- a/Objects/memoryobject.c +++ b/Objects/memoryobject.c @@ -381,8 +381,9 @@ copy_rec(const Py_ssize_t *shape, Py_ssize_t ndim, Py_ssize_t itemsize, /* Faster copying of one-dimensional arrays. */ static int -copy_single(const Py_buffer *dest, const Py_buffer *src) +copy_single(PyMemoryViewObject *self, const Py_buffer *dest, const Py_buffer *src) { + CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ char *mem = NULL; assert(dest->ndim == 1); @@ -2593,7 +2594,7 @@ memory_ass_sub(PyMemoryViewObject *self, PyObject *key, PyObject *value) goto end_block; dest.len = dest.shape[0] * dest.itemsize; - ret = copy_single(&dest, &src); + ret = copy_single(self, &dest, &src); end_block: PyBuffer_Release(&src); From c818990bec7eca524a8cc6d35b859fd54f8bc303 Mon Sep 17 00:00:00 2001 From: Ken Date: Mon, 23 May 2022 21:49:13 +0800 Subject: [PATCH 06/11] Add more tests by Serhiy Co-Authored-By: Serhiy Storchaka <3659035+serhiy-storchaka@users.noreply.github.com> --- Lib/test/test_memoryview.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/Lib/test/test_memoryview.py b/Lib/test/test_memoryview.py index 4399e420b6cc1b..4e3e355bd39052 100644 --- a/Lib/test/test_memoryview.py +++ b/Lib/test/test_memoryview.py @@ -577,6 +577,13 @@ def __bool__(self): m = memoryview(bytearray(b'\xff'*size)) self.assertEqual(list(m[MyIndex():8]), [255] * 4) + ba = None + m = memoryview(bytearray(b'\xff'*size)).cast('B', (64, 2)) + self.assertEqual(m[MyIndex(), 0], 0) + ba = None + m = memoryview(bytearray(b'\xff'*size)).cast('B', (2, 64)) + self.assertEqual(m[0, MyIndex()], 0) + ba = None m = memoryview(bytearray(b'\xff'*size)) with self.assertRaisesRegex(ValueError, "operation forbidden"): @@ -595,6 +602,17 @@ def __bool__(self): m[MyIndex():8] = b'spam' self.assertEqual(ba[:8], b'\0'*8) + ba = None + m = memoryview(bytearray(b'\xff'*size)).cast('B', (64, 2)) + with self.assertRaisesRegex(ValueError, "operation forbidden"): + m[MyIndex(), 0] = 42 + self.assertEqual(ba[8:16], b'\0'*8) + ba = None + m = memoryview(bytearray(b'\xff'*size)).cast('B', (2, 64)) + with self.assertRaisesRegex(ValueError, "operation forbidden"): + m[0, MyIndex()] = 42 + self.assertEqual(ba[:8], b'\0'*8) + ba = None m = memoryview(bytearray(b'\xff'*size)) with self.assertRaisesRegex(ValueError, "operation forbidden"): From 42a706c9597a7e7b06cc4380fef5a742053e083f Mon Sep 17 00:00:00 2001 From: Ken Date: Mon, 23 May 2022 21:50:16 +0800 Subject: [PATCH 07/11] Fix newline --- .../Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst b/Misc/NEWS.d/next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst index cb93b9be151d8f..4841b8a90a0879 100644 --- a/Misc/NEWS.d/next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst +++ b/Misc/NEWS.d/next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst @@ -1 +1,2 @@ -Fix ``memoryview`` use after free when accessing the backing buffer in certain cases. \ No newline at end of file +Fix ``memoryview`` use after free when accessing the backing buffer in certain cases. + From b6507642e25d462b48e27eb12f65c42de34787b0 Mon Sep 17 00:00:00 2001 From: Ken Date: Mon, 23 May 2022 22:45:16 +0800 Subject: [PATCH 08/11] Fix for tuple index --- Lib/test/test_memoryview.py | 11 +++++++---- Objects/memoryobject.c | 31 +++++++++++++++++-------------- 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/Lib/test/test_memoryview.py b/Lib/test/test_memoryview.py index 4e3e355bd39052..537943efa2e8e5 100644 --- a/Lib/test/test_memoryview.py +++ b/Lib/test/test_memoryview.py @@ -579,10 +579,13 @@ def __bool__(self): ba = None m = memoryview(bytearray(b'\xff'*size)).cast('B', (64, 2)) - self.assertEqual(m[MyIndex(), 0], 0) + with self.assertRaisesRegex(ValueError, "operation forbidden"): + m[MyIndex(), 0] + ba = None m = memoryview(bytearray(b'\xff'*size)).cast('B', (2, 64)) - self.assertEqual(m[0, MyIndex()], 0) + with self.assertRaisesRegex(ValueError, "operation forbidden"): + m[0, MyIndex()], 0 ba = None m = memoryview(bytearray(b'\xff'*size)) @@ -592,13 +595,13 @@ def __bool__(self): ba = None m = memoryview(bytearray(b'\xff'*size)) - with self.assertRaisesRegex(ValueError, "operation forbidden"): + with self.assertRaisesRegex(ValueError, "operation forbidden"): m[:MyIndex()] = b'spam' self.assertEqual(ba[:8], b'\0'*8) ba = None m = memoryview(bytearray(b'\xff'*size)) - with self.assertRaisesRegex(ValueError, "operation forbidden"): + with self.assertRaisesRegex(ValueError, "operation forbidden"): m[MyIndex():8] = b'spam' self.assertEqual(ba[:8], b'\0'*8) diff --git a/Objects/memoryobject.c b/Objects/memoryobject.c index 45743fc319ff8c..37dec08efd2d86 100644 --- a/Objects/memoryobject.c +++ b/Objects/memoryobject.c @@ -1678,7 +1678,7 @@ pylong_as_zu(PyObject *item) module syntax. This function is very sensitive to small changes. With this layout gcc automatically generates a fast jump table. */ static inline PyObject * -unpack_single(const char *ptr, const char *fmt) +unpack_single(PyMemoryViewObject *self, const char *ptr, const char *fmt) { unsigned long long llu; unsigned long lu; @@ -1690,6 +1690,8 @@ unpack_single(const char *ptr, const char *fmt) unsigned char uc; void *p; + CHECK_RELEASED(self); /* See gh-92888 for why we need this here */ + switch (fmt[0]) { /* signed integers and fast path for 'B' */ @@ -1779,6 +1781,8 @@ pack_single(PyMemoryViewObject *self, char *ptr, PyObject *item, const char *fmt double d; void *p; + CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ + switch (fmt[0]) { /* signed integers */ case 'b': case 'h': case 'i': case 'l': @@ -2058,7 +2062,7 @@ adjust_fmt(const Py_buffer *view) /* Base case for multi-dimensional unpacking. Assumption: ndim == 1. */ static PyObject * -tolist_base(const char *ptr, const Py_ssize_t *shape, +tolist_base(PyMemoryViewObject *self, const char *ptr, const Py_ssize_t *shape, const Py_ssize_t *strides, const Py_ssize_t *suboffsets, const char *fmt) { @@ -2071,7 +2075,7 @@ tolist_base(const char *ptr, const Py_ssize_t *shape, for (i = 0; i < shape[0]; ptr+=strides[0], i++) { const char *xptr = ADJUST_PTR(ptr, suboffsets, 0); - item = unpack_single(xptr, fmt); + item = unpack_single(self, xptr, fmt); if (item == NULL) { Py_DECREF(lst); return NULL; @@ -2085,7 +2089,7 @@ tolist_base(const char *ptr, const Py_ssize_t *shape, /* Unpack a multi-dimensional array into a nested list. Assumption: ndim >= 1. */ static PyObject * -tolist_rec(const char *ptr, Py_ssize_t ndim, const Py_ssize_t *shape, +tolist_rec(PyMemoryViewObject *self, const char *ptr, Py_ssize_t ndim, const Py_ssize_t *shape, const Py_ssize_t *strides, const Py_ssize_t *suboffsets, const char *fmt) { @@ -2097,7 +2101,7 @@ tolist_rec(const char *ptr, Py_ssize_t ndim, const Py_ssize_t *shape, assert(strides != NULL); if (ndim == 1) - return tolist_base(ptr, shape, strides, suboffsets, fmt); + return tolist_base(self, ptr, shape, strides, suboffsets, fmt); lst = PyList_New(shape[0]); if (lst == NULL) @@ -2105,7 +2109,7 @@ tolist_rec(const char *ptr, Py_ssize_t ndim, const Py_ssize_t *shape, for (i = 0; i < shape[0]; ptr+=strides[0], i++) { const char *xptr = ADJUST_PTR(ptr, suboffsets, 0); - item = tolist_rec(xptr, ndim-1, shape+1, + item = tolist_rec(self, xptr, ndim-1, shape+1, strides+1, suboffsets ? suboffsets+1 : NULL, fmt); if (item == NULL) { @@ -2139,15 +2143,15 @@ memoryview_tolist_impl(PyMemoryViewObject *self) if (fmt == NULL) return NULL; if (view->ndim == 0) { - return unpack_single(view->buf, fmt); + return unpack_single(self, view->buf, fmt); } else if (view->ndim == 1) { - return tolist_base(view->buf, view->shape, + return tolist_base(self, view->buf, view->shape, view->strides, view->suboffsets, fmt); } else { - return tolist_rec(view->buf, view->ndim, view->shape, + return tolist_rec(self, view->buf, view->ndim, view->shape, view->strides, view->suboffsets, fmt); } @@ -2355,7 +2359,7 @@ memory_item(PyMemoryViewObject *self, Py_ssize_t index) char *ptr = ptr_from_index(view, index); if (ptr == NULL) return NULL; - return unpack_single(ptr, fmt); + return unpack_single(self, ptr, fmt); } PyErr_SetString(PyExc_NotImplementedError, @@ -2386,7 +2390,7 @@ memory_item_multi(PyMemoryViewObject *self, PyObject *tup) ptr = ptr_from_tuple(view, tup); if (ptr == NULL) return NULL; - return unpack_single(ptr, fmt); + return unpack_single(self, ptr, fmt); } static inline int @@ -2473,7 +2477,7 @@ memory_subscript(PyMemoryViewObject *self, PyObject *key) const char *fmt = adjust_fmt(view); if (fmt == NULL) return NULL; - return unpack_single(view->buf, fmt); + return unpack_single(self, view->buf, fmt); } else if (key == Py_Ellipsis) { Py_INCREF(self); @@ -2567,7 +2571,6 @@ memory_ass_sub(PyMemoryViewObject *self, PyObject *key, PyObject *value) index = PyNumber_AsSsize_t(key, PyExc_IndexError); if (index == -1 && PyErr_Occurred()) return -1; - CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ ptr = ptr_from_index(view, index); if (ptr == NULL) return -1; @@ -3211,7 +3214,7 @@ memoryiter_next(memoryiterobject *it) if (ptr == NULL) { return NULL; } - return unpack_single(ptr, it->it_fmt); + return unpack_single(seq, ptr, it->it_fmt); } it->it_seq = NULL; From 47bd91f3710c7478f3ecfb514a3ca7fe6b436430 Mon Sep 17 00:00:00 2001 From: Ken Date: Mon, 23 May 2022 23:45:18 +0800 Subject: [PATCH 09/11] Address review --- Lib/test/test_memoryview.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_memoryview.py b/Lib/test/test_memoryview.py index 537943efa2e8e5..3cd62ef8db27f9 100644 --- a/Lib/test/test_memoryview.py +++ b/Lib/test/test_memoryview.py @@ -581,11 +581,11 @@ def __bool__(self): m = memoryview(bytearray(b'\xff'*size)).cast('B', (64, 2)) with self.assertRaisesRegex(ValueError, "operation forbidden"): m[MyIndex(), 0] - + ba = None m = memoryview(bytearray(b'\xff'*size)).cast('B', (2, 64)) with self.assertRaisesRegex(ValueError, "operation forbidden"): - m[0, MyIndex()], 0 + m[0, MyIndex()] ba = None m = memoryview(bytearray(b'\xff'*size)) From 68907139c943d168f19b2a4c116f9a2b055e3180 Mon Sep 17 00:00:00 2001 From: Ken Jin Date: Sun, 29 May 2022 14:36:06 +0800 Subject: [PATCH 10/11] Address Victor's review --- Lib/test/test_memoryview.py | 8 +++----- Objects/memoryobject.c | 29 ++++++++++++++++------------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/Lib/test/test_memoryview.py b/Lib/test/test_memoryview.py index 3cd62ef8db27f9..33e806f1008c36 100644 --- a/Lib/test/test_memoryview.py +++ b/Lib/test/test_memoryview.py @@ -546,6 +546,9 @@ def test_pickle(self): pickle.dumps(m, proto) def test_use_released_memory(self): + # gh-92888: Previously it was possible to use a memoryview even after + # backing buffer is freed in certain cases. This tests that those + # cases raise an exception. size = 128 def release(): m.release() @@ -564,25 +567,20 @@ def __bool__(self): release() return True - ba = None m = memoryview(bytearray(b'\xff'*size)) with self.assertRaises(ValueError): m[MyIndex()] - ba = None m = memoryview(bytearray(b'\xff'*size)) self.assertEqual(list(m[:MyIndex()]), [255] * 4) - ba = None m = memoryview(bytearray(b'\xff'*size)) self.assertEqual(list(m[MyIndex():8]), [255] * 4) - ba = None m = memoryview(bytearray(b'\xff'*size)).cast('B', (64, 2)) with self.assertRaisesRegex(ValueError, "operation forbidden"): m[MyIndex(), 0] - ba = None m = memoryview(bytearray(b'\xff'*size)).cast('B', (2, 64)) with self.assertRaisesRegex(ValueError, "operation forbidden"): m[0, MyIndex()] diff --git a/Objects/memoryobject.c b/Objects/memoryobject.c index 37dec08efd2d86..e958ab45963582 100644 --- a/Objects/memoryobject.c +++ b/Objects/memoryobject.c @@ -193,6 +193,11 @@ PyTypeObject _PyManagedBuffer_Type = { return -1; \ } +/* See gh-92888. These macros signal that we need to check the memoryview + again due to possible read after frees. */ +#define CHECK_RELEASED_AGAIN(mv) CHECK_RELEASED(mv) +#define CHECK_RELEASED_INT_AGAIN(mv) CHECK_RELEASED_INT(mv) + #define CHECK_LIST_OR_TUPLE(v) \ if (!PyList_Check(v) && !PyTuple_Check(v)) { \ PyErr_SetString(PyExc_TypeError, \ @@ -383,7 +388,7 @@ copy_rec(const Py_ssize_t *shape, Py_ssize_t ndim, Py_ssize_t itemsize, static int copy_single(PyMemoryViewObject *self, const Py_buffer *dest, const Py_buffer *src) { - CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ + CHECK_RELEASED_INT_AGAIN(self); char *mem = NULL; assert(dest->ndim == 1); @@ -1690,7 +1695,7 @@ unpack_single(PyMemoryViewObject *self, const char *ptr, const char *fmt) unsigned char uc; void *p; - CHECK_RELEASED(self); /* See gh-92888 for why we need this here */ + CHECK_RELEASED_AGAIN(self); switch (fmt[0]) { @@ -1781,15 +1786,13 @@ pack_single(PyMemoryViewObject *self, char *ptr, PyObject *item, const char *fmt double d; void *p; - CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ - switch (fmt[0]) { /* signed integers */ case 'b': case 'h': case 'i': case 'l': ld = pylong_as_ld(item); if (ld == -1 && PyErr_Occurred()) goto err_occurred; - CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ + CHECK_RELEASED_INT_AGAIN(self); switch (fmt[0]) { case 'b': if (ld < SCHAR_MIN || ld > SCHAR_MAX) goto err_range; @@ -1810,7 +1813,7 @@ pack_single(PyMemoryViewObject *self, char *ptr, PyObject *item, const char *fmt lu = pylong_as_lu(item); if (lu == (unsigned long)-1 && PyErr_Occurred()) goto err_occurred; - CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ + CHECK_RELEASED_INT_AGAIN(self); switch (fmt[0]) { case 'B': if (lu > UCHAR_MAX) goto err_range; @@ -1831,14 +1834,14 @@ pack_single(PyMemoryViewObject *self, char *ptr, PyObject *item, const char *fmt lld = pylong_as_lld(item); if (lld == -1 && PyErr_Occurred()) goto err_occurred; - CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ + CHECK_RELEASED_INT_AGAIN(self); PACK_SINGLE(ptr, lld, long long); break; case 'Q': llu = pylong_as_llu(item); if (llu == (unsigned long long)-1 && PyErr_Occurred()) goto err_occurred; - CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ + CHECK_RELEASED_INT_AGAIN(self); PACK_SINGLE(ptr, llu, unsigned long long); break; @@ -1847,14 +1850,14 @@ pack_single(PyMemoryViewObject *self, char *ptr, PyObject *item, const char *fmt zd = pylong_as_zd(item); if (zd == -1 && PyErr_Occurred()) goto err_occurred; - CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ + CHECK_RELEASED_INT_AGAIN(self); PACK_SINGLE(ptr, zd, Py_ssize_t); break; case 'N': zu = pylong_as_zu(item); if (zu == (size_t)-1 && PyErr_Occurred()) goto err_occurred; - CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ + CHECK_RELEASED_INT_AGAIN(self); PACK_SINGLE(ptr, zu, size_t); break; @@ -1863,7 +1866,7 @@ pack_single(PyMemoryViewObject *self, char *ptr, PyObject *item, const char *fmt d = PyFloat_AsDouble(item); if (d == -1.0 && PyErr_Occurred()) goto err_occurred; - CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ + CHECK_RELEASED_INT_AGAIN(self); if (fmt[0] == 'f') { PACK_SINGLE(ptr, d, float); } @@ -1877,7 +1880,7 @@ pack_single(PyMemoryViewObject *self, char *ptr, PyObject *item, const char *fmt ld = PyObject_IsTrue(item); if (ld < 0) return -1; /* preserve original error */ - CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ + CHECK_RELEASED_INT_AGAIN(self); PACK_SINGLE(ptr, ld, _Bool); break; @@ -1895,7 +1898,7 @@ pack_single(PyMemoryViewObject *self, char *ptr, PyObject *item, const char *fmt p = PyLong_AsVoidPtr(item); if (p == NULL && PyErr_Occurred()) goto err_occurred; - CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */ + CHECK_RELEASED_INT_AGAIN(self); PACK_SINGLE(ptr, p, void *); break; From d3edf749ee8ff143c3c9333d969eeffbf6786798 Mon Sep 17 00:00:00 2001 From: Ken Jin Date: Mon, 30 May 2022 17:25:07 +0800 Subject: [PATCH 11/11] re-add ba=None --- Lib/test/test_memoryview.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Lib/test/test_memoryview.py b/Lib/test/test_memoryview.py index 33e806f1008c36..9d1e1f3063ca55 100644 --- a/Lib/test/test_memoryview.py +++ b/Lib/test/test_memoryview.py @@ -567,20 +567,25 @@ def __bool__(self): release() return True + ba = None m = memoryview(bytearray(b'\xff'*size)) with self.assertRaises(ValueError): m[MyIndex()] + ba = None m = memoryview(bytearray(b'\xff'*size)) self.assertEqual(list(m[:MyIndex()]), [255] * 4) + ba = None m = memoryview(bytearray(b'\xff'*size)) self.assertEqual(list(m[MyIndex():8]), [255] * 4) + ba = None m = memoryview(bytearray(b'\xff'*size)).cast('B', (64, 2)) with self.assertRaisesRegex(ValueError, "operation forbidden"): m[MyIndex(), 0] + ba = None m = memoryview(bytearray(b'\xff'*size)).cast('B', (2, 64)) with self.assertRaisesRegex(ValueError, "operation forbidden"): m[0, MyIndex()]