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

Refactor miri pointer checks #62081

Merged
merged 8 commits into from Jun 24, 2019

Conversation

Projects
None yet
4 participants
@RalfJung
Copy link
Member

commented Jun 23, 2019

Centralize bounds, alignment and NULL checking for memory accesses in one function: memory.check_ptr_access. That function also takes care of converting a Scalar to a Pointer, should that be needed. Not all accesses need that though: if the access has size 0, None is returned. Everyone accessing memory based on a Scalar should use this method to get the Pointer they need.

All operations on the Allocation work on Pointer inputs and expect all the checks to have happened (and will ICE if the bounds are violated). The operations on Memory work on Scalar inputs and do the checks themselves.

The only other public method to check pointers is memory.ptr_may_be_null, which is needed in a few places. No need for check_align or similar methods. That makes the public API surface much easier to use and harder to mis-use.

This should be largely no-functional-change, except that ZST accesses to a "true" pointer that is dangling or out-of-bounds are now considered UB. This is to be conservative wrt. whatever LLVM might be doing.

While I am at it, this also removes the assumption that the vtable part of a dyn Trait-fat-pointer is a Pointer (as opposed to a pointer cast to an integer, stored as raw bits).

r? @oli-obk

RalfJung added some commits Jun 23, 2019

Centralize bounds, alignment and NULL checking for memory accesses in…
… one function: memory.check_ptr_access

That function also takes care of converting a Scalar to a Pointer, should that be needed.  Not all accesses need that though: if the access has size 0, None is returned.
Everyone accessing memory based on a Scalar should use this method to get the Pointer they need.

All operations on the Allocation work on Pointer inputs and expect all the checks to have happened (and will ICE if the bounds are violated).
The operations on Memory work on Scalar inputs and do the checks themselves.

The only other public method to check pointers is memory.ptr_may_be_null, which is needed in a few places.
With this, we can make all the other methods (tests for a pointer being in-bounds and checking alignment) private helper methods, used to implement the two public methods.
That maks the public API surface much easier to use and harder to mis-use.

While I am at it, this also removes the assumption that the vtable part of a `dyn Trait`-fat-pointer is a `Pointer` (as opposed to a pointer cast to an integer, stored as raw bits).

RalfJung added some commits Jun 23, 2019

@RalfJung

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2019

The Miri side of this is at rust-lang/miri#787.

// The biggest power of two through which `offset` is divisible.
let offset_pow2 = 1 << offset.trailing_zeros();
err!(AlignmentCheckFailed {
has: Align::from_bytes(offset_pow2).unwrap(),

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jun 24, 2019

Author Member

The old code used offset % align.bytes() as has, but I think that's just wrong. In fact it's not even always a power of 2, so there could be ICEs there.

This comment has been minimized.

Show resolved Hide resolved src/librustc/mir/interpret/allocation.rs Outdated
Show resolved Hide resolved src/librustc_mir/interpret/memory.rs
@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

r=me with typo nit (multiple occurrences) fixed or not

@RalfJung

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

@bors r=oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

📌 Commit 7e83028 has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

⌛️ Testing commit 7e83028 with merge 7e08576...

bors added a commit that referenced this pull request Jun 24, 2019

Auto merge of #62081 - RalfJung:miri-pointer-checks, r=oli-obk
Refactor miri pointer checks

Centralize bounds, alignment and NULL checking for memory accesses in one function: `memory.check_ptr_access`. That function also takes care of converting a `Scalar` to a `Pointer`, should that be needed.  Not all accesses need that though: if the access has size 0, `None` is returned. Everyone accessing memory based on a `Scalar` should use this method to get the `Pointer` they need.

All operations on the `Allocation` work on `Pointer` inputs and expect all the checks to have happened (and will ICE if the bounds are violated). The operations on `Memory` work on `Scalar` inputs and do the checks themselves.

The only other public method to check pointers is `memory.ptr_may_be_null`, which is needed in a few places. No need for `check_align` or similar methods. That makes the public API surface much easier to use and harder to mis-use.

This should be largely no-functional-change, except that ZST accesses to a "true" pointer that is dangling or out-of-bounds are now considered UB. This is to be conservative wrt. whatever LLVM might be doing.

While I am at it, this also removes the assumption that the vtable part of a `dyn Trait`-fat-pointer is a `Pointer` (as opposed to a pointer cast to an integer, stored as raw bits).

r? @oli-obk
@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 7e08576 to master...

@bors bors added the merged-by-bors label Jun 24, 2019

@bors bors merged commit 7e83028 into rust-lang:master Jun 24, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
pr Build #20190624.20 succeeded
Details
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 24, 2019

📣 Toolstate changed by #62081!

Tested on commit 7e08576.
Direct link to PR: #62081

💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jun 24, 2019

📣 Toolstate changed by rust-lang/rust#62081!
Tested on commit rust-lang/rust@7e08576.
Direct link to PR: <rust-lang/rust#62081>

💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).

bors added a commit to rust-lang/miri that referenced this pull request Jun 24, 2019

Auto merge of #787 - RalfJung:pointer-checks, r=RalfJung
adjust for refactored memory pointer checks

The Miri side of rust-lang/rust#62081.
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.