From bba3c49e844895cdc32dc6ea1acbf0c6555beefd Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 26 Oct 2018 11:31:20 +0200 Subject: [PATCH] basic retagging (no fn_entry); this also makes us catch more bugs even with optimizations and we can finally stop mutating the state on deref --- src/lib.rs | 4 +- src/stacked_borrows.rs | 76 +++++++++---------- .../stacked_borrows/alias_through_mutation.rs | 3 - .../stacked_borrows/buggy_as_mut_slice.rs | 3 - .../stacked_borrows/buggy_split_at_mut.rs | 3 - .../stacked_borrows/illegal_write2.rs | 3 - .../stacked_borrows/illegal_write4.rs | 34 ++------- .../stacked_borrows/shared_confusion.rs | 10 ++- .../stacked_borrows/shared_confusion_opt.rs | 25 ++++++ 9 files changed, 81 insertions(+), 80 deletions(-) create mode 100644 tests/compile-fail/stacked_borrows/shared_confusion_opt.rs diff --git a/src/lib.rs b/src/lib.rs index 5d20078082f..e1ae805fb15 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -528,9 +528,11 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> { fn_entry: bool, place: PlaceTy<'tcx, Borrow>, ) -> EvalResult<'tcx> { - if !ecx.tcx.sess.opts.debugging_opts.mir_emit_retag || !ecx.machine.validate { + if !ecx.tcx.sess.opts.debugging_opts.mir_emit_retag || !Self::enforce_validity(ecx) { // No tracking, or no retagging. This is possible because a dependency of ours might be // called with different flags than we are, + // Also, honor the whitelist in `enforce_validity` because otherwise we might retag + // uninitialized data. return Ok(()) } ecx.retag(fn_entry, place) diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 22216345b65..cb8c2a3e5e6 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -1,6 +1,6 @@ -use std::cell::{Cell, RefCell}; +use std::cell::RefCell; -use rustc::ty::{Ty, layout::Size}; +use rustc::ty::{self, Ty, layout::Size}; use rustc::hir; use super::{ @@ -101,12 +101,12 @@ impl From> for RefKind { /// Extra global machine state #[derive(Clone, Debug)] pub struct State { - clock: Cell + clock: Timestamp } impl State { pub fn new() -> State { - State { clock: Cell::new(0) } + State { clock: 0 } } } @@ -129,6 +129,7 @@ impl Default for Stack { /// Extra per-allocation state #[derive(Clone, Debug, Default)] pub struct Stacks { + // Even reading memory can have effects on the stack, so we need a `RefCell` here. stacks: RefCell>, } @@ -249,9 +250,9 @@ impl<'tcx> Stack { } impl State { - fn increment_clock(&self) -> Timestamp { - let val = self.clock.get(); - self.clock.set(val + 1); + fn increment_clock(&mut self) -> Timestamp { + let val = self.clock; + self.clock = val + 1; val } } @@ -334,14 +335,8 @@ impl<'tcx> Stacks { } pub trait EvalContextExt<'tcx> { - fn tag_for_pointee( - &self, - pointee_ty: Ty<'tcx>, - ref_kind: RefKind, - ) -> Borrow; - fn tag_reference( - &self, + &mut self, ptr: Pointer, pointee_ty: Ty<'tcx>, size: Size, @@ -371,13 +366,16 @@ pub trait EvalContextExt<'tcx> { } impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, 'tcx> { - fn tag_for_pointee( - &self, + /// Called for place-to-value conversion. + fn tag_reference( + &mut self, + ptr: Pointer, pointee_ty: Ty<'tcx>, + size: Size, ref_kind: RefKind, - ) -> Borrow { + ) -> EvalResult<'tcx, Borrow> { let time = self.machine.stacked_borrows.increment_clock(); - match ref_kind { + let new_bor = match ref_kind { RefKind::Mut => Borrow::Mut(Mut::Uniq(time)), RefKind::Shr => // FIXME This does not do enough checking when only part of the data has @@ -390,18 +388,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' Borrow::Mut(Mut::Raw) }, RefKind::Raw => Borrow::Mut(Mut::Raw), - } - } - - /// Called for place-to-value conversion. - fn tag_reference( - &self, - ptr: Pointer, - pointee_ty: Ty<'tcx>, - size: Size, - ref_kind: RefKind, - ) -> EvalResult<'tcx, Borrow> { - let new_bor = self.tag_for_pointee(pointee_ty, ref_kind); + }; trace!("tag_reference: Creating new reference ({:?}) for {:?} (pointee {}, size {}): {:?}", ref_kind, ptr, pointee_ty, size.bytes(), new_bor); @@ -424,7 +411,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' fn tag_dereference( &self, ptr: Pointer, - pointee_ty: Ty<'tcx>, + _pointee_ty: Ty<'tcx>, size: Size, ref_kind: RefKind, ) -> EvalResult<'tcx, Borrow> { @@ -454,13 +441,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' // also using `var`, and that would be okay. } (RefKind::Shr, Borrow::Mut(Mut::Uniq(_))) => { - // A mut got transmuted to shr. High time we freeze this location! - // Make this a delayed reborrow. Redundant reborows to shr are okay, - // so we do not have to be worried about doing too much. - // FIXME: Reconsider if we really want to mutate things while doing just a deref, - // which, in particular, validation does. - trace!("tag_dereference: Lazy freezing of {:?}", ptr); - return self.tag_reference(ptr, pointee_ty, size, ref_kind); + // A mut got transmuted to shr. The mut borrow must be reactivatable. } (RefKind::Mut, Borrow::Frz(_)) => { // This is just invalid. @@ -516,9 +497,24 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' fn retag( &mut self, _fn_entry: bool, - _place: PlaceTy<'tcx, Borrow> + place: PlaceTy<'tcx, Borrow> ) -> EvalResult<'tcx> { - // TODO do something + // For now, we only retag if the toplevel type is a reference. + // TODO: Recurse into structs and enums, sharing code with validation. + let mutbl = match place.layout.ty.sty { + ty::Ref(_, _, mutbl) => mutbl, // go ahead + _ => return Ok(()), // don't do a thing + }; + // We want to reborrow the reference stored there. This will call the hooks + // above. First deref. + // (This is somewhat redundant because validation already did the same thing, + // but what can you do.) + let val = self.read_value(self.place_to_op(place)?)?; + let dest = self.ref_to_mplace(val)?; + // Now put a new ref into the old place. + // FIXME: Honor `fn_entry`! + let val = self.create_ref(dest, Some(mutbl))?; + self.write_value(val, place)?; Ok(()) } } diff --git a/tests/compile-fail/stacked_borrows/alias_through_mutation.rs b/tests/compile-fail/stacked_borrows/alias_through_mutation.rs index 7d56f30b3e6..eb8966f3a5c 100644 --- a/tests/compile-fail/stacked_borrows/alias_through_mutation.rs +++ b/tests/compile-fail/stacked_borrows/alias_through_mutation.rs @@ -1,6 +1,3 @@ -// With optimizations, we just store a raw in `x`, and there is no problem. -// compile-flags: -Zmir-opt-level=0 - #![allow(unused_variables)] // This makes a ref that was passed to us via &mut alias with things it should not alias with diff --git a/tests/compile-fail/stacked_borrows/buggy_as_mut_slice.rs b/tests/compile-fail/stacked_borrows/buggy_as_mut_slice.rs index dc1e3cc81e9..e86eb9ba6de 100644 --- a/tests/compile-fail/stacked_borrows/buggy_as_mut_slice.rs +++ b/tests/compile-fail/stacked_borrows/buggy_as_mut_slice.rs @@ -1,6 +1,3 @@ -// FIXME: Without retagging, optimization kills finding this problem -// compile-flags: -Zmir-opt-level=0 - #![allow(unused_variables)] mod safe { diff --git a/tests/compile-fail/stacked_borrows/buggy_split_at_mut.rs b/tests/compile-fail/stacked_borrows/buggy_split_at_mut.rs index a697ba9167c..a4f5f536b77 100644 --- a/tests/compile-fail/stacked_borrows/buggy_split_at_mut.rs +++ b/tests/compile-fail/stacked_borrows/buggy_split_at_mut.rs @@ -1,6 +1,3 @@ -// FIXME: Without retagging, optimization kills finding this problem -// compile-flags: -Zmir-opt-level=0 - #![allow(unused_variables)] mod safe { diff --git a/tests/compile-fail/stacked_borrows/illegal_write2.rs b/tests/compile-fail/stacked_borrows/illegal_write2.rs index ac9c3397f53..f4fefaad5e2 100644 --- a/tests/compile-fail/stacked_borrows/illegal_write2.rs +++ b/tests/compile-fail/stacked_borrows/illegal_write2.rs @@ -1,6 +1,3 @@ -// The reborow gets optimized away, so we can only detect this issue without optimizations -// compile-flags: -Zmir-opt-level=0 - #![allow(unused_variables)] fn main() { diff --git a/tests/compile-fail/stacked_borrows/illegal_write4.rs b/tests/compile-fail/stacked_borrows/illegal_write4.rs index 094a3895197..12deb518b4e 100644 --- a/tests/compile-fail/stacked_borrows/illegal_write4.rs +++ b/tests/compile-fail/stacked_borrows/illegal_write4.rs @@ -1,31 +1,13 @@ -// The compiler inserts some reborrows, enable optimizations to -// get rid of them. -// compile-flags: -Zmir-opt-level=1 - use std::mem; -// This is an example of a piece of code that intuitively seems like we might -// want to reject it, but that doesn't turn out to be possible. - fn main() { - let target = 42; - // Make sure a cannot use a raw-tagged `&mut` pointing to a frozen location, not - // even to create a raw. - let reference = ⌖ // freeze + let mut target = 42; + // Make sure we cannot use a raw-tagged `&mut` pointing to a frozen location. + // Even just creating it unfreezes. + let raw = &mut target as *mut _; // let this leak to raw + let reference = unsafe { &*raw }; // freeze let ptr = reference as *const _ as *mut i32; // raw ptr, with raw tag - let mut_ref: &mut i32 = unsafe { mem::transmute(ptr) }; // &mut, with raw tag - // Now we have an &mut to a frozen location, but that is completely normal: - // We'd just unfreeze the location if we used it. - let bad_ptr = mut_ref as *mut i32; // even just creating this is like a use of `mut_ref`. - // That violates the location being frozen! However, we do not properly detect this: - // We first see a `&mut` with a `Raw` tag being deref'd for a frozen location, - // which can happen legitimately if the compiler optimized away an `&mut*` that - // turns a raw into a `&mut`. Next, we create a raw ref to a frozen location - // from a `Raw` tag, which can happen legitimately when interior mutability - // is involved. - let _val = *reference; // Make sure it is still frozen. - - // We only actually unfreeze once we muteate through the bad pointer. - unsafe { *bad_ptr = 42 }; //~ ERROR does not exist on the stack - let _val = *reference; + let _mut_ref: &mut i32 = unsafe { mem::transmute(ptr) }; // &mut, with raw tag + // Now we retag, making our ref top-of-stack -- and, in particular, unfreezing. + let _val = *reference; //~ ERROR Location should be frozen } diff --git a/tests/compile-fail/stacked_borrows/shared_confusion.rs b/tests/compile-fail/stacked_borrows/shared_confusion.rs index e3e4c4da776..c02822ed4d3 100644 --- a/tests/compile-fail/stacked_borrows/shared_confusion.rs +++ b/tests/compile-fail/stacked_borrows/shared_confusion.rs @@ -1,3 +1,11 @@ +// Optimization kills all the reborrows, enough to make this error go away. There are +// no retags either because we don't retag immediately after a `&[mut]`; we rely on +// that creating a fresh reference. +// See `shared_confusion_opt.rs` for a variant that is caught even with optimizations. +// Keep this test to make sure that without optimizations, we do not have to actually +// use the `x_inner_shr`. +// compile-flags: -Zmir-opt-level=0 + #![allow(unused_variables)] use std::cell::RefCell; @@ -9,7 +17,7 @@ fn test(r: &mut RefCell) { { let x_inner_shr = &*x_inner; // frozen let y = &*r; // outer ref, not freezing - let x_inner_shr2 = &*x_inner; // freezing again + let x_inner_shr = &*x_inner; // freezing again } // Our old raw should be dead by now unsafe { *x_evil = 0; } // this falls back to some Raw higher up the stack diff --git a/tests/compile-fail/stacked_borrows/shared_confusion_opt.rs b/tests/compile-fail/stacked_borrows/shared_confusion_opt.rs new file mode 100644 index 00000000000..4a88d4d6413 --- /dev/null +++ b/tests/compile-fail/stacked_borrows/shared_confusion_opt.rs @@ -0,0 +1,25 @@ +// A variant of `shared_confusion.rs` that gets flagged even with optimizations. + +#![allow(unused_variables)] +use std::cell::RefCell; + +fn test(r: &mut RefCell) { + let x = &*r; // not freezing because interior mutability + let mut x_ref = x.borrow_mut(); + let x_inner : &mut i32 = &mut *x_ref; // Uniq reference + let x_evil = x_inner as *mut _; + { + let x_inner_shr = &*x_inner; // frozen + let _val = *x_inner_shr; + let y = &*r; // outer ref, not freezing + let x_inner_shr = &*x_inner; // freezing again + let _val = *x_inner_shr; + } + // Our old raw should be dead by now + unsafe { *x_evil = 0; } // this falls back to some Raw higher up the stack + *x_inner = 12; //~ ERROR Mut reference with non-reactivatable tag +} + +fn main() { + test(&mut RefCell::new(0)); +}