Skip to content

Conversation

@taskevich
Copy link

@taskevich taskevich commented Sep 7, 2025

@python-cla-bot
Copy link

python-cla-bot bot commented Sep 7, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@bedevere-app

This comment was marked as resolved.

@picnixz picnixz changed the title gh-60107: Read directly to bytes object gh-60107: avoid io.RawIOBase.read from reading into a temporary bytearray Sep 7, 2025
@picnixz
Copy link
Member

picnixz commented Sep 7, 2025

Can you indicate how efficient this actually becomes by using pyperf? there was not clear consensus on the issue considering what should be done.

@picnixz
Copy link
Member

picnixz commented Sep 7, 2025

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 readinto

@picnixz picnixz removed the skip news label Sep 7, 2025
Copy link

@auvipy auvipy left a 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
Copy link
Member

picnixz commented Sep 7, 2025

This was already mentioned:

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 readinto

@taskevich taskevich closed this Sep 7, 2025
@taskevich taskevich reopened this Sep 7, 2025
self.assertEqual(rawio.read(2), b"")

def test_exact_RawIOBase(self):
rawio = self.MockRawIOWithoutRead((b"ab", b"cd"))
Copy link
Member

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:

class EvilReadInto(MockRawIOWithoutRead):
    def readinto(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 patch

@picnixz picnixz marked this pull request as draft September 7, 2025 14:19
@picnixz
Copy link
Member

picnixz 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 readinto when using main, but we want that evil thing to be impossible with this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants