From 51ff9fdaf684c89c12ac5bf41980a53eed44ee2d Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Thu, 17 Nov 2016 14:48:34 +0100 Subject: [PATCH 1/3] deallocate all locals on function exit and transitively freeze constants through pointers --- src/error.rs | 8 ++++- src/interpreter/mod.rs | 43 +++++++++++++++++++++-- src/interpreter/step.rs | 5 +-- src/interpreter/terminator/mod.rs | 7 ++-- src/memory.rs | 23 +++++++++++- tests/compile-fail/modifying_constants.rs | 6 ++++ tests/run-pass/move-arg-3-unique.rs | 18 ++++++++++ 7 files changed, 101 insertions(+), 9 deletions(-) create mode 100644 tests/compile-fail/modifying_constants.rs create mode 100644 tests/run-pass/move-arg-3-unique.rs diff --git a/src/error.rs b/src/error.rs index 481815996a0e..0fd35ff65652 100644 --- a/src/error.rs +++ b/src/error.rs @@ -7,7 +7,7 @@ use memory::Pointer; use rustc_const_math::ConstMathErr; use syntax::codemap::Span; -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq, Eq)] pub enum EvalError<'tcx> { FunctionPointerTyMismatch(Abi, &'tcx FnSig<'tcx>, &'tcx BareFnTy<'tcx>), NoMirFor(String), @@ -48,6 +48,8 @@ pub enum EvalError<'tcx> { AssumptionNotHeld, InlineAsm, TypeNotPrimitive(Ty<'tcx>), + ReallocatedFrozenMemory, + DeallocatedFrozenMemory, } pub type EvalResult<'tcx, T> = Result>; @@ -110,6 +112,10 @@ impl<'tcx> Error for EvalError<'tcx> { "cannot evaluate inline assembly", EvalError::TypeNotPrimitive(_) => "expected primitive type, got nonprimitive", + EvalError::ReallocatedFrozenMemory => + "tried to reallocate frozen memory", + EvalError::DeallocatedFrozenMemory => + "tried to deallocate frozen memory", } } diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index ee503f97d78e..dbc6d7bfb834 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -364,6 +364,20 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { let global_value = self.globals .get_mut(&id) .expect("global should have been cached (freeze)"); + match global_value.data.expect("global should have been initialized") { + Value::ByRef(ptr) => self.memory.freeze(ptr.alloc_id)?, + Value::ByVal(val) => if let Some(alloc_id) = val.relocation { + self.memory.freeze(alloc_id)?; + }, + Value::ByValPair(a, b) => { + if let Some(alloc_id) = a.relocation { + self.memory.freeze(alloc_id)?; + } + if let Some(alloc_id) = b.relocation { + self.memory.freeze(alloc_id)?; + } + }, + } if let Value::ByRef(ptr) = global_value.data.expect("global should have been initialized") { self.memory.freeze(ptr.alloc_id)?; } @@ -375,7 +389,17 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { StackPopCleanup::Goto(target) => self.goto_block(target), StackPopCleanup::None => {}, } - // TODO(solson): Deallocate local variables. + // check that all locals have been deallocated through StorageDead + for (i, local) in frame.locals.into_iter().enumerate() { + if let Some(Value::ByRef(ptr)) = local { + trace!("deallocating local {}: {:?}", i + 1, ptr); + self.memory.dump(ptr.alloc_id); + match self.memory.deallocate(ptr) { + Ok(()) | Err(EvalError::DeallocatedFrozenMemory) => {}, + other => return other, + } + } + } Ok(()) } @@ -729,6 +753,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { // Skip the initial 0 intended for LLVM GEP. for field_index in path { let field_offset = self.get_field_offset(ty, field_index)?; + trace!("field_path_offset_and_ty: {}, {}, {:?}, {:?}", field_index, ty, field_offset, offset); ty = self.get_field_ty(ty, field_index)?; offset = offset.checked_add(field_offset, &self.tcx.data_layout).unwrap(); } @@ -1595,8 +1620,20 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { ) -> EvalResult<'tcx, ()> { let val = self.stack[frame].get_local(local); let val = f(self, val)?; - // can't use `set_local` here, because that's only meant for going to an initialized value - self.stack[frame].locals[local.index() - 1] = val; + if let Some(val) = val { + self.stack[frame].set_local(local, val); + } else { + self.deallocate_local(frame, local)?; + } + Ok(()) + } + + pub fn deallocate_local(&mut self, frame: usize, local: mir::Local) -> EvalResult<'tcx, ()> { + if let Some(Value::ByRef(ptr)) = self.stack[frame].get_local(local) { + self.memory.deallocate(ptr)?; + } + // Subtract 1 because we don't store a value for the ReturnPointer, the local with index 0. + self.stack[frame].locals[local.index() - 1] = None; Ok(()) } } diff --git a/src/interpreter/step.rs b/src/interpreter/step.rs index 4c03b3c42aa1..90564cd2fcc9 100644 --- a/src/interpreter/step.rs +++ b/src/interpreter/step.rs @@ -81,8 +81,9 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { Assign(ref lvalue, ref rvalue) => self.eval_rvalue_into_lvalue(rvalue, lvalue)?, SetDiscriminant { .. } => unimplemented!(), - // Miri can safely ignore these. Only translation needs them. - StorageLive(_) | StorageDead(_) => {} + // Miri can safely ignore these. Only translation needs it. + StorageLive(_) | + StorageDead(_) => {} // Defined to do nothing. These are added by optimization passes, to avoid changing the // size of MIR constantly. diff --git a/src/interpreter/terminator/mod.rs b/src/interpreter/terminator/mod.rs index a80ac216aa23..b0c24fc344f2 100644 --- a/src/interpreter/terminator/mod.rs +++ b/src/interpreter/terminator/mod.rs @@ -144,7 +144,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { Ok(()) } - fn eval_drop_impls(&mut self, drops: Vec<(DefId, Value, &'tcx Substs<'tcx>)>) -> EvalResult<'tcx, ()> { + pub fn eval_drop_impls(&mut self, drops: Vec<(DefId, Value, &'tcx Substs<'tcx>)>) -> EvalResult<'tcx, ()> { let span = self.frame().span; // add them to the stack in reverse order, because the impl that needs to run the last // is the one that needs to be at the bottom of the stack @@ -249,6 +249,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { fn read_discriminant_value(&self, adt_ptr: Pointer, adt_ty: Ty<'tcx>) -> EvalResult<'tcx, u64> { use rustc::ty::layout::Layout::*; let adt_layout = self.type_layout(adt_ty); + trace!("read_discriminant_value {:?}", adt_layout); let discr_val = match *adt_layout { General { discr, .. } | CEnum { discr, signed: false, .. } => { @@ -263,12 +264,14 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { RawNullablePointer { nndiscr, value } => { let discr_size = value.size(&self.tcx.data_layout).bytes() as usize; + trace!("rawnullablepointer with size {}", discr_size); self.read_nonnull_discriminant_value(adt_ptr, nndiscr, discr_size)? } StructWrappedNullablePointer { nndiscr, ref discrfield, .. } => { let (offset, ty) = self.nonnull_offset_and_ty(adt_ty, nndiscr, discrfield)?; let nonnull = adt_ptr.offset(offset.bytes() as isize); + trace!("struct wrapped nullable pointer type: {}", ty); // only the pointer part of a fat pointer is used for this space optimization let discr_size = self.type_size(ty).expect("bad StructWrappedNullablePointer discrfield"); self.read_nonnull_discriminant_value(nonnull, nndiscr, discr_size)? @@ -515,7 +518,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { } /// push DefIds of drop impls and their argument on the given vector - fn drop( + pub fn drop( &mut self, lval: Lvalue<'tcx>, ty: Ty<'tcx>, diff --git a/src/memory.rs b/src/memory.rs index 6670ed66cf02..4421fe23ebab 100644 --- a/src/memory.rs +++ b/src/memory.rs @@ -212,6 +212,9 @@ 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(true) { + return Err(EvalError::ReallocatedFrozenMemory); + } let size = self.get(ptr.alloc_id)?.bytes.len(); @@ -242,6 +245,9 @@ 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(true) { + return Err(EvalError::DeallocatedFrozenMemory); + } if let Some(alloc) = self.alloc_map.remove(&ptr.alloc_id) { self.memory_usage -= alloc.bytes.len(); @@ -446,9 +452,24 @@ 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, ()> { + // FIXME: the comment is wrong and do we really still need this check? // It's not possible to freeze the zero-sized allocation, because it doesn't exist. if alloc_id != ZST_ALLOC_ID { - self.get_mut(alloc_id)?.immutable = true; + // 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 allocs: Vec<_> = match self.alloc_map.get_mut(&alloc_id) { + Some(ref mut alloc) if !alloc.immutable => { + alloc.immutable = true; + alloc.relocations.values().cloned().collect() + }, + None if alloc_id == NEVER_ALLOC_ID || alloc_id == ZST_ALLOC_ID => Vec::new(), + None if !self.functions.contains_key(&alloc_id) => return Err(EvalError::DanglingPointerDeref), + _ => Vec::new(), + }; + // recurse into inner allocations + for alloc in allocs { + self.freeze(alloc)?; + } } Ok(()) } diff --git a/tests/compile-fail/modifying_constants.rs b/tests/compile-fail/modifying_constants.rs new file mode 100644 index 000000000000..5a8fd189aae7 --- /dev/null +++ b/tests/compile-fail/modifying_constants.rs @@ -0,0 +1,6 @@ +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 y = unsafe { &mut *(x as *const i32 as *mut i32) }; + *y = 42; //~ ERROR tried to modify constant memory + assert_eq!(*x, 42); +} diff --git a/tests/run-pass/move-arg-3-unique.rs b/tests/run-pass/move-arg-3-unique.rs new file mode 100644 index 000000000000..2e6320eb8025 --- /dev/null +++ b/tests/run-pass/move-arg-3-unique.rs @@ -0,0 +1,18 @@ +// Copyright 2012 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. + +#![allow(unused_features, unused_variables)] +#![feature(box_syntax)] + +pub fn main() { + let x = box 10; + let y = x; + assert_eq!(*y, 10); +} From 11a0594a1d94aa6ce39692e9dc46059a0d7c4d08 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Fri, 18 Nov 2016 10:35:41 +0100 Subject: [PATCH 2/3] address comments --- src/interpreter/mod.rs | 6 +++++- src/memory.rs | 38 +++++++++++++++++++------------------- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index dbc6d7bfb834..8f3d06f8301f 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -389,12 +389,16 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { StackPopCleanup::Goto(target) => self.goto_block(target), StackPopCleanup::None => {}, } - // check that all locals have been deallocated through StorageDead + // deallocate all locals that are backed by an allocation for (i, local) in frame.locals.into_iter().enumerate() { if let Some(Value::ByRef(ptr)) = local { trace!("deallocating local {}: {:?}", i + 1, ptr); self.memory.dump(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. Ok(()) | Err(EvalError::DeallocatedFrozenMemory) => {}, other => return other, } diff --git a/src/memory.rs b/src/memory.rs index 4421fe23ebab..960e397fd04d 100644 --- a/src/memory.rs +++ b/src/memory.rs @@ -1,7 +1,7 @@ use byteorder::{ReadBytesExt, WriteBytesExt, LittleEndian, BigEndian, self}; use std::collections::Bound::{Included, Excluded}; use std::collections::{btree_map, BTreeMap, HashMap, HashSet, VecDeque}; -use std::{fmt, iter, ptr}; +use std::{fmt, iter, ptr, mem}; use rustc::hir::def_id::DefId; use rustc::ty::{self, BareFnTy, ClosureTy, ClosureSubsts, TyCtxt}; @@ -452,25 +452,25 @@ 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, ()> { - // FIXME: the comment is wrong and do we really still need this check? - // It's not possible to freeze the zero-sized allocation, because it doesn't exist. - if alloc_id != ZST_ALLOC_ID { - // 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 allocs: Vec<_> = match self.alloc_map.get_mut(&alloc_id) { - Some(ref mut alloc) if !alloc.immutable => { - alloc.immutable = true; - alloc.relocations.values().cloned().collect() - }, - None if alloc_id == NEVER_ALLOC_ID || alloc_id == ZST_ALLOC_ID => Vec::new(), - None if !self.functions.contains_key(&alloc_id) => return Err(EvalError::DanglingPointerDeref), - _ => Vec::new(), - }; - // recurse into inner allocations - for alloc in allocs { - self.freeze(alloc)?; - } + // 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; + // take out the relocations vector to free the borrow on self, so we can call + // freeze recursively + mem::replace(&mut alloc.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), + _ => return Ok(()), + }; + // recurse into inner allocations + for &alloc in relocations.values() { + self.freeze(alloc)?; } + // put back the relocations + self.alloc_map.get_mut(&alloc_id).expect("checked above").relocations = relocations; Ok(()) } From fd6a90860c97442df45101ee15ca4637b5ab963f Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Fri, 18 Nov 2016 10:36:01 +0100 Subject: [PATCH 3/3] simplify dumping of pointers to the zst or never alloc --- src/memory.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/memory.rs b/src/memory.rs index 960e397fd04d..548eb78c9678 100644 --- a/src/memory.rs +++ b/src/memory.rs @@ -339,11 +339,8 @@ impl<'a, 'tcx> Memory<'a, 'tcx> { while let Some(id) = allocs_to_print.pop_front() { allocs_seen.insert(id); + if id == ZST_ALLOC_ID || id == NEVER_ALLOC_ID { continue; } let mut msg = format!("Alloc {:<5} ", format!("{}:", id)); - if id == ZST_ALLOC_ID { - trace!("{} zst allocation", msg); - continue; - } let prefix_len = msg.len(); let mut relocations = vec![]; @@ -385,7 +382,12 @@ impl<'a, 'tcx> Memory<'a, 'tcx> { let relocation_width = (self.pointer_size() - 1) * 3; for (i, target_id) in relocations { write!(msg, "{:1$}", "", (i - pos) * 3).unwrap(); - write!(msg, "└{0:─^1$}┘ ", format!("({})", target_id), relocation_width).unwrap(); + let target = match target_id { + ZST_ALLOC_ID => String::from("zst"), + NEVER_ALLOC_ID => String::from("int ptr"), + _ => format!("({})", target_id), + }; + write!(msg, "└{0:─^1$}┘ ", target, relocation_width).unwrap(); pos = i + self.pointer_size(); } trace!("{}", msg);