Use more comprehensive tests by Serhiy

Co-Authored-By: Serhiy Storchaka <3659035+serhiy-storchaka@users.noreply.github.com>
python · Fidget-Spinner · Jun 17, 2022 · May 19, 2022 · May 19, 2022 · May 19, 2022
commit a250b02d74d5e4d642911a421aa8a9c025daec21
diff --git 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
@@ -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.
diff --git 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);
Original file line number	Diff line number	Diff line change
		@@ -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.