Rollup merge of rust-lang#63075 - RalfJung:deref-checks, r=oli-obk

Miri: Check that a ptr is aligned and inbounds already when evaluating `*` This syncs Miri with what the Nomicon and the Reference say, and resolves rust-lang/miri#447. Also this would not have worked without rust-lang#62982 due to new cycles. ;) r? @oli-obk
Centril · Aug 14, 2019 · 95894cb · 95894cb
2 parents be7f5f5 + 647c0e0
commit 95894cb
Show file tree

Hide file tree

Showing 7 changed files with 66 additions and 25 deletions.
diff --git a/src/librustc/mir/interpret/pointer.rs b/src/librustc/mir/interpret/pointer.rs
@@ -189,8 +189,11 @@ impl<'tcx, Tag> Pointer<Tag> {
         Pointer { alloc_id: self.alloc_id, offset: self.offset, tag: () }
     }
 
+    /// Test if the pointer is "inbounds" of an allocation of the given size.
+    /// A pointer is "inbounds" even if its offset is equal to the size; this is
+    /// a "one-past-the-end" pointer.
     #[inline(always)]
-    pub fn check_in_alloc(
+    pub fn check_inbounds_alloc(
         self,
         allocation_size: Size,
         msg: CheckInAllocMsg,

diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs
@@ -368,7 +368,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
                 // It is sufficient to check this for the end pointer. The addition
                 // checks for overflow.
                 let end_ptr = ptr.offset(size, self)?;
-                end_ptr.check_in_alloc(allocation_size, CheckInAllocMsg::MemoryAccessTest)?;
+                end_ptr.check_inbounds_alloc(allocation_size, CheckInAllocMsg::MemoryAccessTest)?;
                 // Test align. Check this last; if both bounds and alignment are violated
                 // we want the error to be about the bounds.
                 if let Some(align) = align {
@@ -400,7 +400,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
     ) -> bool {
         let (size, _align) = self.get_size_and_align(ptr.alloc_id, AllocCheck::MaybeDead)
             .expect("alloc info with MaybeDead cannot fail");
-        ptr.check_in_alloc(size, CheckInAllocMsg::NullPointerTest).is_err()
+        ptr.check_inbounds_alloc(size, CheckInAllocMsg::NullPointerTest).is_err()
     }
 }
 

diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs
@@ -246,7 +246,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             return Ok(None);
         }
 
-        let ptr = match self.check_mplace_access(mplace, None)? {
+        let ptr = match self.check_mplace_access(mplace, None)
+            .expect("places should be checked on creation")
+        {
             Some(ptr) => ptr,
             None => return Ok(Some(ImmTy { // zero-sized type
                 imm: Scalar::zst().into(),

diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs
@@ -277,6 +277,10 @@ where
 {
     /// Take a value, which represents a (thin or fat) reference, and make it a place.
     /// Alignment is just based on the type.  This is the inverse of `MemPlace::to_ref()`.
+    ///
+    /// Only call this if you are sure the place is "valid" (aligned and inbounds), or do not
+    /// want to ever use the place for memory access!
+    /// Generally prefer `deref_operand`.
     pub fn ref_to_mplace(
         &self,
         val: ImmTy<'tcx, M::PointerTag>,
@@ -304,7 +308,8 @@ where
     ) -> InterpResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> {
         let val = self.read_immediate(src)?;
         trace!("deref to {} on {:?}", val.layout.ty, *val);
-        self.ref_to_mplace(val)
+        let place = self.ref_to_mplace(val)?;
+        self.mplace_access_checked(place)
     }
 
     /// Check if the given place is good for memory access with the given
@@ -327,6 +332,23 @@ where
         self.memory.check_ptr_access(place.ptr, size, place.align)
     }
 
+    /// Return the "access-checked" version of this `MPlace`, where for non-ZST
+    /// this is definitely a `Pointer`.
+    pub fn mplace_access_checked(
+        &self,
+        mut place: MPlaceTy<'tcx, M::PointerTag>,
+    ) -> InterpResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> {
+        let (size, align) = self.size_and_align_of_mplace(place)?
+            .unwrap_or((place.layout.size, place.layout.align.abi));
+        assert!(place.mplace.align <= align, "dynamic alignment less strict than static one?");
+        place.mplace.align = align; // maximally strict checking
+        // When dereferencing a pointer, it must be non-NULL, aligned, and live.
+        if let Some(ptr) = self.check_mplace_access(place, Some(size))? {
+            place.mplace.ptr = ptr.into();
+        }
+        Ok(place)
+    }
+
     /// Force `place.ptr` to a `Pointer`.
     /// Can be helpful to avoid lots of `force_ptr` calls later, if this place is used a lot.
     pub fn force_mplace_ptr(
@@ -750,7 +772,9 @@ where
         // to handle padding properly, which is only correct if we never look at this data with the
         // wrong type.
 
-        let ptr = match self.check_mplace_access(dest, None)? {
+        let ptr = match self.check_mplace_access(dest, None)
+            .expect("places should be checked on creation")
+        {
             Some(ptr) => ptr,
             None => return Ok(()), // zero-sized access
         };
@@ -853,8 +877,10 @@ where
         });
         assert_eq!(src.meta, dest.meta, "Can only copy between equally-sized instances");
 
-        let src = self.check_mplace_access(src, Some(size))?;
-        let dest = self.check_mplace_access(dest, Some(size))?;
+        let src = self.check_mplace_access(src, Some(size))
+            .expect("places should be checked on creation");
+        let dest = self.check_mplace_access(dest, Some(size))
+            .expect("places should be checked on creation");
         let (src_ptr, dest_ptr) = match (src, dest) {
             (Some(src_ptr), Some(dest_ptr)) => (src_ptr, dest_ptr),
             (None, None) => return Ok(()), // zero-sized copy

diff --git a/src/librustc_mir/interpret/step.rs b/src/librustc_mir/interpret/step.rs
@@ -240,8 +240,12 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
 
             Ref(_, _, ref place) => {
                 let src = self.eval_place(place)?;
-                let val = self.force_allocation(src)?;
-                self.write_immediate(val.to_ref(), dest)?;
+                let place = self.force_allocation(src)?;
+                if place.layout.size.bytes() > 0 {
+                    // definitely not a ZST
+                    assert!(place.ptr.is_ptr(), "non-ZST places should be normalized to `Pointer`");
+                }
+                self.write_immediate(place.to_ref(), dest)?;
             }
 
             NullaryOp(mir::NullOp::Box, _) => {

diff --git a/src/test/ui/consts/const-eval/ub-nonnull.rs b/src/test/ui/consts/const-eval/ub-nonnull.rs
@@ -11,10 +11,11 @@ const NON_NULL_PTR: NonNull<u8> = unsafe { mem::transmute(&1) };
 const NULL_PTR: NonNull<u8> = unsafe { mem::transmute(0usize) };
 //~^ ERROR it is undefined behavior to use this value
 
+#[deny(const_err)] // this triggers a `const_err` so validation does not even happen
 const OUT_OF_BOUNDS_PTR: NonNull<u8> = { unsafe {
-//~^ ERROR it is undefined behavior to use this value
-    let ptr: &(u8, u8, u8) = mem::transmute(&0u8); // &0 gets promoted so it does not dangle
-    let out_of_bounds_ptr = &ptr.2; // use address-of-field for pointer arithmetic
+    let ptr: &[u8; 256] = mem::transmute(&0u8); // &0 gets promoted so it does not dangle
+    // Use address-of-element for pointer arithmetic. This could wrap around to NULL!
+    let out_of_bounds_ptr = &ptr[255]; //~ ERROR any use of this value will cause an error
     mem::transmute(out_of_bounds_ptr)
 } };
 

diff --git a/src/test/ui/consts/const-eval/ub-nonnull.stderr b/src/test/ui/consts/const-eval/ub-nonnull.stderr
@@ -6,53 +6,58 @@ LL | const NULL_PTR: NonNull<u8> = unsafe { mem::transmute(0usize) };
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
-error[E0080]: it is undefined behavior to use this value
-  --> $DIR/ub-nonnull.rs:14:1
+error: any use of this value will cause an error
+  --> $DIR/ub-nonnull.rs:18:29
    |
 LL | / const OUT_OF_BOUNDS_PTR: NonNull<u8> = { unsafe {
-LL | |
-LL | |     let ptr: &(u8, u8, u8) = mem::transmute(&0u8); // &0 gets promoted so it does not dangle
-LL | |     let out_of_bounds_ptr = &ptr.2; // use address-of-field for pointer arithmetic
+LL | |     let ptr: &[u8; 256] = mem::transmute(&0u8); // &0 gets promoted so it does not dangle
+LL | |     // Use address-of-element for pointer arithmetic. This could wrap around to NULL!
+LL | |     let out_of_bounds_ptr = &ptr[255];
+   | |                             ^^^^^^^^^ Memory access failed: pointer must be in-bounds at offset 256, but is outside bounds of allocation 6 which has size 1
 LL | |     mem::transmute(out_of_bounds_ptr)
 LL | | } };
-   | |____^ type validation failed: encountered a potentially NULL pointer, but expected something that cannot possibly fail to be greater or equal to 1
+   | |____-
    |
-   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
+note: lint level defined here
+  --> $DIR/ub-nonnull.rs:14:8
+   |
+LL | #[deny(const_err)] // this triggers a `const_err` so validation does not even happen
+   |        ^^^^^^^^^
 
 error[E0080]: it is undefined behavior to use this value
-  --> $DIR/ub-nonnull.rs:21:1
+  --> $DIR/ub-nonnull.rs:22:1
    |
 LL | const NULL_U8: NonZeroU8 = unsafe { mem::transmute(0u8) };
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something greater or equal to 1
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
 error[E0080]: it is undefined behavior to use this value
-  --> $DIR/ub-nonnull.rs:23:1
+  --> $DIR/ub-nonnull.rs:24:1
    |
 LL | const NULL_USIZE: NonZeroUsize = unsafe { mem::transmute(0usize) };
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something greater or equal to 1
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
 error[E0080]: it is undefined behavior to use this value
-  --> $DIR/ub-nonnull.rs:30:1
+  --> $DIR/ub-nonnull.rs:31:1
    |
 LL | const UNINIT: NonZeroU8 = unsafe { Transmute { uninit: () }.out };
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized bytes, but expected something greater or equal to 1
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
 error[E0080]: it is undefined behavior to use this value
-  --> $DIR/ub-nonnull.rs:38:1
+  --> $DIR/ub-nonnull.rs:39:1
    |
 LL | const BAD_RANGE1: RestrictedRange1 = unsafe { RestrictedRange1(42) };
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 42, but expected something in the range 10..=30
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
 error[E0080]: it is undefined behavior to use this value
-  --> $DIR/ub-nonnull.rs:44:1
+  --> $DIR/ub-nonnull.rs:45:1
    |
 LL | const BAD_RANGE2: RestrictedRange2 = unsafe { RestrictedRange2(20) };
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 20, but expected something less or equal to 10, or greater or equal to 30