Auto merge of #114333 - RalfJung:dangling-ptr-offset, r=oli-obk

Miri: fix error on dangling pointer inbounds offset

We used to claim that the pointer was "dereferenced", but that is just not true.

Can be reviewed commit-by-commit. The first commit is an unrelated rename that didn't seem worth splitting into its own PR.

r? `@oli-obk`

compiler/rustc_const_eval/messages.ftl

@@ -282,7 +282,7 @@ const_eval_pointer_out_of_bounds =
         *[many] bytes
     } starting at offset {$ptr_offset} is out-of-bounds
 const_eval_pointer_use_after_free =
-    pointer to {$allocation} was dereferenced after this allocation got freed
+    {$bad_pointer_message}: {$alloc_id} has been freed, so this pointer is dangling
 const_eval_ptr_as_bytes_1 =
     this code performed an operation that depends on the underlying bytes representing a pointer
 const_eval_ptr_as_bytes_2 =

compiler/rustc_const_eval/src/const_eval/machine.rs

@@ -214,9 +214,9 @@ impl<'mir, 'tcx: 'mir> CompileTimeEvalContext<'mir, 'tcx> {
             // &str or &&str
             assert!(args.len() == 1);
 
-            let mut msg_place = self.deref_operand(&args[0])?;
+            let mut msg_place = self.deref_pointer(&args[0])?;
             while msg_place.layout.ty.is_ref() {
-                msg_place = self.deref_operand(&msg_place)?;
+                msg_place = self.deref_pointer(&msg_place)?;
             }
 
             let msg = Symbol::intern(self.read_str(&msg_place)?);

compiler/rustc_const_eval/src/const_eval/valtrees.rs

@@ -102,7 +102,7 @@ pub(crate) fn const_to_valtree_inner<'tcx>(
         ty::FnPtr(_) | ty::RawPtr(_) => Err(ValTreeCreationError::NonSupportedType),
 
         ty::Ref(_, _, _)  => {
-            let Ok(derefd_place)= ecx.deref_operand(place) else {
+            let Ok(derefd_place)= ecx.deref_pointer(place) else {
                 return Err(ValTreeCreationError::Other);
             };
             debug!(?derefd_place);

compiler/rustc_const_eval/src/errors.rs

@@ -492,7 +492,7 @@ impl<'a> ReportErrorExt for UndefinedBehaviorInfo<'a> {
             InvalidMeta(InvalidMetaKind::SliceTooBig) => const_eval_invalid_meta_slice,
             InvalidMeta(InvalidMetaKind::TooBig) => const_eval_invalid_meta,
             UnterminatedCString(_) => const_eval_unterminated_c_string,
-            PointerUseAfterFree(_) => const_eval_pointer_use_after_free,
+            PointerUseAfterFree(_, _) => const_eval_pointer_use_after_free,
             PointerOutOfBounds { ptr_size: Size::ZERO, .. } => const_eval_zst_pointer_out_of_bounds,
             PointerOutOfBounds { .. } => const_eval_pointer_out_of_bounds,
             DanglingIntPointer(0, _) => const_eval_dangling_null_pointer,
@@ -545,8 +545,10 @@ impl<'a> ReportErrorExt for UndefinedBehaviorInfo<'a> {
             UnterminatedCString(ptr) | InvalidFunctionPointer(ptr) | InvalidVTablePointer(ptr) => {
                 builder.set_arg("pointer", ptr);
             }
-            PointerUseAfterFree(allocation) => {
-                builder.set_arg("allocation", allocation);
+            PointerUseAfterFree(alloc_id, msg) => {
+                builder
+                    .set_arg("alloc_id", alloc_id)
+                    .set_arg("bad_pointer_message", bad_pointer_message(msg, handler));
             }
             PointerOutOfBounds { alloc_id, alloc_size, ptr_offset, ptr_size, msg } => {
                 builder

compiler/rustc_const_eval/src/interpret/intrinsics.rs

@@ -144,7 +144,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             }
 
             sym::min_align_of_val | sym::size_of_val => {
-                // Avoid `deref_operand` -- this is not a deref, the ptr does not have to be
+                // Avoid `deref_pointer` -- this is not a deref, the ptr does not have to be
                 // dereferenceable!
                 let place = self.ref_to_mplace(&self.read_immediate(&args[0])?)?;
                 let (size, align) = self
@@ -225,7 +225,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 self.write_scalar(val, dest)?;
             }
             sym::discriminant_value => {
-                let place = self.deref_operand(&args[0])?;
+                let place = self.deref_pointer(&args[0])?;
                 let variant = self.read_discriminant(&place)?;
                 let discr = self.discriminant_for_variant(place.layout, variant)?;
                 self.write_scalar(discr, dest)?;

compiler/rustc_const_eval/src/interpret/memory.rs

@@ -317,7 +317,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                         kind = "static_mem"
                     )
                 }
-                None => err_ub!(PointerUseAfterFree(alloc_id)),
+                None => err_ub!(PointerUseAfterFree(alloc_id, CheckInAllocMsg::MemoryAccessTest)),
             }
             .into());
         };
@@ -380,7 +380,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             M::enforce_alignment(self),
             CheckInAllocMsg::MemoryAccessTest,
             |alloc_id, offset, prov| {
-                let (size, align) = self.get_live_alloc_size_and_align(alloc_id)?;
+                let (size, align) = self
+                    .get_live_alloc_size_and_align(alloc_id, CheckInAllocMsg::MemoryAccessTest)?;
                 Ok((size, align, (alloc_id, offset, prov)))
             },
         )
@@ -404,7 +405,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             CheckAlignment::Error,
             msg,
             |alloc_id, _, _| {
-                let (size, align) = self.get_live_alloc_size_and_align(alloc_id)?;
+                let (size, align) = self.get_live_alloc_size_and_align(alloc_id, msg)?;
                 Ok((size, align, ()))
             },
         )?;
@@ -414,7 +415,9 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     /// Low-level helper function to check if a ptr is in-bounds and potentially return a reference
     /// to the allocation it points to. Supports both shared and mutable references, as the actual
     /// checking is offloaded to a helper closure. `align` defines whether and which alignment check
-    /// is done. Returns `None` for size 0, and otherwise `Some` of what `alloc_size` returned.
+    /// is done.
+    ///
+    /// If this returns `None`, the size is 0; it can however return `Some` even for size 0.
     fn check_and_deref_ptr<T>(
         &self,
         ptr: Pointer<Option<M::Provenance>>,
@@ -515,7 +518,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             }
             Some(GlobalAlloc::Function(..)) => throw_ub!(DerefFunctionPointer(id)),
             Some(GlobalAlloc::VTable(..)) => throw_ub!(DerefVTablePointer(id)),
-            None => throw_ub!(PointerUseAfterFree(id)),
+            None => throw_ub!(PointerUseAfterFree(id, CheckInAllocMsg::MemoryAccessTest)),
             Some(GlobalAlloc::Static(def_id)) => {
                 assert!(self.tcx.is_static(def_id));
                 assert!(!self.tcx.is_thread_local_static(def_id));
@@ -761,11 +764,15 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         }
     }
 
-    /// Obtain the size and alignment of a live allocation.
-    pub fn get_live_alloc_size_and_align(&self, id: AllocId) -> InterpResult<'tcx, (Size, Align)> {
+    /// Obtain the size and alignment of a *live* allocation.
+    fn get_live_alloc_size_and_align(
+        &self,
+        id: AllocId,
+        msg: CheckInAllocMsg,
+    ) -> InterpResult<'tcx, (Size, Align)> {
         let (size, align, kind) = self.get_alloc_info(id);
         if matches!(kind, AllocKind::Dead) {
-            throw_ub!(PointerUseAfterFree(id))
+            throw_ub!(PointerUseAfterFree(id, msg))
         }
         Ok((size, align))
     }

compiler/rustc_const_eval/src/interpret/place.rs

@@ -419,7 +419,7 @@ where
     ///
     /// 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`.
+    /// Generally prefer `deref_pointer`.
     pub fn ref_to_mplace(
         &self,
         val: &ImmTy<'tcx, M::Provenance>,
@@ -439,8 +439,9 @@ where
     }
 
     /// Take an operand, representing a pointer, and dereference it to a place.
+    /// Corresponds to the `*` operator in Rust.
     #[instrument(skip(self), level = "debug")]
-    pub fn deref_operand(
+    pub fn deref_pointer(
         &self,
         src: &impl Readable<'tcx, M::Provenance>,
     ) -> InterpResult<'tcx, MPlaceTy<'tcx, M::Provenance>> {

compiler/rustc_const_eval/src/interpret/projection.rs

@@ -290,7 +290,7 @@ where
             OpaqueCast(ty) => base.transmute(self.layout_of(ty)?, self)?,
             Field(field, _) => self.project_field(base, field.index())?,
             Downcast(_, variant) => self.project_downcast(base, variant)?,
-            Deref => self.deref_operand(&base.to_op(self)?)?.into(),
+            Deref => self.deref_pointer(&base.to_op(self)?)?.into(),
             Index(local) => {
                 let layout = self.layout_of(self.tcx.types.usize)?;
                 let n = self.local_to_op(self.frame(), local, Some(layout))?;

compiler/rustc_const_eval/src/interpret/step.rs

@@ -224,8 +224,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
 
             Len(place) => {
                 let src = self.eval_place(place)?;
-                let op = self.place_to_op(&src)?;
-                let len = op.len(self)?;
+                let len = src.len(self)?;
                 self.write_scalar(Scalar::from_target_usize(len, self), &dest)?;
             }
 

compiler/rustc_const_eval/src/interpret/terminator.rs

@@ -661,7 +661,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 let receiver_place = loop {
                     match receiver.layout.ty.kind() {
                         ty::Ref(..) | ty::RawPtr(..) => {
-                            // We do *not* use `deref_operand` here: we don't want to conceptually
+                            // We do *not* use `deref_pointer` here: we don't want to conceptually
                             // create a place that must be dereferenceable, since the receiver might
                             // be a raw pointer and (for `*const dyn Trait`) we don't need to
                             // actually access memory to resolve this method.

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -345,6 +345,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.
         let place = self.ecx.ref_to_mplace(&self.read_immediate(value, ptr_kind.into())?)?;
         // Handle wide pointers.
         // Check metadata early, for better diagnostics
@@ -515,9 +516,6 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                 Ok(true)
             }
             ty::RawPtr(..) => {
-                // We are conservative with uninit for integers, but try to
-                // actually enforce the strict rules for raw pointers (mostly because
-                // that lets us re-use `ref_to_mplace`).
                 let place =
                     self.ecx.ref_to_mplace(&self.read_immediate(value, ExpectedKind::RawPtr)?)?;
                 if place.layout.is_unsized() {

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

@@ -282,8 +282,8 @@ pub enum UndefinedBehaviorInfo<'a> {
     InvalidMeta(InvalidMetaKind),
     /// Reading a C string that does not end within its allocation.
     UnterminatedCString(Pointer),
-    /// Dereferencing a dangling pointer after it got freed.
-    PointerUseAfterFree(AllocId),
+    /// Using a pointer after it got freed.
+    PointerUseAfterFree(AllocId, CheckInAllocMsg),
     /// 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 {

src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs

@@ -18,7 +18,7 @@ use rustc_middle::ty::{
     layout::{HasParamEnv, LayoutOf},
     Ty,
 };
-use rustc_target::abi::{Abi, Size};
+use rustc_target::abi::{Abi, Align, Size};
 
 use crate::borrow_tracker::{
     stacked_borrows::diagnostics::{AllocHistory, DiagnosticCx, DiagnosticCxBuilder},
@@ -619,6 +619,8 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
         retag_info: RetagInfo, // diagnostics info about this retag
     ) -> InterpResult<'tcx, Option<AllocId>> {
         let this = self.eval_context_mut();
+        // Ensure we bail out if the pointer goes out-of-bounds (see miri#1050).
+        this.check_ptr_access_align(place.ptr, size, Align::ONE, CheckInAllocMsg::InboundsTest)?;
 
         // It is crucial that this gets called on all code paths, to ensure we track tag creation.
         let log_creation = |this: &MiriInterpCx<'mir, 'tcx>,
@@ -707,18 +709,6 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
         let (alloc_id, base_offset, orig_tag) = this.ptr_get_alloc_id(place.ptr)?;
         log_creation(this, Some((alloc_id, base_offset, orig_tag)))?;
 
-        // Ensure we bail out if the pointer goes out-of-bounds (see miri#1050).
-        let (alloc_size, _) = this.get_live_alloc_size_and_align(alloc_id)?;
-        if base_offset + size > alloc_size {
-            throw_ub!(PointerOutOfBounds {
-                alloc_id,
-                alloc_size,
-                ptr_offset: this.target_usize_to_isize(base_offset.bytes()),
-                ptr_size: size,
-                msg: CheckInAllocMsg::InboundsTest
-            });
-        }
-
         trace!(
             "reborrow: reference {:?} derived from {:?} (pointee {}): {:?}, size {}",
             new_tag,

src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs

@@ -1,6 +1,6 @@
 use log::trace;
 
-use rustc_target::abi::{Abi, Size};
+use rustc_target::abi::{Abi, Align, Size};
 
 use crate::borrow_tracker::{AccessKind, GlobalStateInner, ProtectorKind, RetagFields};
 use rustc_middle::{
@@ -182,6 +182,8 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
         new_tag: BorTag,
     ) -> InterpResult<'tcx, Option<(AllocId, BorTag)>> {
         let this = self.eval_context_mut();
+        // Ensure we bail out if the pointer goes out-of-bounds (see miri#1050).
+        this.check_ptr_access_align(place.ptr, ptr_size, Align::ONE, CheckInAllocMsg::InboundsTest)?;
 
         // It is crucial that this gets called on all code paths, to ensure we track tag creation.
         let log_creation = |this: &MiriInterpCx<'mir, 'tcx>,
@@ -202,51 +204,33 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
         };
 
         trace!("Reborrow of size {:?}", ptr_size);
-        let (alloc_id, base_offset, parent_prov) = if ptr_size > Size::ZERO {
-            this.ptr_get_alloc_id(place.ptr)?
-        } else {
-            match this.ptr_try_get_alloc_id(place.ptr) {
-                Ok(data) => data,
-                Err(_) => {
-                    // This pointer doesn't come with an AllocId, so there's no
-                    // memory to do retagging in.
-                    trace!(
-                        "reborrow of size 0: reference {:?} derived from {:?} (pointee {})",
-                        new_tag,
-                        place.ptr,
-                        place.layout.ty,
-                    );
-                    log_creation(this, None)?;
-                    return Ok(None);
-                }
+        let (alloc_id, base_offset, parent_prov) = match this.ptr_try_get_alloc_id(place.ptr) {
+            Ok(data) => {
+                // Unlike SB, we *do* a proper retag for size 0 if can identify the allocation.
+                // After all, the pointer may be lazily initialized outside this initial range.
+                data
+            },
+            Err(_) => {
+                assert_eq!(ptr_size, Size::ZERO); // we did the deref check above, size has to be 0 here
+                // This pointer doesn't come with an AllocId, so there's no
+                // memory to do retagging in.
+                trace!(
+                    "reborrow of size 0: reference {:?} derived from {:?} (pointee {})",
+                    new_tag,
+                    place.ptr,
+                    place.layout.ty,
+                );
+                log_creation(this, None)?;
+                return Ok(None);
             }
         };
+        log_creation(this, Some((alloc_id, base_offset, parent_prov)))?;
+
         let orig_tag = match parent_prov {
             ProvenanceExtra::Wildcard => return Ok(None), // TODO: handle wildcard pointers
             ProvenanceExtra::Concrete(tag) => tag,
         };
 
-        // Protection against trying to get a reference to a vtable:
-        // vtables do not have an alloc_extra so the call to
-        // `get_alloc_extra` that follows fails.
-        let (alloc_size, _align, alloc_kind) = this.get_alloc_info(alloc_id);
-        if ptr_size == Size::ZERO && !matches!(alloc_kind, AllocKind::LiveData) {
-            return Ok(Some((alloc_id, orig_tag)));
-        }
-
-        log_creation(this, Some((alloc_id, base_offset, parent_prov)))?;
-
-        // Ensure we bail out if the pointer goes out-of-bounds (see miri#1050).
-        if base_offset + ptr_size > alloc_size {
-            throw_ub!(PointerOutOfBounds {
-                alloc_id,
-                alloc_size,
-                ptr_offset: this.target_usize_to_isize(base_offset.bytes()),
-                ptr_size,
-                msg: CheckInAllocMsg::InboundsTest
-            });
-        }
-
         trace!(
             "reborrow: reference {:?} derived from {:?} (pointee {}): {:?}, size {}",
             new_tag,

src/tools/miri/src/concurrency/sync.rs

@@ -206,7 +206,7 @@ pub(super) trait EvalContextExtPriv<'mir, 'tcx: 'mir>:
     ) -> InterpResult<'tcx, Option<Id>> {
         let this = self.eval_context_mut();
         let value_place =
-            this.deref_operand_and_offset(lock_op, offset, lock_layout, this.machine.layouts.u32)?;
+            this.deref_pointer_and_offset(lock_op, offset, lock_layout, this.machine.layouts.u32)?;
 
         // Since we are lazy, this update has to be atomic.
         let (old, success) = this