From 01ac19d3585ef90d9aeb70aab501c4fbda931fca Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 7 Feb 2017 19:20:16 +0100 Subject: [PATCH] fix `static mut` accidental dealloc or freeze --- src/error.rs | 8 ++--- src/eval_context.rs | 32 +++++++++--------- src/memory.rs | 52 +++++++++++++++++++++--------- src/step.rs | 8 ++--- src/vtable.rs | 2 +- tests/run-pass/const-vec-of-fns.rs | 29 +++++++++++++++++ tests/run-pass/static_mut.rs | 17 ++++++++++ 7 files changed, 106 insertions(+), 42 deletions(-) create mode 100644 tests/run-pass/const-vec-of-fns.rs create mode 100644 tests/run-pass/static_mut.rs diff --git a/src/error.rs b/src/error.rs index e0434c48e5..30bd5beba2 100644 --- a/src/error.rs +++ b/src/error.rs @@ -49,8 +49,8 @@ pub enum EvalError<'tcx> { AssumptionNotHeld, InlineAsm, TypeNotPrimitive(Ty<'tcx>), - ReallocatedFrozenMemory, - DeallocatedFrozenMemory, + ReallocatedStaticMemory, + DeallocatedStaticMemory, Layout(layout::LayoutError<'tcx>), Unreachable, ExpectedConcreteFunction(Function<'tcx>), @@ -118,9 +118,9 @@ impl<'tcx> Error for EvalError<'tcx> { "cannot evaluate inline assembly", EvalError::TypeNotPrimitive(_) => "expected primitive type, got nonprimitive", - EvalError::ReallocatedFrozenMemory => + EvalError::ReallocatedStaticMemory => "tried to reallocate frozen memory", - EvalError::DeallocatedFrozenMemory => + EvalError::DeallocatedStaticMemory => "tried to deallocate frozen memory", EvalError::Layout(_) => "rustc layout computation failed", diff --git a/src/eval_context.rs b/src/eval_context.rs index d372f37698..fbad94b67a 100644 --- a/src/eval_context.rs +++ b/src/eval_context.rs @@ -100,9 +100,11 @@ pub struct Frame<'tcx> { #[derive(Clone, Debug, Eq, PartialEq, Hash)] pub enum StackPopCleanup { /// The stackframe existed to compute the initial value of a static/constant, make sure it - /// isn't modifyable afterwards. The allocation of the result is frozen iff it's an - /// actual allocation. `PrimVal`s are unmodifyable anyway. - Freeze, + /// isn't modifyable afterwards in case of constants. + /// In case of `static mut`, mark the memory to ensure it's never marked as immutable through + /// references or deallocated + /// The bool decides whether the value is mutable (true) or not (false) + MarkStatic(bool), /// A regular stackframe added due to a function call will need to get forwarded to the next /// block Goto(mir::BasicBlock), @@ -170,7 +172,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { // FIXME: cache these allocs let ptr = self.memory.allocate(s.len() as u64, 1)?; self.memory.write_bytes(ptr, s.as_bytes())?; - self.memory.freeze(ptr.alloc_id)?; + self.memory.mark_static(ptr.alloc_id, false)?; Ok(Value::ByValPair(PrimVal::Ptr(ptr), PrimVal::from_u128(s.len() as u128))) } @@ -194,7 +196,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { ByteStr(ref bs) => { let ptr = self.memory.allocate(bs.len() as u64, 1)?; self.memory.write_bytes(ptr, bs)?; - self.memory.freeze(ptr.alloc_id)?; + self.memory.mark_static(ptr.alloc_id, false)?; PrimVal::Ptr(ptr) } @@ -310,25 +312,25 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { ::log_settings::settings().indentation -= 1; let frame = self.stack.pop().expect("tried to pop a stack frame, but there were none"); match frame.return_to_block { - StackPopCleanup::Freeze => if let Lvalue::Global(id) = frame.return_lvalue { + StackPopCleanup::MarkStatic(mutable) => if let Lvalue::Global(id) = frame.return_lvalue { let global_value = self.globals.get_mut(&id) - .expect("global should have been cached (freeze)"); + .expect("global should have been cached (freeze/static)"); match global_value.value { - Value::ByRef(ptr) => self.memory.freeze(ptr.alloc_id)?, + Value::ByRef(ptr) => self.memory.mark_static(ptr.alloc_id, mutable)?, Value::ByVal(val) => if let PrimVal::Ptr(ptr) = val { - self.memory.freeze(ptr.alloc_id)?; + self.memory.mark_static(ptr.alloc_id, mutable)?; }, Value::ByValPair(val1, val2) => { if let PrimVal::Ptr(ptr) = val1 { - self.memory.freeze(ptr.alloc_id)?; + self.memory.mark_static(ptr.alloc_id, mutable)?; } if let PrimVal::Ptr(ptr) = val2 { - self.memory.freeze(ptr.alloc_id)?; + self.memory.mark_static(ptr.alloc_id, mutable)?; } }, } assert!(global_value.mutable); - global_value.mutable = false; + global_value.mutable = mutable; } else { bug!("StackPopCleanup::Freeze on: {:?}", frame.return_lvalue); }, @@ -345,7 +347,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { // by a constant. We could alternatively check whether the alloc_id is frozen // before calling deallocate, but this is much simpler and is probably the // rare case. - Ok(()) | Err(EvalError::DeallocatedFrozenMemory) => {}, + Ok(()) | Err(EvalError::DeallocatedStaticMemory) => {}, other => return other, } } @@ -868,9 +870,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { _ => { let ptr = self.alloc_ptr_with_substs(global_val.ty, cid.substs)?; self.write_value_to_ptr(global_val.value, ptr, global_val.ty)?; - if !global_val.mutable { - self.memory.freeze(ptr.alloc_id)?; - } + self.memory.mark_static(ptr.alloc_id, global_val.mutable)?; let lval = self.globals.get_mut(&cid).expect("already checked"); *lval = Global { value: Value::ByRef(ptr), diff --git a/src/memory.rs b/src/memory.rs index 3b7356cf06..8c22232153 100644 --- a/src/memory.rs +++ b/src/memory.rs @@ -38,9 +38,19 @@ pub struct Allocation { /// The alignment of the allocation to detect unaligned reads. pub align: u64, /// Whether the allocation may be modified. - /// Use the `freeze` method of `Memory` to ensure that an error occurs, if the memory of this + /// Use the `mark_static` method of `Memory` to ensure that an error occurs, if the memory of this /// allocation is modified in the future. - pub immutable: bool, + pub static_kind: StaticKind, +} + +#[derive(Debug, PartialEq, Copy, Clone)] +pub enum StaticKind { + /// may be deallocated without breaking miri's invariants + NotStatic, + /// may be modified, but never deallocated + Mutable, + /// may neither be modified nor deallocated + Immutable, } #[derive(Copy, Clone, Debug, Eq, PartialEq)] @@ -272,7 +282,7 @@ impl<'a, 'tcx> Memory<'a, 'tcx> { relocations: BTreeMap::new(), undef_mask: UndefMask::new(size), align, - immutable: false, + static_kind: StaticKind::NotStatic, }; let id = self.next_id; self.next_id.0 += 1; @@ -290,8 +300,8 @@ impl<'a, 'tcx> Memory<'a, 'tcx> { if ptr.points_to_zst() { return self.allocate(new_size, align); } - if self.get(ptr.alloc_id).map(|alloc| alloc.immutable).ok() == Some(true) { - return Err(EvalError::ReallocatedFrozenMemory); + if self.get(ptr.alloc_id).ok().map_or(false, |alloc| alloc.static_kind != StaticKind::NotStatic) { + return Err(EvalError::ReallocatedStaticMemory); } let size = self.get(ptr.alloc_id)?.bytes.len() as u64; @@ -325,8 +335,8 @@ impl<'a, 'tcx> Memory<'a, 'tcx> { // TODO(solson): Report error about non-__rust_allocate'd pointer. return Err(EvalError::Unimplemented(format!("bad pointer offset: {}", ptr.offset))); } - if self.get(ptr.alloc_id).map(|alloc| alloc.immutable).ok() == Some(true) { - return Err(EvalError::DeallocatedFrozenMemory); + if self.get(ptr.alloc_id).ok().map_or(false, |alloc| alloc.static_kind != StaticKind::NotStatic) { + return Err(EvalError::DeallocatedStaticMemory); } if let Some(alloc) = self.alloc_map.remove(&ptr.alloc_id) { @@ -430,8 +440,11 @@ impl<'a, 'tcx> Memory<'a, 'tcx> { pub fn get_mut(&mut self, id: AllocId) -> EvalResult<'tcx, &mut Allocation> { match self.alloc_map.get_mut(&id) { - Some(ref alloc) if alloc.immutable => Err(EvalError::ModifiedConstantMemory), - Some(alloc) => Ok(alloc), + Some(alloc) => match alloc.static_kind { + StaticKind::Mutable | + StaticKind::NotStatic => Ok(alloc), + StaticKind::Immutable => Err(EvalError::ModifiedConstantMemory), + }, None => match self.functions.get(&id) { Some(_) => Err(EvalError::DerefFunctionPointer), None if id == NEVER_ALLOC_ID || id == ZST_ALLOC_ID => Err(EvalError::InvalidMemoryAccess), @@ -514,7 +527,11 @@ impl<'a, 'tcx> Memory<'a, 'tcx> { } } - let immutable = if alloc.immutable { " (immutable)" } else { "" }; + let immutable = match alloc.static_kind { + StaticKind::Mutable => "(static mut)", + StaticKind::Immutable => "(immutable)", + StaticKind::NotStatic => "", + }; trace!("{}({} bytes){}", msg, alloc.bytes.len(), immutable); if !relocations.is_empty() { @@ -607,15 +624,20 @@ impl<'a, 'tcx> Memory<'a, 'tcx> { /// Reading and writing impl<'a, 'tcx> Memory<'a, 'tcx> { - pub fn freeze(&mut self, alloc_id: AllocId) -> EvalResult<'tcx> { + /// mark an allocation as static, either mutable or not + pub fn mark_static(&mut self, alloc_id: AllocId, mutable: bool) -> EvalResult<'tcx> { // do not use `self.get_mut(alloc_id)` here, because we might have already frozen a // sub-element or have circular pointers (e.g. `Rc`-cycles) let relocations = match self.alloc_map.get_mut(&alloc_id) { - Some(ref mut alloc) if !alloc.immutable => { - alloc.immutable = true; + Some(&mut Allocation { ref mut relocations, static_kind: ref mut kind @ StaticKind::NotStatic, .. }) => { + *kind = if mutable { + StaticKind::Mutable + } else { + StaticKind::Immutable + }; // take out the relocations vector to free the borrow on self, so we can call // freeze recursively - mem::replace(&mut alloc.relocations, Default::default()) + mem::replace(relocations, Default::default()) }, None if alloc_id == NEVER_ALLOC_ID || alloc_id == ZST_ALLOC_ID => return Ok(()), None if !self.functions.contains_key(&alloc_id) => return Err(EvalError::DanglingPointerDeref), @@ -623,7 +645,7 @@ impl<'a, 'tcx> Memory<'a, 'tcx> { }; // recurse into inner allocations for &alloc in relocations.values() { - self.freeze(alloc)?; + self.mark_static(alloc, mutable)?; } // put back the relocations self.alloc_map.get_mut(&alloc_id).expect("checked above").relocations = relocations; diff --git a/src/step.rs b/src/step.rs index 33237b8463..0200e45801 100644 --- a/src/step.rs +++ b/src/step.rs @@ -156,11 +156,7 @@ impl<'a, 'b, 'tcx> ConstantExtractor<'a, 'b, 'tcx> { self.try(|this| { let mir = this.ecx.load_mir(def_id)?; this.ecx.globals.insert(cid, Global::uninitialized(mir.return_ty)); - let cleanup = if immutable && !mir.return_ty.type_contents(this.ecx.tcx).interior_unsafe() { - StackPopCleanup::Freeze - } else { - StackPopCleanup::None - }; + let cleanup = StackPopCleanup::MarkStatic(!immutable || mir.return_ty.type_contents(this.ecx.tcx).interior_unsafe()); this.ecx.push_stack_frame(def_id, span, mir, substs, Lvalue::Global(cid), cleanup, Vec::new()) }); } @@ -210,7 +206,7 @@ impl<'a, 'b, 'tcx> Visitor<'tcx> for ConstantExtractor<'a, 'b, 'tcx> { mir, this.substs, Lvalue::Global(cid), - StackPopCleanup::Freeze, + StackPopCleanup::MarkStatic(false), Vec::new()) }); } diff --git a/src/vtable.rs b/src/vtable.rs index fb9ec7124c..0b43b34944 100644 --- a/src/vtable.rs +++ b/src/vtable.rs @@ -112,7 +112,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { } } - self.memory.freeze(vtable.alloc_id)?; + self.memory.mark_static(vtable.alloc_id, false)?; Ok(vtable) } diff --git a/tests/run-pass/const-vec-of-fns.rs b/tests/run-pass/const-vec-of-fns.rs new file mode 100644 index 0000000000..0338a766e2 --- /dev/null +++ b/tests/run-pass/const-vec-of-fns.rs @@ -0,0 +1,29 @@ +// Copyright 2013 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// pretty-expanded FIXME #23616 + +/*! + * Try to double-check that static fns have the right size (with or + * without dummy env ptr, as appropriate) by iterating a size-2 array. + * If the static size differs from the runtime size, the second element + * should be read as a null or otherwise wrong pointer and crash. + */ + +fn f() { } +static mut CLOSURES: &'static mut [fn()] = &mut [f as fn(), f as fn()]; + +pub fn main() { + unsafe { + for closure in &mut *CLOSURES { + (*closure)() + } + } +} diff --git a/tests/run-pass/static_mut.rs b/tests/run-pass/static_mut.rs new file mode 100644 index 0000000000..be5830698b --- /dev/null +++ b/tests/run-pass/static_mut.rs @@ -0,0 +1,17 @@ +#![allow(dead_code)] + +static mut FOO: i32 = 42; +static BAR: Foo = Foo(unsafe { &FOO as *const _} ); + +struct Foo(*const i32); + +unsafe impl Sync for Foo {} + +fn main() { + unsafe { + assert_eq!(*BAR.0, 42); + FOO = 5; + assert_eq!(FOO, 5); + assert_eq!(*BAR.0, 5); + } +}