Skip to content

/rust

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

Use little-endian encoding for Blake2 hashing on all architectures #38960

Merged
merged 1 commit into from Jan 11, 2017

Conversation

Reviewers

@michaelwoerister michaelwoerister

Assignees

@michaelwoerister michaelwoerister

Labels
beta-accepted
Projects
None yet
Milestone
No milestone
5 participants
@arielb1 @bors @nikomatsakis @michaelwoerister @alexcrichton
@arielb1
Copy link
Contributor

arielb1 commented Jan 10, 2017 •
edited

Like many hash functions, the blake2 hash is mathematically defined on
a sequence of 64-bit words. As Rust's hash interface operates on
sequences of octets, some encoding must be used to bridge that
difference.

The Blake2 RFC (RFC 7693) specifies that:

   Byte (octet) streams are interpreted as words in little-endian order,
   with the least-significant byte first.

So use that encoding consistently.

Fixes #38891.

Beta-nominating since this is a regression since 1.15.

r? @michaelwoerister

@rust-highfive rust-highfive assigned michaelwoerister Jan 10, 2017

@michaelwoerister

michaelwoerister reviewed Jan 10, 2017
View changes

Copy link
Contributor

michaelwoerister left a comment

Thanks for the fix!
I think that one comment is should say little-endian, instead of big endian. And we probably should a comment that all data going in must be converted to little-endian (which the hasher should already do).
src/librustc_data_structures/blake2b.rs
if cfg!(target_endian = "big") {
for word in &mut m[..] {
*word = word.to_be();
*word = u64::from_le(*word);

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Jan 10, 2017

Contributor

Good catch!
src/librustc_data_structures/blake2b.rs
let m: &mut [u64; 16] = unsafe {
let b: &mut [u8; 128] = &mut ctx.b;
::std::mem::transmute(b)
};

// It's OK to modify the buffer in place since this is the last time
// this data will be accessed before it's overwritten

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Jan 10, 2017

Contributor

Why remove this comment?
src/librustc_data_structures/blake2b.rs Outdated
// Re-interpret the input buffer in the state as u64s
// Re-interpret the input buffer in the state as
// an array of big-endian u64s, converting them
// to machine endianness.

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Jan 10, 2017

Contributor

Isn't it the other way round: We always make sure that this buffer is little-endian and now we are making sure to convert it to machine endianess?

@arielb1 arielb1 force-pushed the arielb1:blake-endian branch from 2489791 to bccd756 Jan 10, 2017

@arielb1

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

arielb1 commented Jan 10, 2017

Fixed nits.

And we probably should a comment that all data going in must be converted to little-endian (which the hasher should already do).

Blake2b is a standard bytestream hasher. It should not do anything weird with its Hasher impl.

@bors r=michaelwoerister
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 10, 2017

📌 Commit bccd756 has been approved by michaelwoerister

@arielb1 arielb1 added the beta-nominated label Jan 10, 2017

@arielb1 arielb1 changed the title Use big-endian encoding for Blake2 hashing on all architectures Use little-endian encoding for Blake2 hashing on all architectures Jan 10, 2017

@arielb1
Use little-endian encoding for Blake2 hashing on all architectures 
Like many hash functions, the blake2 hash is mathematically defined on
a sequence of 64-bit words. As Rust's hash interface operates on
sequences of octets, some encoding must be used to bridge that
difference.

The Blake2 RFC (RFC 7693) specifies that:
   Byte (octet) streams are interpreted as words in little-endian order,
   with the least-significant byte first.

So use that encoding consistently.

Fixes #38891.
Loading status checks…
a89475d

@arielb1 arielb1 force-pushed the arielb1:blake-endian branch from bccd756 to a89475d Jan 10, 2017

@arielb1

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

arielb1 commented Jan 10, 2017

@bors r=michaelwoerister
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 10, 2017

📌 Commit a89475d has been approved by michaelwoerister
@arielb1

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

arielb1 commented Jan 10, 2017

@bors rollup
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jan 11, 2017

@bors rollup-
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jan 11, 2017

@bors p=1

  • Since this will likely go into beta, we need it to land ASAP

@nikomatsakis nikomatsakis added the beta-accepted label Jan 11, 2017

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jan 11, 2017

Accepting for beta. Small patch, regression. cc @rust-lang/compiler

@nikomatsakis nikomatsakis referenced this pull request Jan 11, 2017

Closed

ppc64/s390x `assertion failed: eps.windows(2).all(|w| w[0].cmp(self, &w[1]) != Ordering::Greater)` #38891

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 11, 2017

⌛️ Testing commit a89475d with merge e4fee52...

bors added a commit that referenced this pull request Jan 11, 2017

@bors
Auto merge of #38960 - arielb1:blake-endian, r=michaelwoerister 
Use little-endian encoding for Blake2 hashing on all architectures

Like many hash functions, the blake2 hash is mathematically defined on
a sequence of 64-bit words. As Rust's hash interface operates on
sequences of octets, some encoding must be used to bridge that
difference.

The Blake2 RFC (RFC 7693) specifies that:
```
   Byte (octet) streams are interpreted as words in little-endian order,
   with the least-significant byte first.
```

So use that encoding consistently.

Fixes #38891.

Beta-nominating since this is a regression since 1.15.

r? @michaelwoerister
Loading status checks…
e4fee52
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 11, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing e4fee52 to master...

@bors bors merged commit a89475d into rust-lang:master Jan 11, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@cuviper cuviper referenced this pull request Jan 12, 2017

Merged

Backport #38960 to beta #39008

alexcrichton added a commit that referenced this pull request Jan 12, 2017

@alexcrichton
Merge pull request #39008 from cuviper/beta-blake-endian 
Backport #38960 to beta
Loading status checks…
dc4f0e8

@alexcrichton alexcrichton removed the beta-nominated label Jan 12, 2017

@cuviper cuviper referenced this pull request Jan 31, 2017

Closed

Linking with panic machinery fails on Linux PPC64 #37996

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.