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

Allowing passing a null pointer to getrandom() when length is 0 #884

Merged
merged 3 commits into from
Aug 4, 2019

Conversation

Aaron1011
Copy link
Member

The Linux kernel will handle a null pointer passed to 'getrandom'
without error, as long as the length is also 0. The getrandom crate
relies on this behavior: https://github.com/rust-random/getrandom/blob/ab44edf3c7af721a00e22648b6c811ccb559ba81/src/linux_android.rs#L26

Since it works fine on the actual kernel (and should continue to, due to
the kernel's backwards-compatibility guarantees), Miri should support it
as well.

@RalfJung
Copy link
Member

RalfJung commented Aug 4, 2019

We already handle NULL pointers just fine when the length is 0. The not_undef just checks that the pointer is initialized, and we should definitely still do that.

I think you ran into the same getrandom bug that I also just ran into. See rust-random/getrandom#73.

(Also, did you test that your patch fixes this? Because it doesn't look like it should. I got ICEs in to_usize.)

@RalfJung
Copy link
Member

RalfJung commented Aug 4, 2019

FWIW, even with this PR, getrandom still fails here. (You can do cargo update && cargo miri test in test-cargo-miri/ to reproduce.)

@Aaron1011
Copy link
Member Author

@RalfJung: Wow - I opened a duplicate PR within a few minutes of yours: rust-random/getrandom#74

However, applying either of our PRs is not sufficient to make that call to getrandom work. I think the blame actually lies with gen_random:

let ptr = match this.memory().check_ptr_access(ptr, Size::from_bytes(len as u64), Align::from_bytes(1).unwrap())? {

We unconditionally try to use that pointer. I think we should add a flag to skip doing anything when the length is 0. We can enable that behavior for getrandom, and keep the current behavior for all other usages of gen_random.

@RalfJung
Copy link
Member

RalfJung commented Aug 4, 2019

I opened a duplicate PR within a few minutes of yours: rust-random/getrandom#74

Talk about race conditions. ;)

However, applying either of our PRs is not sufficient to make that call to getrandom work. I think the blame actually lies with gen_random:

Ah, good catch. check_ptr_access complains on NULL. So dangling pointers (and unaligned pointers) work fine here with size 0, but NULL pointers do not.

I find it likely that all users of gen_random are fine with NULL pointers, so I think gen_random itself should be changed to bail early if the size is 0.

@bors
Copy link
Contributor

bors commented Aug 4, 2019

☔ The latest upstream changes (presumably #883) made this pull request unmergeable. Please resolve the merge conflicts.

This preserves compatibility with programs that pass a null pointer and
a length of zero to getrandom(), or their platform's equivalent.
@Aaron1011
Copy link
Member Author

@RalfJung: I've made the change you requested

src/helpers.rs Outdated Show resolved Hide resolved
src/helpers.rs Outdated Show resolved Hide resolved
Co-Authored-By: Ralf Jung <[email protected]>
src/helpers.rs Outdated Show resolved Hide resolved
@Aaron1011
Copy link
Member Author

@RalfJung Updated

@RalfJung
Copy link
Member

RalfJung commented Aug 4, 2019

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Aug 4, 2019

📌 Commit 4d3398f has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Aug 4, 2019

⌛ Testing commit 4d3398f with merge bc82f83...

bors added a commit that referenced this pull request Aug 4, 2019
Allowing passing a null pointer to getrandom() when length is 0

The Linux kernel will handle a null pointer passed to 'getrandom'
without error, as long as the length is also 0. The `getrandom` crate
relies on this behavior: https://github.com/rust-random/getrandom/blob/ab44edf3c7af721a00e22648b6c811ccb559ba81/src/linux_android.rs#L26

Since it works fine on the actual kernel (and should continue to, due to
the kernel's backwards-compatibility guarantees), Miri should support it
as well.
@bors
Copy link
Contributor

bors commented Aug 4, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing bc82f83 to master...

@bors bors merged commit 4d3398f into rust-lang:master Aug 4, 2019
bors added a commit that referenced this pull request Aug 6, 2019
test-cargo-miri: cargo update

With both rust-random/getrandom#74 and #884 having landed, this should work now.
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