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

Wasm | varint - incorrect counts for expected bytes #17

Closed
ghost opened this issue Oct 6, 2022 · 3 comments · Fixed by #62
Closed

Wasm | varint - incorrect counts for expected bytes #17

ghost opened this issue Oct 6, 2022 · 3 comments · Fixed by #62
Assignees
Labels
bug Something isn't working

Comments

@ghost
Copy link

ghost commented Oct 6, 2022

nova/wasm/src/varint.rs

Lines 56 to 61 in f903792

if length >= 64 {
return Err(Error::TooManyBytes {
expected: 5,
found: length / 8,
});
}

U64 may definitely read over 5 bytes.

Furthermore, the expected/found situation (between both u32 and u64) is possibly incorrect (and somewhat weird), as for u32 one may have 5 bytes, with 0xFF for the first four, and 0x7F for the last - this is 4 continuation bits, and 5*7 = 35 bits in the integer value. 35 bits (2³⁵ - 1) will overflow the u32.

@ghost ghost added the bug Something isn't working label Oct 6, 2022
@ghost ghost assigned MierenManz Oct 6, 2022
@ghost ghost changed the title Wasm, varint - incorrect counts for expected bytes Wasm | varint - incorrect counts for expected bytes Oct 6, 2022
@MierenManz
Copy link
Collaborator

It doesn't make sense to read over the count. Only that we do. Will change the error

@ghost
Copy link
Author

ghost commented Oct 30, 2023

We should pull my code from the Discord for this. But anyways. isn't the WebAssembly engine abandoned now :)

@ghost
Copy link
Author

ghost commented Oct 31, 2023

I didn't write up my corresponding implementation for u64. I only have u32. I'll write up the u64 impl soon then use both here. I want to get this done within a few hours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant