Prepare miri engine for enforcing validity invariant during execution #54762

RalfJung · 2018-10-02T19:56:28Z

In particular, make recursive checking of references optional, and add a const_mode parameter that says whether usize is allowed to contain a pointer. Also refactor validation a bit to be type-driven at the "leafs" (primitive types), and separately validate scalar layout to catch NonNull violations (which it did not properly validate before).

Fixes #53826
Also fixes #54751

r? @oli-obk

rust-highfive · 2018-10-02T19:56:38Z

⚠️ Warning ⚠️

These commits modify submodules.

RalfJung · 2018-10-02T20:00:58Z

We have an interesting test failure: This extern static is considered not sufficiently aligned

#[link_name = "check_static_recursion_foreign_helper"]
extern "C" {
    #[allow(dead_code)]
    static test_static: c_int;
}

static B: &'static c_int = unsafe { &test_static };

Seems like we set some rather arbitrary alignment for these? Ideally we'd pick an alignment based on the type, would that make sense? Where is the code deciding that?

RalfJung · 2018-10-02T20:17:16Z

For now, I made it skip pointers to foreign statics entirely, not even checking their alignment. I think we could do better, but this is not a regression.

src/librustc_mir/interpret/terminator.rs

oli-obk · 2018-10-03T08:22:52Z

src/librustc_mir/interpret/validity.rs


+    /// Make sure that `value` matches the


I see value as the subject in this sentence, so it shouldn't need a "the". It's like a name: "Make sure that Berlin matches..."

oli-obk · 2018-10-03T08:23:49Z

src/librustc_mir/interpret/validity.rs

+        trace!("validate scalar by layout: {:#?}, {:#?}, {:#?}", value, size, layout);
+        let (lo, hi) = layout.valid_range.clone().into_inner();
+        if lo == u128::min_value() && hi == u128::max_value() {
+            // Nothing to check


what about undef checks?

Do we or do we not want to allow undef when the range is the entire possible range? That would mean validate_scalar_layout would also have to take a const_mode.

Note that we have type-based checks for that later, where we decide on a type-by-type basis whether we want undef or not. This here is only for additional restrictions that may be imposed on top of what the primitive types say.

oli-obk · 2018-10-03T08:25:06Z

src/librustc_mir/interpret/validity.rs

+    ) -> EvalResult<'tcx> {
+        trace!("validate scalar by layout: {:#?}, {:#?}, {:#?}", value, size, layout);
+        let (lo, hi) = layout.valid_range.clone().into_inner();
+        if lo == u128::min_value() && hi == u128::max_value() {


technically we should be checking whether hi.overflowing_add(1) == lo, because there are 2^128 possible ways to encode the full range

That's not correct either, we actually pick the range depending on the size of the scalar. I am now using

let max_hi = u128::max_value() >> (128 - size.bits()); // as big as the size fits

src/librustc_mir/interpret/validity.rs

oli-obk · 2018-10-03T08:27:17Z

src/librustc_mir/interpret/validity.rs

+            Scalar::Ptr(_) => {
+                // Comparing a ptr with a range is not meaningfully possible.
+                // In principle, *if* the pointer is inbonds, we could exclude NULL, but
+                // that does not seem worth it.


I'm fairly sure I had a test for just this case (enum Foo { E = 0 } and then transmuting a pointer to the enum type)

I made this more strict with the latest changes, could you have a look?

I found your test in ub-enum.rs. Bot it kept failing as expected... maybe because this is actually not considered a primitive type, but an enum, and hence it loads the discriminant and that always fails when it is a pointer?

src/librustc_mir/interpret/validity.rs

src/test/ui/consts/const-eval/ub-nonnull.stderr

RalfJung · 2018-10-03T09:57:37Z

I finally found a way to unify handling of thin and fat pointers, so I could not resist adding that to this PR.

RalfJung · 2018-10-03T10:05:51Z

Seems like we set some rather arbitrary alignment for these? Ideally we'd pick an alignment based on the type, would that make sense? Where is the code deciding that?

Actually we do not set any alignment for these foreign statics, we get a "dangling pointer" error even when just trying to check alignment. For now, I think it's best to keep ignoring them.

src/librustc_mir/interpret/terminator.rs

RalfJung · 2018-10-03T10:39:03Z

src/test/ui/issues/issue-14227.rs

 }
-static CRASH: () = symbol;
+static CRASH: u32 = symbol;


I had to change this test because "reading" a () does not actually read anything...

RalfJung · 2018-10-04T06:42:42Z

Might be worth doing a perf run.

@bors try

bors · 2018-10-04T06:42:56Z

⌛ Trying commit 5dfc8f1 with merge 98f2e1b...

@oli-obk

Prepare miri engine for enforcing validity invariant during execution In particular, make recursive checking of references optional, and add a `const_mode` parameter that says whether `usize` is allowed to contain a pointer. Also refactor validation a bit to be type-driven at the "leafs" (primitive types), and separately validate scalar layout to catch `NonNull` violations (which it did not properly validate before). Fixes #53826 Also fixes #54751 r? @oli-obk

bors · 2018-10-04T08:57:54Z

☀️ Test successful - status-travis
State: approved= try=True

RalfJung · 2018-10-04T11:30:11Z

@rust-timer build 98f2e1b

rust-timer · 2018-10-04T11:30:12Z

Success: Queued 98f2e1b with parent c67ea54, comparison URL.

RalfJung · 2018-10-04T18:02:43Z

Unexpectedly things got a bit slower (because now it does that scalar check quite more often than it used to). It's 5% only for very short benchmarks though (clean incremental), and more around 1-2% for the stress tests.

oli-obk · 2018-10-08T07:08:44Z

src/librustc_mir/interpret/validity.rs

+            Scalar::Ptr(ptr) => {
+                if lo == 1 && hi == max_hi {
+                    // only NULL is not allowed.
+                    // We can call `check_align` to check non-NULL-ness, but have to also look


I don't see how a pointer with an actual (dead or live) allocation could ever be null.

If you offset a pointer enough, it can overflow to NULL.

The only way we can know it is not NULL is to make sure it is inbounds.

If you overflow it far enough so it is inbounds again, won't we have the same problem?

Why would that be a problem? The overflow itself is okay. We only allow overflow when using wrapping_offset.

oli-obk · 2018-10-08T07:09:19Z

src/librustc_mir/interpret/validity.rs

+                        self.memory.get_fn(ptr).is_ok();
+                    if !non_null {
+                        // could be NULL
+                        return validation_failure!("a potentially NULL pointer", path);


needs a test if this is reachable at all, otherwise, remove

I think this is currently unreachable because we have no way in CTFE to add an offset to a pointer. It will be reachable once that is a possibility.

…lidation msgs on error

This does not actually regress anything. It would regress NonNull, but we didn't handle that correctly previously either.

Fixes rust-lang#54751

…ling out of aggregate handling Also, make enum variant handling a bit nicer

also less verbose logging

RalfJung · 2018-10-09T13:58:28Z

The only actually surprising perf regression seems to be syn, and that one I unfortunately cannot reproduce locally... Also note syn has a ? indicating it has high variance.

"clean-opt" is supposed to regress 3%, it's 1% here (and 1% I have found impossible to debug, there's just too much noise). Looking at perf report, this spends almost all its time in LLVM. No idea how these changes here should affect anything.

I also tried reproducing "patched incremental: println-opt" for syn, but I am getting vastly different numbers for the instruction count (on the order of 47 billion instead of 27 billion), so I must be doing something else. On those measurements, this patch is a <0.1% slowdown. Here's the commands I used:

# prepare
export CARGO_INCREMENTAL=1
git reset --hard HEAD && rm target -rf && cargo +stage2.2 build --release
# bench
patch -p1 < 0-println.patch && perf stat -- cargo +stage2.2 build --release

RalfJung · 2018-10-09T14:33:55Z

I managed to get the perf collector running locally, and used it to re-run the syn benchmarks. I am again seeing numbers around 45 billion instead of the 27 billion on the website, and I am seeing a regression of around 0.1%. I'd call that noise.

I can't think of anything else I could do.

oli-obk · 2018-10-09T15:27:26Z

@bors r+

bors · 2018-10-09T15:27:27Z

📌 Commit fe96f82 has been approved by oli-obk

bors · 2018-10-09T17:20:04Z

⌛ Testing commit fe96f82 with merge 0e07c42...

@oli-obk

Prepare miri engine for enforcing validity invariant during execution In particular, make recursive checking of references optional, and add a `const_mode` parameter that says whether `usize` is allowed to contain a pointer. Also refactor validation a bit to be type-driven at the "leafs" (primitive types), and separately validate scalar layout to catch `NonNull` violations (which it did not properly validate before). Fixes #53826 Also fixes #54751 r? @oli-obk

bors · 2018-10-09T19:57:42Z

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 0e07c42 to master...

rust-highfive · 2018-10-09T19:59:36Z

📣 Toolstate changed by #54762!

Tested on commit 0e07c42.
Direct link to PR: #54762

🎉 miri on windows: build-fail → test-pass.
🎉 miri on linux: build-fail → test-pass.

Tested on commit rust-lang/rust@0e07c42. Direct link to PR: <rust-lang/rust#54762> 🎉 miri on windows: build-fail → test-pass. 🎉 miri on linux: build-fail → test-pass.

alexcrichton · 2018-10-17T18:51:43Z

It looks like this may have caused a minor regression across a number of targets on perf

RalfJung · 2018-10-17T19:30:26Z

These targets that regressed all have large constants.

That'll likely be caused by us now checking both layout and type invariants. There is some redundancy in the checking there, which I am not sure how to avoid (while keeping the code somewhat reasonably organized).

oli-obk · 2018-10-17T20:16:34Z

Maybe we could not run the layout checks on those value where we know the type checks to be sufficient? E.g. on char, bool and basic integers

RalfJung · 2018-10-18T06:32:15Z

You want to determine that by type? ;) I think you can add references and raw pointers to that list. (References have a layout restriction but we also check that and more in the type-based check.) In fact, for all the types which have a type-based check, that check should be sufficient.

Sure, worth a try I guess. I am not convinced it will help much but there is only one way to find out.

rust-highfive assigned oli-obk Oct 2, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 2, 2018

RalfJung force-pushed the miri-validate branch from 18c4e21 to 3e22673 Compare October 2, 2018 20:16

This comment has been minimized.

Sign in to view

oli-obk requested changes Oct 3, 2018

View reviewed changes

RalfJung commented Oct 3, 2018

View reviewed changes

src/librustc_mir/interpret/terminator.rs Show resolved Hide resolved

RalfJung commented Oct 3, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung mentioned this pull request Oct 5, 2018

rustup; test for return type mismatch rust-lang/miri#467

Merged

oli-obk reviewed Oct 8, 2018

View reviewed changes

RalfJung force-pushed the miri-validate branch from 4935c3d to 9491f2e Compare October 8, 2018 12:45

RalfJung added 6 commits October 9, 2018 13:08

miri validity: make recursive ref checking optional

bf5e6eb

check that entire ref is in-bounds before recursing; add macro for va…

ff5a245

…lidation msgs on error

switch validation of scalars to be type-driven

f65d3b5

This does not actually regress anything. It would regress NonNull, but we didn't handle that correctly previously either.

fix validating arrays of ZSTs

0a2fae6

Fixes rust-lang#54751

also validate everything that has a Scalar layout, to catch NonNull

69a320f

move a test to a better place

13bdc16

RalfJung added 7 commits October 9, 2018 13:08

unify handling of thin and fat pointers by moving primitive type hand…

322017b

…ling out of aggregate handling Also, make enum variant handling a bit nicer

fix nits and handling of extern static

e09e3c8

add fixme for potential perf optimization

fcf6b5c

update miri

db1663d

box is also a primitive type

6899af8

dont fail when validating non-local closures

976880a

validity: check dynamic size, not static

fe96f82

also less verbose logging

RalfJung force-pushed the miri-validate branch from 9491f2e to fe96f82 Compare October 9, 2018 11:47

oli-obk approved these changes Oct 9, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 9, 2018

bors merged commit fe96f82 into rust-lang:master Oct 9, 2018

This was referenced Oct 9, 2018

Report const eval error inside the query #53821

Merged

miri engine: basic support for pointer provenance tracking #54461

Merged

RalfJung mentioned this pull request Oct 10, 2018

Tracking issue for a minimal subset of RFC 911, const fn #53555

Closed

4 tasks

RalfJung deleted the miri-validate branch October 17, 2018 19:29

eddyb mentioned this pull request Jul 30, 2022

Regression in consteval: error[E0080]: could not evaluate static initializer (unable to turn pointer into raw bytes) #99923

Closed

Prepare miri engine for enforcing validity invariant during execution #54762

Prepare miri engine for enforcing validity invariant during execution #54762

Conversation

RalfJung commented Oct 2, 2018

rust-highfive commented Oct 2, 2018

RalfJung commented Oct 2, 2018 • edited Loading

RalfJung commented Oct 2, 2018

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Oct 3, 2018

RalfJung commented Oct 3, 2018

Choose a reason for hiding this comment

RalfJung commented Oct 4, 2018

bors commented Oct 4, 2018

bors commented Oct 4, 2018

This comment has been minimized.

RalfJung commented Oct 4, 2018

rust-timer commented Oct 4, 2018

RalfJung commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Oct 9, 2018 • edited Loading

RalfJung commented Oct 9, 2018

oli-obk commented Oct 9, 2018

bors commented Oct 9, 2018

bors commented Oct 9, 2018

bors commented Oct 9, 2018

rust-highfive commented Oct 9, 2018

alexcrichton commented Oct 17, 2018

RalfJung commented Oct 17, 2018 • edited Loading

oli-obk commented Oct 17, 2018

RalfJung commented Oct 18, 2018

RalfJung commented Oct 2, 2018 •

edited

Loading

RalfJung commented Oct 9, 2018 •

edited

Loading

RalfJung commented Oct 17, 2018 •

edited

Loading