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.

Sign up for GitHub

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

Jump to bottom

Check type of place base (not projection) for Freeze in IndirectlyMutableLocals #65030

Closed
wants to merge 6 commits into from
Closed

Check type of place base (not projection) for Freeze in IndirectlyMutableLocals #65030

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
64457a1
Check type of place base (not projection) for `!Freeze`
ecstatic-morse Oct 2, 2019
da0969f
Update `IndirectlyMutableLocals` test with correct behavior
ecstatic-morse Oct 2, 2019
6487ce0
Add more peek tests for `IndirectlyMutableLocals`
ecstatic-morse Oct 2, 2019
2f89429
Remove `borrowck_graphviz_postflow` from test
ecstatic-morse Oct 2, 2019
895f9c4
Downgrade unnecessary `warn` to `trace`
ecstatic-morse Oct 2, 2019
91c77e9
Enusure tests would build outside of peek
ecstatic-morse Oct 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/librustc_mir/dataflow/impls/indirect_mutation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,15 +104,17 @@ impl<'tcx> TransferFunction<'_, '_, 'tcx> {
kind: mir::BorrowKind,
borrowed_place: &mir::Place<'tcx>,
) -> bool {
let borrowed_ty = borrowed_place.ty(self.body, self.tcx).ty;
let base_ty = borrowed_place.base.ty(self.body).ty;

// Zero-sized types cannot be mutated, since there is nothing inside to mutate.
//
// FIXME: For now, we only exempt arrays of length zero. We need to carefully
// consider the effects before extending this to all ZSTs.
if let ty::Array(_, len) = borrowed_ty.kind {
if len.try_eval_usize(self.tcx, self.param_env) == Some(0) {
return false;
if borrowed_place.projection.is_empty() {
if let ty::Array(_, len) = base_ty.kind {
if len.try_eval_usize(self.tcx, self.param_env) == Some(0) {
return false;
}
}
}

Expand All @@ -122,7 +124,7 @@ impl<'tcx> TransferFunction<'_, '_, 'tcx> {
| mir::BorrowKind::Shared
| mir::BorrowKind::Shallow
| mir::BorrowKind::Unique
=> !borrowed_ty.is_freeze(self.tcx, self.param_env, DUMMY_SP),
=> !base_ty.is_freeze(self.tcx, self.param_env, DUMMY_SP),
}
}
}
Expand Down
10 changes: 7 additions & 3 deletions src/test/ui/mir-dataflow/indirect-mutation-offset.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ const BOO: i32 = {
cell: UnsafeCell::new(0),
};

let p_zst: *const _ = &x.zst ; // Doesn't cause `x` to get marked as indirectly mutable.
unsafe { rustc_peek(x) }; //~ ERROR rustc_peek: bit not set

let p_zst: *const _ = &x.zst ;

unsafe { rustc_peek(x) };

let rmut_cell = unsafe {
// Take advantage of the fact that `zst` and `cell` are at the same location in memory.
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse Oct 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure at all. Maybe @RalfJung can have a look?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let val = *rmut_cell;
unsafe { rustc_peek(x) }; //~ ERROR rustc_peek: bit not set
unsafe { rustc_peek(x) };

val
};
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/mir-dataflow/indirect-mutation-offset.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: rustc_peek: bit not set
--> $DIR/indirect-mutation-offset.rs:35:14
--> $DIR/indirect-mutation-offset.rs:21:14
|
LL | unsafe { rustc_peek(x) };
| ^^^^^^^^^^^^^
Expand Down