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

Apply sign extension when decoding int #732

Merged
merged 2 commits into from
Aug 30, 2024

Conversation

paolobarbolini
Copy link
Contributor

This PR fixes a regression introduced in #280.

During the removal of the byteorder crate all conversions between Buf/BufMut and primitive integer types were replaced with std implementations (from_be_bytes, from_le_bytes, to_be_bytes, to_le_bytes etc.). When dealing with variable length signed integers (get_int and get_int_le), the representation of the integer was simply padded with 0 bytes and then interpreted as i64. This caused the bug.

Let's take val = -42i64 with nbytes = 2, assume little-endian and see the order of operations of the current implementation:

Step Value
i64 input 0xffffffffffffffd6
i64 input -> [u8; 8] \xd6\xff\xff\xff\xff\xff\xff\xff
[u8; 8] -> truncated &[u8] \xd6\xff
truncated &[u8] -> decoded i64 (by appending 0) 0xffd6 (65494)
decoded i64 converted back to &[u8] just to show the mistake \xd6\xff\x00\x00\x00\x00\x00\x00 <- see how we have \x00 instead of \0xff

The decoded value is incorrect. To get the correct value, Sign extension must be applied to the bits representing the integer. The new operations will be:

Step Value
i64 input 0xffffffffffffffd6
i64 input -> [u8; 8] \xd6\xff\xff\xff\xff\xff\xff\xff
[u8; 8] -> truncated &[u8] \xd6\xff
truncated &[u8] -> u64 0xffd6
u64 << 48 0xffd6000000000000
(u64 << 48) as i64 >> 48 0xffffffffffffffd6 (-42) <-- it matches the input value

Inspired by the implementation in byteorder: https://github.com/BurntSushi/byteorder/blob/18f32ca3a41c9823138e782752bc439e99ef7ec8/src/lib.rs#L87-L91


I also found it interesting to see the difference in the resulting assembly: https://rust.godbolt.org/z/vfWdjdc3b

Fixes #730

@paolobarbolini paolobarbolini changed the title Sign extension Apply sign extension when decoding int Aug 19, 2024
@seanmonstar seanmonstar merged commit 79fb853 into tokio-rs:master Aug 30, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buf::get_int() implementation for Bytes returns positive number instead of negative when nbytes < 8.
2 participants