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

valtree's to_le() actually produces different result on big-endian from little-endian. #103183

Closed
oli-obk opened this issue Oct 18, 2022 · 3 comments
Labels
C-bug Category: This is a bug.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Oct 18, 2022

   For commit https://github.com/rust-lang/rust/pull/96591/commits/e14b34c386ad2809e937e0e6e0379c5cc5474954 , the two `to_le()` actually produces different result on big-endian from little-endian.
// Note: Don't use `StableHashResult` impl of `u64` here directly, since that
// would lead to endianness problems.
let hash: u128 = hasher.finish();
let hash_short = (hash.to_le() as u64).to_le();

Assume hash: u128=0x5e0f03940fda80bb6348c650c7b26618, then on LE, hash_short: u64 = 0x6348c650c7b26618, while BE hash_short: u64 = 0x5e0f03940fda80bb, which fails some unit tests on big endian targets because of hash mismatch.

If it's expected behavior to make hash value the same on BE/LE, code here should remove two to_le calls. We don't need to adjust endianness unless converting between multi-byte integer and byte sequences.

Originally posted by @ecnelises in #96591 (comment)

@Rageking8
Copy link
Contributor

@rustbot label +C-bug

@rustbot rustbot added the C-bug Category: This is a bug. label Oct 18, 2022
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 22, 2022
Remove byte swap of valtree hash on big endian

This addresses problem reported in rust-lang#103183. The code was originally introduced in rust-lang@e14b34c. (see rust-lang#96591)

On big-endian environment, this operation sequence actually put the other half from 128-bit result, thus we got different hash result on LE and BE.
@ecnelises
Copy link
Contributor

Can we close this now? (I forgot to mention Fixed xxxx in commit msg to trigger the bot) Not sure if I have the permission

@rustbot close

@rustbot
Copy link
Collaborator

rustbot commented Nov 23, 2022

Error: The feature close is not enabled in this repository.
To enable it add its section in the triagebot.toml in the root of the repository.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

Aaron1011 pushed a commit to Aaron1011/rust that referenced this issue Jan 6, 2023
Remove byte swap of valtree hash on big endian

This addresses problem reported in rust-lang#103183. The code was originally introduced in rust-lang@e14b34c. (see rust-lang#96591)

On big-endian environment, this operation sequence actually put the other half from 128-bit result, thus we got different hash result on LE and BE.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug.
Projects
None yet
Development

No branches or pull requests

5 participants