From f23b782101c03331413870a11a787d4b605b84df Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 2 Jul 2019 09:03:45 +0200 Subject: [PATCH 1/4] align small malloc-allocations even less, and test that we do --- src/machine.rs | 4 +- src/shims/foreign_items.rs | 70 +++++++++++++++--------- tests/run-pass/{realloc.rs => malloc.rs} | 11 ++++ 3 files changed, 58 insertions(+), 27 deletions(-) rename tests/run-pass/{realloc.rs => malloc.rs} (70%) diff --git a/src/machine.rs b/src/machine.rs index 0ddb2d40b8..a58903f6d5 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -25,6 +25,8 @@ pub enum MiriMemoryKind { Rust, /// `malloc` memory. C, + /// Windows `HeapAlloc` memory. + WinHeap, /// Part of env var emulation. Env, /// Statics. @@ -407,7 +409,7 @@ impl MayLeak for MiriMemoryKind { fn may_leak(self) -> bool { use self::MiriMemoryKind::*; match self { - Rust | C => false, + Rust | C | WinHeap => false, Env | Static => true, } } diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index 2a3644e45c..38fc36609e 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -10,8 +10,8 @@ use crate::*; impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { - /// Returns the minimum alignment for the target architecture. - fn min_align(&self) -> Align { + /// Returns the minimum alignment for the target architecture for allocations of the given size. + fn min_align(&self, size: u64, kind: MiriMemoryKind) -> Align { let this = self.eval_context_ref(); // List taken from `libstd/sys_common/alloc.rs`. let min_align = match this.tcx.tcx.sess.target.target.arch.as_str() { @@ -19,21 +19,39 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx "x86_64" | "aarch64" | "mips64" | "s390x" | "sparc64" => 16, arch => bug!("Unsupported target architecture: {}", arch), }; - Align::from_bytes(min_align).unwrap() + // Windows always aligns, even small allocations. + // Source: + // But jemalloc does not, so for the C heap we only align if the allocation is sufficiently big. + if kind == MiriMemoryKind::WinHeap || size >= min_align { + return Align::from_bytes(min_align).unwrap(); + } + // We have `size < min_align`. Round `size` *down* to the next power of two and use that. + fn prev_power_of_two(x: u64) -> u64 { + let next_pow2 = x.next_power_of_two(); + if next_pow2 == x { + // x *is* a power of two, just use that. + x + } else { + // x is between two powers, so next = 2*prev. + next_pow2 / 2 + } + } + Align::from_bytes(prev_power_of_two(size)).unwrap() } fn malloc( &mut self, size: u64, zero_init: bool, + kind: MiriMemoryKind, ) -> Scalar { let this = self.eval_context_mut(); let tcx = &{this.tcx.tcx}; if size == 0 { Scalar::from_int(0, this.pointer_size()) } else { - let align = this.min_align(); - let ptr = this.memory_mut().allocate(Size::from_bytes(size), align, MiriMemoryKind::C.into()); + let align = this.min_align(size, kind); + let ptr = this.memory_mut().allocate(Size::from_bytes(size), align, kind.into()); if zero_init { // We just allocated this, the access cannot fail this.memory_mut() @@ -47,6 +65,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx fn free( &mut self, ptr: Scalar, + kind: MiriMemoryKind, ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); if !this.is_null(ptr)? { @@ -54,7 +73,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.memory_mut().deallocate( ptr, None, - MiriMemoryKind::C.into(), + kind.into(), )?; } Ok(()) @@ -64,39 +83,38 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx &mut self, old_ptr: Scalar, new_size: u64, + kind: MiriMemoryKind, ) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - let align = this.min_align(); + let new_align = this.min_align(new_size, kind); if this.is_null(old_ptr)? { if new_size == 0 { Ok(Scalar::from_int(0, this.pointer_size())) } else { let new_ptr = this.memory_mut().allocate( Size::from_bytes(new_size), - align, - MiriMemoryKind::C.into() + new_align, + kind.into() ); Ok(Scalar::Ptr(new_ptr)) } } else { let old_ptr = this.force_ptr(old_ptr)?; let memory = this.memory_mut(); - let old_size = Size::from_bytes(memory.get(old_ptr.alloc_id)?.bytes.len() as u64); if new_size == 0 { memory.deallocate( old_ptr, - Some((old_size, align)), - MiriMemoryKind::C.into(), + None, + kind.into(), )?; Ok(Scalar::from_int(0, this.pointer_size())) } else { let new_ptr = memory.reallocate( old_ptr, - old_size, - align, + None, Size::from_bytes(new_size), - align, - MiriMemoryKind::C.into(), + new_align, + kind.into(), )?; Ok(Scalar::Ptr(new_ptr)) } @@ -145,14 +163,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx match link_name { "malloc" => { let size = this.read_scalar(args[0])?.to_usize(this)?; - let res = this.malloc(size, /*zero_init:*/ false); + let res = this.malloc(size, /*zero_init:*/ false, MiriMemoryKind::C); this.write_scalar(res, dest)?; } "calloc" => { let items = this.read_scalar(args[0])?.to_usize(this)?; let len = this.read_scalar(args[1])?.to_usize(this)?; let size = items.checked_mul(len).ok_or_else(|| InterpError::Overflow(mir::BinOp::Mul))?; - let res = this.malloc(size, /*zero_init:*/ true); + let res = this.malloc(size, /*zero_init:*/ true, MiriMemoryKind::C); this.write_scalar(res, dest)?; } "posix_memalign" => { @@ -187,12 +205,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } "free" => { let ptr = this.read_scalar(args[0])?.not_undef()?; - this.free(ptr)?; + this.free(ptr, MiriMemoryKind::C)?; } "realloc" => { let old_ptr = this.read_scalar(args[0])?.not_undef()?; let new_size = this.read_scalar(args[1])?.to_usize(this)?; - let res = this.realloc(old_ptr, new_size)?; + let res = this.realloc(old_ptr, new_size, MiriMemoryKind::C)?; this.write_scalar(res, dest)?; } @@ -262,12 +280,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if !align.is_power_of_two() { return err!(HeapAllocNonPowerOfTwoAlignment(align)); } + let align = Align::from_bytes(align).unwrap(); let new_ptr = this.memory_mut().reallocate( ptr, - Size::from_bytes(old_size), - Align::from_bytes(align).unwrap(), + Some((Size::from_bytes(old_size), align)), Size::from_bytes(new_size), - Align::from_bytes(align).unwrap(), + align, MiriMemoryKind::Rust.into(), )?; this.write_scalar(Scalar::Ptr(new_ptr), dest)?; @@ -765,14 +783,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let flags = this.read_scalar(args[1])?.to_u32()?; let size = this.read_scalar(args[2])?.to_usize(this)?; let zero_init = (flags & 0x00000008) != 0; // HEAP_ZERO_MEMORY - let res = this.malloc(size, zero_init); + let res = this.malloc(size, zero_init, MiriMemoryKind::WinHeap); this.write_scalar(res, dest)?; } "HeapFree" => { let _handle = this.read_scalar(args[0])?.to_isize(this)?; let _flags = this.read_scalar(args[1])?.to_u32()?; let ptr = this.read_scalar(args[2])?.not_undef()?; - this.free(ptr)?; + this.free(ptr, MiriMemoryKind::WinHeap)?; this.write_scalar(Scalar::from_int(1, Size::from_bytes(4)), dest)?; } "HeapReAlloc" => { @@ -780,7 +798,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let _flags = this.read_scalar(args[1])?.to_u32()?; let ptr = this.read_scalar(args[2])?.not_undef()?; let size = this.read_scalar(args[3])?.to_usize(this)?; - let res = this.realloc(ptr, size)?; + let res = this.realloc(ptr, size, MiriMemoryKind::WinHeap)?; this.write_scalar(res, dest)?; } diff --git a/tests/run-pass/realloc.rs b/tests/run-pass/malloc.rs similarity index 70% rename from tests/run-pass/realloc.rs rename to tests/run-pass/malloc.rs index c23b3e645c..a50b3f3606 100644 --- a/tests/run-pass/realloc.rs +++ b/tests/run-pass/malloc.rs @@ -1,4 +1,5 @@ //ignore-windows: Uses POSIX APIs +//compile-flags: -Zmiri-seed= #![feature(rustc_private)] @@ -7,6 +8,16 @@ use core::{slice, ptr}; extern crate libc; fn main() { + // Test that small allocations sometimes *are* not very aligned. + let saw_unaligned = (0..64).any(|_| unsafe { + let p = libc::malloc(3); + let addr = p as usize; + let unaligned = addr % 4 != 0; // test that this is not 4-aligned + libc::free(p); // FIXME have to free *after* test; should allow ptr-to-int of dangling ptr. + unaligned + }); + assert!(saw_unaligned); + unsafe { // Use calloc for initialized memory let p1 = libc::calloc(20, 1); From 1729965808dec239aa46f2eafaa39261f491a0b4 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 5 Jul 2019 23:47:10 +0200 Subject: [PATCH 2/4] rename InterpretCx -> InterpCx --- src/eval.rs | 8 ++++---- src/machine.rs | 22 +++++++++++----------- src/shims/foreign_items.rs | 2 +- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index 91a563fa56..b29ce35377 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -8,7 +8,7 @@ use rustc::hir::def_id::DefId; use rustc::mir; use crate::{ - InterpResult, InterpError, InterpretCx, StackPopCleanup, struct_error, + InterpResult, InterpError, InterpCx, StackPopCleanup, struct_error, Scalar, Tag, Pointer, MemoryExtra, MiriMemoryKind, Evaluator, TlsEvalContextExt, }; @@ -28,8 +28,8 @@ pub fn create_ecx<'mir, 'tcx: 'mir>( tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig, -) -> InterpResult<'tcx, InterpretCx<'mir, 'tcx, Evaluator<'tcx>>> { - let mut ecx = InterpretCx::new( +) -> InterpResult<'tcx, InterpCx<'mir, 'tcx, Evaluator<'tcx>>> { + let mut ecx = InterpCx::new( tcx.at(syntax::source_map::DUMMY_SP), ty::ParamEnv::reveal_all(), Evaluator::new(), @@ -43,7 +43,7 @@ pub fn create_ecx<'mir, 'tcx: 'mir>( config.validate }; - // FIXME: InterpretCx::new should take an initial MemoryExtra + // FIXME: InterpCx::new should take an initial MemoryExtra ecx.memory_mut().extra = MemoryExtra::new(config.seed.map(StdRng::seed_from_u64), validate); let main_instance = ty::Instance::mono(ecx.tcx.tcx, main_id); diff --git a/src/machine.rs b/src/machine.rs index a58903f6d5..930eeee96b 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -105,8 +105,8 @@ impl<'tcx> Evaluator<'tcx> { } } -/// A rustc InterpretCx for Miri. -pub type MiriEvalContext<'mir, 'tcx> = InterpretCx<'mir, 'tcx, Evaluator<'tcx>>; +/// A rustc InterpCx for Miri. +pub type MiriEvalContext<'mir, 'tcx> = InterpCx<'mir, 'tcx, Evaluator<'tcx>>; /// A little trait that's useful to be inherited by extension traits. pub trait MiriEvalContextExt<'mir, 'tcx> { @@ -138,14 +138,14 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> { const STATIC_KIND: Option = Some(MiriMemoryKind::Static); #[inline(always)] - fn enforce_validity(ecx: &InterpretCx<'mir, 'tcx, Self>) -> bool { + fn enforce_validity(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool { ecx.memory().extra.validate } /// Returns `Ok()` when the function was handled; fail otherwise. #[inline(always)] fn find_fn( - ecx: &mut InterpretCx<'mir, 'tcx, Self>, + ecx: &mut InterpCx<'mir, 'tcx, Self>, instance: ty::Instance<'tcx>, args: &[OpTy<'tcx, Tag>], dest: Option>, @@ -156,7 +156,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> { #[inline(always)] fn call_intrinsic( - ecx: &mut rustc_mir::interpret::InterpretCx<'mir, 'tcx, Self>, + ecx: &mut rustc_mir::interpret::InterpCx<'mir, 'tcx, Self>, instance: ty::Instance<'tcx>, args: &[OpTy<'tcx, Tag>], dest: PlaceTy<'tcx, Tag>, @@ -166,7 +166,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> { #[inline(always)] fn ptr_op( - ecx: &rustc_mir::interpret::InterpretCx<'mir, 'tcx, Self>, + ecx: &rustc_mir::interpret::InterpCx<'mir, 'tcx, Self>, bin_op: mir::BinOp, left: ImmTy<'tcx, Tag>, right: ImmTy<'tcx, Tag>, @@ -175,7 +175,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> { } fn box_alloc( - ecx: &mut InterpretCx<'mir, 'tcx, Self>, + ecx: &mut InterpCx<'mir, 'tcx, Self>, dest: PlaceTy<'tcx, Tag>, ) -> InterpResult<'tcx> { trace!("box_alloc for {:?}", dest.layout.ty); @@ -241,7 +241,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> { } #[inline(always)] - fn before_terminator(_ecx: &mut InterpretCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> + fn before_terminator(_ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> { // We are not interested in detecting loops. Ok(()) @@ -311,7 +311,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> { #[inline(always)] fn retag( - ecx: &mut InterpretCx<'mir, 'tcx, Self>, + ecx: &mut InterpCx<'mir, 'tcx, Self>, kind: mir::RetagKind, place: PlaceTy<'tcx, Tag>, ) -> InterpResult<'tcx> { @@ -325,14 +325,14 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> { #[inline(always)] fn stack_push( - ecx: &mut InterpretCx<'mir, 'tcx, Self>, + ecx: &mut InterpCx<'mir, 'tcx, Self>, ) -> InterpResult<'tcx, stacked_borrows::CallId> { Ok(ecx.memory().extra.stacked_borrows.borrow_mut().new_call()) } #[inline(always)] fn stack_pop( - ecx: &mut InterpretCx<'mir, 'tcx, Self>, + ecx: &mut InterpCx<'mir, 'tcx, Self>, extra: stacked_borrows::CallId, ) -> InterpResult<'tcx> { Ok(ecx.memory().extra.stacked_borrows.borrow_mut().end_call(extra)) diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index 38fc36609e..2fe2ecc195 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -345,7 +345,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx trace!("__rust_maybe_catch_panic: {:?}", f_instance); // Now we make a function call. - // TODO: consider making this reusable? `InterpretCx::step` does something similar + // TODO: consider making this reusable? `InterpCx::step` does something similar // for the TLS destructors, and of course `eval_main`. let mir = this.load_mir(f_instance.def)?; let ret_place = MPlaceTy::dangling(this.layout_of(this.tcx.mk_unit())?, this).into(); From b75e9179bf21254a198e5b7359589cd0502d135c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 5 Jul 2019 23:49:30 +0200 Subject: [PATCH 3/4] bump rustc --- rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust-version b/rust-version index 9e4ddf2aca..cbd8e33577 100644 --- a/rust-version +++ b/rust-version @@ -1 +1 @@ -088b987307b91612ab164026e1dcdd0129fdb62b +24a9bcbb7cb0d8bdc11b8252a9c13f7562c7e4ca From 029a29407acd30454fb41f340a8d108a47bef349 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 5 Jul 2019 23:51:11 +0200 Subject: [PATCH 4/4] dangling-ptr-to-int should work now; move to noseed --- tests/{run-pass => run-pass-noseed}/malloc.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) rename tests/{run-pass => run-pass-noseed}/malloc.rs (84%) diff --git a/tests/run-pass/malloc.rs b/tests/run-pass-noseed/malloc.rs similarity index 84% rename from tests/run-pass/malloc.rs rename to tests/run-pass-noseed/malloc.rs index a50b3f3606..bf51baacd3 100644 --- a/tests/run-pass/malloc.rs +++ b/tests/run-pass-noseed/malloc.rs @@ -11,10 +11,8 @@ fn main() { // Test that small allocations sometimes *are* not very aligned. let saw_unaligned = (0..64).any(|_| unsafe { let p = libc::malloc(3); - let addr = p as usize; - let unaligned = addr % 4 != 0; // test that this is not 4-aligned - libc::free(p); // FIXME have to free *after* test; should allow ptr-to-int of dangling ptr. - unaligned + libc::free(p); + (p as usize) % 4 != 0 // find any that this is *not* 4-aligned }); assert!(saw_unaligned);