From c4f1d3f4886d1bcb144aa5b929ca76e5d2804435 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 4 Nov 2019 12:11:31 +0100 Subject: [PATCH 1/6] test that 0 cannot be offset-inbounds by 0 --- tests/compile-fail/ptr_offset_0_plus_0.rs | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 tests/compile-fail/ptr_offset_0_plus_0.rs diff --git a/tests/compile-fail/ptr_offset_0_plus_0.rs b/tests/compile-fail/ptr_offset_0_plus_0.rs new file mode 100644 index 0000000000..4abcc5c1b8 --- /dev/null +++ b/tests/compile-fail/ptr_offset_0_plus_0.rs @@ -0,0 +1,7 @@ +// error-pattern: outside bounds of allocation + +fn main() { + let x = 0 as *mut i32; + let _x = x.wrapping_offset(8); // ok, this has no inbounds tag + let _x = unsafe { x.offset(0) }; // UB despite offset 0, NULL is never inbounds +} From 9eb4e00f6fe0f720d9bc7116bf89c20acd35f00b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 4 Nov 2019 12:13:51 +0100 Subject: [PATCH 2/6] refactor ptr_offset_inbounds: it can be reduced to check_ptr_access, after all! --- src/operator.rs | 50 ++++++++++++++----------------------------------- 1 file changed, 14 insertions(+), 36 deletions(-) diff --git a/src/operator.rs b/src/operator.rs index a60bc8c0b1..0c83b5bed9 100644 --- a/src/operator.rs +++ b/src/operator.rs @@ -1,16 +1,11 @@ use std::convert::TryFrom; -use rustc::ty::{Ty, layout::LayoutOf}; +use rustc::ty::{Ty, layout::{Size, Align, LayoutOf}}; use rustc::mir; use crate::*; pub trait EvalContextExt<'tcx> { - fn pointer_inbounds( - &self, - ptr: Pointer - ) -> InterpResult<'tcx>; - fn binary_ptr_op( &self, bin_op: mir::BinOp, @@ -33,13 +28,6 @@ pub trait EvalContextExt<'tcx> { } 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_inbounds_alloc(size, CheckInAllocMsg::InboundsTest) - } - fn binary_ptr_op( &self, bin_op: mir::BinOp, @@ -110,9 +98,8 @@ impl<'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'mir, 'tcx> { } /// Raises an error if the offset moves the pointer outside of its allocation. - /// We consider ZSTs their own huge allocation that doesn't overlap with anything (and nothing - /// moves in there because the size is 0). We also consider the NULL pointer its own separate - /// allocation, and all the remaining integers pointers their own allocation. + /// For integers, we consider each of them their own tiny allocation of size 0, + /// so offset-by-0 is okay for them -- except for NULL, which we rule out entirely. fn pointer_offset_inbounds( &self, ptr: Scalar, @@ -123,25 +110,16 @@ impl<'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'mir, 'tcx> { let offset = offset .checked_mul(pointee_size) .ok_or_else(|| err_panic!(Overflow(mir::BinOp::Mul)))?; - // Now let's see what kind of pointer this is. - let ptr = if offset == 0 { - match ptr { - Scalar::Ptr(ptr) => ptr, - Scalar::Raw { .. } => { - // Offset 0 on an integer. We accept that, pretending there is - // a little zero-sized allocation here. - return Ok(ptr); - } - } - } else { - // Offset > 0. We *require* a pointer. - self.force_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.pointer_inbounds(ptr)?; - let ptr = ptr.signed_offset(offset, self)?; - self.pointer_inbounds(ptr)?; - Ok(Scalar::Ptr(ptr)) + // We do this forst, to rule out overflows. + let offset_ptr = ptr.ptr_signed_offset(offset, self)?; + // What we need to check is that starting at `ptr`, + // we could do an access of size `offset`. Alignment does not matter. + self.memory.check_ptr_access( + ptr, + Size::from_bytes(u64::try_from(offset).unwrap()), + Align::from_bytes(1).unwrap(), + )?; + // That's it! + Ok(offset_ptr) } } From 1f8cb476eabe3b809907890e69fbf3003a8bb8dc Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 4 Nov 2019 12:17:25 +0100 Subject: [PATCH 3/6] fix test erorr msg --- tests/compile-fail/ptr_offset_0_plus_0.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/compile-fail/ptr_offset_0_plus_0.rs b/tests/compile-fail/ptr_offset_0_plus_0.rs index 4abcc5c1b8..96a9fb8402 100644 --- a/tests/compile-fail/ptr_offset_0_plus_0.rs +++ b/tests/compile-fail/ptr_offset_0_plus_0.rs @@ -1,4 +1,4 @@ -// error-pattern: outside bounds of allocation +// error-pattern: invalid use of NULL pointer fn main() { let x = 0 as *mut i32; From 4b9a7d8cffc3aefff5d660f07c90fc6f183c4357 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 4 Nov 2019 12:29:15 +0100 Subject: [PATCH 4/6] fix error message details --- src/operator.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/operator.rs b/src/operator.rs index 0c83b5bed9..dea5f1539b 100644 --- a/src/operator.rs +++ b/src/operator.rs @@ -1,6 +1,6 @@ use std::convert::TryFrom; -use rustc::ty::{Ty, layout::{Size, Align, LayoutOf}}; +use rustc::ty::{Ty, layout::{Size, LayoutOf}}; use rustc::mir; use crate::*; @@ -110,14 +110,15 @@ impl<'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'mir, 'tcx> { let offset = offset .checked_mul(pointee_size) .ok_or_else(|| err_panic!(Overflow(mir::BinOp::Mul)))?; - // We do this forst, to rule out overflows. + // We do this first, to rule out overflows. let offset_ptr = ptr.ptr_signed_offset(offset, self)?; // What we need to check is that starting at `ptr`, // we could do an access of size `offset`. Alignment does not matter. - self.memory.check_ptr_access( + self.memory.check_ptr_access_align( ptr, Size::from_bytes(u64::try_from(offset).unwrap()), - Align::from_bytes(1).unwrap(), + None, + CheckInAllocMsg::InboundsTest, )?; // That's it! Ok(offset_ptr) From b2a2f4dd152787cf506e75ec936c42b0b84a599f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 6 Nov 2019 09:25:02 +0100 Subject: [PATCH 5/6] rustup --- rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust-version b/rust-version index 5511ef0ad6..6b17a5dbeb 100644 --- a/rust-version +++ b/rust-version @@ -1 +1 @@ -3a1b3b30c6cdd674049b144a3ced7b711de962b2 +e4931eaaa3d95189b30e90d3af9f0db17c41bbb0 From 8b0c14f9f6ada1f042b3a734308bb60bdf4ac65e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 6 Nov 2019 10:51:06 +0100 Subject: [PATCH 6/6] ptr_offset: handle negative offsets --- src/operator.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/operator.rs b/src/operator.rs index dea5f1539b..2a90d3e12f 100644 --- a/src/operator.rs +++ b/src/operator.rs @@ -112,11 +112,18 @@ impl<'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'mir, 'tcx> { .ok_or_else(|| err_panic!(Overflow(mir::BinOp::Mul)))?; // We do this first, to rule out overflows. let offset_ptr = ptr.ptr_signed_offset(offset, self)?; - // What we need to check is that starting at `ptr`, - // we could do an access of size `offset`. Alignment does not matter. + // What we need to check is that starting at `min(ptr, offset_ptr)`, + // we could do an access of size `abs(offset)`. Alignment does not matter. + let (min_ptr, abs_offset) = if offset >= 0 { + (ptr, u64::try_from(offset).unwrap()) + } else { + // Negative offset. + // If the negation overflows, the result will be negative so the try_from will fail. + (offset_ptr, u64::try_from(-offset).unwrap()) + }; self.memory.check_ptr_access_align( - ptr, - Size::from_bytes(u64::try_from(offset).unwrap()), + min_ptr, + Size::from_bytes(abs_offset), None, CheckInAllocMsg::InboundsTest, )?;