don't UB on dangling ptr deref, instead check inbounds on projections

Author: RalfJung

compiler/rustc_const_eval/messages.ftl

@@ -61,7 +61,6 @@ const_eval_deref_coercion_non_const =
     .target_note = deref defined here
 const_eval_deref_function_pointer =
     accessing {$allocation} which contains a function
-const_eval_deref_test = dereferencing pointer failed
 const_eval_deref_vtable_pointer =
     accessing {$allocation} which contains a vtable
 const_eval_different_allocations =

compiler/rustc_const_eval/src/errors.rs

@@ -459,7 +459,6 @@ fn bad_pointer_message(msg: CheckInAllocMsg, handler: &Handler) -> String {
     use crate::fluent_generated::*;
 
     let msg = match msg {
-        CheckInAllocMsg::DerefTest => const_eval_deref_test,
         CheckInAllocMsg::MemoryAccessTest => const_eval_memory_access_test,
         CheckInAllocMsg::PointerArithmeticTest => const_eval_pointer_arithmetic_test,
         CheckInAllocMsg::OffsetFromTest => const_eval_offset_from_test,

compiler/rustc_const_eval/src/interpret/intern.rs

@@ -161,7 +161,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx, const_eval::Memory
 
     #[inline(always)]
     fn ecx(&self) -> &InterpCx<'mir, 'tcx, M> {
-        &self.ecx
+        self.ecx
     }
 
     fn visit_value(&mut self, mplace: &MPlaceTy<'tcx>) -> InterpResult<'tcx> {

compiler/rustc_const_eval/src/interpret/intrinsics.rs

@@ -571,16 +571,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     pub fn ptr_offset_inbounds(
         &self,
         ptr: Pointer<Option<M::Provenance>>,
-        pointee_ty: Ty<'tcx>,
-        offset_count: i64,
+        offset_bytes: i64,
     ) -> InterpResult<'tcx, Pointer<Option<M::Provenance>>> {
-        // We cannot overflow i64 as a type's size must be <= isize::MAX.
-        let pointee_size = i64::try_from(self.layout_of(pointee_ty)?.size.bytes()).unwrap();
-        // The computed offset, in bytes, must not overflow an isize.
-        // `checked_mul` enforces a too small bound, but no actual allocation can be big enough for
-        // the difference to be noticeable.
-        let offset_bytes =
-            offset_count.checked_mul(pointee_size).ok_or(err_ub!(PointerArithOverflow))?;
         // The offset being in bounds cannot rely on "wrapping around" the address space.
         // So, first rule out overflows in the pointer arithmetic.
         let offset_ptr = ptr.signed_offset(offset_bytes, self)?;

compiler/rustc_const_eval/src/interpret/machine.rs

@@ -436,6 +436,7 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
         place: &PlaceTy<'tcx, Self::Provenance>,
     ) -> InterpResult<'tcx> {
         // Without an aliasing model, all we can do is put `Uninit` into the place.
+        // Conveniently this also ensures that the place actually points to suitable memory.
         ecx.write_uninit(place)
     }
 

compiler/rustc_const_eval/src/interpret/operand.rs

@@ -219,6 +219,17 @@ impl<'tcx, Prov: Provenance> ImmTy<'tcx, Prov> {
     /// given layout.
     // Not called `offset` to avoid confusion with the trait method.
     fn offset_(&self, offset: Size, layout: TyAndLayout<'tcx>, cx: &impl HasDataLayout) -> Self {
+        debug_assert!(layout.is_sized(), "unsized immediates are not a thing");
+        // `ImmTy` have already been checked to be in-bounds, so we can just check directly if this
+        // remains in-bounds. This cannot actually be violated since projections are type-checked
+        // and bounds-checked.
+        assert!(
+            offset + layout.size <= self.layout.size,
+            "attempting to project to field at offset {} with size {} into immediate with layout {:#?}",
+            offset.bytes(),
+            layout.size.bytes(),
+            self.layout,
+        );
         // This makes several assumptions about what layouts we will encounter; we match what
         // codegen does as good as we can (see `extract_field` in `rustc_codegen_ssa/src/mir/operand.rs`).
         let inner_val: Immediate<_> = match (**self, self.layout.abi) {
@@ -387,7 +398,6 @@ impl<'tcx, Prov: Provenance> Projectable<'tcx, Prov> for OpTy<'tcx, Prov> {
         match self.as_mplace_or_imm() {
             Left(mplace) => Ok(mplace.offset_with_meta(offset, meta, layout, ecx)?.into()),
             Right(imm) => {
-                debug_assert!(layout.is_sized(), "unsized immediates are not a thing");
                 assert_matches!(meta, MemPlaceMeta::None); // no place to store metadata here
                 // Every part of an uninit is uninit.
                 Ok(imm.offset_(offset, layout, ecx).into())

compiler/rustc_const_eval/src/interpret/operator.rs

@@ -1,7 +1,7 @@
 use rustc_apfloat::{Float, FloatConvert};
 use rustc_middle::mir;
 use rustc_middle::mir::interpret::{InterpResult, Scalar};
-use rustc_middle::ty::layout::TyAndLayout;
+use rustc_middle::ty::layout::{LayoutOf, TyAndLayout};
 use rustc_middle::ty::{self, FloatTy, Ty};
 use rustc_span::symbol::sym;
 use rustc_target::abi::Abi;
@@ -337,7 +337,15 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 let offset_count = right.to_scalar().to_target_isize(self)?;
                 let pointee_ty = left.layout.ty.builtin_deref(true).unwrap().ty;
 
-                let offset_ptr = self.ptr_offset_inbounds(ptr, pointee_ty, offset_count)?;
+                // We cannot overflow i64 as a type's size must be <= isize::MAX.
+                let pointee_size = i64::try_from(self.layout_of(pointee_ty)?.size.bytes()).unwrap();
+                // The computed offset, in bytes, must not overflow an isize.
+                // `checked_mul` enforces a too small bound, but no actual allocation can be big enough for
+                // the difference to be noticeable.
+                let offset_bytes =
+                    offset_count.checked_mul(pointee_size).ok_or(err_ub!(PointerArithOverflow))?;
+
+                let offset_ptr = self.ptr_offset_inbounds(ptr, offset_bytes)?;
                 Ok((
                     ImmTy::from_scalar(Scalar::from_maybe_pointer(offset_ptr, self), left.layout),
                     false,

compiler/rustc_const_eval/src/interpret/place.rs

@@ -15,9 +15,9 @@ use rustc_middle::ty::Ty;
 use rustc_target::abi::{Abi, Align, FieldIdx, HasDataLayout, Size, FIRST_VARIANT};
 
 use super::{
-    alloc_range, mir_assign_valid_types, AllocId, AllocRef, AllocRefMut, CheckInAllocMsg, ImmTy,
-    Immediate, InterpCx, InterpResult, Machine, MemoryKind, OpTy, Operand, Pointer,
-    PointerArithmetic, Projectable, Provenance, Readable, Scalar,
+    alloc_range, mir_assign_valid_types, AllocId, AllocRef, AllocRefMut, ImmTy, Immediate,
+    InterpCx, InterpResult, Machine, MemoryKind, OpTy, Operand, Pointer, PointerArithmetic,
+    Projectable, Provenance, Readable, Scalar,
 };
 
 #[derive(Copy, Clone, Hash, PartialEq, Eq, Debug)]
@@ -88,17 +88,22 @@ impl<Prov: Provenance> MemPlace<Prov> {
 
     #[inline]
     // Not called `offset_with_meta` to avoid confusion with the trait method.
-    fn offset_with_meta_<'tcx>(
+    fn offset_with_meta_<'mir, 'tcx, M: Machine<'mir, 'tcx, Provenance = Prov>>(
         self,
         offset: Size,
         meta: MemPlaceMeta<Prov>,
-        cx: &impl HasDataLayout,
+        ecx: &InterpCx<'mir, 'tcx, M>,
     ) -> InterpResult<'tcx, Self> {
         debug_assert!(
             !meta.has_meta() || self.meta.has_meta(),
             "cannot use `offset_with_meta` to add metadata to a place"
         );
-        Ok(MemPlace { ptr: self.ptr.offset(offset, cx)?, meta })
+        if offset > ecx.data_layout().max_size_of_val() {
+            throw_ub!(PointerArithOverflow);
+        }
+        let offset: i64 = offset.bytes().try_into().unwrap();
+        let ptr = ecx.ptr_offset_inbounds(self.ptr, offset)?;
+        Ok(MemPlace { ptr, meta })
     }
 }
 
@@ -310,15 +315,18 @@ impl<'tcx, Prov: Provenance> Projectable<'tcx, Prov> for PlaceTy<'tcx, Prov> {
             Right((frame, local, old_offset)) => {
                 debug_assert!(layout.is_sized(), "unsized locals should live in memory");
                 assert_matches!(meta, MemPlaceMeta::None); // we couldn't store it anyway...
-                let new_offset = ecx
-                    .data_layout()
-                    .offset(old_offset.unwrap_or(Size::ZERO).bytes(), offset.bytes())?;
+                // `Place::Local` are always in-bounds of their surrounding local, so we can just
+                // check directly if this remains in-bounds. This cannot actually be violated since
+                // projections are type-checked and bounds-checked.
+                assert!(offset + layout.size <= self.layout.size);
+
+                let new_offset = Size::from_bytes(
+                    ecx.data_layout()
+                        .offset(old_offset.unwrap_or(Size::ZERO).bytes(), offset.bytes())?,
+                );
+
                 PlaceTy {
-                    place: Place::Local {
-                        frame,
-                        local,
-                        offset: Some(Size::from_bytes(new_offset)),
-                    },
+                    place: Place::Local { frame, local, offset: Some(new_offset) },
                     align: self.align.restrict_for_offset(offset),
                     layout,
                 }
@@ -464,7 +472,6 @@ where
         }
 
         let mplace = self.ref_to_mplace(&val)?;
-        self.check_mplace(&mplace)?;
         Ok(mplace)
     }
 
@@ -494,17 +501,6 @@ where
         self.get_ptr_alloc_mut(mplace.ptr(), size, mplace.align)
     }
 
-    /// Check if this mplace is dereferenceable and sufficiently aligned.
-    pub fn check_mplace(&self, mplace: &MPlaceTy<'tcx, M::Provenance>) -> InterpResult<'tcx> {
-        let (size, _align) = self
-            .size_and_align_of_mplace(&mplace)?
-            .unwrap_or((mplace.layout.size, mplace.layout.align.abi));
-        // Due to packed places, only `mplace.align` matters.
-        let align = if M::enforce_alignment(self) { mplace.align } else { Align::ONE };
-        self.check_ptr_access_align(mplace.ptr(), size, align, CheckInAllocMsg::DerefTest)?;
-        Ok(())
-    }
-
     /// Converts a repr(simd) place into a place where `place_index` accesses the SIMD elements.
     /// Also returns the number of elements.
     pub fn mplace_to_simd(

compiler/rustc_const_eval/src/interpret/projection.rs

@@ -58,7 +58,6 @@ pub trait Projectable<'tcx, Prov: Provenance>: Sized + std::fmt::Debug {
         ecx: &InterpCx<'mir, 'tcx, M>,
     ) -> InterpResult<'tcx, Self>;
 
-    #[inline]
     fn offset<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>(
         &self,
         offset: Size,
@@ -69,7 +68,6 @@ pub trait Projectable<'tcx, Prov: Provenance>: Sized + std::fmt::Debug {
         self.offset_with_meta(offset, MemPlaceMeta::None, layout, ecx)
     }
 
-    #[inline]
     fn transmute<'mir, M: Machine<'mir, 'tcx, Provenance = Prov>>(
         &self,
         layout: TyAndLayout<'tcx>,

compiler/rustc_const_eval/src/interpret/terminator.rs

@@ -1,6 +1,5 @@
 use std::borrow::Cow;
 
-use either::Either;
 use rustc_ast::ast::InlineAsmOptions;
 use rustc_middle::{
     mir,
@@ -729,13 +728,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                             callee_ty: callee_fn_abi.ret.layout.ty
                         });
                     }
-                    // Ensure the return place is aligned and dereferenceable, and protect it for
-                    // in-place return value passing.
-                    if let Either::Left(mplace) = destination.as_mplace_or_local() {
-                        self.check_mplace(&mplace)?;
-                    } else {
-                        // Nothing to do for locals, they are always properly allocated and aligned.
-                    }
+                    // Protect return place for in-place return value passing.
                     M::protect_in_place_function_argument(self, destination)?;
 
                     // Don't forget to mark "initially live" locals as live.

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -355,7 +355,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
         value: &OpTy<'tcx, M::Provenance>,
         ptr_kind: PointerKind,
     ) -> InterpResult<'tcx> {
-        // Not using `deref_pointer` since we do the dereferenceable check ourselves below.
+        // Not using `deref_pointer` since we want to use our `read_immediate` wrapper.
         let place = self.ecx.ref_to_mplace(&self.read_immediate(value, ptr_kind.into())?)?;
         // Handle wide pointers.
         // Check metadata early, for better diagnostics
@@ -645,7 +645,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
 
     #[inline(always)]
     fn ecx(&self) -> &InterpCx<'mir, 'tcx, M> {
-        &self.ecx
+        self.ecx
     }
 
     fn read_discriminant(

compiler/rustc_middle/src/mir/interpret/error.rs

@@ -218,8 +218,6 @@ pub enum InvalidProgramInfo<'tcx> {
 /// Details of why a pointer had to be in-bounds.
 #[derive(Debug, Copy, Clone, TyEncodable, TyDecodable, HashStable)]
 pub enum CheckInAllocMsg {
-    /// We are dereferencing a pointer (i.e., creating a place).
-    DerefTest,
     /// We are access memory.
     MemoryAccessTest,
     /// We are doing pointer arithmetic.

src/tools/miri/src/helpers.rs

@@ -700,23 +700,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
 
         let mplace = MPlaceTy::from_aligned_ptr(ptr, layout);
 
-        this.check_mplace(&mplace)?;
-
-        Ok(mplace)
-    }
-
-    /// Deref' a pointer *without* checking that the place is dereferenceable.
-    fn deref_pointer_unchecked(
-        &self,
-        val: &ImmTy<'tcx, Provenance>,
-        layout: TyAndLayout<'tcx>,
-    ) -> InterpResult<'tcx, MPlaceTy<'tcx, Provenance>> {
-        let this = self.eval_context_ref();
-        let mut mplace = this.ref_to_mplace(val)?;
-
-        mplace.layout = layout;
-        mplace.align = layout.align.abi;
-
         Ok(mplace)
     }
 

src/tools/miri/src/machine.rs

@@ -1285,6 +1285,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
         // We do need to write `uninit` so that even after the call ends, the former contents of
         // this place cannot be observed any more. We do the write after retagging so that for
         // Tree Borrows, this is considered to activate the new tag.
+        // Conveniently this also ensures that the place actually points to suitable memory.
         ecx.write_uninit(&protected_place)?;
         // Now we throw away the protected place, ensuring its tag is never used again.
         Ok(())

src/tools/miri/src/shims/unix/linux/sync.rs

@@ -85,9 +85,8 @@ pub fn futex<'tcx>(
                 return Ok(());
             }
 
-            // `read_timespec` will check the place when it is not null.
-            let timeout = this.deref_pointer_unchecked(
-                &this.read_immediate(&args[3])?,
+            let timeout = this.deref_pointer_as(
+                &args[3],
                 this.libc_ty_layout("timespec"),
             )?;
             let timeout_time = if this.ptr_is_null(timeout.ptr())? {

src/tools/miri/tests/compiletest.rs

@@ -181,6 +181,7 @@ regexes! {
     r"0x[0-9a-fA-F]+[0-9a-fA-F]{2,2}" => "$$HEX",
     // erase specific alignments
     "alignment [0-9]+"               => "alignment ALIGN",
+    "[0-9]+ byte alignment but found [0-9]+" => "ALIGN byte alignment but found ALIGN",
     // erase thread caller ids
     r"call [0-9]+"                  => "call ID",
     // erase platform module paths

src/tools/miri/tests/fail-dep/shims/mmap_use_after_munmap.stderr

@@ -13,11 +13,11 @@ LL |         libc::munmap(ptr, 4096);
    = note: BACKTRACE:
    = note: inside `main` at $DIR/mmap_use_after_munmap.rs:LL:CC
 
-error: Undefined Behavior: dereferencing pointer failed: ALLOC has been freed, so this pointer is dangling
+error: Undefined Behavior: memory access failed: ALLOC has been freed, so this pointer is dangling
   --> $DIR/mmap_use_after_munmap.rs:LL:CC
    |
 LL |         let _x = *(ptr as *mut u8);
-   |                  ^^^^^^^^^^^^^^^^^ dereferencing pointer failed: ALLOC has been freed, so this pointer is dangling
+   |                  ^^^^^^^^^^^^^^^^^ memory access failed: ALLOC has been freed, so this pointer is dangling
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

src/tools/miri/tests/fail/alloc/reallocate-change-alloc.stderr

@@ -1,8 +1,8 @@
-error: Undefined Behavior: dereferencing pointer failed: ALLOC has been freed, so this pointer is dangling
+error: Undefined Behavior: memory access failed: ALLOC has been freed, so this pointer is dangling
   --> $DIR/reallocate-change-alloc.rs:LL:CC
    |
 LL |         let _z = *x;
-   |                  ^^ dereferencing pointer failed: ALLOC has been freed, so this pointer is dangling
+   |                  ^^ memory access failed: ALLOC has been freed, so this pointer is dangling
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

src/tools/miri/tests/fail/concurrency/thread_local_static_dealloc.stderr

@@ -1,8 +1,8 @@
-error: Undefined Behavior: dereferencing pointer failed: ALLOC has been freed, so this pointer is dangling
+error: Undefined Behavior: memory access failed: ALLOC has been freed, so this pointer is dangling
   --> $DIR/thread_local_static_dealloc.rs:LL:CC
    |
 LL |         let _val = *dangling_ptr.0;
-   |                    ^^^^^^^^^^^^^^^ dereferencing pointer failed: ALLOC has been freed, so this pointer is dangling
+   |                    ^^^^^^^^^^^^^^^ memory access failed: ALLOC has been freed, so this pointer is dangling
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

src/tools/miri/tests/fail/dangling_pointers/dangling_pointer_addr_of.rs

@@ -1,8 +1,8 @@
-error: Undefined Behavior: dereferencing pointer failed: ALLOC has been freed, so this pointer is dangling
+error: Undefined Behavior: memory access failed: ALLOC has been freed, so this pointer is dangling
   --> $DIR/dangling_pointer_deref.rs:LL:CC
    |
 LL |     let x = unsafe { *p };
-   |                      ^^ dereferencing pointer failed: ALLOC has been freed, so this pointer is dangling
+   |                      ^^ memory access failed: ALLOC has been freed, so this pointer is dangling
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information