Do a basic sanity check for all constant values #51361

oli-obk · 2018-06-05T07:21:15Z

Motivation and high level overview

There has been some back and forth in this PR between @RalfJung and me in here about the motivation for this change and the stance it takes on unsafe coding guidelines.

The initial implementation ran its checks on every value read (so *x, y = x, ...). In unsafe code that isn't reasonable, because we might be invalidating invariants for a short time in order to build up a proper value.

The current implementation is a lint that runs its checks statics and constants. There is no need to check array lengths and enum variants, because it's a hard error to end up with anything but a number, and that one even has to have the required bits to be defined.

What checks are done?

Some type related checks
- char needs to be a correct unicode character as defined by char::from_u32
- A reference to a ZST must have the correct alignment (and be nonzero)
- A reference to anything is dereferenced and its value is checked
Layout checks use the information from ty::Layout to check
- all fields of structs
- all elements of arrays
- enum discriminants
- the fields of an enum variant (the variant is decided by the discriminant)
- whether any union field succeeds in being checked (if none match the memory pattern, the check fails)
- if the value is in the range described by the layout (e.g. for NonZero* types)

Changing the layout of a type will thus automatically cause the checks to check for the new layout.

fixes #51330
fixes #51471

cc @RalfJung

r? @eddyb

RalfJung · 2018-06-05T11:34:51Z

src/librustc_mir/interpret/eval_context.rs

+                    if self.validate_ptr_target(ptr, ptr_align, field_layout).is_ok() {
+                        return Ok(());
+                    }
+                }


I don't think we make any validity assumptions for what is inside a union. But this is an open topic for discussion, and RFC-worthy. Saying "at least one field has to be valid" is probably conservative, but there should at least be a comment saying that no final decision has been made yet.

I can turn this check into a lint that notes that we don't know if this is correct, pending an RFC

I think having an error is okay, as it's forwarding-compatible to allow this later (right?). But either the error or the code should say that this is still subject of discussion.

RalfJung · 2018-06-05T11:36:29Z

I don't understand the motivation. What is this solving? Where in http://play.rust-lang.org/?gist=99912c4c87d4d676817c8a7263289c9b&version=undefined&mode=undefined would you expect which kind of error to be raised?

RalfJung · 2018-06-05T11:38:31Z

src/librustc_mir/interpret/eval_context.rs

+                },
+                Scalar::Ptr(_) => {
+                    // we cannot reason about Ptr numeric ranges other than zero
+                    if bound.start_bound() == Bound::Included(&0) && bound.end_bound() == Bound::Included(&0) {


So this says that the range has to be just (0,0) for the pointer to invalidate the range? When would such ranges even occur? I have no idea what scalar.valid_range is for e.g. pointer types.

yes, I just did the most conservative thing i could think of. Usually you have 0.. or 1.. as the valid range.

But this means usually, this will never fire?

Also, raw pointers can be null. Does this check to make sure we're talking about a reference?

And finally, what about alignment?

But this means usually, this will never fire?

Yea, unless you define a clike enum with 2^32-1 variants without it having one where the discriminant is 0

Also, raw pointers can be null. Does this check to make sure we're talking about a reference?

Oh that's not the issue, if you have a 0 you do not get into the Scalar::Ptr arm, this is an abstract pointer. You'd be in the Scalar::Bits arm.

Also we're not checking types here, we're checking layout. And the layout of a raw pointer allows having a 0.

And finally, what about alignment?

That would require us to look at the type. I can add more checks, I just wanted to start with the very basic layout checks.

Oh, that's the Scalar miri value thing, not layout.

Yea, unless you define a clike enum with 2^32-1 variants without it having one where the discriminant is 0

Now I am confused. The only way this fires, from what I can see, is having an enum where 0 is the only variant, and then having a pointer value at that enum type.

That would require us to look at the type.

I see, makes sense.

valid_range is indeed 1..=0 for some full-range types.

You get 0..=0 for enum Foo { Bar = 0 }

Either way, it'd be simpler if you always created two complete RangeInclusive's rather than things like ..=hi and lo...

indeed, no generics means I can use a closure

0..=0 meaning exactly one value, 0, makes sense, but at the same time, it means your check for pointers doesn't make sense, since they'd never have the 0..=0 range, would they?

The pointer check is checking the opposite. It checks if you have a type enum Foo { Bar = 0 } but the value is a pointer. So if you transmuted a &1 to a Foo, then this check triggers. Transmuting a &1 to an enum Foo { Bar = 42 }; is uncheckable at compile-time, because the pointer might actually point to address 42

That's confusing. Why not check that the range includes all the possible values of the pointer? Sadly, this doesn't work very well with splitting into two ranges, I don't think, so the splitting should probably only be done for Scalar::Bits.

A Scalar::Pointer could have any address, so assuming it's any specific one, is generally a bad idea.

true. I fixed this

RalfJung · 2018-06-05T11:38:58Z

src/librustc_mir/interpret/eval_context.rs

+            layout::FieldPlacement::Union(0) => {
+                match layout.abi {
+                    // nothing to do, whatever the pointer points to, it is never going to be read
+                    layout::Abi::Uninhabited => Ok(()),


Wait, shouldn't it be insta-UB to see a reference to something uninhabited?

might be a *const !? idk, I'll add some tests.

Behind a *const T, nothing has to be valid, the pointer can be null or unaligned or dangling -- so none of this validation should ever run for these. Or did I misunderstand something?

fbstj · 2018-06-05T11:40:40Z

src/librustc_mir/interpret/eval_context.rs

+            trace!("variant layout: {:#?}", layout);
+        }
+        match layout.fields {
+            // primitives are unions with zero fields, because that's totally obvious and documented


might be worth pointing at documentation/reference? unless it's worth refactoring this weirdness out?

Maybe it should check layout.abi before looking at layout.fields?

layout.fields is for getting field offsets, and only Array has any guarantees attached to it. If there are 0 fields, the choice doesn't matter and I didn't want to put another Option around it.

I documented it ;)

To me this sounds like indeed, it should first check layout.abi?

RalfJung · 2018-06-05T11:41:35Z

src/test/ui/const-eval/union-const-eval-field.stderr

+  --> $DIR/union-const-eval-field.rs:38:5
+   |
+LL |     const FIELD3: Field3 = unsafe { UNION.field3 }; //~ ERROR E0080
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ attempted to read undefined bytes


Oh wow, this is taking a strong stance in one particular camp of unsafe code guidelines! Do we really want this to error so early?

If you do 0u8 - 1 inside a constant you also get a lint about an unusable constant. You can still publish your crate with these broken constants, but actually using them anywhere will produce erorrs.

I have no idea what that statement has to do with my question... this is a hard error, is it not? It says ERROR in all-caps. How's this related to overflow lints?

uh... indeed. I need to check that out... goot catch.

I had a look at the code again. Using a broken constant to obtain a runtime value will error, not lint + produce an undef. So maybe we cannot do this PR because it would be a breaking change.

RalfJung · 2018-06-05T11:43:24Z

Also, given that the goal seems to be to maintain some kind of invariant a la "values of type X are always satisfying property Y", I'd like to see some kind of high-level documentation what these invariants are, and where they are maintained.

fbstj · 2018-06-05T11:44:23Z

src/librustc_mir/interpret/eval_context.rs

+            layout::FieldPlacement::Union(0) => {
+                match layout.abi {
+                    // nothing to do, whatever the pointer points to, it is never going to be read
+                    layout::Abi::Uninhabited => Ok(()),


this check is only done in this case, is that because it's the (only?) base case of recursive validate_ptr_target?

if so is it only the inner-most types that have this property on the other cases? as in, is it valid to recurse down to here on nested types or would they themselves be Uninhabited in a way that would mean they aren't recursable?

I'm not sure. I'll investigate. But I also do remember that there were some issues with aggregates containing uninhabited types not being uninhabited, because you could create a local variable of such an aggregate and then initialize the inhabited parts

oli-obk · 2018-06-05T12:06:16Z

Also, given that the goal seems to be to maintain some kind of invariant a la "values of type X are always satisfying property Y", I'd like to see some kind of high-level documentation what these invariants are, and where they are maintained.

I assumed that if you break any rules of the layouting stuff, then you are in guaranteed UB-land. They seemed like the absolute lowest bar for sanity checks.

I don't understand the motivation. What is this solving?

I assumed it was bad to have constants that allow you to safely create undef llvm values. The test from the test-suite went past various reviewers and none noticed the UB.

Miri is already differing from runtime behaviour if undefined bytes are involved, because if one byte of a u64 is undef, and you read that u64, you get a u64 with all bytes undefined. So even if you turn it back into a smaller type, you'd not get your original bytes back. This seems like a dangerous thing.

Where in http://play.rust-lang.org/?gist=99912c4c87d4d676817c8a7263289c9b&version=undefined&mode=undefined would you expect which kind of error to be raised?

The "error" is a lint, and it is raised at the definition of FIELD3.

It is only an error if you are initialzing a static, an enum variant discriminant or an array length. The latter apparently is an ICE right now: http://play.rust-lang.org/?gist=d14c833404f369a4565b725207fe4769&version=nightly&mode=debug (although that ICE could be fixed in a different way than what this PR is proposing).

All this PR does is move errors from use-sites to declaration-sites or emit lints at declaration-sites and keep producing undef at use-sites.

RalfJung · 2018-06-05T12:30:20Z

I assumed it was bad to have constants that allow you to safely create undef llvm values. The test from the test-suite went past various reviewers and none noticed the UB.

What's safe about this? The code has unsafe all over it!

Miri is already differing from runtime behaviour if undefined bytes are involved, because if one byte of a u64 is undef, and you read that u64, you get a u64 with all bytes undefined. So even if you turn it back into a smaller type, you'd not get your original bytes back. This seems like a dangerous thing.

LLVMs semantics in this case are unclear. Also, how is this dangerous? This will just lead to more error messages, right?

The "error" is a lint, and it is raised at the definition of FIELD3.

The test case says "ERROR 0080: attempted to read undefined bytes", are we talking about the same thing?

You didn't answer my question, though -- but I think I found part of the answer: The problem is that this reads 8 bytes, 4 of which are undef, into a u64. I don't even see why this code is evaluated at compile time (there is no constant or static or array length initialized with read_field3), but even when it does, I expected it to be an error when miri produces an Undef.

My question about what problem you are even fixing here remains open.

RalfJung · 2018-06-05T12:36:42Z

I assumed that if you break any rules of the layouting stuff, then you are in guaranteed UB-land. They seemed like the absolute lowest bar for sanity checks.

Agreed. But e.g. layouting stuff doesn't look behind pointer indirections (which validate_ptr_target sounds like it does?), nor does it make any restrictions on e.g. u64. So what you are checking here goes way beyond layout invariants.

OTOH, bool being either 0 or 1 is a layout invariant; where is that enforced?

Also, that only answer the part of which invariant you intend to maintain, not when. I assume it's "all the time", i.e. whenever such a value is created in any possible way?

oli-obk · 2018-06-05T12:44:28Z

What's safe about this? The code has unsafe all over it!

if that were in a crate, the user of the constant doesn't know that. Now I know that with functions you have the same problem, but this is const eval, the value of a constant is part of the API.

LLVMs semantics in this case are unclear. Also, how is this dangerous? This will just lead to more error messages, right?

I'm just pointing out that there's weird stuff going on, and I don't think const eval should randomly change what it's doing even in the presence of UB. Instead we should be aborting, like we already do for other kinds of UB.

The test case says "ERROR", are we talking about the same thing?

The lint is deny-by-default.

I don't even see why this code is evaluated at compile time (there is no constant or static or array length initialized with read_field3), but even when it does, I expected it to be an error when miri produces an Undef.

Inside the function there's a constant. That constant's value is then returned.

My question about what problem you are even fixing here remains open.

I'm just trying to catch more bad stuff earlier.

Agreed. But e.g. layouting stuff doesn't look behind pointer indirections (which validate_ptr_target sounds like it does?), nor does it make any restrictions on e.g. u64. So what you are checking here goes way beyond layout invariants.

Well... to check whether a value is a valid u64 you need to access the value, and that access is failing. It's not the actual checking code ;)

We do not traverse through pointers, no, we'd need to act upon type information for that.

OTOH, bool being either 0 or 1 is a layout invariant; where is that enforced?

The layout is a scalar with 0..=1 as the valid range, there's no special code here, it just falls out of the rest of the checks.

Also, that only answer the part of which invariant you intend to maintain, not when. I assume it's "all the time", i.e. whenever such a value is created in any possible way?

I'm fine with just checking the final initializer value of a static/constant. You're right, the current check is a little overeager and also checks the locals in

const FOO: usize = {
    let x = 42;
    let y = x + 1;
    y
};

RalfJung · 2018-06-05T12:57:40Z

if that were in a crate, the user of the constant doesn't know that. Now I know that with functions you have the same problem, but this is const eval, the value of a constant is part of the API.

You still seem to assume that I know way more than I do. :) Which part do you move to another create to make this a problematic example? Everything but main? But how is that different from, you know, exactly the same code without any const, which also returns undef?

I'm just pointing out that there's weird stuff going on, and I don't think const eval should randomly change what it's doing even in the presence of UB. Instead we should be aborting, like we already do for other kinds of UB.

Well yes, and I am still asking why this doesn't happen already but you ~~refuse to answer~~skipped those questions. "catch more bad stuff earlier", that's not saying anything. Earlier than what, and when is earlier? I still don't understand. I'm really not trying to be obnoxious here but you still didn't spell out what problem this is solving.

Remember, I am not familiar at all with how the outside world (aka rustc) interacts with miri.

Inside the function there's a constant. That constant's value is then returned.

Okay so the fact that this is a const fn doesn't matter? It's about the const FIELD3: Field3 = ...;? In that case I'd remove the const fn as it's just distracting.

The layout is a scalar with 0..=1 as the valid range, there's no special code here, it just falls out of the rest of the checks.

Ah! That's nice. Could we have a test that transmutes 2 to bool and fails immediately?

I'm fine with just checking the final initializer value of a static/constant. You're right, the current check is a little overeager and also checks the locals

Now I feel we're getting there. To repeat my question from above, what is currently checked on the final initializer value? I stated above that I assume it must not be undef, you didn't comment on that.

RalfJung · 2018-06-05T13:02:12Z

src/librustc_mir/interpret/eval_context.rs

-                    None => return err!(InvalidChar(c as u128)),
+        self.memory.dump_alloc(ptr.alloc_id);
+        trace!("validate_ptr_target: {:#?}", layout);
+        if let layout::Variants::Tagged { .. } = layout.variants {


I assume there's an invariant that says we don't have to do while let here?

This is exactly how we do it elsewhere. We'll have to change a lot of things if that invariant is invalidated.

RalfJung · 2018-06-05T13:03:38Z

Well... to check whether a value is a valid u64 you need to access the value, and that access is failing. It's not the actual checking code ;)

We do not traverse through pointers, no, we'd need to act upon type information for that.

Oh, that's what this does. Could we get a rustdoc comment on validate_ptr_target explaining that much?

oli-obk · 2018-06-05T13:17:28Z

Which part do you move to another create to make this a problematic example? Everything but main?

just the constants, make them public, and then access them from the given functions

But how is that different from, you know, exactly the same code without any const, which also returns undef?

It is not, except that we know there's an undef here. It's just as if you wrote intrinsics::undefined::<T>() where T is the type of the constant/return type of the function.

Well yes, and I am still asking why this doesn't happen already but you refuse to answer. "More bad stuff earlier", I still don't understand. I'm really not trying to be obnoxious here but you still didn't spell out what problem this is solving.

It's solving the problem that you can accidentally do UB in constants and not notice until it becomes UB in your binary and starts to segfault.

I am still asking why this doesn't happen already

because noone implemented this PR before ;)

Okay so the fact that this is a const fn doesn't matter? It's about the const FIELD3: Field3 = ...;? In that case I'd remove the const fn as it's just distracting.

No, the const fn is irrelevant indeed. I just took the original failing test and adjusted it.

Ah! That's nice. Could we have a test that transmutes 2 to bool and fails immediately?

transmute is not const fn, but I already added a test for union-converting 42 to a bool: https://github.com/rust-lang/rust/pull/51361/files#diff-457c99ee767b575ea31313a78d84dbceR36

Now I feel we're getting there.

yay!

To repeat my question from above, what is currently checked on the final initializer value? I stated above that I assume it must not be undef, you didn't comment on that.

Oh, so any constant has a backing Allocation (which consists of the memory-bytes, pointer-relocations and the undef-bytes-mask). We create a Pointer (AllocId + offset into the allocation) to the constant. For constants that is with offset == 0. Then we call the validate_ptr_target function with said Pointer and the TyLayout of the constant.

RalfJung · 2018-06-05T14:01:50Z

just the constants, make them public, and then access them from the given functions

The constants inside the functions? Okay. I'd never have guessed that.^^

transmute is not const fn, but I already added a test for union-converting 42 to a bool

👍

Then we call the validate_ptr_target function with said Pointer and the TyLayout of the constant.

Ah, and the current version doesn't even complain when a u64 is Undef?

And then what does trans do? Emit an LLVM undef? Or an error?

And the goal is to make sure that the trans error/undef never happens (i.e., turn it into an ICE) by doing more checks when the const is computed? In that case, yes please! Why didn't we always do that? ;)

However, calling validate_ptr_target in try_read seems entirely orthogonal, and maybe should be discussed separately. "What CTFE needs for the result of evaluation" is possibly different from "what we want to enforce all the time during CTFE/miri execution".

I tried looking at master to see what happens, but right now, I cannot find a single caller for validate_ptr_target. So after this PR, the only caller would be try_read. Shouldn't there be a call somewhere on the const machinery that wires rustc to miri?

oli-obk · 2018-06-05T14:33:01Z

Ah, and the current version doesn't even complain when a u64 is Undef?

no, thus this PR ;)

And then what does trans do? Emit an LLVM undef? Or an error?

before this PR it produced an undef, now it produces an error

And the goal is to make sure that the trans error/undef never happens (i.e., turn it into an ICE) by doing more checks when the const is computed? In that case, yes please!

great!

Why didn't we always do that? ;)

Just an oversight

However, calling validate_ptr_target in try_read seems entirely orthogonal, and maybe should be discussed separately. "What CTFE needs for the result of evaluation" is possibly different from "what we want to enforce all the time during CTFE/miri execution".

I moved it to the const_eval_provider just before it would otherwise successfully return a constant

I tried looking at master to see what happens, but right now, I cannot find a single caller for validate_ptr_target.

Yea, it was commented out. Must have happened during some PR/rebase/refactoring.

RalfJung · 2018-06-06T07:23:08Z

src/test/ui/const-eval/ub-enum-ptr.stderr

+  --> $DIR/ub-enum-ptr.rs:23:1
+   |
+LL | const BAD_ENUM: Enum = unsafe { Foo { a: &1 }.b}; //~ ERROR this constant cannot be used
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ a raw memory access tried to access part of a pointer value as raw bytes


Is that the error we would expect? Shouldn't it be something like "out of bounds"?

We can't report out of bounds before we know the bits, reading the bits will result in this error though ;)

Hm, that error is not great though because nothing here tries to read the pointer as bytes, Also, I don't think I understand which line in validate_scalar triggers this -- the Scalar::Ptr(_) case doesn't do any reads.

IOW, how is this failure at all related to Enum having 0 as its only value? If it's about the read, it'll probably happen with any enum, right? So, the comment about non-null in the test seems to have nothing to do with what the test actually tests.

I found the issue...

enum Foo { Bar = 0, }

has Tagged variants, so reading the tag will bug out.

I'll do what you proposed all along and move the Abi check up (just for Uninhabited and Scalar).

Ah so it's a Scalar but also Tagged? Makes sense.

We'd still get that error though of it's an enum with data. Could we get a more specific error message like "invalid: enum discriminant is a pointer" or "error determining enum discriminant" in that case?

sure, I can just run the scalar-checks on the discriminant before trying to read the discriminant.

Hm, I thought of a simple .map_err, but I have no idea what would work better.

RalfJung · 2018-06-06T07:28:30Z

before this PR it produced an undef, now it produces an error

Trans hasn't been touched though, so if something slips through the test it'll still emit undef. Can we safeguard against that? Or is undef actually legitimate e.g. if the constant is a struct with padding?

Is it a problem that validate_ptr_target doesn't validate pointers transitively, i.e., what about constants that point to something invalid (instead of being something invalid)?

I moved it to the const_eval_provider just before it would otherwise successfully return a constant

Great, thanks. I'm not necessarily opposed to checking these things earlier, I just wanted to understand what is being checked and, more important, where -- try_read is probably not the only place that would need to call this, I assume.

How far does layout validation go? E.g., if the type is Option<usize>, does it check that the discriminant is in-bounds?

oli-obk · 2018-06-06T08:07:27Z

Trans hasn't been touched though, so if something slips through the test it'll still emit undef. Can we safeguard against that? Or is undef actually legitimate e.g. if the constant is a struct with padding?

Yes undef bytes are legitimate. We could probably guard against all bits being undef and just bail out for that, too, but I'm not sure if that code wouldn't be dead anyway with the above checks in place

Is it a problem that validate_ptr_target doesn't validate pointers transitively, i.e., what about constants that point to something invalid (instead of being something invalid)?

I'll try to cook up something sensible. I didn't want to go overboard with this PR.

How far does layout validation go? E.g., if the type is Option, does it check that the discriminant is in-bounds?

Oh yea :D we read the discriminant with the regular miri-discriminant reader, which totally produces an error if that discriminant does not exist. If it does exist, then we change the layout to the variant that the discriminant selects and check whether that layout fits the bill.

RalfJung · 2018-06-06T08:55:29Z

I'll try to cook up something sensible. I didn't want to go overboard with this PR.

Fair enough.
But also, if the constant points to something, that something also has to be extracted from miri's memory somewhere somehow, won't that take the same path and then run validation?

we read the discriminant with the regular miri-discriminant reader, which totally produces an error if that discriminant does not exist.

Awesome!

RalfJung · 2018-06-06T17:57:39Z

src/librustc_mir/interpret/eval_context.rs

@@ -1406,18 +1407,17 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M

    /// This function checks the memory where `ptr` points to.
    /// It will error if the bits at the destination do not match the ones described by the layout.
-    ///
-    /// Note: this function does not transitively dereference other pointers. All that is checked
-    /// for pointers is whether they are not allowed to be zero, but are.
    pub fn validate_ptr_target(


Well it still doesn't transitively do anything itself, it just fills seen and todo accordingly, right? The docs should say that.

oli-obk · 2018-06-06T17:57:52Z

src/librustc_mir/interpret/const_eval.rs

+                    // this would be nice-to-have, but we are inside the query that
+                    // evaluates said static, and const eval will try to run this query again
+                    // if we do this kind of validation
+                    // We could run the checks as an additional lint after evaluation is done


Now I'm thinking we should totally do this generally. If we accidentally make any checks too strict, users can opt out of them for specific constants until we fix it.

What do you mean by "generally"?

Adding new checks to const eval as lints instead of breaking the evaluation

Deny-by-default lint? Yeah I guess that makes sense. We can continue compilation, after all. It just probably doesn't do what people want (as in, it raises UB...).

RalfJung · 2018-06-06T17:58:48Z

I see you now added some kind of logic to validate pointers transitively. What I am surprised by is that this needed some new kind of traversal logic; shouldn't similar logic exist already to "translate" CTFE results (transitively through pointers) into things that will later be passed to LLVM?

bors · 2018-07-29T13:22:56Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout sanity_check_consts (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self sanity_check_consts --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

warning: Cannot merge binary files: src/Cargo.lock (HEAD vs. heads/homu-tmp)
Auto-merging src/librustc_lint/builtin.rs
Auto-merging src/librustc/lib.rs
Auto-merging src/Cargo.lock
CONFLICT (content): Merge conflict in src/Cargo.lock
Automatic merge failed; fix conflicts and then commit the result.

RalfJung · 2018-07-29T18:26:00Z

Rebased.

@bors r=nikomatsakis

bors · 2018-07-29T18:26:00Z

📌 Commit 86e59cc has been approved by nikomatsakis

bors · 2018-07-29T19:37:37Z

⌛ Testing commit 86e59cc with merge 70cac59...

@RalfJung

Do a basic sanity check for all constant values ## Motivation and high level overview There has been some back and forth in this PR between @RalfJung and me in here about the motivation for this change and the stance it takes on unsafe coding guidelines. The initial implementation ran its checks on every value read (so `*x`, `y = x`, ...). In unsafe code that isn't reasonable, because we might be invalidating invariants for a short time in order to build up a proper value. The current implementation is a lint that runs its checks statics and constants. There is no need to check array lengths and enum variants, because it's a hard error to end up with anything but a number, and that one even has to have the required bits to be defined. ## What checks are done? * Some type related checks * `char` needs to be a correct unicode character as defined by `char::from_u32` * A reference to a ZST must have the correct alignment (and be nonzero) * A reference to anything is dereferenced and its value is checked * Layout checks use the information from `ty::Layout` to check * all fields of structs * all elements of arrays * enum discriminants * the fields of an enum variant (the variant is decided by the discriminant) * whether any union field succeeds in being checked (if none match the memory pattern, the check fails) * if the value is in the range described by the layout (e.g. for `NonZero*` types) Changing the layout of a type will thus automatically cause the checks to check for the new layout. fixes #51330 fixes #51471 cc @RalfJung r? @eddyb

bors · 2018-07-29T21:37:42Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 70cac59 to master...

rust-highfive · 2018-07-29T21:39:00Z

📣 Toolstate changed by #51361!

Tested on commit 70cac59.
Direct link to PR: #51361

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

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

Eh2406 · 2018-07-30T14:11:00Z

drive by comment, this is a dramatic performance regression. perf

kennytm · 2018-07-30T14:12:44Z

@Eh2406 This is a known issue #52849.

Eh2406 · 2018-07-30T14:17:09Z

Ok, sorry for the spam. I looked thru the end of this thread and did not see a comment or link. I am glad that it has been noticed.

vertexclique · 2018-08-10T01:34:09Z

This breaks the unions with generic type that uses dropping:

pub const fn transmute<S>(x: S) -> [usize; 64] {
    #[allow(unions_with_drop_fields)]
    union U<T> {
        a: T,
        b: [usize; 64]
    }
    unsafe { U { a: x }.b }
}

oli-obk · 2018-08-10T04:37:06Z

I'm not sure I count this as breaking. Using your function is UB at runtime. It also is unrelated to drop. the issue is that when you use it for a type smaller than 64 usizes, you get usizes which are undefined, and reading those is UB, and if your type is bigger, you are throwing away part of it.

Can you provide your use case so we can find the solution to your problem without invoking UB?

RalfJung · 2018-08-10T19:03:46Z

Agreed -- erroring here is "working as intended", to avoid undefs floating around.

rust-highfive assigned eddyb Jun 5, 2018

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

oli-obk mentioned this pull request Jun 5, 2018

Update to rustc sanity check branch rust-lang/miri#381

Merged

RalfJung reviewed Jun 5, 2018

View reviewed changes

fbstj reviewed Jun 5, 2018

View reviewed changes

RalfJung reviewed Jun 5, 2018

View reviewed changes

fbstj reviewed Jun 5, 2018

View reviewed changes

RalfJung reviewed Jun 5, 2018

View reviewed changes

RalfJung reviewed Jun 6, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

oli-obk force-pushed the sanity_check_consts branch from 4d647cb to 61c4741 Compare June 6, 2018 16:02

RalfJung reviewed Jun 6, 2018

View reviewed changes

oli-obk commented Jun 6, 2018

View reviewed changes

RalfJung mentioned this pull request Jul 29, 2018

[DO NOT MERGE] break codegen test #52826

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 29, 2018

oli-obk and others added 5 commits July 29, 2018 20:22

Sanity-check all constants

9377041

Update miri submodule

b92f047

Move a test that depends on the arch bitwidth to compile-fail

3e8337e

update codegen tests

5ba7633

dont hardcode vtable size in codegen test

86e59cc

RalfJung force-pushed the sanity_check_consts branch from 34a85dd to 86e59cc Compare July 29, 2018 18:25

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 29, 2018

bors merged commit 86e59cc into rust-lang:master Jul 29, 2018

RalfJung mentioned this pull request Aug 6, 2018

A pretty big performance hit after a recent change (PR #51361) #52849

Closed

oli-obk deleted the sanity_check_consts branch August 10, 2018 04:38

dtolnay mentioned this pull request Oct 1, 2018

Regression from stable: pointer to usize conversion no longer compiles #54709

Closed

pnkfelix mentioned this pull request Oct 9, 2018

error: internal compiler error: Accessing (*_310) with the kind Write(Move) shouldn't be possible #54597

Closed

Do a basic sanity check for all constant values #51361

Do a basic sanity check for all constant values #51361

Conversation

oli-obk commented Jun 5, 2018 • edited Loading

Motivation and high level overview

What checks are done?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Jun 5, 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

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

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

Choose a reason for hiding this comment

RalfJung commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Jun 5, 2018

RalfJung commented Jun 5, 2018 • edited Loading

RalfJung commented Jun 5, 2018

oli-obk commented Jun 5, 2018

RalfJung commented Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Jun 5, 2018 • edited Loading

oli-obk commented Jun 5, 2018

RalfJung commented Jun 5, 2018 • edited Loading

oli-obk commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Jun 6, 2018 • edited Loading

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 Jun 6, 2018

oli-obk commented Jun 6, 2018

RalfJung commented Jun 6, 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

RalfJung commented Jun 6, 2018

bors commented Jul 29, 2018

RalfJung commented Jul 29, 2018

bors commented Jul 29, 2018

bors commented Jul 29, 2018

bors commented Jul 29, 2018

rust-highfive commented Jul 29, 2018

Eh2406 commented Jul 30, 2018

kennytm commented Jul 30, 2018

Eh2406 commented Jul 30, 2018

vertexclique commented Aug 10, 2018

oli-obk commented Aug 10, 2018 • edited Loading

RalfJung commented Aug 10, 2018

oli-obk commented Jun 5, 2018 •

edited

Loading

RalfJung commented Jun 5, 2018 •

edited

Loading

RalfJung commented Jun 5, 2018 •

edited

Loading

RalfJung commented Jun 5, 2018 •

edited

Loading

RalfJung commented Jun 5, 2018 •

edited

Loading

RalfJung Jun 6, 2018 •

edited

Loading

oli-obk commented Aug 10, 2018 •

edited

Loading