gh-60198: Prevent memoryview pointing to freed heap memory #105290
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Martin Panter vadmium+py@gmail.com
gpshead
added
type-security
AA-Turner
changed the title
ambv
reviewed
Jul 31, 2023
Comment on lines
+1563
to
+1571
| PyObject *exc = PyErr_GetRaisedException(); | ||
| release_res = PyObject_CallMethod(memobj, "release", NULL); | ||
| _PyErr_ChainExceptions1(exc); | ||
| Py_DECREF(memobj); | ||
| if (release_res == NULL) { | ||
| Py_XDECREF(res); | ||
| return -1; | ||
| } | ||
| Py_DECREF(release_res); |
There was a problem hiding this comment.
This makes sense to me. The performance impact should be minimal as it's only a cleanup method call after the buffer's been entirely read.
However, since mv.release() can potentially raise, this is a change in behavior so we have to decide whether the risk of existing code breakage is worth backporting the patch to older releases.
I see @gpshead decided this isn't a security issue so we won't be bringing it to 3.8, 3.9, 3.10. Question is if we want it in 3.11 and 3.12.
furkanonder
changed the title
|
Edit: Ah, my comment below is just an example of point number 1 here: #60198 (comment) I'm not very familiar with the Python C APIs, so I might be mistaken, but I don't think this change is enough to categorically prevent use-after-free here. This PR is releasing the diff --git a/Lib/test/test_memoryview.py b/Lib/test/test_memoryview.py
index 0eac7bc06a..922104d55c 100644
--- a/Lib/test/test_memoryview.py
+++ b/Lib/test/test_memoryview.py
@@ -660,13 +660,13 @@ def __bool__(self):
def test_gh60198(self):
global view
class File(io.RawIOBase):
def readinto(self, buf):
global view
- view = buf
+ view = memoryview(buf)
def readable(self):
return True
f = io.BufferedReader(File())
# get view of buffer used by BufferedReader |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Co-authored-by: Martin Panter vadmium+py@gmail.com