From 01ac19d3585ef90d9aeb70aab501c4fbda931fca Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 7 Feb 2017 19:20:16 +0100 Subject: [PATCH 1/4] 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 e0434c48e51..30bd5beba25 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 @@ fn description(&self) -> &str { "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 d372f37698d..fbad94b67ae 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 @@ pub(super) fn str_to_value(&mut self, s: &str) -> EvalResult<'tcx, Value> { // 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 @@ pub(super) fn const_to_value(&mut self, const_val: &ConstVal) -> EvalResult<'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 @@ pub(super) fn pop_stack_frame(&mut self) -> EvalResult<'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 @@ pub(super) fn pop_stack_frame(&mut self) -> EvalResult<'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 @@ pub(super) fn force_allocation( _ => { 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 3b7356cf06f..8c222321532 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 @@ pub fn allocate(&mut self, size: u64, align: u64) -> EvalResult<'tcx, Pointer> { 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 @@ pub fn reallocate(&mut self, ptr: Pointer, new_size: u64, align: u64) -> EvalRes 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 @@ pub fn deallocate(&mut self, ptr: Pointer) -> EvalResult<'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 @@ pub fn get(&self, id: AllocId) -> EvalResult<'tcx, &Allocation> { 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 @@ pub fn dump_allocs(&self, mut allocs: Vec) { } } - 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 @@ fn get_bytes_mut(&mut self, ptr: Pointer, size: u64, align: u64) -> EvalResult<' /// 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 @@ pub fn freeze(&mut self, alloc_id: AllocId) -> EvalResult<'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 33237b84637..0200e458014 100644 --- a/src/step.rs +++ b/src/step.rs @@ -156,11 +156,7 @@ fn global_item(&mut self, def_id: DefId, substs: &'tcx subst::Substs<'tcx>, span 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 @@ fn visit_constant(&mut self, constant: &mir::Constant<'tcx>, location: mir::Loca mir, this.substs, Lvalue::Global(cid), - StackPopCleanup::Freeze, + StackPopCleanup::MarkStatic(false), Vec::new()) }); } diff --git a/src/vtable.rs b/src/vtable.rs index fb9ec7124cc..0b43b349443 100644 --- a/src/vtable.rs +++ b/src/vtable.rs @@ -112,7 +112,7 @@ pub fn get_vtable(&mut self, trait_ref: ty::PolyTraitRef<'tcx>) -> EvalResult<'t } } - 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 00000000000..0338a766e26 --- /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 00000000000..be5830698b2 --- /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); + } +} From 98cda6cb07f09a910c493d6c16d8cb47234bf39a Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 7 Feb 2017 20:28:54 +0100 Subject: [PATCH 2/4] freeze -> static --- src/error.rs | 4 ++-- src/eval_context.rs | 10 ++++------ src/memory.rs | 6 +++--- tests/compile-fail/modifying_constants.rs | 2 +- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/error.rs b/src/error.rs index 30bd5beba25..f806110190c 100644 --- a/src/error.rs +++ b/src/error.rs @@ -119,9 +119,9 @@ fn description(&self) -> &str { EvalError::TypeNotPrimitive(_) => "expected primitive type, got nonprimitive", EvalError::ReallocatedStaticMemory => - "tried to reallocate frozen memory", + "tried to reallocate static memory", EvalError::DeallocatedStaticMemory => - "tried to deallocate frozen memory", + "tried to deallocate static memory", EvalError::Layout(_) => "rustc layout computation failed", EvalError::UnterminatedCString(_) => diff --git a/src/eval_context.rs b/src/eval_context.rs index ac6fffd1e0d..1996bbf9dcb 100644 --- a/src/eval_context.rs +++ b/src/eval_context.rs @@ -314,7 +314,7 @@ pub(super) fn pop_stack_frame(&mut self) -> EvalResult<'tcx> { match frame.return_to_block { 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/static)"); + .expect("global should have been cached (static)"); match global_value.value { Value::ByRef(ptr) => self.memory.mark_static(ptr.alloc_id, mutable)?, Value::ByVal(val) => if let PrimVal::Ptr(ptr) = val { @@ -332,7 +332,7 @@ pub(super) fn pop_stack_frame(&mut self) -> EvalResult<'tcx> { assert!(global_value.mutable); global_value.mutable = mutable; } else { - bug!("StackPopCleanup::Freeze on: {:?}", frame.return_lvalue); + bug!("StackPopCleanup::MarkStatic on: {:?}", frame.return_lvalue); }, StackPopCleanup::Goto(target) => self.goto_block(target), StackPopCleanup::None => {}, @@ -343,10 +343,8 @@ pub(super) fn pop_stack_frame(&mut self) -> EvalResult<'tcx> { trace!("deallocating local"); self.memory.dump_alloc(ptr.alloc_id); match self.memory.deallocate(ptr) { - // Any frozen memory means that it belongs to a constant or something referenced - // 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. + // We could alternatively check whether the alloc_id is static before calling + // deallocate, but this is much simpler and is probably the rare case. Ok(()) | Err(EvalError::DeallocatedStaticMemory) => {}, other => return other, } diff --git a/src/memory.rs b/src/memory.rs index 8c222321532..0b5f90f3fc8 100644 --- a/src/memory.rs +++ b/src/memory.rs @@ -39,7 +39,7 @@ pub struct Allocation { pub align: u64, /// Whether the allocation may be modified. /// Use the `mark_static` method of `Memory` to ensure that an error occurs, if the memory of this - /// allocation is modified in the future. + /// allocation is modified or deallocated in the future. pub static_kind: StaticKind, } @@ -626,7 +626,7 @@ fn get_bytes_mut(&mut self, ptr: Pointer, size: u64, align: u64) -> EvalResult<' impl<'a, 'tcx> Memory<'a, '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 + // do not use `self.get_mut(alloc_id)` here, because we might have already marked a // sub-element or have circular pointers (e.g. `Rc`-cycles) let relocations = match self.alloc_map.get_mut(&alloc_id) { Some(&mut Allocation { ref mut relocations, static_kind: ref mut kind @ StaticKind::NotStatic, .. }) => { @@ -636,7 +636,7 @@ pub fn mark_static(&mut self, alloc_id: AllocId, mutable: bool) -> EvalResult<'t StaticKind::Immutable }; // take out the relocations vector to free the borrow on self, so we can call - // freeze recursively + // mark recursively mem::replace(relocations, Default::default()) }, None if alloc_id == NEVER_ALLOC_ID || alloc_id == ZST_ALLOC_ID => return Ok(()), diff --git a/tests/compile-fail/modifying_constants.rs b/tests/compile-fail/modifying_constants.rs index 5a8fd189aae..cb2e7217d57 100644 --- a/tests/compile-fail/modifying_constants.rs +++ b/tests/compile-fail/modifying_constants.rs @@ -1,5 +1,5 @@ fn main() { - let x = &1; // the `&1` is promoted to a constant, but it used to be that only the pointer is frozen, not the pointee + 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 let y = unsafe { &mut *(x as *const i32 as *mut i32) }; *y = 42; //~ ERROR tried to modify constant memory assert_eq!(*x, 42); From aaba692effbcaff0acc6a0700f66818b648cdf47 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 7 Feb 2017 20:38:23 +0100 Subject: [PATCH 3/4] add regression test for #120 --- tests/run-pass/recursive_static.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 tests/run-pass/recursive_static.rs diff --git a/tests/run-pass/recursive_static.rs b/tests/run-pass/recursive_static.rs new file mode 100644 index 00000000000..5b27324964b --- /dev/null +++ b/tests/run-pass/recursive_static.rs @@ -0,0 +1,11 @@ +#![feature(static_recursion)] + +struct S(&'static S); +static S1: S = S(&S2); +static S2: S = S(&S1); + +fn main() { + let p: *const S = S2.0; + let q: *const S = &S1; + assert_eq!(p, q); +} From fbfd2d4bca94ba2d19b89724d8450cbff52a0484 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 8 Feb 2017 09:17:48 +0100 Subject: [PATCH 4/4] re-add spaces before static kind --- src/memory.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/memory.rs b/src/memory.rs index 0b5f90f3fc8..8f0c3a5622c 100644 --- a/src/memory.rs +++ b/src/memory.rs @@ -528,8 +528,8 @@ pub fn dump_allocs(&self, mut allocs: Vec) { } let immutable = match alloc.static_kind { - StaticKind::Mutable => "(static mut)", - StaticKind::Immutable => "(immutable)", + StaticKind::Mutable => " (static mut)", + StaticKind::Immutable => " (immutable)", StaticKind::NotStatic => "", }; trace!("{}({} bytes){}", msg, alloc.bytes.len(), immutable);