miri: ensure validity of references and pointers we dereference and cast - #160012
Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt
cc @rust-lang/miri Some changes occurred to the CTFE machinery Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri, @oli-obk, @lcnr Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri |
rustbot
added
S-waiting-on-review
|
r? @chenyukang rustbot has assigned @chenyukang. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
|
Does this PR affect whether consteval code compiles? If so, do we need a crater run? |
|
This probably improves our const-eval UB detection capabilities, but the cases this affects are so niche I don't think it needs a crater run. |
This comment has been minimized.
This comment has been minimized.
Hmm... I don't like it, but I see why that would happen. Tho I worry that will be an issue for miri + mir-opts, as the latter don't expect box derefs anymore. Ideally we'd track extra stuff to check as some sort of marker like the now-gone StatementKind::Retag. But that's also a lot of noise for a weird edge case. Cant you instead detect the box deref via the long chained projection? Can't be worse than having to manually turn a box deref back into a longer projection |
|
Yeah I don't like it either, I just could not think of anything better. Maybe we could have separate MIR phase for "post box deref removal" and a flag that somehow skips that entire phase in the MIR pipeline (so you can't even turn on the passes in that phase manually) but I can't think of a nice way to do that.
If someone actually spells out that projection, I don't think it should behave like a typed load of the entire |
d83d238 to
45ae4d4
Compare
Doesn't seem like much of an issue to me considering how special
hmm... special casing box more (again?) doesn't seem too great either. What's missing here for a general solution? Can we get there via some intermediate marker on |
Well, Box fundamentally is special in the opsem. So what's missing is rust-lang/rfcs#3712. ;) As long as Box is special, that needs to be represented somehow. This PR represents it the way we used to, with a primitive
I think among these three I prefer the last one. |
|
cc @bjorn3 This PR changes rustc_public cc @oli-obk, @celinval, @ouz-a, @makai410 This PR changes MIR cc @oli-obk, @JakobDegen, @vakaras |
I have now implemented option three. |
This comment has been minimized.
This comment has been minimized.
|
cc @rust-lang/clippy |
This comment has been minimized.
This comment has been minimized.
miri: ensure validity of references and pointers we dereference and cast try-job: x86_64-gnu-llvm-21
|
@bors r=oli-obk |
rust-bors
Bot
added
S-waiting-on-bors
fa7f448 to
907a562
Compare
rust-bors
Bot
added
S-waiting-on-author
|
This pull request was unapproved. |
|
I realized |
rust-bors
Bot
added
S-waiting-on-bors
miri: ensure validity of references and pointers we dereference and cast
Conceptually, when we evaluate the place expression `*place`, there are two steps that happen:
- perform a (typed) load from `place`, which yields a value of pointer/ref type
- construct a new place from that value
Since this is a typed load, we have to ensure that the value satisfies the validity invariant. We forgot to do that, which led to Miri missing a bunch of UB. This PR fixes that by adding the missing checks.
Triggering this UB is a bit non-trivial: you cannot just store an invalid value into `place` using regular assignment (that would already be caught as part of the assignment). However, you can take a raw pointer to `place` and then use that to mutate the contents of `place` in a way that its validity invariant no longer holds. For example:
```rust
fn main() {
let mut x = &();
// Overwrite `x` with null.
unsafe { (&raw mut x).cast::<usize>().write(0) };
let _val = *x; //~ERROR: null reference
}
```
This program clearly (IMO) should have UB, and has UB in MiniRust, but Miri accepted that program before this PR. The same applies to references that are invalid because they are unaligned (even if we end up only accessing a 1-aligned field, i.e. the rest of the place expression evaluates just fine), and to references that are invalid because they are not dereferenceable (even if we end up projecting to a zero-sized field, i.e., the rest of the place expression evaluates just fine).
Even raw pointers have this problem: a raw pointer can be invalid if its vtable pointer is wrong, and we did not check that.
And finally, this also affects `Box`, and this is where things get tricky. `Box` derefs are not even present any more in the MIR Miri sees; they have been replaced by derefs of the underlying raw pointer. But that means we have no way to know that we should check all those things. So to fix that I adjusted the ElaborateBoxDerefs to emit a new special kind of cast that's almost a transmute but also checks `Box` validity.
Fixes rust-lang/miri#5226
Fixes rust-lang/unsafe-code-guidelines#617
Cc @rust-lang/opsem
miri: ensure validity of references and pointers we dereference and cast
Conceptually, when we evaluate the place expression `*place`, there are two steps that happen:
- perform a (typed) load from `place`, which yields a value of pointer/ref type
- construct a new place from that value
Since this is a typed load, we have to ensure that the value satisfies the validity invariant. We forgot to do that, which led to Miri missing a bunch of UB. This PR fixes that by adding the missing checks.
Triggering this UB is a bit non-trivial: you cannot just store an invalid value into `place` using regular assignment (that would already be caught as part of the assignment). However, you can take a raw pointer to `place` and then use that to mutate the contents of `place` in a way that its validity invariant no longer holds. For example:
```rust
fn main() {
let mut x = &();
// Overwrite `x` with null.
unsafe { (&raw mut x).cast::<usize>().write(0) };
let _val = *x; //~ERROR: null reference
}
```
This program clearly (IMO) should have UB, and has UB in MiniRust, but Miri accepted that program before this PR. The same applies to references that are invalid because they are unaligned (even if we end up only accessing a 1-aligned field, i.e. the rest of the place expression evaluates just fine), and to references that are invalid because they are not dereferenceable (even if we end up projecting to a zero-sized field, i.e., the rest of the place expression evaluates just fine).
Even raw pointers have this problem: a raw pointer can be invalid if its vtable pointer is wrong, and we did not check that.
And finally, this also affects `Box`, and this is where things get tricky. `Box` derefs are not even present any more in the MIR Miri sees; they have been replaced by derefs of the underlying raw pointer. But that means we have no way to know that we should check all those things. So to fix that I adjusted the ElaborateBoxDerefs to emit a new special kind of cast that's almost a transmute but also checks `Box` validity.
Fixes rust-lang/miri#5226
Fixes rust-lang/unsafe-code-guidelines#617
Cc @rust-lang/opsem
…uwer Rollup of 10 pull requests Successful merges: - #157572 (stabilize size_of_val_raw, align_of_val_raw, Layout::for_value_raw) - #160012 (miri: ensure validity of references and pointers we dereference and cast) - #160294 (Update Enzyme to resolve one of the open bugs) - #159503 (allocations: document that they can be read-only) - #160250 (When issuing suggestions for missing trait items, label unstable items) - #160251 (Replace unsafe usage of `NonNull::new_unchecked` with `Box::into_non_null`) - #160311 (Remove final use of sealed traits from stdlib) - #160313 (Make the noundef-on-Cast size guard explicit) - #160323 (Box::leak: tell people to avoid unleaking) - #160328 (Move `check_track_caller` into the attribute parser)
…uwer Rollup of 12 pull requests Successful merges: - #157572 (stabilize size_of_val_raw, align_of_val_raw, Layout::for_value_raw) - #160012 (miri: ensure validity of references and pointers we dereference and cast) - #160294 (Update Enzyme to resolve one of the open bugs) - #159503 (allocations: document that they can be read-only) - #160179 (std: Update `wasip3` crate dependency) - #160250 (When issuing suggestions for missing trait items, label unstable items) - #160251 (Replace unsafe usage of `NonNull::new_unchecked` with `Box::into_non_null`) - #160311 (Remove final use of sealed traits from stdlib) - #160313 (Make the noundef-on-Cast size guard explicit) - #160323 (Box::leak: tell people to avoid unleaking) - #160328 (Move `check_track_caller` into the attribute parser) - #160333 (Remove itertools dependency from `rustc_ast_pretty`)
Rollup merge of #160012 - RalfJung:deref-validity, r=oli-obk miri: ensure validity of references and pointers we dereference and cast Conceptually, when we evaluate the place expression `*place`, there are two steps that happen: - perform a (typed) load from `place`, which yields a value of pointer/ref type - construct a new place from that value Since this is a typed load, we have to ensure that the value satisfies the validity invariant. We forgot to do that, which led to Miri missing a bunch of UB. This PR fixes that by adding the missing checks. Triggering this UB is a bit non-trivial: you cannot just store an invalid value into `place` using regular assignment (that would already be caught as part of the assignment). However, you can take a raw pointer to `place` and then use that to mutate the contents of `place` in a way that its validity invariant no longer holds. For example: ```rust fn main() { let mut x = &(); // Overwrite `x` with null. unsafe { (&raw mut x).cast::<usize>().write(0) }; let _val = *x; //~ERROR: null reference } ``` This program clearly (IMO) should have UB, and has UB in MiniRust, but Miri accepted that program before this PR. The same applies to references that are invalid because they are unaligned (even if we end up only accessing a 1-aligned field, i.e. the rest of the place expression evaluates just fine), and to references that are invalid because they are not dereferenceable (even if we end up projecting to a zero-sized field, i.e., the rest of the place expression evaluates just fine). Even raw pointers have this problem: a raw pointer can be invalid if its vtable pointer is wrong, and we did not check that. And finally, this also affects `Box`, and this is where things get tricky. `Box` derefs are not even present any more in the MIR Miri sees; they have been replaced by derefs of the underlying raw pointer. But that means we have no way to know that we should check all those things. So to fix that I adjusted the ElaborateBoxDerefs to emit a new special kind of cast that's almost a transmute but also checks `Box` validity. Fixes rust-lang/miri#5226 Fixes rust-lang/unsafe-code-guidelines#617 Cc @rust-lang/opsem
|
@rust-timer build fe46b75 For #160339. |
|
Queued fe46b75 with parent c47bb2f, future comparison URL. |
9 participants
View all comments
Conceptually, when we evaluate the place expression
*place, there are two steps that happen:place, which yields a value of pointer/ref typeSince this is a typed load, we have to ensure that the value satisfies the validity invariant. We forgot to do that, which led to Miri missing a bunch of UB. This PR fixes that by adding the missing checks.
Triggering this UB is a bit non-trivial: you cannot just store an invalid value into
placeusing regular assignment (that would already be caught as part of the assignment). However, you can take a raw pointer toplaceand then use that to mutate the contents ofplacein a way that its validity invariant no longer holds. For example:This program clearly (IMO) should have UB, and has UB in MiniRust, but Miri accepted that program before this PR. The same applies to references that are invalid because they are unaligned (even if we end up only accessing a 1-aligned field, i.e. the rest of the place expression evaluates just fine), and to references that are invalid because they are not dereferenceable (even if we end up projecting to a zero-sized field, i.e., the rest of the place expression evaluates just fine).
Even raw pointers have this problem: a raw pointer can be invalid if its vtable pointer is wrong, and we did not check that.
And finally, this also affects
Box, and this is where things get tricky.Boxderefs are not even present any more in the MIR Miri sees; they have been replaced by derefs of the underlying raw pointer. But that means we have no way to know that we should check all those things. So to fix that I adjusted the ElaborateBoxDerefs to emit a new special kind of cast that's almost a transmute but also checksBoxvalidity.Fixes rust-lang/miri#5226
Fixes rust-lang/unsafe-code-guidelines#617
Cc @rust-lang/opsem