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 bug in ReadAdapter #345

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Dec 11, 2024

This PR attempts to fix a bug which results in a deserialization error, spotted during the Miden standard library deserialization.

Copy link
Contributor Author

@Fumuran Fumuran left a comment

Choose a reason for hiding this comment

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

I spotted two bugs, the first of which wasn't causing the initial problem with stdlib, but was causing a deserialization error during the attempt to read a long array or slice. @bitwalker correct me if I'm wrong somewhere, I'm not 100% sure I got the logic.

Comment on lines +396 to +399
_ => {
let needed = N - n;
drop(reader_buf);
self.buffer_at_least(needed)?;
self.buffer_at_least(needed, true)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case of a long array (with length >454) we will execute this match branch. There are two problems:

  1. The needed amount of bytes was insufficient: we compute it as N - m - n, but as a result we add this needed amount of bytes only to the n (and not to the n + m), and compare it with N which (as far as I can see) always results in debug assert failure. We move this needed amount of bytes from the internal reader buffer (which length here is represented with m), but we don't use the remaining internal reader bytes for reading. To fix that I calculate the needed amount as N - n which is sufficient to make resulting n at least N.
  2. The second problem was with buffer_at_least() function. As far as I can see, the idea of using it here was to add the needed amount of bytes to the high-level buffer. The problem is that this procedure stops after the buffer has at least needed bytes, which could even do nothing, if the length of the buffer already bigger than needed. So, as a clumsy but at least working solution I added a flag, telling the function that we need to append all needed bytes, whatever the length of the buffer. I can't just change this function to always append the bytes, since the original version is needed for the read_slice.

Copy link
Contributor Author

@Fumuran Fumuran left a comment

Choose a reason for hiding this comment

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

The second bug, which was causing and error during the deserialization of the stdlib, was connected with reading a long vector of something bigger than u8 (u16, u32 and u64).

I can't refer to the code block, since it wasn't modified, so I'll describe the idea. Every time we read some, for example, u32 value (as an element of the vector) we essentially read an array of length 4. First we read a high-lever buffer, and when it's become empty — the internal reader buffer (and execute the lines 342-353). The length of the reader buffer in my tests wasn't divisible by 4, so eventually we have a case when the length of the reader buffer is smaller than 4, but bigger than 0 (in my case it was 3 or 2, depending on test). non_empty_reader_buffer_mut() function is not filling the internal buffer with some more bytes, since it is still not empty, but the following if returns an UnexpectedEOF error, since the length of the buffer is smaller than a size of the array.

I tried to solve it by moving the internal buffer contents to the high-level buffer in case it is got empty, but my implementation is far from the perfection, and also I'm not sure that this is the best approach. Probably it makes sense to rewrite the read_exact function to work properly, but I don't think that I'm competent enough: it will either take a long time, or the quality of the code will be low.

@bitwalker can you give an advice, how do you think this function could be reworked to fix this bug? Also it is possible that the functions don't work the way I think they do, so let me know if I missed something.

@Fumuran
Copy link
Contributor Author

Fumuran commented Dec 12, 2024

I fixed the previous, second bug by moving the not empty contents of the internal reader buffer to the high-level empty one. I don't think that this is a clean solution, but it fixes the bug.

Unfortunately, it wasn't the last bug. The stdlib is still can not be deserialized, as far as I can see now the error occurs in the read_slice() function during the usize deserialization. For now I didn't manage to pick the inputs which will emulate this situation, but as far as I can see this error related to the incorrect handling of the high-level buffer.

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.

2 participants