Skip to content

Conversation

@bitwalker
Copy link
Contributor

@bitwalker bitwalker commented Jun 6, 2025

When looking into a deserialization issue in another project, I discovered that we were incorrectly checking the length of the unread buffer when attempting to read N bytes as a slice. As a result, deserialization would fail any time read_slice was called when the current total buffer length as >= N, but the unread portion was < N.

@bitwalker
Copy link
Contributor Author

@irakliyk I think it might be problematic to run clippy's nightly version when the MSRV of these crates is much lower, at least as a required check. At least in this case, it causes CI to fail even though I haven't made any changes to the affected code.

Copy link
Collaborator

@irakliyk irakliyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you! Should we also add a test to prevent potential future regressions?

@irakliyk I think it might be problematic to run clippy's nightly version when the MSRV of these crates is much lower, at least as a required check. At least in this case, it causes CI to fail even though I haven't made any changes to the affected code.

No worries - I can merge as is and then fix the lints in a follow-up commit.

@bitwalker
Copy link
Contributor Author

Should we also add a test to prevent potential future regressions

Certainly!

No worries - I can merge as is and then fix the lints in a follow-up commit.

This is actually the problem - I would have done that myself, but the issue is that the clippy lint cannot be satisfied without breaking compatibility with the MSRV of the crates involved. I'm also not sure if we can disable those lints using a crate-wide attribute without hitting an error when clippy is run with an older toolchain (e.g. the MSRV version, because the lint won't exist there). I'm not 100% sure on whether unknown lints trigger an error though, and ignoring lints that way might be the solution, but the downside is that one has to remember to remove all of those ignored lints when bumping the MSRV, but that might be the better tradeoff to make.

I'm not sure there is any great options, but figured I would mention it since the CI run failed in this case.

When determining whether we have sufficient bytes available to read a
slice, we were incorrectly looking at the total length of the current
buffer, not the portion of the buffer that is unread. As a result, we
wouldn't attempt to fill the buffer with more bytes, and a subsequent
attempt to read the requisite bytes from the buffer would fail, because
there weren't in fact enough to fufill the request from the unread
portion of the buffer.
@bitwalker bitwalker force-pushed the bitwalker/slice-reader-fix branch from 9ae2b3d to 345e5c2 Compare June 9, 2025 16:00
@bitwalker
Copy link
Contributor Author

@irakliyk I've added a test that reproduced the original issue without the patch, which now passes with this patch. The issue is a bit more subtle than I originally described, in that it only affects cases where we are reading from something that doesn't implement AsRef<[u8]>, such as a File, since only in those cases do we buffer in the ReadAdapter itself, which is why the existing tests didn't catch the bug (that, and the existing file test didn't meet the conditions to trigger it).

Should be good to go now!

@irakliyk irakliyk merged commit 47700ad into facebook:main Jun 13, 2025
7 of 8 checks passed
@irakliyk irakliyk mentioned this pull request Jun 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants