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

Miri: Check that a ptr is aligned and inbounds already when evaluating * #63075

Merged
merged 7 commits into from
Aug 15, 2019

Conversation

RalfJung
Copy link
Member

This syncs Miri with what the Nomicon and the Reference say, and resolves rust-lang/miri#447.

Also this would not have worked without #62982 due to new cycles. ;)

r? @oli-obk

@RalfJung RalfJung changed the title Check that a ptr is aligned and inbounds already when evaluating * Miri: Check that a ptr is aligned and inbounds already when evaluating * Jul 28, 2019
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2019
@RalfJung
Copy link
Member Author

See rust-lang/miri#863 for what this does with the Miri test suite.

@rust-highfive

This comment has been minimized.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 31, 2019

@mjbshaw I just realized that this PR as-is would break your CTFE version of offset_of!. I wrote this PR purely to make Miri better in catching UB (concretely, as a reaction to this discussion, I wanted to align what Miri checks with what the Nomicon has said all along), but it turns out that this breaks your macro.

I am not sure what to do about this. On the one hand this makes this PR a breaking change, on the other hand the unsafe CTFE code that it breaks is UB and has been called out as such in the reference since forever, and on yet another hand I don't know of anything else you can currently do in CTFE to do offset_of!.

@mjbshaw
Copy link
Contributor

mjbshaw commented Jul 31, 2019

Thanks for the heads up, @RalfJung.

On the one hand this makes this PR a breaking change

I don't really see it as a breaking change (at least not the kind that break's Rust's promises). I'm relying on UB, and I know that. UB can change (including break) at any time, which I'm okay with.

I've got mixed feelings here. On the one hand, I always knew this day would come and I can't blame you/anyone for making improvements like this. On the other hand, it sucks not being able to implement offset_of!. It's been a frustrating journey. I'll try to set aside some time though to work on a magic-macro-based implementation of offset_of!.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 31, 2019

Could you use a Union to create an undef value of your type, take a reference to the field you want, make it a pointer, subtract that from the base pointer? Undef values are pretty well defined in miri

@RalfJung
Copy link
Member Author

use a Union to create an undef value of your type

That only works for Copy types, or you have to use MaybeUninit.

@mjbshaw
Copy link
Contributor

mjbshaw commented Jul 31, 2019

@oli-obk No. It's not possible to use a real object at all because Miri won't let you. Doing this requires:

  • #![feature(const_raw_ptr_deref)] because of the expression &(*base).field.
  • #![feature(const_raw_ptr_to_usize_cast)] to case the pointers to usize (so you can subtract them).
  • Transmuting the *const MaybeUninit<T> to *const T since MaybeUninit::as_ptr() is non-const.

But even with all of that, Miri won't let you use the offset value.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 31, 2019

Why doesn't the error say what the actual problem is...?

error: any use of this value will cause an error
     | 
    ::: <source>:13:1
     |
13   | / pub const OFFSET: usize = {
14   | |     let uninit = std::mem::MaybeUninit::<Struct>::uninit();
15   | |     let base_ptr: *const Struct = unsafe { TransmuteHack { from: &uninit }.to };
16   | |     let field_ptr = unsafe { &(*base_ptr).field as *const _ };
17   | |     let offset = unsafe { (field_ptr as usize).wrapping_sub(base_ptr as usize) };
18   | |     offset
19   | | };
     | |__-
     |
     = note: `#[deny(const_err)]` on by default

My guess is that wrapping_sub errors, the CTFE engine is not able to subtract pointers from each other. And once #62946 lands, actually the field_ptr as usize will already fail as it will try to get the raw bits of a pointer, which is not possible in CTFE.

Miri won't let you

Nit: Miri-the-tool has no issue with any of this. The problem here is the variant of the Miri engine used by CTFE.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 31, 2019

Let's make wrapping_offset_from const fn. there's no reason it isn't.

Also yea, what's up with that horrible diagnostic

@RalfJung
Copy link
Member Author

Let's make wrapping_offset_from const fn. there's no reason it isn't.

Uh, implementing this in general requires ptr-to-int casts... CTFE can only do this for pointers to the same object.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 31, 2019

Sure, but that's fine imo. Casting raw pointers to usize will error for non-integer values, so calling wrapping_offset_from will error for pointers into distinct allocations. Or we just support offset_from which is unsafe and states

Both the starting and other pointer must be either in bounds or one byte past the end of the same allocated object.

@RalfJung
Copy link
Member Author

But then I don't see how this helps...
Which implementation of offset_of! is helped by an integer-only wrapping_offset_from?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 31, 2019

Why integer-only?. wrapping_offset_from using ptr to int casts is an implementation detail. We can add an intrinsic for it.

@RalfJung
Copy link
Member Author

So you want to have a wrapping_offset_from that, despite being a safe function, fails for some inputs?

This is an "unconst" function. We should likely not allow calling it outside unsafe blocks in const context.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 31, 2019

Oops, sorry, I meant offset_from. We don't need the wrapping version

@RalfJung
Copy link
Member Author

That I guess we could do. It would have to become an intrinsic or so I suppose.

Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
Miri: Check that a ptr is aligned and inbounds already when evaluating `*`

This syncs Miri with what the Nomicon and the Reference say, and resolves rust-lang/miri#447.

Also this would not have worked without rust-lang#62982 due to new cycles. ;)

r? @oli-obk
Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
Miri: Check that a ptr is aligned and inbounds already when evaluating `*`

This syncs Miri with what the Nomicon and the Reference say, and resolves rust-lang/miri#447.

Also this would not have worked without rust-lang#62982 due to new cycles. ;)

r? @oli-obk
bors added a commit that referenced this pull request Aug 14, 2019
Rollup of 10 pull requests

Successful merges:

 - #62984 (Add lint for excess trailing semicolons)
 - #63075 (Miri: Check that a ptr is aligned and inbounds already when evaluating `*`)
 - #63490 (libsyntax: cleanup and refactor `pat.rs`)
 - #63495 ( Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`.)
 - #63509 (Point at the right enclosing scope when using `await` in non-async fn)
 - #63528 (syntax: Remove `DummyResult::expr_only`)
 - #63534 (Bump to 1.39)
 - #63537 (expand: Unimplement `MutVisitor` on `MacroExpander`)
 - #63542 (Add NodeId for Arm, Field and FieldPat)
 - #63560 (move test that shouldn't be in test/run-pass/)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
Miri: Check that a ptr is aligned and inbounds already when evaluating `*`

This syncs Miri with what the Nomicon and the Reference say, and resolves rust-lang/miri#447.

Also this would not have worked without rust-lang#62982 due to new cycles. ;)

r? @oli-obk
Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
Miri: Check that a ptr is aligned and inbounds already when evaluating `*`

This syncs Miri with what the Nomicon and the Reference say, and resolves rust-lang/miri#447.

Also this would not have worked without rust-lang#62982 due to new cycles. ;)

r? @oli-obk
bors added a commit that referenced this pull request Aug 15, 2019
Rollup of 11 pull requests

Successful merges:

 - #62984 (Add lint for excess trailing semicolons)
 - #63075 (Miri: Check that a ptr is aligned and inbounds already when evaluating `*`)
 - #63490 (libsyntax: cleanup and refactor `pat.rs`)
 - #63507 (When needing type annotations in local bindings, account for impl Trait and closures)
 - #63509 (Point at the right enclosing scope when using `await` in non-async fn)
 - #63528 (syntax: Remove `DummyResult::expr_only`)
 - #63537 (expand: Unimplement `MutVisitor` on `MacroExpander`)
 - #63542 (Add NodeId for Arm, Field and FieldPat)
 - #63543 (Merge Variant and Variant_)
 - #63560 (move test that shouldn't be in test/run-pass/)
 - #63570 (Adjust tracking issues for `MaybeUninit<T>` gates)

Failed merges:

r? @ghost
@bors bors merged commit 647c0e0 into rust-lang:master Aug 15, 2019
bors added a commit to rust-lang/miri that referenced this pull request Aug 15, 2019
adjust tests for eager pointer checks on deref

The Miri side of rust-lang/rust#63075.

Fixes #447.
@RalfJung RalfJung deleted the deref-checks branch August 15, 2019 09:31
Centril added a commit to Centril/rust that referenced this pull request Oct 18, 2019
…ng,nikic

Make <*const/mut T>::offset_from `const fn`

This reenables offset_of cc @mjbshaw 	after rust-lang#63075 broke it
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 22, 2019
…ng,nikic

Make <*const/mut T>::offset_from `const fn`

This reenables offset_of cc @mjbshaw 	after rust-lang#63075 broke it
Centril added a commit to Centril/rust that referenced this pull request Oct 25, 2019
…ng,nikic

Make <*const/mut T>::offset_from `const fn`

This reenables offset_of cc @mjbshaw 	after rust-lang#63075 broke it
Centril added a commit to Centril/rust that referenced this pull request Oct 25, 2019
…ng,nikic

Make <*const/mut T>::offset_from `const fn`

This reenables offset_of cc @mjbshaw 	after rust-lang#63075 broke it
Centril added a commit to Centril/rust that referenced this pull request Oct 25, 2019
…ng,nikic

Make <*const/mut T>::offset_from `const fn`

This reenables offset_of cc @mjbshaw 	after rust-lang#63075 broke it
bors added a commit that referenced this pull request Nov 2, 2019
Make <*const/mut T>::offset_from `const fn`

This reenables offset_of cc @mjbshaw 	after #63075 broke it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check that offset is not too big, check projection offset to be inbounds
6 participants