From 26bb4f79dc4897b3ddbae196f793c7ee3cecdeab Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 22 Oct 2018 18:01:32 +0200 Subject: [PATCH] get rid of implicit Raw at bottom of stack; locals get a uniq at their bottom --- src/fn_call.rs | 16 ++-- src/lib.rs | 21 ++++- src/stacked_borrows.rs | 82 +++++++++++++++---- tests/compile-fail/copy_nonoverlapping.rs | 4 + .../stacked_borrows/illegal_write1.rs | 11 +-- .../stacked_borrows/illegal_write2.rs | 6 +- .../stacked_borrows/illegal_write3.rs | 4 +- .../stacked_borrows/illegal_write4.rs | 4 +- .../stacked_borrows/outdated_local.rs | 9 ++ .../stacked_borrows/pointer_smuggling.rs | 3 +- .../stacked_borrows/unescaped_local.rs | 10 +++ tests/compile-fail/zst.rs | 2 +- tests/run-pass/observed_local_mut.rs | 3 + tests/run-pass/ptr_arith_offset_overflow.rs | 1 + 14 files changed, 134 insertions(+), 42 deletions(-) create mode 100644 tests/compile-fail/stacked_borrows/outdated_local.rs create mode 100644 tests/compile-fail/stacked_borrows/unescaped_local.rs diff --git a/src/fn_call.rs b/src/fn_call.rs index 04601b3cd1f..88c31f63fbb 100644 --- a/src/fn_call.rs +++ b/src/fn_call.rs @@ -126,7 +126,7 @@ impl<'a, 'mir, 'tcx: 'mir + 'a> EvalContextExt<'tcx, 'mir> for super::MiriEvalCo } else { let align = self.tcx.data_layout.pointer_align; let ptr = self.memory_mut().allocate(Size::from_bytes(size), align, MiriMemoryKind::C.into())?; - self.write_scalar(Scalar::Ptr(ptr), dest)?; + self.write_scalar(Scalar::Ptr(ptr.with_default_tag()), dest)?; } } @@ -153,7 +153,7 @@ impl<'a, 'mir, 'tcx: 'mir + 'a> EvalContextExt<'tcx, 'mir> for super::MiriEvalCo let ptr = self.memory_mut().allocate(Size::from_bytes(size), Align::from_bytes(align, align).unwrap(), MiriMemoryKind::Rust.into())?; - self.write_scalar(Scalar::Ptr(ptr), dest)?; + self.write_scalar(Scalar::Ptr(ptr.with_default_tag()), dest)?; } "__rust_alloc_zeroed" => { let size = self.read_scalar(args[0])?.to_usize(&self)?; @@ -164,9 +164,11 @@ impl<'a, 'mir, 'tcx: 'mir + 'a> EvalContextExt<'tcx, 'mir> for super::MiriEvalCo if !align.is_power_of_two() { return err!(HeapAllocNonPowerOfTwoAlignment(align)); } - let ptr = self.memory_mut().allocate(Size::from_bytes(size), - Align::from_bytes(align, align).unwrap(), - MiriMemoryKind::Rust.into())?; + let ptr = self.memory_mut().allocate( + Size::from_bytes(size), + Align::from_bytes(align, align).unwrap(), + MiriMemoryKind::Rust.into() + )?.with_default_tag(); self.memory_mut().write_repeat(ptr.into(), 0, Size::from_bytes(size))?; self.write_scalar(Scalar::Ptr(ptr), dest)?; } @@ -205,7 +207,7 @@ impl<'a, 'mir, 'tcx: 'mir + 'a> EvalContextExt<'tcx, 'mir> for super::MiriEvalCo Align::from_bytes(align, align).unwrap(), MiriMemoryKind::Rust.into(), )?; - self.write_scalar(Scalar::Ptr(new_ptr), dest)?; + self.write_scalar(Scalar::Ptr(new_ptr.with_default_tag()), dest)?; } "syscall" => { @@ -390,7 +392,7 @@ impl<'a, 'mir, 'tcx: 'mir + 'a> EvalContextExt<'tcx, 'mir> for super::MiriEvalCo Size::from_bytes((value.len() + 1) as u64), Align::from_bytes(1, 1).unwrap(), MiriMemoryKind::Env.into(), - )?; + )?.with_default_tag(); self.memory_mut().write_bytes(value_copy.into(), &value)?; let trailing_zero_ptr = value_copy.offset(Size::from_bytes(value.len() as u64), &self)?.into(); self.memory_mut().write_bytes(trailing_zero_ptr, &[0])?; diff --git a/src/lib.rs b/src/lib.rs index 8925afa5f60..128b338f943 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -108,7 +108,7 @@ pub fn create_ecx<'a, 'mir: 'a, 'tcx: 'mir>( let mut args = ecx.frame().mir.args_iter(); // First argument: pointer to main() - let main_ptr = ecx.memory_mut().create_fn_alloc(main_instance); + let main_ptr = ecx.memory_mut().create_fn_alloc(main_instance).with_default_tag(); let dest = ecx.eval_place(&mir::Place::Local(args.next().unwrap()))?; ecx.write_scalar(Scalar::Ptr(main_ptr), dest)?; @@ -119,7 +119,7 @@ pub fn create_ecx<'a, 'mir: 'a, 'tcx: 'mir>( // FIXME: extract main source file path // Third argument (argv): &[b"foo"] let dest = ecx.eval_place(&mir::Place::Local(args.next().unwrap()))?; - let foo = ecx.memory_mut().allocate_static_bytes(b"foo\0"); + let foo = ecx.memory_mut().allocate_static_bytes(b"foo\0").with_default_tag(); let foo_ty = ecx.tcx.mk_imm_ptr(ecx.tcx.types.u8); let foo_layout = ecx.layout_of(foo_ty)?; let foo_place = ecx.allocate(foo_layout, MiriMemoryKind::Env.into())?; @@ -404,7 +404,7 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> { Ok(()) } - fn static_with_default_tag( + fn adjust_static_allocation( alloc: &'_ Allocation ) -> Cow<'_, Allocation> { let alloc: Allocation = Allocation { @@ -477,4 +477,19 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> { Ok(MemPlace { ptr, ..place }) } } + + #[inline(always)] + fn tag_new_allocation( + ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>, + ptr: Pointer, + kind: MemoryKind, + ) -> EvalResult<'tcx, Pointer> { + if !ecx.machine.validate { + // No tracking + Ok(ptr.with_default_tag()) + } else { + let tag = ecx.tag_new_allocation(ptr.alloc_id, kind); + Ok(Pointer::new_with_tag(ptr.alloc_id, ptr.offset, tag)) + } + } } diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 993287502ae..127958476bc 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -4,7 +4,7 @@ use rustc::ty::{Ty, layout::Size}; use rustc::hir; use super::{ - MemoryAccess, RangeMap, EvalResult, + MemoryAccess, MemoryKind, MiriMemoryKind, RangeMap, EvalResult, AllocId, Pointer, }; @@ -104,7 +104,7 @@ struct Stack { impl Default for Stack { fn default() -> Self { Stack { - borrows: Vec::new(), + borrows: vec![BorStackItem::Mut(Mut::Raw)], frozen_since: None, } } @@ -157,8 +157,8 @@ impl<'tcx> Stack { } } } - // Simulate a "virtual raw" element at the bottom of the stack. - acc_m.is_raw() + // Nothing to be found. + false } /// Reactive `bor` for this stack. If `force_mut` is set, we want to aggressively @@ -204,12 +204,8 @@ impl<'tcx> Stack { } } } - // Nothing to be found. Simulate a "virtual raw" element at the bottom of the stack. - if acc_m.is_raw() { - Ok(()) - } else { - err!(MachineError(format!("Borrow-to-reactivate does not exist on the stack"))) - } + // Nothing to be found. + err!(MachineError(format!("Borrow-to-reactivate does not exist on the stack"))) } /// Initiate `bor`; mostly this means freezing or pushing. @@ -301,7 +297,15 @@ impl<'tcx> Stacks { } else { // If we are creating a uniq ref, we certainly want to unfreeze. // Even if we are doing so from a raw. - // FIXME: The blog post says we should `reset` if this is a local. + // Notice that if this is a local, whenever we access it directly the + // tag here will be the bottommost `Uniq` for that local. That `Uniq` + // never is accessible by the program, so it will not be used by any + // other access. IOW, whenever we directly use a local this will pop + // everything else off the stack, invalidating all previous pointers + // and, in particular, *all* raw pointers. This subsumes the explicit + // `reset` which the blog post [1] says to perform when accessing a local. + // + // [1] https://www.ralfj.de/blog/2018/08/07/stacked-borrows.html stack.reactivate(ptr.tag, /*force_mut*/new_bor.is_uniq())?; stack.initiate(new_bor)?; } @@ -309,6 +313,19 @@ impl<'tcx> Stacks { Ok(()) } + + /// Pushes the first borrow to the stacks, must be a mutable one. + pub fn first_borrow( + &mut self, + r#mut: Mut, + size: Size + ) { + for stack in self.stacks.get_mut().iter_mut(Size::ZERO, size) { + assert!(stack.borrows.len() == 1 && stack.frozen_since.is_none()); + assert_eq!(stack.borrows.pop().unwrap(), BorStackItem::Mut(Mut::Raw)); + stack.borrows.push(BorStackItem::Mut(r#mut)); + } + } } pub trait EvalContextExt<'tcx> { @@ -334,6 +351,12 @@ pub trait EvalContextExt<'tcx> { size: Size, ref_kind: RefKind, ) -> EvalResult<'tcx, Borrow>; + + fn tag_new_allocation( + &mut self, + id: AllocId, + kind: MemoryKind, + ) -> Borrow; } impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, 'tcx> { @@ -400,7 +423,12 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' // Notably, the compiler can introduce such transmutes by optimizing away `&[mut]*`. // That can transmute a raw ptr to a (shared/mut) ref, and a mut ref to a shared one. match (ref_kind, ptr.tag) { - (RefKind::Raw, Borrow::Mut(Mut::Raw)) | + (RefKind::Raw, _) => { + // Don't use the tag, this is a raw access! Even if there is a tag, + // that means transmute happened and we ignore the tag. + // Also don't do any further validation, this is raw after all. + return Ok(Borrow::Mut(Mut::Raw)); + } (RefKind::Mut, Borrow::Mut(Mut::Uniq(_))) | (RefKind::Shr, Borrow::Frz(_)) | (RefKind::Shr, Borrow::Mut(Mut::Raw)) => { @@ -408,15 +436,12 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' // FIXME: We probably shouldn't accept this if we got a raw shr without // interior mutability. } - (_, Borrow::Mut(Mut::Raw)) => { - // Raw transmuted to (shr/mut) ref. Keep this as raw access. + (RefKind::Mut, Borrow::Mut(Mut::Raw)) => { + // Raw transmuted to mut ref. Keep this as raw access. // We cannot reborrow here; there might be a raw in `&(*var).1` where // `var` is an `&mut`. The other field of the struct might be already frozen, // also using `var`, and that would be okay. } - (RefKind::Raw, _) => { - // Someone transmuted a ref to a raw. Treat this like a ref, their fault. - } (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, @@ -451,4 +476,27 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' // All is good. Ok(ptr.tag) } + + fn tag_new_allocation( + &mut self, + id: AllocId, + kind: MemoryKind, + ) -> Borrow { + let r#mut = match kind { + MemoryKind::Stack => { + // New unique borrow + let time = self.machine.stacked_borrows.increment_clock(); + Mut::Uniq(time) + } + _ => { + // Raw for everything else + Mut::Raw + } + }; + // Make this the active borrow for this allocation + let alloc = self.memory_mut().get_mut(id).expect("This is a new allocation, it must still exist"); + let size = Size::from_bytes(alloc.bytes.len() as u64); + alloc.extra.first_borrow(r#mut, size); + Borrow::Mut(r#mut) + } } diff --git a/tests/compile-fail/copy_nonoverlapping.rs b/tests/compile-fail/copy_nonoverlapping.rs index 18fbc61b6d0..704711640f7 100644 --- a/tests/compile-fail/copy_nonoverlapping.rs +++ b/tests/compile-fail/copy_nonoverlapping.rs @@ -10,6 +10,10 @@ #![feature(core_intrinsics)] +// FIXME: Validation disabled because it doesn't let us escape an entire slice +// to the raw universe. +// compile-flags: -Zmiri-disable-validation + //error-pattern: copy_nonoverlapping called on overlapping ranges fn main() { diff --git a/tests/compile-fail/stacked_borrows/illegal_write1.rs b/tests/compile-fail/stacked_borrows/illegal_write1.rs index 86b96e2880e..ff3a3cd8224 100644 --- a/tests/compile-fail/stacked_borrows/illegal_write1.rs +++ b/tests/compile-fail/stacked_borrows/illegal_write1.rs @@ -1,11 +1,12 @@ fn evil(x: &u32) { - let x : &mut u32 = unsafe { &mut *(x as *const _ as *mut _) }; - *x = 42; // mutating shared ref without `UnsafeCell` + // mutating shared ref without `UnsafeCell` + let x : *mut u32 = x as *const _ as *mut _; + unsafe { *x = 42; } } fn main() { - let target = 42; - let ref_ = ⌖ - evil(ref_); // invalidates shared ref + let target = Box::new(42); // has an implicit raw + let ref_ = &*target; + evil(ref_); // invalidates shared ref, activates raw let _x = *ref_; //~ ERROR Shr reference with non-reactivatable tag Frz } diff --git a/tests/compile-fail/stacked_borrows/illegal_write2.rs b/tests/compile-fail/stacked_borrows/illegal_write2.rs index 22648ba7118..f4fefaad5e2 100644 --- a/tests/compile-fail/stacked_borrows/illegal_write2.rs +++ b/tests/compile-fail/stacked_borrows/illegal_write2.rs @@ -4,7 +4,7 @@ fn main() { let target = &mut 42; let target2 = target as *mut _; drop(&mut *target); // reborrow - // Now make sure our ref is still the only one - unsafe { *target2 = 13; } // invalidate our ref - let _val = *target; //~ ERROR Mut reference with non-reactivatable tag Mut(Uniq + // Now make sure our ref is still the only one. + unsafe { *target2 = 13; } //~ ERROR does not exist on the stack + let _val = *target; } diff --git a/tests/compile-fail/stacked_borrows/illegal_write3.rs b/tests/compile-fail/stacked_borrows/illegal_write3.rs index 26c30a4122b..a653aa5003f 100644 --- a/tests/compile-fail/stacked_borrows/illegal_write3.rs +++ b/tests/compile-fail/stacked_borrows/illegal_write3.rs @@ -3,6 +3,6 @@ fn main() { // Make sure raw ptr with raw tag cannot mutate frozen location without breaking the shared ref. let r#ref = ⌖ // freeze let ptr = r#ref as *const _ as *mut _; // raw ptr, with raw tag - unsafe { *ptr = 42; } - let _val = *r#ref; //~ ERROR Shr reference with non-reactivatable tag Frz + unsafe { *ptr = 42; } //~ ERROR does not exist on the stack + let _val = *r#ref; } diff --git a/tests/compile-fail/stacked_borrows/illegal_write4.rs b/tests/compile-fail/stacked_borrows/illegal_write4.rs index b2ffac865bd..2006d7262f8 100644 --- a/tests/compile-fail/stacked_borrows/illegal_write4.rs +++ b/tests/compile-fail/stacked_borrows/illegal_write4.rs @@ -26,6 +26,6 @@ fn main() { let _val = *r#ref; // Make sure it is still frozen. // We only actually unfreeze once we muteate through the bad pointer. - unsafe { *bad_ptr = 42 }; - let _val = *r#ref; //~ ERROR Shr reference with non-reactivatable tag Frz + unsafe { *bad_ptr = 42 }; //~ ERROR does not exist on the stack + let _val = *r#ref; } diff --git a/tests/compile-fail/stacked_borrows/outdated_local.rs b/tests/compile-fail/stacked_borrows/outdated_local.rs new file mode 100644 index 00000000000..64a8ff69108 --- /dev/null +++ b/tests/compile-fail/stacked_borrows/outdated_local.rs @@ -0,0 +1,9 @@ +fn main() { + let mut x = 0; + let y: *const i32 = &x; + x = 1; // this invalidates y by reactivating the lowermost uniq borrow for this local + + assert_eq!(unsafe { *y }, 1); //~ ERROR does not exist on the stack + + assert_eq!(x, 1); +} diff --git a/tests/compile-fail/stacked_borrows/pointer_smuggling.rs b/tests/compile-fail/stacked_borrows/pointer_smuggling.rs index 678b9b21ba8..68f3d2923b1 100644 --- a/tests/compile-fail/stacked_borrows/pointer_smuggling.rs +++ b/tests/compile-fail/stacked_borrows/pointer_smuggling.rs @@ -10,7 +10,7 @@ fn fun1(x: &mut u8) { fn fun2() { // Now we use a pointer we are not allowed to use - let _x = unsafe { *PTR }; + let _x = unsafe { *PTR }; //~ ERROR does not exist on the stack } fn main() { @@ -18,5 +18,4 @@ fn main() { fun1(val); *val = 2; // this invalidates any raw ptrs `fun1` might have created. fun2(); // if they now use a raw ptr they break our reference - *val = 3; //~ ERROR Mut reference with non-reactivatable tag Mut(Uniq } diff --git a/tests/compile-fail/stacked_borrows/unescaped_local.rs b/tests/compile-fail/stacked_borrows/unescaped_local.rs new file mode 100644 index 00000000000..8627cc44c2e --- /dev/null +++ b/tests/compile-fail/stacked_borrows/unescaped_local.rs @@ -0,0 +1,10 @@ +use std::mem; + +// Make sure we cannot use raw ptrs to access a local that +// has never been escaped to the raw world. +fn main() { + let mut x = 42; + let ptr = &mut x; + let raw: *mut i32 = unsafe { mem::transmute(ptr) }; + unsafe { *raw = 13; } //~ ERROR does not exist on the stack +} diff --git a/tests/compile-fail/zst.rs b/tests/compile-fail/zst.rs index 0f4c945c85b..544d65a1bff 100644 --- a/tests/compile-fail/zst.rs +++ b/tests/compile-fail/zst.rs @@ -1,4 +1,4 @@ fn main() { let x = &() as *const () as *const i32; - let _ = unsafe { *x }; //~ ERROR outside bounds of allocation + let _ = unsafe { *x }; //~ ERROR access memory with alignment 1, but alignment 4 is required } diff --git a/tests/run-pass/observed_local_mut.rs b/tests/run-pass/observed_local_mut.rs index a4ecf1e635d..c58e8c84bb2 100644 --- a/tests/run-pass/observed_local_mut.rs +++ b/tests/run-pass/observed_local_mut.rs @@ -1,3 +1,6 @@ +// Validation catches this (correctly) as UB. +// compile-flags: -Zmiri-disable-validation + // This test is intended to guard against the problem described in commit // 39bb1254d1eaf74f45a4e741097e33fc942168d5. // diff --git a/tests/run-pass/ptr_arith_offset_overflow.rs b/tests/run-pass/ptr_arith_offset_overflow.rs index 3383c3b8014..9eabc914260 100644 --- a/tests/run-pass/ptr_arith_offset_overflow.rs +++ b/tests/run-pass/ptr_arith_offset_overflow.rs @@ -1,6 +1,7 @@ fn main() { let v = [1i16, 2]; let x = &v[1] as *const i16; + let _ = &v[0] as *const i16; // we need to leak the 2nd thing too or it cannot be accessed through a raw ptr // Adding 2*isize::max and then 1 is like substracting 1 let x = x.wrapping_offset(isize::max_value()); let x = x.wrapping_offset(isize::max_value());