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

Fix next_u64_via_fill to use little endian order #1026

Closed
dhardy opened this issue Aug 28, 2020 · 6 comments · Fixed by #1061
Closed

Fix next_u64_via_fill to use little endian order #1026

dhardy opened this issue Aug 28, 2020 · 6 comments · Fixed by #1061

Comments

@dhardy
Copy link
Member

dhardy commented Aug 28, 2020

See: #1011 (comment)

@vks
Copy link
Collaborator

vks commented Aug 28, 2020

A first step could be adding tests for next_u{32,64}_via_fill.

@dhardy
Copy link
Member Author

dhardy commented Aug 28, 2020

These methods are only used twice: in OsRng (where it doesn't matter) and in ReadRng. The latter has this test (and a similar one for u64):

        let v = [0u8, 0, 0, 1, 0, 0, 0, 2, 0, 0, 0, 3];
        let mut rng = ReadRng::new(&v[..]);

        assert_eq!(rng.next_u32(), 1_u32.to_be());
        assert_eq!(rng.next_u32(), 2_u32.to_be());
        assert_eq!(rng.next_u32(), 3_u32.to_be());

Thus, currently on an LE platform the number if read in with only the most significant byte set, and the comparator gets a byte-swap. On BE, the number is read in with the least-significant byte set and the comparator does not get a byte-swap. Thus the tests pass but are wrong.

@dhardy
Copy link
Member Author

dhardy commented Aug 28, 2020

Fixing this properly requires bumping the rand_core version. Are we planning on doing that before rand 0.8?

Alternatively we could just call it a bug-fix and note that basically no-one is affected (though we don't really know).

@josephlr
Copy link
Member

Fixing this properly requires bumping the rand_core version. Are we planning on doing that before rand 0.8?

Alternatively we could just call it a bug-fix and note that basically no-one is affected (though we don't really know).

I think if we're doing rand_core v0.6 anyway (because of getrandom), then we should also just document this as using little endian and use to_le_bytes.

@dhardy
Copy link
Member Author

dhardy commented Sep 15, 2020

Agreed, we should just fix this for v0.6.

@dhardy dhardy changed the title Investigate why next_u64_via_fill uses native endian order Fix next_u64_via_fill to use little endian order Sep 15, 2020
@dhardy
Copy link
Member Author

dhardy commented Oct 19, 2020

Okay, I'll try to get this done today.

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 a pull request may close this issue.

3 participants