From bed3b965aef7f3a3da789b4e0493fa77f75440de Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 17 Jul 2021 20:12:28 +0200 Subject: [PATCH] miri: better ptr-out-of-bounds errors --- .../rustc_middle/src/mir/interpret/error.rs | 30 ++++++++-------- .../rustc_middle/src/mir/interpret/pointer.rs | 14 ++++++++ compiler/rustc_mir/src/interpret/memory.rs | 14 +++++--- src/test/ui/consts/offset_ub.rs | 1 + src/test/ui/consts/offset_ub.stderr | 34 +++++++++++++------ 5 files changed, 65 insertions(+), 28 deletions(-) diff --git a/compiler/rustc_middle/src/mir/interpret/error.rs b/compiler/rustc_middle/src/mir/interpret/error.rs index 432d078dc9b05..94ac303b109a5 100644 --- a/compiler/rustc_middle/src/mir/interpret/error.rs +++ b/compiler/rustc_middle/src/mir/interpret/error.rs @@ -240,12 +240,13 @@ pub enum UndefinedBehaviorInfo<'tcx> { /// Dereferencing a dangling pointer after it got freed. PointerUseAfterFree(AllocId), /// Used a pointer outside the bounds it is valid for. + /// (If `ptr_size > 0`, determines the size of the memory range that was expected to be in-bounds.) PointerOutOfBounds { alloc_id: AllocId, - offset: Size, - size: Size, + alloc_size: Size, + ptr_offset: i64, + ptr_size: Size, msg: CheckInAllocMsg, - allocation_size: Size, }, /// Using an integer as a pointer in the wrong way. DanglingIntPointer(u64, CheckInAllocMsg), @@ -318,24 +319,25 @@ impl fmt::Display for UndefinedBehaviorInfo<'_> { PointerUseAfterFree(a) => { write!(f, "pointer to {} was dereferenced after this allocation got freed", a) } - PointerOutOfBounds { alloc_id, offset, size: Size::ZERO, msg, allocation_size } => { + PointerOutOfBounds { alloc_id, alloc_size, ptr_offset, ptr_size: Size::ZERO, msg } => { write!( f, - "{}{} has size {}, so pointer at offset {} is out-of-bounds", + "{}{alloc_id} has size {alloc_size}, so pointer at offset {ptr_offset} is out-of-bounds", msg, - alloc_id, - allocation_size.bytes(), - offset.bytes(), + alloc_id = alloc_id, + alloc_size = alloc_size.bytes(), + ptr_offset = ptr_offset, ) } - PointerOutOfBounds { alloc_id, offset, size, msg, allocation_size } => write!( + PointerOutOfBounds { alloc_id, alloc_size, ptr_offset, ptr_size, msg } => write!( f, - "{}{} has size {}, so pointer to {} bytes starting at offset {} is out-of-bounds", + "{}{alloc_id} has size {alloc_size}, so pointer to {ptr_size} byte{ptr_size_p} starting at offset {ptr_offset} is out-of-bounds", msg, - alloc_id, - allocation_size.bytes(), - size.bytes(), - offset.bytes(), + alloc_id = alloc_id, + alloc_size = alloc_size.bytes(), + ptr_size = ptr_size.bytes(), + ptr_size_p = pluralize!(ptr_size.bytes()), + ptr_offset = ptr_offset, ), DanglingIntPointer(0, CheckInAllocMsg::InboundsTest) => { write!(f, "null pointer is not a valid pointer for this operation") diff --git a/compiler/rustc_middle/src/mir/interpret/pointer.rs b/compiler/rustc_middle/src/mir/interpret/pointer.rs index 7e7a7119be6a8..568b3f252bf5f 100644 --- a/compiler/rustc_middle/src/mir/interpret/pointer.rs +++ b/compiler/rustc_middle/src/mir/interpret/pointer.rs @@ -36,6 +36,20 @@ pub trait PointerArithmetic: HasDataLayout { i64::try_from(max_isize_plus_1 - 1).unwrap() } + #[inline] + fn machine_usize_to_isize(&self, val: u64) -> i64 { + let val = val as i64; + // Now clamp into the machine_isize range. + if val > self.machine_isize_max() { + // This can only happen the the ptr size is < 64, so we know max_usize_plus_1 fits into + // i64. + let max_usize_plus_1 = 1u128 << self.pointer_size().bits(); + val - i64::try_from(max_usize_plus_1).unwrap() + } else { + val + } + } + /// Helper function: truncate given value-"overflowed flag" pair to pointer size and /// update "overflowed flag" if there was an overflow. /// This should be called by all the other methods before returning! diff --git a/compiler/rustc_mir/src/interpret/memory.rs b/compiler/rustc_mir/src/interpret/memory.rs index d145ad734cf7a..6dcd944a1c3f2 100644 --- a/compiler/rustc_mir/src/interpret/memory.rs +++ b/compiler/rustc_mir/src/interpret/memory.rs @@ -372,7 +372,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { ) } - /// Check if the given pointerpoints to live memory of given `size` and `align` + /// Check if the given pointer points to live memory of given `size` and `align` /// (ignoring `M::enforce_alignment`). The caller can control the error message for the /// out-of-bounds case. #[inline(always)] @@ -451,11 +451,17 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { None } Ok((alloc_id, offset, ptr)) => { - let (allocation_size, alloc_align, ret_val) = alloc_size(alloc_id, offset, ptr)?; + let (alloc_size, alloc_align, ret_val) = alloc_size(alloc_id, offset, ptr)?; // Test bounds. This also ensures non-null. // It is sufficient to check this for the end pointer. Also check for overflow! - if offset.checked_add(size, &self.tcx).map_or(true, |end| end > allocation_size) { - throw_ub!(PointerOutOfBounds { alloc_id, offset, size, allocation_size, msg }) + if offset.checked_add(size, &self.tcx).map_or(true, |end| end > alloc_size) { + throw_ub!(PointerOutOfBounds { + alloc_id, + alloc_size, + ptr_offset: self.machine_usize_to_isize(offset.bytes()), + ptr_size: size, + msg, + }) } // Test align. Check this last; if both bounds and alignment are violated // we want the error to be about the bounds. diff --git a/src/test/ui/consts/offset_ub.rs b/src/test/ui/consts/offset_ub.rs index a22296a7b0085..42a285a6eab49 100644 --- a/src/test/ui/consts/offset_ub.rs +++ b/src/test/ui/consts/offset_ub.rs @@ -13,6 +13,7 @@ pub const OVERFLOW: *const u16 = unsafe { [0u16; 1].as_ptr().offset(isize::MAX) pub const UNDERFLOW: *const u16 = unsafe { [0u16; 1].as_ptr().offset(isize::MIN) }; //~NOTE pub const OVERFLOW_ADDRESS_SPACE: *const u8 = unsafe { (usize::MAX as *const u8).offset(2) }; //~NOTE pub const UNDERFLOW_ADDRESS_SPACE: *const u8 = unsafe { (1 as *const u8).offset(-2) }; //~NOTE +pub const NEGATIVE_OFFSET: *const u8 = unsafe { [0u8; 1].as_ptr().wrapping_offset(-2).offset(-2) }; //~NOTE pub const ZERO_SIZED_ALLOC: *const u8 = unsafe { [0u8; 0].as_ptr().offset(1) }; //~NOTE pub const DANGLING: *const u8 = unsafe { ptr::NonNull::::dangling().as_ptr().offset(4) }; //~NOTE diff --git a/src/test/ui/consts/offset_ub.stderr b/src/test/ui/consts/offset_ub.stderr index 4f7c4f92060a2..66a2722ed4acd 100644 --- a/src/test/ui/consts/offset_ub.stderr +++ b/src/test/ui/consts/offset_ub.stderr @@ -102,13 +102,27 @@ error[E0080]: evaluation of constant value failed LL | unsafe { intrinsics::offset(self, count) } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | | - | pointer arithmetic failed: allocN has size 0, so pointer to 1 bytes starting at offset 0 is out-of-bounds + | pointer arithmetic failed: allocN has size 1, so pointer to 2 bytes starting at offset -4 is out-of-bounds | inside `ptr::const_ptr::::offset` at $SRC_DIR/core/src/ptr/const_ptr.rs:LL:COL | - ::: $DIR/offset_ub.rs:17:50 + ::: $DIR/offset_ub.rs:16:49 + | +LL | pub const NEGATIVE_OFFSET: *const u8 = unsafe { [0u8; 1].as_ptr().wrapping_offset(-2).offset(-2) }; + | ------------------------------------------------ inside `NEGATIVE_OFFSET` at $DIR/offset_ub.rs:16:49 + +error[E0080]: evaluation of constant value failed + --> $SRC_DIR/core/src/ptr/const_ptr.rs:LL:COL + | +LL | unsafe { intrinsics::offset(self, count) } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | | + | pointer arithmetic failed: allocN has size 0, so pointer to 1 byte starting at offset 0 is out-of-bounds + | inside `ptr::const_ptr::::offset` at $SRC_DIR/core/src/ptr/const_ptr.rs:LL:COL + | + ::: $DIR/offset_ub.rs:18:50 | LL | pub const ZERO_SIZED_ALLOC: *const u8 = unsafe { [0u8; 0].as_ptr().offset(1) }; - | --------------------------- inside `ZERO_SIZED_ALLOC` at $DIR/offset_ub.rs:17:50 + | --------------------------- inside `ZERO_SIZED_ALLOC` at $DIR/offset_ub.rs:18:50 error[E0080]: evaluation of constant value failed --> $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL @@ -119,10 +133,10 @@ LL | unsafe { intrinsics::offset(self, count) as *mut T } | 0x1 is not a valid pointer | inside `ptr::mut_ptr::::offset` at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL | - ::: $DIR/offset_ub.rs:18:42 + ::: $DIR/offset_ub.rs:19:42 | LL | pub const DANGLING: *const u8 = unsafe { ptr::NonNull::::dangling().as_ptr().offset(4) }; - | ------------------------------------------------- inside `DANGLING` at $DIR/offset_ub.rs:18:42 + | ------------------------------------------------- inside `DANGLING` at $DIR/offset_ub.rs:19:42 error[E0080]: evaluation of constant value failed --> $SRC_DIR/core/src/ptr/const_ptr.rs:LL:COL @@ -133,10 +147,10 @@ LL | unsafe { intrinsics::offset(self, count) } | pointer arithmetic failed: 0x0 is not a valid pointer | inside `ptr::const_ptr::::offset` at $SRC_DIR/core/src/ptr/const_ptr.rs:LL:COL | - ::: $DIR/offset_ub.rs:21:50 + ::: $DIR/offset_ub.rs:22:50 | LL | pub const NULL_OFFSET_ZERO: *const u8 = unsafe { ptr::null::().offset(0) }; - | --------------------------- inside `NULL_OFFSET_ZERO` at $DIR/offset_ub.rs:21:50 + | --------------------------- inside `NULL_OFFSET_ZERO` at $DIR/offset_ub.rs:22:50 error[E0080]: evaluation of constant value failed --> $SRC_DIR/core/src/ptr/const_ptr.rs:LL:COL @@ -147,11 +161,11 @@ LL | unsafe { intrinsics::offset(self, count) } | 0x7f..f is not a valid pointer | inside `ptr::const_ptr::::offset` at $SRC_DIR/core/src/ptr/const_ptr.rs:LL:COL | - ::: $DIR/offset_ub.rs:24:47 + ::: $DIR/offset_ub.rs:25:47 | LL | pub const UNDERFLOW_ABS: *const u8 = unsafe { (usize::MAX as *const u8).offset(isize::MIN) }; - | -------------------------------------------- inside `UNDERFLOW_ABS` at $DIR/offset_ub.rs:24:47 + | -------------------------------------------- inside `UNDERFLOW_ABS` at $DIR/offset_ub.rs:25:47 -error: aborting due to 11 previous errors +error: aborting due to 12 previous errors For more information about this error, try `rustc --explain E0080`.