From 0c704151f7c365ab8cd84e994b54b1bb424647cd Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 27 May 2019 20:04:37 +0200 Subject: [PATCH 1/4] use new rustc infrastructure to tag the base pointer of static allocations --- src/intrinsic.rs | 5 +-- src/lib.rs | 73 +++++++++++++++++++++--------------------- src/stacked_borrows.rs | 65 +++++++++++++++++++++---------------- 3 files changed, 76 insertions(+), 67 deletions(-) diff --git a/src/intrinsic.rs b/src/intrinsic.rs index a17f576b43..d46d185c5f 100644 --- a/src/intrinsic.rs +++ b/src/intrinsic.rs @@ -5,7 +5,7 @@ use rustc::ty; use crate::{ PlaceTy, OpTy, ImmTy, Immediate, Scalar, ScalarMaybeUndef, Tag, - OperatorEvalContextExt + OperatorEvalContextExt, MiriMemoryKind, }; impl<'a, 'mir, 'tcx> EvalContextExt<'a, 'mir, 'tcx> for crate::MiriEvalContext<'a, 'mir, 'tcx> {} @@ -401,7 +401,8 @@ pub trait EvalContextExt<'a, 'mir, 'tcx: 'a+'mir>: crate::MiriEvalContextExt<'a, "type_name" => { let ty = substs.type_at(0); let ty_name = ty.to_string(); - let value = this.str_to_immediate(&ty_name)?; + let ptr = this.memory_mut().allocate_static_bytes(ty_name.as_bytes(), MiriMemoryKind::Static.into()); + let value = Immediate::new_slice(Scalar::Ptr(ptr), ty_name.len() as u64, this); this.write_immediate(value, dest)?; } diff --git a/src/lib.rs b/src/lib.rs index 89d7964913..bf0c6cd387 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -30,7 +30,7 @@ use rand::SeedableRng; use rustc::ty::{self, TyCtxt, query::TyCtxtAt}; use rustc::ty::layout::{LayoutOf, Size, Align}; -use rustc::hir::{self, def_id::DefId}; +use rustc::hir::def_id::DefId; use rustc::mir; pub use rustc_mir::interpret::*; // Resolve ambiguity. @@ -113,7 +113,7 @@ pub fn create_ecx<'a, 'mir: 'a, 'tcx: 'mir>( // Return value (in static memory so that it does not count as leak). let ret = ecx.layout_of(start_mir.return_ty())?; - let ret_ptr = ecx.allocate(ret, MiriMemoryKind::MutStatic.into()); + let ret_ptr = ecx.allocate(ret, MiriMemoryKind::Static.into()); // Push our stack frame. ecx.push_stack_frame( @@ -128,7 +128,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).with_default_tag(); + let main_ptr = ecx.memory_mut().create_fn_alloc(main_instance); let dest = ecx.eval_place(&mir::Place::Base(mir::PlaceBase::Local(args.next().unwrap())))?; ecx.write_scalar(Scalar::Ptr(main_ptr), dest)?; @@ -162,7 +162,7 @@ pub fn create_ecx<'a, 'mir: 'a, 'tcx: 'mir>( // Add `0` terminator. let mut arg = arg.into_bytes(); arg.push(0); - argvs.push(ecx.memory_mut().allocate_static_bytes(arg.as_slice()).with_default_tag()); + argvs.push(ecx.memory_mut().allocate_static_bytes(arg.as_slice(), MiriMemoryKind::Static.into())); } // Make an array with all these pointers, in the Miri memory. let argvs_layout = ecx.layout_of(ecx.tcx.mk_array(ecx.tcx.mk_imm_ptr(ecx.tcx.types.u8), argvs.len() as u64))?; @@ -299,8 +299,8 @@ pub enum MiriMemoryKind { C, /// Part of env var emulation. Env, - /// Mutable statics. - MutStatic, + /// Statics. + Static, } impl Into> for MiriMemoryKind { @@ -316,7 +316,7 @@ impl MayLeak for MiriMemoryKind { use self::MiriMemoryKind::*; match self { Rust | C => false, - Env | MutStatic => true, + Env | Static => true, } } } @@ -392,7 +392,7 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> { type MemoryMap = MonoHashMap, Allocation)>; - const STATIC_KIND: Option = Some(MiriMemoryKind::MutStatic); + const STATIC_KIND: Option = Some(MiriMemoryKind::Static); #[inline(always)] fn enforce_validity(ecx: &InterpretCx<'a, 'mir, 'tcx, Self>) -> bool { @@ -476,8 +476,7 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> { fn find_foreign_static( def_id: DefId, tcx: TyCtxtAt<'a, 'tcx, 'tcx>, - memory_extra: &Self::MemoryExtra, - ) -> EvalResult<'tcx, Cow<'tcx, Allocation>> { + ) -> EvalResult<'tcx, Cow<'tcx, Allocation>> { let attrs = tcx.get_attrs(def_id); let link_name = match attr::first_attr_value_str_by_name(&attrs, sym::link_name) { Some(name) => name.as_str(), @@ -489,8 +488,7 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> { // This should be all-zero, pointer-sized. let size = tcx.data_layout.pointer_size; let data = vec![0; size.bytes() as usize]; - let extra = Stacks::new(size, Tag::default(), Rc::clone(memory_extra)); - Allocation::from_bytes(&data, tcx.data_layout.pointer_align.abi, extra) + Allocation::from_bytes(&data, tcx.data_layout.pointer_align.abi) } _ => return err!(Unimplemented( format!("can't access foreign static: {}", link_name), @@ -506,47 +504,48 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> { Ok(()) } - fn adjust_static_allocation<'b>( - alloc: &'b Allocation, + fn tag_allocation<'b>( + id: AllocId, + alloc: Cow<'b, Allocation>, + kind: Option>, memory_extra: &Self::MemoryExtra, - ) -> Cow<'b, Allocation> { - let extra = Stacks::new( + ) -> (Cow<'b, Allocation>, Self::PointerTag) { + let kind = kind.expect("we set our STATIC_KIND so this cannot be None"); + let alloc = alloc.into_owned(); + let (extra, base_tag) = Stacks::new_allocation( + id, Size::from_bytes(alloc.bytes.len() as u64), - Tag::default(), Rc::clone(memory_extra), + kind, ); + if kind != MiriMemoryKind::Static.into() { + assert!(alloc.relocations.is_empty(), "Only statics can come initialized with inner pointers"); + // Now we can rely on the inner pointers being static, too. + } + let mut memory_extra = memory_extra.borrow_mut(); let alloc: Allocation = Allocation { - bytes: alloc.bytes.clone(), + bytes: alloc.bytes, relocations: Relocations::from_presorted( alloc.relocations.iter() - .map(|&(offset, ((), alloc))| (offset, (Tag::default(), alloc))) + // The allocations in the relocations (pointers stored *inside* this allocation) + // all get the base pointer tag. + .map(|&(offset, ((), alloc))| (offset, (memory_extra.static_base_ptr(alloc), alloc))) .collect() ), - undef_mask: alloc.undef_mask.clone(), + undef_mask: alloc.undef_mask, align: alloc.align, mutability: alloc.mutability, extra, }; - Cow::Owned(alloc) - } - - #[inline(always)] - fn new_allocation( - size: Size, - extra: &Self::MemoryExtra, - kind: MemoryKind, - ) -> (Self::AllocExtra, Self::PointerTag) { - Stacks::new_allocation(size, extra, kind) + (Cow::Owned(alloc), base_tag) } #[inline(always)] - fn tag_dereference( - _ecx: &InterpretCx<'a, 'mir, 'tcx, Self>, - place: MPlaceTy<'tcx, Tag>, - _mutability: Option, - ) -> EvalResult<'tcx, Scalar> { - // Nothing happens. - Ok(place.ptr) + fn tag_static_base_pointer( + id: AllocId, + memory_extra: &Self::MemoryExtra, + ) -> Self::PointerTag { + memory_extra.borrow_mut().static_base_ptr(id) } #[inline(always)] diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 8c46f7e1b2..71779a6132 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -1,5 +1,5 @@ use std::cell::RefCell; -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::rc::Rc; use std::fmt; use std::num::NonZeroU64; @@ -10,7 +10,7 @@ use rustc::mir::RetagKind; use crate::{ EvalResult, InterpError, MiriEvalContext, HelpersEvalContextExt, Evaluator, MutValueVisitor, - MemoryKind, MiriMemoryKind, RangeMap, Allocation, AllocationExtra, CheckInAllocMsg, + MemoryKind, MiriMemoryKind, RangeMap, Allocation, AllocationExtra, AllocId, CheckInAllocMsg, Pointer, Immediate, ImmTy, PlaceTy, MPlaceTy, }; @@ -92,10 +92,18 @@ pub struct Stacks { /// Extra global state, available to the memory access hooks. #[derive(Debug)] pub struct GlobalState { + /// Next unused pointer ID (tag). next_ptr_id: PtrId, + /// Table storing the "base" tag for each allocation. + /// The base tag is the one used for the initial pointer. + /// We need this in a separate table to handle cyclic statics. + base_ptr_ids: HashMap, + /// Next unused call ID (for protectors). next_call_id: CallId, + /// Those call IDs corresponding to functions that are still running. active_calls: HashSet, } +/// Memory extra state gives us interior mutable access to the global state. pub type MemoryState = Rc>; /// Indicates which kind of access is being performed. @@ -144,6 +152,7 @@ impl Default for GlobalState { fn default() -> Self { GlobalState { next_ptr_id: NonZeroU64::new(1).unwrap(), + base_ptr_ids: HashMap::default(), next_call_id: NonZeroU64::new(1).unwrap(), active_calls: HashSet::default(), } @@ -151,7 +160,7 @@ impl Default for GlobalState { } impl GlobalState { - pub fn new_ptr(&mut self) -> PtrId { + fn new_ptr(&mut self) -> PtrId { let id = self.next_ptr_id; self.next_ptr_id = NonZeroU64::new(id.get() + 1).unwrap(); id @@ -172,6 +181,15 @@ impl GlobalState { fn is_active(&self, id: CallId) -> bool { self.active_calls.contains(&id) } + + pub fn static_base_ptr(&mut self, id: AllocId) -> Tag { + self.base_ptr_ids.get(&id).copied().unwrap_or_else(|| { + let tag = Tag::Tagged(self.new_ptr()); + trace!("New allocation {:?} has base tag {:?}", id, tag); + self.base_ptr_ids.insert(id, tag); + tag + }) + } } // # Stacked Borrows Core Begin @@ -190,14 +208,6 @@ impl GlobalState { /// F3: If an access happens with an `&` outside `UnsafeCell`, /// it requires the `SharedReadOnly` to still be in the stack. -impl Default for Tag { - #[inline(always)] - fn default() -> Tag { - Tag::Untagged - } -} - - /// Core relation on `Permission` to define which accesses are allowed impl Permission { /// This defines for a given permission, whether it permits the given kind of access. @@ -431,12 +441,13 @@ impl<'tcx> Stack { /// Map per-stack operations to higher-level per-location-range operations. impl<'tcx> Stacks { /// Creates new stack with initial tag. - pub(crate) fn new( + fn new( size: Size, + perm: Permission, tag: Tag, extra: MemoryState, ) -> Self { - let item = Item { perm: Permission::Unique, tag, protector: None }; + let item = Item { perm, tag, protector: None }; let stack = Stack { borrows: vec![item], }; @@ -465,27 +476,25 @@ impl<'tcx> Stacks { /// Glue code to connect with Miri Machine Hooks impl Stacks { pub fn new_allocation( + id: AllocId, size: Size, - extra: &MemoryState, + extra: MemoryState, kind: MemoryKind, ) -> (Self, Tag) { - let tag = match kind { - MemoryKind::Stack => { - // New unique borrow. This `Uniq` is not accessible by the program, + let (tag, perm) = match kind { + MemoryKind::Stack => + // New unique borrow. This tag is not accessible by the program, // so it will only ever be used when using the local directly (i.e., - // not through a pointer). That is, whenever we directly use a local, this will pop + // not through a pointer). That is, whenever we directly write to 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]: - Tag::Tagged(extra.borrow_mut().new_ptr()) - } - _ => { - Tag::Untagged - } + // and in particular, *all* raw pointers. + (Tag::Tagged(extra.borrow_mut().new_ptr()), Permission::Unique), + MemoryKind::Machine(MiriMemoryKind::Static) => + (extra.borrow_mut().static_base_ptr(id), Permission::SharedReadWrite), + _ => + (Tag::Untagged, Permission::SharedReadWrite), }; - let stack = Stacks::new(size, tag, Rc::clone(extra)); + let stack = Stacks::new(size, perm, tag, extra); (stack, tag) } } From e03255d62554424805a1959a0cfdad13171429a1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 28 May 2019 18:28:15 +0200 Subject: [PATCH 2/4] fix existing tests fix thread-local example to no longer write to pointers derived from a shared ref; fix compile-fail test --- tests/compile-fail/modifying_constants.rs | 2 ++ tests/run-pass/thread-local.rs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/compile-fail/modifying_constants.rs b/tests/compile-fail/modifying_constants.rs index 27c74e8dc8..4546e8a4d7 100644 --- a/tests/compile-fail/modifying_constants.rs +++ b/tests/compile-fail/modifying_constants.rs @@ -1,3 +1,5 @@ +// This should fail even without validation +// compile-flags: -Zmiri-disable-validation fn main() { let x = &1; // the `&1` is promoted to a constant, but it used to be that only the pointer is marked static, not the pointee diff --git a/tests/run-pass/thread-local.rs b/tests/run-pass/thread-local.rs index aeedb7034c..8de45811be 100644 --- a/tests/run-pass/thread-local.rs +++ b/tests/run-pass/thread-local.rs @@ -56,7 +56,7 @@ fn main() { create(None); // check that the no-dtor case works // Initialize the keys we use to check destructor ordering - for (key, global) in KEYS.iter_mut().zip(GLOBALS.iter()) { + for (key, global) in KEYS.iter_mut().zip(GLOBALS.iter_mut()) { *key = create(Some(mem::transmute(dtor as unsafe extern fn(*mut u64)))); set(*key, global as *const _ as *mut _); } From 9f48b3029ca51997945ae4170f1e2b0ebb23a7a2 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 28 May 2019 19:26:50 +0200 Subject: [PATCH 3/4] test that we cannot access unescaped static memory with a raw ptr --- tests/compile-fail/stacked_borrows/unescaped_static.rs | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 tests/compile-fail/stacked_borrows/unescaped_static.rs diff --git a/tests/compile-fail/stacked_borrows/unescaped_static.rs b/tests/compile-fail/stacked_borrows/unescaped_static.rs new file mode 100644 index 0000000000..0f0467fc5c --- /dev/null +++ b/tests/compile-fail/stacked_borrows/unescaped_static.rs @@ -0,0 +1,7 @@ +static ARRAY: [u8; 2] = [0, 1]; + +fn main() { + let ptr_to_first = &ARRAY[0] as *const u8; + // Illegally use this to access the 2nd element. + let _val = unsafe { *ptr_to_first.add(1) }; //~ ERROR borrow stack +} From b231a7ec9efa58496786c4eed623e150659f370d Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 2 Jun 2019 22:16:18 +0200 Subject: [PATCH 4/4] bump Rust version --- rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust-version b/rust-version index 60d0ba51f7..7604934a98 100644 --- a/rust-version +++ b/rust-version @@ -1 +1 @@ -8b40a188cee5bef97526dfc271afbd2a98008183 +627486af15d222bcba336b12ea92a05237cc9ab1