ReadAdapter is broken #308
Comments
bitwalker
added a commit
to bitwalker/winterfell
that referenced
this issue
Sep 6, 2024
This commit fixes a bug that is present in the implementation of ByteReader for ReadAdapter. Specifically, it does not consume any input, so calling `read_slice` and then attempting to read the next value in the input, will read the same bytes again. The documented behavior of this function is that any of the `read_*` methods consume the corresponding amount of input. To catch this, and to prevent future regressions, a new test was added that serializes some data to a file using one approach, and deserializes it using the ReadAdapter. This ensures that we don't accidentally make choices in the writer that aren't matched by the reader, and vice versa. Closes facebook#308
bitwalker
added a commit
to bitwalker/winterfell
that referenced
this issue
Sep 6, 2024
This commit fixes a bug that is present in the implementation of ByteReader for ReadAdapter. Specifically, it does not consume any input, so calling `read_slice` and then attempting to read the next value in the input, will read the same bytes again. The documented behavior of this function is that any of the `read_*` methods consume the corresponding amount of input. To catch this, and to prevent future regressions, a new test was added that serializes some data to a file using one approach, and deserializes it using the ReadAdapter. This ensures that we don't accidentally make choices in the writer that aren't matched by the reader, and vice versa. Closes facebook#308
bitwalker
added a commit
to bitwalker/winterfell
that referenced
this issue
Sep 6, 2024
This commit fixes a bug that is present in the implementation of ByteReader for ReadAdapter. Specifically, it does not consume any input, so calling `read_slice` and then attempting to read the next value in the input, will read the same bytes again. The documented behavior of this function is that any of the `read_*` methods consume the corresponding amount of input. To catch this, and to prevent future regressions, a new test was added that serializes some data to a file using one approach, and deserializes it using the ReadAdapter. This ensures that we don't accidentally make choices in the writer that aren't matched by the reader, and vice versa. Closes facebook#308
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
It appears that the implementation of ReadAdapter, at least when applied to a
File, is broken. I haven't yet looked to pin down the specific problem, but it appears to me that we may have an off-by-one error of some kind, as we get a panic attempting to read beyond the end of a buffer by 1 byte (at least in the case I have where this comes up).I'll try to take a look at this tomorrow, but wanted to at least get this bug on our radar, as it currently affects various bits of Miden functionality that builds on
winter-utils.The text was updated successfully, but these errors were encountered: