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

take_seek doesn't correctly limit std::io::SeekFrom::End #291

Closed
ismell opened this issue Oct 11, 2024 · 0 comments · May be fixed by #292
Closed

take_seek doesn't correctly limit std::io::SeekFrom::End #291

ismell opened this issue Oct 11, 2024 · 0 comments · May be fixed by #292
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@ismell
Copy link

ismell commented Oct 11, 2024

It seems that take_seek doesn't actually remap anything when calling seek. That means that calling seek can put the pointer outside of the defined buffer.

The following assert fails because the seek operation doesn't take into account the start and end constraints.

file.seek(128)?; // Place the pos at 128
let image: &mut TakeSeek<&mut File> = file.take_seek(128); // Make end 256
let offset = image.seek(SeekFrom::End(-64))?; // Should move the cursor to 192 internally, but it moves it to `EoF - 64`.
assert_eq!(offset, 64) <--- Should be `192 - pos`
@csnover csnover added bug Something isn't working good first issue Good for newcomers labels Oct 11, 2024
csnover added a commit to csnover/binrw that referenced this issue Oct 13, 2024
csnover added a commit that referenced this issue Oct 13, 2024
@csnover csnover self-assigned this Oct 13, 2024
ismell added a commit to ismell/binrw that referenced this issue Oct 14, 2024
This change adds support for the missing SeekFrom operations. It
refactors the internals a bit. We now keep a range of start..end values
that are accessible to the TakeSeek. I dropped manually computed stream
position and instead call the inner's `stream_position()`. This does
mean that `limit` needs to take in a `&mut`. I was also going to change
the return type to `Result<u64>`, but that would force all callers to
handle the `Result`, which I felt like it was a bigger API change.

I fixed the test added in 38b3598
because `StartFrom::Start(0)` used to reset the cursor to the start of
the underlying buffer.

Fixes jam1garner#291.
csnover added a commit to csnover/binrw that referenced this issue Nov 27, 2024
The purpose of `TakeSeek` is to be a binrw-compatible version of
`std::io::Take`, and the purpose of `std::io::Take` is only to
truncate the stream to a certain number of bytes, not to extend the
stream if a limit beyond the length of the inner stream is given.
Thus, `TakeSeek` also needs to pay attention to the true end of the
inner stream and use that as the `SeekFrom::End(0)` position if it
is less than the limit.

Refs jam1garner#291.
csnover added a commit to csnover/binrw that referenced this issue Dec 1, 2024
The purpose of `TakeSeek` is to be a binrw-compatible version of
`std::io::Take`, and the purpose of `std::io::Take` is only to
truncate the stream to a certain number of bytes, not to extend the
stream if a limit beyond the length of the inner stream is given.
Thus, `TakeSeek` also needs to pay attention to the true end of the
inner stream and use that as the `SeekFrom::End(0)` position if it
is less than the limit.

Refs jam1garner#291.
csnover added a commit to csnover/binrw that referenced this issue Dec 1, 2024
The purpose of `TakeSeek` is to be a binrw-compatible version of
`std::io::Take`, and the purpose of `std::io::Take` is only to
truncate the stream to a certain number of bytes, not to extend the
stream if a limit beyond the length of the inner stream is given.
Thus, `TakeSeek` also needs to pay attention to the true end of the
inner stream and use that as the `SeekFrom::End(0)` position if it
is less than the limit.

Refs jam1garner#291.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants