Skip to content
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

fix: prevent buffer overflow in ReadAt copy operation #258

Closed
wants to merge 1 commit into from

Conversation

a4913994
Copy link

@a4913994 a4913994 commented Nov 29, 2024

This prevents potential panics from buffer overflows

@a4913994 a4913994 requested review from a team as code owners November 29, 2024 05:26
Copy link

linux-foundation-easycla bot commented Nov 29, 2024

CLA Signed


The committers listed above are authorized under a signed CLA.

@athre0z
Copy link
Member

athre0z commented Nov 29, 2024

Ah, good catch and thanks for the PR!

I played around with the exact conditions to actually trigger a crash. The min logic was supposed to make this safe without an explicit check, but it seems that I forgot to take skipOffset into account here. If the read offset falls into the tail chunk and over-reads it, it currently causes a panic. I think we can simplify the conditions a bit, to just check the skipOffset. We should also return io.EOF instead of a generic error. That's the expected error for when you over-read in a ReaderAt implementation.

Here's the variant that I came up with: zystem-io@c29f3b9

Would that also fix your problem?

@a4913994
Copy link
Author

Yes, Simplifying the conditions and returning io.EOF is a better approach. Great job!

@athre0z
Copy link
Member

athre0z commented Nov 29, 2024

Cool! Created PR with the alternative variant here to spare you the work of mirroring the changes on your branch: #259. Thanks again for catching this! :)

@florianl
Copy link
Contributor

Closing as resolved with the alternative of #259. Thanks @a4913994 for identifying the issue.

@florianl florianl closed this Nov 29, 2024
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