-
-
Notifications
You must be signed in to change notification settings - Fork 32k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
73ffb29
3581de7
43c66d6
18b921f
2f3c04f
a250b02
c818990
42a706c
b650764
47bd91f
6890713
d3edf74
File filter
Filter by extension
Conversations
Jump to
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): | ||
# 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) | ||
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? 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 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)) | ||
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()] | ||
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? 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 resolved
Hide resolved
|
||
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 resolved
Hide resolved
|
||
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. | ||
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:
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. |
||
|
Uh oh!
There was an error while loading. Please reload this page.