Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-60107: avoid io.RawIOBase.read from reading into a temporary bytearray#138616
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
taskevich commented Sep 7, 2025 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
…table memoryview pointing to it.
python-cla-botbot commented Sep 7, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Misc/NEWS.d/next/C_API/2025-09-07-10-52-09.gh-issue-60107.dhcb2Y.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
io.RawIOBase.read from reading into a temporary bytearraypicnixz commented Sep 7, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Can you indicate how efficient this actually becomes by using |
picnixz commented Sep 7, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
While I think this addition is fine, I'm worried that it could change performances a bit. So I would appreciate if we had numbers indicating whether this changed or not the performance (you can look at https://github.com/python/pyperformance for inspiration of how to write benchmarks). By the way, avoid merging main into your branch unless it's meant to fix CI tests. And remove the NEWS entry for now. We'll add it later if needed. If possible, it would be good if we also had tests where we have a class inheriting from RawIOBase and which tries to mess with the buffer passed to |
Misc/NEWS.d/next/C_API/2025-09-07-10-52-09.gh-issue-60107.dhcb2Y.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
…2Y.rst Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Misc/NEWS.d/next/C_API/2025-09-07-10-52-09.gh-issue-60107.dhcb2Y.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
auvipy left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also consider adding some extra unit tests for this change?
picnixz commented Sep 7, 2025
This was already mentioned:
|
| self.assertEqual(rawio.read(2), b"") | ||
| deftest_exact_RawIOBase(self): | ||
| rawio=self.MockRawIOWithoutRead((b"ab", b"cd")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I wasn't precise enough. RawIOBase.read() is implemented in terms of readinto. So, what we want to do is provide a readinto method that does something bad as it's given the internal buffer we constructed in C. I don't know if it's possible to cause a segfault on main with that approach though.
Stated otherwise, we want some class:
classEvilReadInto(MockRawIOWithoutRead): defreadinto(self, buf): # do something bad with 'buf'And then
r=EvilReadInto(something_here) r.read() # on main, this call should crash, but not with this patchpicnixz commented Sep 7, 2025
I'm going to convert it into a draft until a regression can be found between main and this PR. IOW, we want to be able to do something evil with the buffer we are given in |
cmaloney commented Nov 20, 2025
Thanks for the pull request @taskevich ! A slightly different fix for this has landed in #141532 so going to close this one. |
implements: https://github.com/python/cpython/issues/60107
bytearraytobytesin rawiobase_read() implementation #60107