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 AsByteSliceMut using raw pointers with bad provenance #780

Merged
merged 2 commits into from
Apr 20, 2019

Conversation

RalfJung
Copy link
Contributor

@RalfJung RalfJung commented Apr 19, 2019

&mut slice[0] as *mut _ is a raw pointer that can only be used for the first element.

slice.as_mut_ptr() is not only shorter, but also correctly returns a pointer that can be used for the entire slice.

(Found by running the test suite in Miri.)

@dhardy
Copy link
Member

dhardy commented Apr 19, 2019

LGTM but I think you have a few more tweaks to add before merging.

@RalfJung
Copy link
Contributor Author

Do you want me to add RalfJung@8303309?

@RalfJung
Copy link
Contributor Author

Indeed that commit is the only other change I need to make all test suites pass (except for fill_bytes which I don't know how to fix). So I added it here.

I also have some patches to make running Miri on the test suite feasible; I will submit those separately.

@dhardy
Copy link
Member

dhardy commented Apr 20, 2019

These look good to me thanks! Yes, fill_bytes and test runners can have separate PRs.

@RalfJung
Copy link
Contributor Author

Okay, so this is ready to get merged then?

@dhardy dhardy merged commit 476cb6d into rust-random:master Apr 20, 2019
@@ -183,7 +183,8 @@ where <R as BlockRngCore>::Results: AsRef<[u32]> + AsMut<[u32]>
let read_u64 = |results: &[u32], index| {
if cfg!(any(target_arch = "x86", target_arch = "x86_64")) {
// requires little-endian CPU supporting unaligned reads:
unsafe { *(&results[index] as *const u32 as *const u64) }
let ptr: *const u64 = results[index..index+1].as_ptr() as *const u64;
Copy link
Contributor

Choose a reason for hiding this comment

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

This range still only covers 1 element, not 2 elements, as it uses .. instead of ..=.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I fixed that in #784

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't notice that commit :)

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.

3 participants