Check type of place base (not projection) for Freeze in IndirectlyMutableLocals
#65030
Conversation
This prevents unsafe code from taking a reference to a field that is `Freeze` and using pointer arithmetic to change it to a reference to a field that is not `Freeze` without causing a local to become marked as indirectly mutable. Instead, taking a reference to the `Freeze` field will cause the whole struct to be marked as indirectly mutable.
rust-highfive
added
the
S-waiting-on-review
ecstatic-morse
changed the title
FreezeFreeze in IndirectlyMutableLocals
| @@ -30,9 +34,9 @@ const BOO: i32 = { | |||
| &mut *pmut_cell | |||
| }; | |||
|
|
|||
| *rmut_cell = 42; // Mutates `x` indirectly even though `x` is not marked indirectly mutable!!! | |||
| *rmut_cell = 42; | |||
There was a problem hiding this comment.
I'm a bit confused. This line is UB. You obtained a reference to cell from a reference to zst. I'm certain miri will slap this code around if it were runtime code
There was a problem hiding this comment.
I'm confused about rust's memory model then. If you're not allowed to obtain a reference to one part of a struct and convert it to a reference to a disjoint part of that same struct, then there's no need to consider this in the analysis.
There was a problem hiding this comment.
The following quote is from the ptr::offset docs (non-normative, I know). I don't actually use ptr::offset, because it's not supported in MIRI. Instead I use a zero-sized type in a #[repr(C)] struct to ensure that the two fields have the same address.
Both the starting and resulting pointer must be either in bounds or one byte past the end of the same allocated object. Note that in Rust, every (stack-allocated) variable is considered a separate allocated object.
I was assuming that the stack-allocated instance of PartialInteriorMut was a single "allocated object" in rust's memory model, and thus converting a pointer to one of its fields into a pointer to another was not UB.
There was a problem hiding this comment.
I'm not sure at all. Maybe @RalfJung can have a look?
There was a problem hiding this comment.
I'm confused about rust's memory model then. If you're not allowed to obtain a reference to one part of a struct and convert it to a reference to a disjoint part of that same struct, then there's no need to consider this in the analysis.
Stacked Borrows disallows this, to keep aliasing under control. But we don't have a final memory model yet. See this document for further details on Stacked Borrows.
Also, with Stacked Borrows you can do things like this through raw pointers. Like, &x as *mut T and then do whatever you want with the resulting raw pointer -- anything that C allows, we also allow.
| unsafe { rustc_peek(i) }; //~ ERROR rustc_peek: bit not set | ||
| unsafe { rustc_peek(cell) }; //~ ERROR rustc_peek: bit not set | ||
|
|
||
| let p = &mut cell; |
There was a problem hiding this comment.
cell is immutable, how does this line not error?
There was a problem hiding this comment.
stop_after_dataflow prevents later all subsequent compiler passes from running, so we never actually get to the pass that would fail on this broken code. I'll make sure these tests compile outside of rustc_peek mode when possible though.
|
☔ The latest upstream changes (presumably #65087) made this pull request unmergeable. Please resolve the merge conflicts. |
|
This example is indeed UB in the current model of stacked borrows (see rust-lang/unsafe-code-guidelines#134). However, it is an open question whether the final version of rust's memory model will forbid code like this. I'm going to close this PR along with #65006. I'll mention this as a relevant example on the UCG repo. |
Resolves #65006.
This fixes the test I wrote in #64980, which demonstrated a way for code to circumvent
IndirectlyMutableLocals. Now, we are more conservative and check the type of the entire local for!Freeze(not just the target of the projection) when doing a shared borrow. This shouldn't affect the dataflow-based const validator since references that allow mutation are forbidden anyway in const contexts. I can't rule out an edge case around empty slices though.I also added some simplistic tests for
IndirectlyMutableLocalsto demonstrate the purpose of the analysis.r? @oli-obk