Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
gh-92888: Fix memoryview bad__index__ use after free#92946
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
73ffb293581de743c66d618b921f2f3c04fa250b02c81899042a706cb65076447bd91f6890713d3edf74File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -545,6 +545,107 @@ def test_pickle(self): | ||
| with self.assertRaises(TypeError): | ||
| pickle.dumps(m, proto) | ||
| def test_use_released_memory(self): | ||
Fidget-Spinner marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| # 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() | ||
| nonlocal ba | ||
| ba = bytearray(size) | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. That's useless, no? MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. No, we need it for tests below that tests indexing into Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. We allocate a bytearray of the same size as the bytearray just released in memoryview in hope that it will be allocated at the same memory. It helps to check that we do nor read/write a freed memory. | ||
| class MyIndex: | ||
| def __index__(self): | ||
| 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)) | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. In my PR, I tried to make the code more generic to test more cases:https://github.com/python/cpython/pull/93127/files#diff-d41c6bb40a1e03fea5a20d15c4077413e0ddde65651147922b625b03a66a2f16R399: | ||
| with self.assertRaises(ValueError): | ||
| m[MyIndex()] | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. This test is very long. Can you try to factorize similar code and use loop with subTest(), and put pack operations in one test method and unpack in another test method? Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Then we will need to duplicate the definitions of internal classes. The tested code is so different, that it is difficult to use a loop. And I think that the result will be more complicated. | ||
| 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) | ||
Fidget-Spinner marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| 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()] | ||
| 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) | ||
Fidget-Spinner marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| 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"): | ||
| 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() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Fix ``memoryview`` use after free when accessing the backing buffer in certain cases. | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I propose to mention more explicitly that the protection is about released views: Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Not always an exception is raised. The bug was in reading or wring the freed memory. Now it is prevented -- you either get an exception or free the memory after reading.@Fidget-Spinner's description is more correct. I am going to address such inconsistency in a separate issue. | ||