Auto merge of #1031 - RalfJung:ptr-offset, r=RalfJung

Refactor ptr_offset_inbounds

I finally found a way to write this using basically just `check_ptr_access` while handling all cases (integers and pointers, offset 0 or not) correctly. This changes behavior for NULL ptrs, but I think the change is for the better.

Depends on rust-lang/rust#66081.

rust-version

@@ -1 +1 @@
-3a1b3b30c6cdd674049b144a3ced7b711de962b2
+e4931eaaa3d95189b30e90d3af9f0db17c41bbb0

src/operator.rs

@@ -1,16 +1,11 @@
 use std::convert::TryFrom;
 
-use rustc::ty::{Ty, layout::LayoutOf};
+use rustc::ty::{Ty, layout::{Size, LayoutOf}};
 use rustc::mir;
 
 use crate::*;
 
 pub trait EvalContextExt<'tcx> {
-    fn pointer_inbounds(
-        &self,
-        ptr: Pointer<Tag>
-    ) -> 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<Tag>) -> 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<Tag>,
@@ -123,25 +110,17 @@ 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 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_align(
+            ptr,
+            Size::from_bytes(u64::try_from(offset).unwrap()),
+            None,
+            CheckInAllocMsg::InboundsTest,
+        )?;
+        // That's it!
+        Ok(offset_ptr)
     }
 }

tests/compile-fail/ptr_offset_0_plus_0.rs

@@ -0,0 +1,7 @@
+// error-pattern: invalid use of NULL pointer
+
+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
+}