From 4ba2b82f3151c620edf442228de307fe71278555 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 8 Aug 2017 13:06:14 +0200 Subject: [PATCH] Split the alloc id address space into functions and normal allocs instead of interleaving them as before. The next step is to also separate out static memory into its own address space. --- src/librustc_mir/interpret/error.rs | 2 +- src/librustc_mir/interpret/memory.rs | 144 ++++++++++++++++++--------- tests/compile-fail/fn_ptr_offset.rs | 2 +- 3 files changed, 99 insertions(+), 49 deletions(-) diff --git a/src/librustc_mir/interpret/error.rs b/src/librustc_mir/interpret/error.rs index 7cfff2d9810b..96911c10cca8 100644 --- a/src/librustc_mir/interpret/error.rs +++ b/src/librustc_mir/interpret/error.rs @@ -139,7 +139,7 @@ impl<'tcx> Error for EvalError<'tcx> { DoubleFree => "tried to deallocate dangling pointer", InvalidFunctionPointer => - "tried to use an integer pointer or a dangling pointer as a function pointer", + "tried to use a function pointer after offsetting it", InvalidBool => "invalid boolean value read", InvalidDiscriminant => diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 88fd254a2f25..c476046d3854 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -82,12 +82,55 @@ impl LockInfo { // Allocations and pointers //////////////////////////////////////////////////////////////////////////////// -#[derive(Copy, Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] -pub struct AllocId(pub u64); +#[derive(Copy, Clone, Eq, Hash, Ord, PartialEq, PartialOrd)] +pub struct AllocId(u64); + +#[derive(Debug)] +enum AllocIdKind { + /// We can't ever have more than `usize::max_value` functions at the same time + /// since we never "deallocate" functions + Function(usize), + /// Locals and heap allocations (also statics for now, but those will get their + /// own variant soonish). + Runtime(u64), +} + +impl AllocIdKind { + fn into_alloc_id(self) -> AllocId { + match self { + AllocIdKind::Function(n) => AllocId(n as u64), + AllocIdKind::Runtime(n) => AllocId((1 << 63) | n), + } + } +} + +impl AllocId { + /// Currently yields the top bit to discriminate the `AllocIdKind`s + fn discriminant(self) -> u64 { + self.0 >> 63 + } + /// Yields everything but the discriminant bits + fn index(self) -> u64 { + self.0 & ((1 << 63) - 1) + } + fn destructure(self) -> AllocIdKind { + match self.discriminant() { + 0 => AllocIdKind::Function(self.index() as usize), + 1 => AllocIdKind::Runtime(self.index()), + n => bug!("got discriminant {} for AllocId", n), + } + } +} impl fmt::Display for AllocId { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}", self.0) + write!(f, "{:?}", self.destructure()) + } +} + +impl fmt::Debug for AllocId { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{:?}", self.destructure()) } } @@ -186,10 +229,10 @@ pub struct Memory<'a, 'tcx, M: Machine<'tcx>> { pub data: M::MemoryData, /// Actual memory allocations (arbitrary bytes, may contain pointers into other allocations). - alloc_map: HashMap>, + alloc_map: HashMap>, - /// The AllocId to assign to the next new allocation. Always incremented, never gets smaller. - next_id: AllocId, + /// The AllocId to assign to the next new regular allocation. Always incremented, never gets smaller. + next_alloc_id: u64, /// Set of statics, constants, promoteds, vtables, ... to prevent `mark_static_initalized` from /// stepping out of its own allocations. This set only contains statics backed by an @@ -205,7 +248,7 @@ pub struct Memory<'a, 'tcx, M: Machine<'tcx>> { /// Function "allocations". They exist solely so pointers have something to point to, and /// we can figure out what they point to. - functions: HashMap>, + functions: Vec>, /// Inverse map of `functions` so we don't allocate a new pointer every time we need one function_alloc_cache: HashMap, AllocId>, @@ -231,9 +274,9 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> { Memory { data, alloc_map: HashMap::new(), - functions: HashMap::new(), + functions: Vec::new(), function_alloc_cache: HashMap::new(), - next_id: AllocId(0), + next_alloc_id: 0, layout, memory_size: max_memory, memory_usage: 0, @@ -245,20 +288,20 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> { } } - pub fn allocations(&self) -> ::std::collections::hash_map::Iter> { - self.alloc_map.iter() + pub fn allocations<'x>(&'x self) -> impl Iterator)> { + self.alloc_map.iter().map(|(&id, alloc)| (AllocIdKind::Runtime(id).into_alloc_id(), alloc)) } pub fn create_fn_alloc(&mut self, instance: ty::Instance<'tcx>) -> MemoryPointer { if let Some(&alloc_id) = self.function_alloc_cache.get(&instance) { return MemoryPointer::new(alloc_id, 0); } - let id = self.next_id; + let id = self.functions.len(); debug!("creating fn ptr: {}", id); - self.next_id.0 += 1; - self.functions.insert(id, instance); - self.function_alloc_cache.insert(instance, id); - MemoryPointer::new(id, 0) + self.functions.push(instance); + let alloc_id = AllocIdKind::Function(id).into_alloc_id(); + self.function_alloc_cache.insert(instance, alloc_id); + MemoryPointer::new(alloc_id, 0) } pub fn allocate_cached(&mut self, bytes: &[u8]) -> EvalResult<'tcx, MemoryPointer> { @@ -300,10 +343,10 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> { mutable: Mutability::Mutable, locks: RangeMap::new(), }; - let id = self.next_id; - self.next_id.0 += 1; + let id = self.next_alloc_id; + self.next_alloc_id += 1; self.alloc_map.insert(id, alloc); - Ok(MemoryPointer::new(id, 0)) + Ok(MemoryPointer::new(AllocIdKind::Runtime(id).into_alloc_id(), 0)) } pub fn reallocate( @@ -344,7 +387,13 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> { return err!(DeallocateNonBasePtr); } - let alloc = match self.alloc_map.remove(&ptr.alloc_id) { + let alloc_id = match ptr.alloc_id.destructure() { + AllocIdKind::Function(_) => + return err!(DeallocatedWrongMemoryKind("function".to_string(), format!("{:?}", kind))), + AllocIdKind::Runtime(id) => id, + }; + + let alloc = match self.alloc_map.remove(&alloc_id) { Some(alloc) => alloc, None => return err!(DoubleFree), }; @@ -624,22 +673,22 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> { /// Allocation accessors impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> { pub fn get(&self, id: AllocId) -> EvalResult<'tcx, &Allocation> { - match self.alloc_map.get(&id) { - Some(alloc) => Ok(alloc), - None => match self.functions.get(&id) { - Some(_) => err!(DerefFunctionPointer), + match id.destructure() { + AllocIdKind::Function(_) => err!(DerefFunctionPointer), + AllocIdKind::Runtime(id) => match self.alloc_map.get(&id) { + Some(alloc) => Ok(alloc), None => err!(DanglingPointerDeref), - } + }, } } fn get_mut_unchecked(&mut self, id: AllocId) -> EvalResult<'tcx, &mut Allocation> { - match self.alloc_map.get_mut(&id) { - Some(alloc) => Ok(alloc), - None => match self.functions.get(&id) { - Some(_) => err!(DerefFunctionPointer), + match id.destructure() { + AllocIdKind::Function(_) => err!(DerefFunctionPointer), + AllocIdKind::Runtime(id) => match self.alloc_map.get_mut(&id) { + Some(alloc) => Ok(alloc), None => err!(DanglingPointerDeref), - } + }, } } @@ -657,12 +706,9 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> { return err!(InvalidFunctionPointer); } debug!("reading fn ptr: {}", ptr.alloc_id); - match self.functions.get(&ptr.alloc_id) { - Some(&fndef) => Ok(fndef), - None => match self.alloc_map.get(&ptr.alloc_id) { - Some(_) => err!(ExecuteMemory), - None => err!(InvalidFunctionPointer), - } + match ptr.alloc_id.destructure() { + AllocIdKind::Function(id) => Ok(self.functions[id]), + AllocIdKind::Runtime(_) => err!(ExecuteMemory), } } @@ -684,17 +730,18 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> { let prefix_len = msg.len(); let mut relocations = vec![]; - let alloc = match (self.alloc_map.get(&id), self.functions.get(&id)) { - (Some(a), None) => a, - (None, Some(instance)) => { - trace!("{} {}", msg, instance); + let alloc = match id.destructure() { + AllocIdKind::Function(id) => { + trace!("{} {}", msg, self.functions[id]); continue; }, - (None, None) => { - trace!("{} (deallocated)", msg); - continue; + AllocIdKind::Runtime(id) => match self.alloc_map.get(&id) { + Some(a) => a, + None => { + trace!("{} (deallocated)", msg); + continue; + } }, - (Some(_), Some(_)) => bug!("miri invariant broken: an allocation id exists that points to both a function and a memory location"), }; for i in 0..(alloc.bytes.len() as u64) { @@ -745,7 +792,7 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> { .iter() .filter_map(|(&key, val)| { if val.kind != Kind::Static { - Some(key) + Some(AllocIdKind::Runtime(key).into_alloc_id()) } else { None } @@ -834,6 +881,10 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> { trace!("mark_static_initalized {:?}, mutability: {:?}", alloc_id, mutability); // 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 alloc_id = match alloc_id.destructure() { + AllocIdKind::Function(_) => return Ok(()), + AllocIdKind::Runtime(id) => id, + }; let relocations = match self.alloc_map.get_mut(&alloc_id) { Some(&mut Allocation { ref mut relocations, ref mut kind, ref mut mutable, .. }) => { match *kind { @@ -854,8 +905,7 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> { // mark recursively mem::replace(relocations, Default::default()) }, - None if !self.functions.contains_key(&alloc_id) => return err!(DanglingPointerDeref), - _ => return Ok(()), + None => return err!(DanglingPointerDeref), }; // recurse into inner allocations for &alloc in relocations.values() { diff --git a/tests/compile-fail/fn_ptr_offset.rs b/tests/compile-fail/fn_ptr_offset.rs index 3e4c5d6ad391..45e32142a8c4 100644 --- a/tests/compile-fail/fn_ptr_offset.rs +++ b/tests/compile-fail/fn_ptr_offset.rs @@ -10,5 +10,5 @@ fn main() { let y : *mut u8 = unsafe { mem::transmute(x) }; let y = y.wrapping_offset(1); let x : fn() = unsafe { mem::transmute(y) }; - x(); //~ ERROR: tried to use an integer pointer or a dangling pointer as a function pointer + x(); //~ ERROR: tried to use a function pointer after offsetting it }