From 440c4778fa81779bd39dd99edbc7bd42a02c7335 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 3 Jul 2017 16:47:58 -0700 Subject: [PATCH] validate size and alignment on reallocate and deallocate --- src/eval_context.rs | 4 +- src/memory.rs | 37 ++++++++++++------- src/terminator/mod.rs | 14 +++---- .../compile-fail/deallocate-bad-alignment.rs | 13 +++++++ tests/compile-fail/deallocate-bad-size.rs | 13 +++++++ .../compile-fail/reallocate-bad-alignment.rs | 13 +++++++ tests/compile-fail/reallocate-bad-size.rs | 13 +++++++ 7 files changed, 84 insertions(+), 23 deletions(-) create mode 100644 tests/compile-fail/deallocate-bad-alignment.rs create mode 100644 tests/compile-fail/deallocate-bad-size.rs create mode 100644 tests/compile-fail/reallocate-bad-alignment.rs create mode 100644 tests/compile-fail/reallocate-bad-size.rs diff --git a/src/eval_context.rs b/src/eval_context.rs index 630f878f7232..b101a3967a72 100644 --- a/src/eval_context.rs +++ b/src/eval_context.rs @@ -410,7 +410,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { if let Some(Value::ByRef(ptr)) = local { trace!("deallocating local"); self.memory.dump_alloc(ptr.alloc_id); - match self.memory.deallocate(ptr) { + match self.memory.deallocate(ptr, None) { // 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) => {}, @@ -1724,7 +1724,7 @@ pub fn eval_main<'a, 'tcx: 'a>( while ecx.step()? {} if let Some(cleanup_ptr) = cleanup_ptr { - ecx.memory.deallocate(cleanup_ptr)?; + ecx.memory.deallocate(cleanup_ptr, None)?; } return Ok(()); } diff --git a/src/memory.rs b/src/memory.rs index 75d612d599c4..bd38b737a40f 100644 --- a/src/memory.rs +++ b/src/memory.rs @@ -223,10 +223,10 @@ impl<'a, 'tcx> Memory<'a, 'tcx> { // TODO(solson): Track which allocations were returned from __rust_allocate and report an error // when reallocating/deallocating any others. - pub fn reallocate(&mut self, ptr: Pointer, new_size: u64, align: u64) -> EvalResult<'tcx, Pointer> { + pub fn reallocate(&mut self, ptr: Pointer, old_size: u64, new_size: u64, align: u64) -> EvalResult<'tcx, Pointer> { assert!(align.is_power_of_two()); // TODO(solson): Report error about non-__rust_allocate'd pointer. - if ptr.offset != 0 { + if ptr.offset != 0 || self.get(ptr.alloc_id).is_err() { return Err(EvalError::ReallocateNonBasePtr); } if self.get(ptr.alloc_id).ok().map_or(false, |alloc| alloc.static_kind != StaticKind::NotStatic) { @@ -234,12 +234,15 @@ impl<'a, 'tcx> Memory<'a, 'tcx> { } let size = self.get(ptr.alloc_id)?.bytes.len() as u64; + let real_align = self.get(ptr.alloc_id)?.align; + if size != old_size || real_align != align { + return Err(EvalError::IncorrectAllocationInformation); + } if new_size > size { let amount = new_size - size; self.memory_usage += amount; let alloc = self.get_mut(ptr.alloc_id)?; - // FIXME: check alignment here assert_eq!(amount as usize as u64, amount); alloc.bytes.extend(iter::repeat(0).take(amount as usize)); alloc.undef_mask.grow(amount, false); @@ -247,7 +250,6 @@ impl<'a, 'tcx> Memory<'a, 'tcx> { self.memory_usage -= size - new_size; self.clear_relocations(ptr.offset(new_size, self.layout)?, size - new_size)?; let alloc = self.get_mut(ptr.alloc_id)?; - // FIXME: check alignment here // `as usize` is fine here, since it is smaller than `size`, which came from a usize alloc.bytes.truncate(new_size as usize); alloc.bytes.shrink_to_fit(); @@ -267,21 +269,28 @@ impl<'a, 'tcx> Memory<'a, 'tcx> { } // TODO(solson): See comment on `reallocate`. - pub fn deallocate(&mut self, ptr: Pointer) -> EvalResult<'tcx> { - if ptr.offset != 0 { + pub fn deallocate(&mut self, ptr: Pointer, size_and_align: Option<(u64, u64)>) -> EvalResult<'tcx> { + if ptr.offset != 0 || self.get(ptr.alloc_id).is_err() { // TODO(solson): Report error about non-__rust_allocate'd pointer. return Err(EvalError::DeallocateNonBasePtr); } - if self.get(ptr.alloc_id).ok().map_or(false, |alloc| alloc.static_kind != StaticKind::NotStatic) { - return Err(EvalError::DeallocatedStaticMemory); + + { + // Some code somewhere seems to rely on us *not* removing the allocation when we yield this kind of error. + // So we do this test in advance. + let alloc = self.get(ptr.alloc_id)?; + if alloc.static_kind != StaticKind::NotStatic { + return Err(EvalError::DeallocatedStaticMemory); + } + if let Some((size, align)) = size_and_align { + if size != alloc.bytes.len() as u64 || align != alloc.align { + return Err(EvalError::IncorrectAllocationInformation); + } + } } - if let Some(alloc) = self.alloc_map.remove(&ptr.alloc_id) { - self.memory_usage -= alloc.bytes.len() as u64; - } else { - debug!("deallocated a pointer twice: {}", ptr.alloc_id); - return Err(EvalError::DeallocateNonBasePtr); - } + let alloc = self.alloc_map.remove(&ptr.alloc_id).expect("already verified"); + self.memory_usage -= alloc.bytes.len() as u64; debug!("deallocated : {}", ptr.alloc_id); Ok(()) diff --git a/src/terminator/mod.rs b/src/terminator/mod.rs index 68d03b2aaffa..4bcf7470ba89 100644 --- a/src/terminator/mod.rs +++ b/src/terminator/mod.rs @@ -589,7 +589,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { "free" => { let ptr = args[0].read_ptr(&self.memory)?; if !ptr.is_null()? { - self.memory.deallocate(ptr.to_ptr()?)?; + self.memory.deallocate(ptr.to_ptr()?, None)?; } } @@ -638,7 +638,6 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { "__rust_deallocate" => { let ptr = args[0].read_ptr(&self.memory)?.to_ptr()?; - // FIXME: insert sanity check for size and align? let old_size = self.value_to_primval(args[1], usize)?.to_u64()?; let align = self.value_to_primval(args[2], usize)?.to_u64()?; if old_size == 0 { @@ -647,20 +646,21 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { if !align.is_power_of_two() { return Err(EvalError::HeapAllocNonPowerOfTwoAlignment(align)); } - self.memory.deallocate(ptr)?; + self.memory.deallocate(ptr, Some((old_size, align)))?; }, "__rust_reallocate" => { let ptr = args[0].read_ptr(&self.memory)?.to_ptr()?; + let old_size = self.value_to_primval(args[1], usize)?.to_u64()?; let size = self.value_to_primval(args[2], usize)?.to_u64()?; let align = self.value_to_primval(args[3], usize)?.to_u64()?; - if size == 0 { + if old_size == 0 || size == 0 { return Err(EvalError::HeapAllocZeroBytes); } if !align.is_power_of_two() { return Err(EvalError::HeapAllocNonPowerOfTwoAlignment(align)); } - let new_ptr = self.memory.reallocate(ptr, size, align)?; + let new_ptr = self.memory.reallocate(ptr, old_size, size, align)?; self.write_primval(dest, PrimVal::Ptr(new_ptr), dest_ty)?; } @@ -768,7 +768,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { } if let Some(old) = success { if let Some(var) = old { - self.memory.deallocate(var)?; + self.memory.deallocate(var, None)?; } self.write_primval(dest, PrimVal::Bytes(0), dest_ty)?; } else { @@ -795,7 +795,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { self.memory.write_bytes(value_copy, &value)?; self.memory.write_bytes(value_copy.offset(value.len() as u64, self.memory.layout)?, &[0])?; if let Some(var) = self.env_vars.insert(name.to_owned(), value_copy) { - self.memory.deallocate(var)?; + self.memory.deallocate(var, None)?; } self.write_primval(dest, PrimVal::Bytes(0), dest_ty)?; } else { diff --git a/tests/compile-fail/deallocate-bad-alignment.rs b/tests/compile-fail/deallocate-bad-alignment.rs new file mode 100644 index 000000000000..fb3c865fa250 --- /dev/null +++ b/tests/compile-fail/deallocate-bad-alignment.rs @@ -0,0 +1,13 @@ +#![feature(alloc, heap_api)] + +extern crate alloc; + +// error-pattern: tried to deallocate or reallocate using incorrect alignment or size + +use alloc::heap::*; +fn main() { + unsafe { + let x = allocate(1, 1); + deallocate(x, 1, 2); + } +} diff --git a/tests/compile-fail/deallocate-bad-size.rs b/tests/compile-fail/deallocate-bad-size.rs new file mode 100644 index 000000000000..fb3c865fa250 --- /dev/null +++ b/tests/compile-fail/deallocate-bad-size.rs @@ -0,0 +1,13 @@ +#![feature(alloc, heap_api)] + +extern crate alloc; + +// error-pattern: tried to deallocate or reallocate using incorrect alignment or size + +use alloc::heap::*; +fn main() { + unsafe { + let x = allocate(1, 1); + deallocate(x, 1, 2); + } +} diff --git a/tests/compile-fail/reallocate-bad-alignment.rs b/tests/compile-fail/reallocate-bad-alignment.rs new file mode 100644 index 000000000000..2edc13ee1a10 --- /dev/null +++ b/tests/compile-fail/reallocate-bad-alignment.rs @@ -0,0 +1,13 @@ +#![feature(alloc, heap_api)] + +extern crate alloc; + +// error-pattern: tried to deallocate or reallocate using incorrect alignment or size + +use alloc::heap::*; +fn main() { + unsafe { + let x = allocate(1, 1); + let _y = reallocate(x, 1, 1, 2); + } +} diff --git a/tests/compile-fail/reallocate-bad-size.rs b/tests/compile-fail/reallocate-bad-size.rs new file mode 100644 index 000000000000..f7f1b48a7f24 --- /dev/null +++ b/tests/compile-fail/reallocate-bad-size.rs @@ -0,0 +1,13 @@ +#![feature(alloc, heap_api)] + +extern crate alloc; + +// error-pattern: tried to deallocate or reallocate using incorrect alignment or size + +use alloc::heap::*; +fn main() { + unsafe { + let x = allocate(1, 1); + let _y = reallocate(x, 2, 1, 1); + } +}