Auto merge of #54762 - RalfJung:miri-validate, r=oli-obk

Prepare miri engine for enforcing validity invariant during execution

In particular, make recursive checking of references optional, and add a `const_mode` parameter that says whether `usize` is allowed to contain a pointer. Also refactor validation a bit to be type-driven at the "leafs" (primitive types), and separately validate scalar layout to catch `NonNull` violations (which it did not properly validate before).

Fixes #53826
Also fixes #54751

r? @oli-obk

Author: bors

src/librustc/ich/impls_ty.rs

@@ -560,6 +560,10 @@ for ::mir::interpret::EvalErrorKind<'gcx, O> {
                 a.hash_stable(hcx, hasher);
                 b.hash_stable(hcx, hasher)
             },
+            FunctionRetMismatch(a, b) => {
+                a.hash_stable(hcx, hasher);
+                b.hash_stable(hcx, hasher)
+            },
             NoMirFor(ref s) => s.hash_stable(hcx, hasher),
             UnterminatedCString(ptr) => ptr.hash_stable(hcx, hasher),
             PointerOutOfBounds {

src/librustc/mir/interpret/error.rs

@@ -186,6 +186,7 @@ pub enum EvalErrorKind<'tcx, O> {
 
     FunctionAbiMismatch(Abi, Abi),
     FunctionArgMismatch(Ty<'tcx>, Ty<'tcx>),
+    FunctionRetMismatch(Ty<'tcx>, Ty<'tcx>),
     FunctionArgCountMismatch,
     NoMirFor(String),
     UnterminatedCString(Pointer),
@@ -294,7 +295,8 @@ impl<'tcx, O> EvalErrorKind<'tcx, O> {
         use self::EvalErrorKind::*;
         match *self {
             MachineError(ref inner) => inner,
-            FunctionAbiMismatch(..) | FunctionArgMismatch(..) | FunctionArgCountMismatch =>
+            FunctionAbiMismatch(..) | FunctionArgMismatch(..) | FunctionRetMismatch(..)
+            | FunctionArgCountMismatch =>
                 "tried to call a function through a function pointer of incompatible type",
             InvalidMemoryAccess =>
                 "tried to access memory through an invalid pointer",
@@ -470,6 +472,10 @@ impl<'tcx, O: fmt::Debug> fmt::Debug for EvalErrorKind<'tcx, O> {
                 write!(f, "tried to call a function with argument of type {:?} \
                            passing data of type {:?}",
                     callee_ty, caller_ty),
+            FunctionRetMismatch(caller_ty, callee_ty) =>
+                write!(f, "tried to call a function with return type {:?} \
+                           passing return place of type {:?}",
+                    callee_ty, caller_ty),
             FunctionArgCountMismatch =>
                 write!(f, "tried to call a function with incorrect number of arguments"),
             BoundsCheck { ref len, ref index } =>

src/librustc/ty/structural_impls.rs

@@ -492,6 +492,10 @@ impl<'a, 'tcx, O: Lift<'tcx>> Lift<'tcx> for interpret::EvalErrorKind<'a, O> {
                 tcx.lift(&a)?,
                 tcx.lift(&b)?,
             ),
+            FunctionRetMismatch(a, b) => FunctionRetMismatch(
+                tcx.lift(&a)?,
+                tcx.lift(&b)?,
+            ),
             FunctionArgCountMismatch => FunctionArgCountMismatch,
             NoMirFor(ref s) => NoMirFor(s.clone()),
             UnterminatedCString(ptr) => UnterminatedCString(ptr),

src/librustc_lint/builtin.rs

@@ -1558,15 +1558,13 @@ fn validate_const<'a, 'tcx>(
     let ecx = ::rustc_mir::const_eval::mk_eval_cx(tcx, gid.instance, param_env).unwrap();
     let result = (|| {
         let op = ecx.const_to_op(constant)?;
-        let mut todo = vec![(op, Vec::new())];
-        let mut seen = FxHashSet();
-        seen.insert(op);
-        while let Some((op, mut path)) = todo.pop() {
+        let mut ref_tracking = ::rustc_mir::interpret::RefTracking::new(op);
+        while let Some((op, mut path)) = ref_tracking.todo.pop() {
             ecx.validate_operand(
                 op,
                 &mut path,
-                &mut seen,
-                &mut todo,
+                Some(&mut ref_tracking),
+                /* const_mode */ true,
             )?;
         }
         Ok(())

src/librustc_mir/const_eval.rs

@@ -274,6 +274,7 @@ impl<'a, 'mir, 'tcx> interpret::Machine<'a, 'mir, 'tcx>
     type MemoryKinds = !;
 
     const MUT_STATIC_KIND: Option<!> = None; // no mutating of statics allowed
+    const ENFORCE_VALIDITY: bool = false; // for now, we don't
 
     fn find_fn(
         ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>,

src/librustc_mir/interpret/eval_context.rs

@@ -231,6 +231,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
     /// Mark a storage as live, killing the previous content and returning it.
     /// Remember to deallocate that!
     pub fn storage_live(&mut self, local: mir::Local) -> EvalResult<'tcx, LocalValue> {
+        assert!(local != mir::RETURN_PLACE, "Cannot make return place live");
         trace!("{:?} is now live", local);
 
         let layout = self.layout_of_local(self.cur_frame(), local)?;
@@ -242,6 +243,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
     /// Returns the old value of the local.
     /// Remember to deallocate that!
     pub fn storage_dead(&mut self, local: mir::Local) -> LocalValue {
+        assert!(local != mir::RETURN_PLACE, "Cannot make return place dead");
         trace!("{:?} is now dead", local);
 
         mem::replace(&mut self.frame_mut().locals[local], LocalValue::Dead)
@@ -446,6 +448,9 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
             let dummy =
                 LocalValue::Live(Operand::Immediate(Value::Scalar(ScalarMaybeUndef::Undef)));
             let mut locals = IndexVec::from_elem(dummy, &mir.local_decls);
+            // Return place is handled specially by the `eval_place` functions, and the
+            // entry in `locals` should never be used. Make it dead, to be sure.
+            locals[mir::RETURN_PLACE] = LocalValue::Dead;
             // Now mark those locals as dead that we do not want to initialize
             match self.tcx.describe_def(instance.def_id()) {
                 // statics and constants don't have `Storage*` statements, no need to look for them

src/librustc_mir/interpret/machine.rs

@@ -33,6 +33,9 @@ pub trait Machine<'a, 'mir, 'tcx>: Sized {
     /// The memory kind to use for mutated statics -- or None if those are not supported.
     const MUT_STATIC_KIND: Option<Self::MemoryKinds>;
 
+    /// Whether to enforce the validity invariant
+    const ENFORCE_VALIDITY: bool;
+
     /// Called before a basic block terminator is executed.
     /// You can use this to detect endlessly running programs.
     fn before_terminator(ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>) -> EvalResult<'tcx>;

src/librustc_mir/interpret/memory.rs

@@ -19,7 +19,7 @@
 use std::collections::VecDeque;
 use std::ptr;
 
-use rustc::ty::{self, Instance, query::TyCtxtAt};
+use rustc::ty::{self, Instance, ParamEnv, query::TyCtxtAt};
 use rustc::ty::layout::{self, Align, TargetDataLayout, Size, HasDataLayout};
 use rustc::mir::interpret::{Pointer, AllocId, Allocation, ConstValue, GlobalId,
                             EvalResult, Scalar, EvalErrorKind, AllocType, PointerArithmetic,
@@ -235,7 +235,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
         // Check non-NULL/Undef, extract offset
         let (offset, alloc_align) = match ptr {
             Scalar::Ptr(ptr) => {
-                let (size, align) = self.get_size_and_align(ptr.alloc_id)?;
+                let (size, align) = self.get_size_and_align(ptr.alloc_id);
                 // check this is not NULL -- which we can ensure only if this is in-bounds
                 // of some (potentially dead) allocation.
                 if ptr.offset > size {
@@ -284,7 +284,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
     /// If you want to check bounds before doing a memory access, be sure to
     /// check the pointer one past the end of your access, then everything will
     /// work out exactly.
-    pub fn check_bounds(&self, ptr: Pointer, access: bool) -> EvalResult<'tcx> {
+    pub fn check_bounds_ptr(&self, ptr: Pointer, access: bool) -> EvalResult<'tcx> {
         let alloc = self.get(ptr.alloc_id)?;
         let allocation_size = alloc.bytes.len() as u64;
         if ptr.offset.bytes() > allocation_size {
@@ -296,6 +296,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
         }
         Ok(())
     }
+
+    /// Check if the memory range beginning at `ptr` and of size `Size` is "in-bounds".
+    #[inline(always)]
+    pub fn check_bounds(&self, ptr: Pointer, size: Size, access: bool) -> EvalResult<'tcx> {
+        // if ptr.offset is in bounds, then so is ptr (because offset checks for overflow)
+        self.check_bounds_ptr(ptr.offset(size, &*self)?, access)
+    }
 }
 
 /// Allocation accessors
@@ -352,19 +359,28 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
         }
     }
 
-    pub fn get_size_and_align(&self, id: AllocId) -> EvalResult<'tcx, (Size, Align)> {
-        Ok(match self.get(id) {
-            Ok(alloc) => (Size::from_bytes(alloc.bytes.len() as u64), alloc.align),
-            Err(err) => match err.kind {
-                EvalErrorKind::DanglingPointerDeref =>
-                    // This should be in the dead allocation map
-                    *self.dead_alloc_map.get(&id).expect(
-                        "allocation missing in dead_alloc_map"
-                    ),
-                // E.g. a function ptr allocation
-                _ => return Err(err)
+    pub fn get_size_and_align(&self, id: AllocId) -> (Size, Align) {
+        if let Ok(alloc) = self.get(id) {
+            return (Size::from_bytes(alloc.bytes.len() as u64), alloc.align);
+        }
+        // Could also be a fn ptr or extern static
+        match self.tcx.alloc_map.lock().get(id) {
+            Some(AllocType::Function(..)) => (Size::ZERO, Align::from_bytes(1, 1).unwrap()),
+            Some(AllocType::Static(did)) => {
+                // The only way `get` couldnÄt have worked here is if this is an extern static
+                assert!(self.tcx.is_foreign_item(did));
+                // Use size and align of the type
+                let ty = self.tcx.type_of(did);
+                let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap();
+                (layout.size, layout.align)
             }
-        })
+            _ => {
+                // Must be a deallocated pointer
+                *self.dead_alloc_map.get(&id).expect(
+                    "allocation missing in dead_alloc_map"
+                )
+            }
+        }
     }
 
     pub fn get_mut(
@@ -524,8 +540,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
     ) -> EvalResult<'tcx, &[u8]> {
         assert_ne!(size.bytes(), 0, "0-sized accesses should never even get a `Pointer`");
         self.check_align(ptr.into(), align)?;
-        // if ptr.offset is in bounds, then so is ptr (because offset checks for overflow)
-        self.check_bounds(ptr.offset(size, &*self)?, true)?;
+        self.check_bounds(ptr, size, true)?;
 
         if check_defined_and_ptr {
             self.check_defined(ptr, size)?;
@@ -569,8 +584,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
     ) -> EvalResult<'tcx, &mut [u8]> {
         assert_ne!(size.bytes(), 0, "0-sized accesses should never even get a `Pointer`");
         self.check_align(ptr.into(), align)?;
-        // if ptr.offset is in bounds, then so is ptr (because offset checks for overflow)
-        self.check_bounds(ptr.offset(size, &self)?, true)?;
+        self.check_bounds(ptr, size, true)?;
 
         self.mark_definedness(ptr, size, true)?;
         self.clear_relocations(ptr, size)?;

src/librustc_mir/interpret/mod.rs

@@ -35,3 +35,5 @@ pub use self::memory::{Memory, MemoryKind};
 pub use self::machine::Machine;
 
 pub use self::operand::{ScalarMaybeUndef, Value, ValTy, Operand, OpTy};
+
+pub use self::validity::RefTracking;

src/librustc_mir/interpret/place.rs

@@ -131,6 +131,18 @@ impl MemPlace {
 }
 
 impl<'tcx> MPlaceTy<'tcx> {
+    /// Produces a MemPlace that works for ZST but nothing else
+    #[inline]
+    pub fn dangling(layout: TyLayout<'tcx>, cx: impl HasDataLayout) -> Self {
+        MPlaceTy {
+            mplace: MemPlace::from_scalar_ptr(
+                Scalar::from_uint(layout.align.abi(), cx.pointer_size()),
+                layout.align
+            ),
+            layout
+        }
+    }
+
     #[inline]
     fn from_aligned_ptr(ptr: Pointer, layout: TyLayout<'tcx>) -> Self {
         MPlaceTy { mplace: MemPlace::from_ptr(ptr, layout.align), layout }
@@ -555,6 +567,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
         dest: PlaceTy<'tcx>,
     ) -> EvalResult<'tcx> {
         trace!("write_value: {:?} <- {:?}", *dest, src_val);
+        // Check that the value actually is okay for that type
+        if M::ENFORCE_VALIDITY {
+            // Something changed somewhere, better make sure it matches the type!
+            let op = OpTy { op: Operand::Immediate(src_val), layout: dest.layout };
+            self.validate_operand(op, &mut vec![], None, /*const_mode*/false)?;
+        }
+
         // See if we can avoid an allocation. This is the counterpart to `try_read_value`,
         // but not factored as a separate function.
         let mplace = match dest.place {
@@ -576,7 +595,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
         self.write_value_to_mplace(src_val, dest)
     }
 
-    /// Write a value to memory
+    /// Write a value to memory. This does NOT do validation, so you better had already
+    /// done that before calling this!
     fn write_value_to_mplace(
         &mut self,
         value: Value,
@@ -640,12 +660,18 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
         };
         // Slow path, this does not fit into an immediate. Just memcpy.
         trace!("copy_op: {:?} <- {:?}", *dest, *src);
-        let (dest_ptr, dest_align) = self.force_allocation(dest)?.to_scalar_ptr_align();
+        let dest = self.force_allocation(dest)?;
+        let (dest_ptr, dest_align) = dest.to_scalar_ptr_align();
         self.memory.copy(
             src_ptr, src_align,
             dest_ptr, dest_align,
             src.layout.size, false
-        )
+        )?;
+        if M::ENFORCE_VALIDITY {
+            // Something changed somewhere, better make sure it matches the type!
+            self.validate_operand(dest.into(), &mut vec![], None, /*const_mode*/false)?;
+        }
+        Ok(())
     }
 
     /// Make sure that a place is in memory, and return where it is.
@@ -668,6 +694,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
                         // that has different alignment than the outer field.
                         let local_layout = self.layout_of_local(frame, local)?;
                         let ptr = self.allocate(local_layout, MemoryKind::Stack)?;
+                        // We don't have to validate as we can assume the local
+                        // was already valid for its type.
                         self.write_value_to_mplace(value, ptr)?;
                         let mplace = ptr.mplace;
                         // Update the local

src/librustc_mir/interpret/terminator.rs

@@ -287,7 +287,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
 
                 let return_place = match dest {
                     Some(place) => *place,
-                    None => Place::null(&self),
+                    None => Place::null(&self), // any access will error. good!
                 };
                 self.push_stack_frame(
                     instance,
@@ -373,6 +373,20 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
                         trace!("Caller has too many args over");
                         return err!(FunctionArgCountMismatch);
                     }
+                    // Don't forget to check the return type!
+                    if let Some(caller_ret) = dest {
+                        let callee_ret = self.eval_place(&mir::Place::Local(mir::RETURN_PLACE))?;
+                        if !Self::check_argument_compat(caller_ret.layout, callee_ret.layout) {
+                            return err!(FunctionRetMismatch(
+                                caller_ret.layout.ty, callee_ret.layout.ty
+                            ));
+                        }
+                    } else {
+                        // FIXME: The caller thinks this function cannot return. How do
+                        // we verify that the callee agrees?
+                        // On the plus side, the the callee ever writes to its return place,
+                        // that will be detected as UB (because we set that to NULL above).
+                    }
                     Ok(())
                 })();
                 match res {