Skip to content

fix: try_get_int/try_get_int_le do not sign-extend n-byte integers#839

Open
teddytennant wants to merge 1 commit into
tokio-rs:masterfrom
teddytennant:fix-try-get-int-sign-extend
Open

fix: try_get_int/try_get_int_le do not sign-extend n-byte integers#839
teddytennant wants to merge 1 commit into
tokio-rs:masterfrom
teddytennant:fix-try-get-int-sign-extend

Conversation

@teddytennant

Copy link
Copy Markdown

What

Buf::try_get_int and Buf::try_get_int_le now sign-extend the value they read for nbytes < 8, matching the behavior of the panicking Buf::get_int / Buf::get_int_le.

Why

The panicking getters sign-extend via sign_extend(self.get_uint(nbytes), nbytes). Their fallible twins instead used buf_try_get_impl!, which copies the bytes into the low bytes of a zeroed [0u8; 8] and calls i64::from_{be,le}_bytes. For nbytes < 8 the high bytes stay zero, so no sign extension happens and a negative value is returned as a wrong positive number.

For example, the 3-byte big-endian value 0xffffff (which get_int(3) returns as -1) was returned by try_get_int(3) as 16777215.

try_get_int_ne delegates to these two methods and inherited the bug; it is fixed transitively. No Buf implementation overrides these methods, so all implementors were affected.

The fix makes the fallible getters mirror their panicking counterparts:

fn try_get_int(&mut self, nbytes: usize) -> Result<i64, TryGetError> {
    Ok(sign_extend(self.try_get_uint(nbytes)?, nbytes))
}
fn try_get_int_le(&mut self, nbytes: usize) -> Result<i64, TryGetError> {
    Ok(sign_extend(self.try_get_uint_le(nbytes)?, nbytes))
}

Edge cases are preserved: nbytes > 8 still panics via try_get_uint's documented check, nbytes == 8 is an identity shift, and insufficient data still returns the same Err(TryGetError { requested, available }) as before (existing doctests unchanged).

Testing

  • Added try_get_int_sign_extends to tests/test_buf.rs, covering big/little/native-endian sign extension, a positive value, full width, and the insufficient-data error shape. It fails on master (try_get_int(3) returns Ok(16777215)) and passes with this change.
  • cargo test passes all suites and doctests, including the existing try_get_int/try_get_int_le doctests.
  • cargo fmt --all --check is clean.

`Buf::try_get_int` and `Buf::try_get_int_le` read the requested bytes
into the low bytes of a zeroed `[u8; 8]`, so for `nbytes < 8` the high
bytes stayed zero and no sign extension happened. A negative value such
as the 3-byte `0xffffff` was returned as `16777215` instead of `-1`,
unlike the panicking `get_int`/`get_int_le` which sign-extend via
`sign_extend`.

Make the fallible getters mirror their panicking counterparts by
sign-extending the result of `try_get_uint`/`try_get_uint_le`.
`try_get_int_ne` delegates to these and is fixed transitively. Error and
panic behavior is unchanged.
Copilot AI review requested due to automatic review settings July 8, 2026 21:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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.

2 participants