From 4dc188a60eb19dc770db071a5e368e90b5a1fab3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 23 Jun 2019 17:26:12 +0200 Subject: [PATCH] adjust for refactored memory pointer checks --- src/fn_call.rs | 5 ++- src/intrinsic.rs | 15 ++++--- src/operator.rs | 41 +++++++++++++------- src/stacked_borrows.rs | 7 ++-- tests/compile-fail/ptr_eq_dangling.rs | 2 +- tests/compile-fail/ptr_eq_out_of_bounds.rs | 2 +- tests/compile-fail/rc_as_raw.rs | 2 + tests/compile-fail/unaligned_ptr_cast1.rs | 4 +- tests/compile-fail/unaligned_ptr_cast2.rs | 4 +- tests/compile-fail/validity/dangling_ref1.rs | 2 +- tests/compile-fail/validity/dangling_ref2.rs | 2 +- tests/compile-fail/zst.rs | 4 -- tests/compile-fail/zst1.rs | 5 +++ tests/compile-fail/zst2.rs | 12 ++++++ tests/compile-fail/zst3.rs | 15 +++++++ tests/run-pass/zst.rs | 9 ----- 16 files changed, 86 insertions(+), 45 deletions(-) delete mode 100644 tests/compile-fail/zst.rs create mode 100644 tests/compile-fail/zst1.rs create mode 100644 tests/compile-fail/zst2.rs create mode 100644 tests/compile-fail/zst3.rs diff --git a/src/fn_call.rs b/src/fn_call.rs index eca1d9144ec4..b053d8c51e70 100644 --- a/src/fn_call.rs +++ b/src/fn_call.rs @@ -654,7 +654,7 @@ fn emulate_foreign_item( // Hook pthread calls that go to the thread-local storage memory subsystem. "pthread_key_create" => { - let key_ptr = this.read_scalar(args[0])?.to_ptr()?; + let key_ptr = this.read_scalar(args[0])?.not_undef()?; // Extract the function type out of the signature (that seems easier than constructing it ourselves). let dtor = match this.read_scalar(args[1])?.not_undef()? { @@ -681,7 +681,8 @@ fn emulate_foreign_item( return err!(OutOfTls); } - this.memory().check_align(key_ptr.into(), key_layout.align.abi)?; + let key_ptr = this.memory().check_ptr_access(key_ptr, key_layout.size, key_layout.align.abi)? + .expect("cannot be a ZST"); this.memory_mut().get_mut(key_ptr.alloc_id)?.write_scalar( tcx, key_ptr, diff --git a/src/intrinsic.rs b/src/intrinsic.rs index 33bab4642a90..3f9c4e53f09d 100644 --- a/src/intrinsic.rs +++ b/src/intrinsic.rs @@ -517,13 +517,16 @@ fn call_intrinsic( let val_byte = this.read_scalar(args[1])?.to_u8()?; let ptr = this.read_scalar(args[0])?.not_undef()?; let count = this.read_scalar(args[2])?.to_usize(this)?; - this.memory().check_align(ptr, ty_layout.align.abi)?; let byte_count = ty_layout.size * count; - if byte_count.bytes() != 0 { - let ptr = ptr.to_ptr()?; - this.memory_mut() - .get_mut(ptr.alloc_id)? - .write_repeat(tcx, ptr, val_byte, byte_count)?; + match this.memory().check_ptr_access(ptr, byte_count, ty_layout.align.abi)? { + Some(ptr) => { + this.memory_mut() + .get_mut(ptr.alloc_id)? + .write_repeat(tcx, ptr, val_byte, byte_count)?; + } + None => { + // Size is 0, nothing to do. + } } } diff --git a/src/operator.rs b/src/operator.rs index b05d435ef644..b735cbcba4ec 100644 --- a/src/operator.rs +++ b/src/operator.rs @@ -4,6 +4,11 @@ use crate::*; pub trait EvalContextExt<'tcx> { + fn pointer_inbounds( + &self, + ptr: Pointer + ) -> InterpResult<'tcx>; + fn ptr_op( &self, bin_op: mir::BinOp, @@ -34,6 +39,13 @@ fn pointer_offset_inbounds( } impl<'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'mir, 'tcx> { + /// Test if the pointer is in-bounds of a live allocation. + #[inline] + fn pointer_inbounds(&self, ptr: Pointer) -> InterpResult<'tcx> { + let (size, _align) = self.memory().get_size_and_align(ptr.alloc_id, AllocCheck::Live)?; + ptr.check_in_alloc(size, CheckInAllocMsg::InboundsTest) + } + fn ptr_op( &self, bin_op: mir::BinOp, @@ -114,8 +126,8 @@ fn ptr_op( let left = left.to_ptr().expect("we checked is_ptr"); let right = right.to_bits(self.memory().pointer_size()).expect("we checked is_bits"); let (_alloc_size, alloc_align) = self.memory() - .get_size_and_align(left.alloc_id, InboundsCheck::MaybeDead) - .expect("determining size+align of dead ptr cannot fail"); + .get_size_and_align(left.alloc_id, AllocCheck::MaybeDead) + .expect("alloc info with MaybeDead cannot fail"); let min_ptr_val = u128::from(alloc_align.bytes()) + u128::from(left.offset.bytes()); let result = match bin_op { Gt => min_ptr_val > right, @@ -170,6 +182,7 @@ fn ptr_eq( if left.alloc_id == right.alloc_id { left.offset == right.offset } else { + // Make sure both pointers are in-bounds. // This accepts one-past-the end. Thus, there is still technically // some non-determinism that we do not fully rule out when two // allocations sit right next to each other. The C/C++ standards are @@ -179,10 +192,12 @@ fn ptr_eq( // Dead allocations in miri cannot overlap with live allocations, but // on read hardware this can easily happen. Thus for comparisons we require // both pointers to be live. - self.memory().check_bounds_ptr(left, InboundsCheck::Live, CheckInAllocMsg::InboundsTest)?; - self.memory().check_bounds_ptr(right, InboundsCheck::Live, CheckInAllocMsg::InboundsTest)?; - // Two in-bounds pointers, we can compare across allocations. - left == right + if self.pointer_inbounds(left).is_ok() && self.pointer_inbounds(right).is_ok() { + // Two in-bounds pointers, we can compare across allocations. + left == right + } else { + return err!(InvalidPointerMath); + } } } // Comparing ptr and integer. @@ -202,16 +217,16 @@ fn ptr_eq( // alignment 32 or higher, hence the limit of 32. // FIXME: Once we support intptrcast, we could try to fix these holes. if bits < 32 { - // Test if the ptr is in-bounds. Then it cannot be NULL. - // Even dangling pointers cannot be NULL. - if self.memory().check_bounds_ptr(ptr, InboundsCheck::MaybeDead, CheckInAllocMsg::NullPointerTest).is_ok() { + // Test if the pointer can be different from NULL or not. + // We assume that pointers that are not NULL are also not "small". + if !self.memory().ptr_may_be_null(ptr) { return Ok(false); } } let (alloc_size, alloc_align) = self.memory() - .get_size_and_align(ptr.alloc_id, InboundsCheck::MaybeDead) - .expect("determining size+align of dead ptr cannot fail"); + .get_size_and_align(ptr.alloc_id, AllocCheck::MaybeDead) + .expect("alloc info with MaybeDead cannot fail"); // Case II: Alignment gives it away if ptr.offset.bytes() % alloc_align.bytes() == 0 { @@ -359,9 +374,9 @@ fn pointer_offset_inbounds( if let Scalar::Ptr(ptr) = ptr { // Both old and new pointer must be in-bounds of a *live* allocation. // (Of the same allocation, but that part is trivial with our representation.) - self.memory().check_bounds_ptr(ptr, InboundsCheck::Live, CheckInAllocMsg::InboundsTest)?; + self.pointer_inbounds(ptr)?; let ptr = ptr.signed_offset(offset, self)?; - self.memory().check_bounds_ptr(ptr, InboundsCheck::Live, CheckInAllocMsg::InboundsTest)?; + self.pointer_inbounds(ptr)?; Ok(Scalar::Ptr(ptr)) } else { // An integer pointer. They can only be offset by 0, and we pretend there diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index f7617676701c..3a0c257428e1 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -10,7 +10,7 @@ use crate::{ InterpResult, InterpError, MiriEvalContext, HelpersEvalContextExt, Evaluator, MutValueVisitor, - MemoryKind, MiriMemoryKind, RangeMap, Allocation, AllocationExtra, AllocId, CheckInAllocMsg, + MemoryKind, MiriMemoryKind, RangeMap, Allocation, AllocationExtra, AllocId, Pointer, Immediate, ImmTy, PlaceTy, MPlaceTy, }; @@ -531,13 +531,14 @@ fn reborrow( ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); let protector = if protect { Some(this.frame().extra) } else { None }; - let ptr = place.ptr.to_ptr()?; + let ptr = this.memory().check_ptr_access(place.ptr, size, place.align) + .expect("validity checks should have excluded dangling/unaligned pointer") + .expect("we shouldn't get here for ZST"); trace!("reborrow: {} reference {:?} derived from {:?} (pointee {}): {:?}, size {}", kind, new_tag, ptr.tag, place.layout.ty, ptr.erase_tag(), size.bytes()); // Get the allocation. It might not be mutable, so we cannot use `get_mut`. let alloc = this.memory().get(ptr.alloc_id)?; - alloc.check_bounds(this, ptr, size, CheckInAllocMsg::InboundsTest)?; // Update the stacks. // Make sure that raw pointers and mutable shared references are reborrowed "weak": // There could be existing unique pointers reborrowed from them that should remain valid! diff --git a/tests/compile-fail/ptr_eq_dangling.rs b/tests/compile-fail/ptr_eq_dangling.rs index d05996a13d56..5badf099e439 100644 --- a/tests/compile-fail/ptr_eq_dangling.rs +++ b/tests/compile-fail/ptr_eq_dangling.rs @@ -6,5 +6,5 @@ fn main() { let y = &*b as *const i32; // different allocation // We cannot compare these even though both are inbounds -- they *could* be // equal if memory was reused. - assert!(x != y); //~ ERROR dangling pointer + assert!(x != y); //~ ERROR invalid arithmetic on pointers } diff --git a/tests/compile-fail/ptr_eq_out_of_bounds.rs b/tests/compile-fail/ptr_eq_out_of_bounds.rs index af4eed8d4e32..7efa446d7fca 100644 --- a/tests/compile-fail/ptr_eq_out_of_bounds.rs +++ b/tests/compile-fail/ptr_eq_out_of_bounds.rs @@ -5,5 +5,5 @@ fn main() { let y = &*b as *const i32; // different allocation // We cannot compare these even though both allocations are live -- they *could* be // equal (with the right base addresses). - assert!(x != y); //~ ERROR outside bounds + assert!(x != y); //~ ERROR invalid arithmetic on pointers } diff --git a/tests/compile-fail/rc_as_raw.rs b/tests/compile-fail/rc_as_raw.rs index 3e6e96456fca..086467eea311 100644 --- a/tests/compile-fail/rc_as_raw.rs +++ b/tests/compile-fail/rc_as_raw.rs @@ -1,3 +1,5 @@ +// This should fail even without validation +// compile-flags: -Zmiri-disable-validation #![feature(weak_into_raw)] use std::rc::{Rc, Weak}; diff --git a/tests/compile-fail/unaligned_ptr_cast1.rs b/tests/compile-fail/unaligned_ptr_cast1.rs index e64594d3e7ed..92e1fae75a04 100644 --- a/tests/compile-fail/unaligned_ptr_cast1.rs +++ b/tests/compile-fail/unaligned_ptr_cast1.rs @@ -2,8 +2,8 @@ // compile-flags: -Zmiri-disable-validation fn main() { - let x = &2u16; - let x = x as *const _ as *const u32; + let x = [2u16, 3, 4]; // Make it big enough so we don't get an out-of-bounds error. + let x = &x[0] as *const _ as *const u32; // This must fail because alignment is violated let _x = unsafe { *x }; //~ ERROR tried to access memory with alignment 2, but alignment 4 is required } diff --git a/tests/compile-fail/unaligned_ptr_cast2.rs b/tests/compile-fail/unaligned_ptr_cast2.rs index 9fb138e353fe..783661c55710 100644 --- a/tests/compile-fail/unaligned_ptr_cast2.rs +++ b/tests/compile-fail/unaligned_ptr_cast2.rs @@ -2,8 +2,8 @@ // compile-flags: -Zmiri-disable-validation fn main() { - let x = &2u16; - let x = x as *const _ as *const *const u8; + let x = [2u16, 3, 4, 5]; // Make it big enough so we don't get an out-of-bounds error. + let x = &x[0] as *const _ as *const *const u8; // This must fail because alignment is violated. Test specifically for loading pointers, // which have special code in miri's memory. let _x = unsafe { *x }; diff --git a/tests/compile-fail/validity/dangling_ref1.rs b/tests/compile-fail/validity/dangling_ref1.rs index c5845cb693bb..4318c7c90271 100644 --- a/tests/compile-fail/validity/dangling_ref1.rs +++ b/tests/compile-fail/validity/dangling_ref1.rs @@ -1,5 +1,5 @@ use std::mem; fn main() { - let _x: &i32 = unsafe { mem::transmute(16usize) }; //~ ERROR tried to interpret some bytes as a pointer + let _x: &i32 = unsafe { mem::transmute(16usize) }; //~ ERROR integer pointer in non-ZST reference } diff --git a/tests/compile-fail/validity/dangling_ref2.rs b/tests/compile-fail/validity/dangling_ref2.rs index 21650ebf9506..ef76b93d11e2 100644 --- a/tests/compile-fail/validity/dangling_ref2.rs +++ b/tests/compile-fail/validity/dangling_ref2.rs @@ -3,5 +3,5 @@ fn main() { let val = 14; let ptr = (&val as *const i32).wrapping_offset(1); - let _x: &i32 = unsafe { mem::transmute(ptr) }; //~ ERROR outside bounds of allocation + let _x: &i32 = unsafe { mem::transmute(ptr) }; //~ ERROR encountered dangling (not entirely in bounds) reference } diff --git a/tests/compile-fail/zst.rs b/tests/compile-fail/zst.rs deleted file mode 100644 index 0488926870a2..000000000000 --- a/tests/compile-fail/zst.rs +++ /dev/null @@ -1,4 +0,0 @@ -fn main() { - let x = &() as *const () as *const i32; - let _val = unsafe { *x }; //~ ERROR access memory with alignment 1, but alignment 4 is required -} diff --git a/tests/compile-fail/zst1.rs b/tests/compile-fail/zst1.rs new file mode 100644 index 000000000000..e4ce7b8ecdf8 --- /dev/null +++ b/tests/compile-fail/zst1.rs @@ -0,0 +1,5 @@ +fn main() { + // make sure ZST locals cannot be accessed + let x = &() as *const () as *const i8; + let _val = unsafe { *x }; //~ ERROR pointer must be in-bounds +} diff --git a/tests/compile-fail/zst2.rs b/tests/compile-fail/zst2.rs new file mode 100644 index 000000000000..950f7b3d60e6 --- /dev/null +++ b/tests/compile-fail/zst2.rs @@ -0,0 +1,12 @@ +fn main() { + // Not using the () type here, as writes of that type do not even have MIR generated. + // Also not assigning directly as that's array initialization, not assignment. + let zst_val = [1u8; 0]; + + // make sure ZST accesses are checked against being "truly" dangling pointers + // (into deallocated allocations). + let mut x_box = Box::new(1u8); + let x = &mut *x_box as *mut _ as *mut [u8; 0]; + drop(x_box); + unsafe { *x = zst_val; } //~ ERROR dangling pointer was dereferenced +} diff --git a/tests/compile-fail/zst3.rs b/tests/compile-fail/zst3.rs new file mode 100644 index 000000000000..a16ec0186d77 --- /dev/null +++ b/tests/compile-fail/zst3.rs @@ -0,0 +1,15 @@ +fn main() { + // Not using the () type here, as writes of that type do not even have MIR generated. + // Also not assigning directly as that's array initialization, not assignment. + let zst_val = [1u8; 0]; + + // make sure ZST accesses are checked against being "truly" dangling pointers + // (that are out-of-bounds). + let mut x_box = Box::new(1u8); + let x = (&mut *x_box as *mut u8).wrapping_offset(1); + // This one is just "at the egde", but still okay + unsafe { *(x as *mut [u8; 0]) = zst_val; } + // One byte further is OOB. + let x = x.wrapping_offset(1); + unsafe { *(x as *mut [u8; 0]) = zst_val; } //~ ERROR pointer must be in-bounds +} diff --git a/tests/run-pass/zst.rs b/tests/run-pass/zst.rs index 9d97210b73db..988473570913 100644 --- a/tests/run-pass/zst.rs +++ b/tests/run-pass/zst.rs @@ -21,13 +21,4 @@ fn main() { // Reading and writing is ok. unsafe { *x = zst_val; } unsafe { let _y = *x; } - - // We should even be able to use "true" pointers for ZST when the allocation has been - // removed already. The box is for a non-ZST to make sure there actually is an allocation. - let mut x_box = Box::new(((), 1u8)); - let x = &mut x_box.0 as *mut _ as *mut [u8; 0]; - drop(x_box); - // Reading and writing is ok. - unsafe { *x = zst_val; } - unsafe { let _y = *x; } }