validate return value on stack pop

Author: RalfJung

src/librustc_mir/const_eval.rs

@@ -33,7 +33,7 @@ use rustc::mir::interpret::{
     Scalar, Allocation, AllocId, ConstValue,
 };
 use interpret::{self,
-    Place, PlaceTy, MemPlace, OpTy, Operand, Value,
+    PlaceTy, MemPlace, OpTy, Operand, Value,
     EvalContext, StackPopCleanup, MemoryKind,
     snapshot,
 };
@@ -55,13 +55,14 @@ pub fn mk_borrowck_eval_cx<'a, 'mir, 'tcx>(
     let param_env = tcx.param_env(instance.def_id());
     let mut ecx = EvalContext::new(tcx.at(span), param_env, CompileTimeInterpreter::new(), ());
     // insert a stack frame so any queries have the correct substs
+    // cannot use `push_stack_frame`; if we do `const_prop` explodes
     ecx.stack.push(interpret::Frame {
         block: mir::START_BLOCK,
         locals: IndexVec::new(),
         instance,
         span,
         mir,
-        return_place: Place::null(tcx),
+        return_place: None,
         return_to_block: StackPopCleanup::Goto(None), // never pop
         stmt: 0,
     });
@@ -82,7 +83,7 @@ pub fn mk_eval_cx<'a, 'tcx>(
         instance,
         mir.span,
         mir,
-        Place::null(tcx),
+        None,
         StackPopCleanup::Goto(None), // never pop
     )?;
     Ok(ecx)
@@ -187,7 +188,7 @@ fn eval_body_using_ecx<'mir, 'tcx>(
         cid.instance,
         mir.span,
         mir,
-        Place::Ptr(*ret),
+        Some(ret.into()),
         StackPopCleanup::None { cleanup: false },
     )?;
 

src/librustc_mir/interpret/eval_context.rs

@@ -31,7 +31,7 @@ use rustc::mir::interpret::{
 use syntax::source_map::{self, Span};
 
 use super::{
-    Value, Operand, MemPlace, MPlaceTy, Place, ScalarMaybeUndef,
+    Value, Operand, MemPlace, MPlaceTy, Place, PlaceTy, ScalarMaybeUndef,
     Memory, Machine
 };
 
@@ -73,8 +73,9 @@ pub struct Frame<'mir, 'tcx: 'mir, Tag=()> {
     /// Work to perform when returning from this function
     pub return_to_block: StackPopCleanup,
 
-    /// The location where the result of the current stack frame should be written to.
-    pub return_place: Place<Tag>,
+    /// The location where the result of the current stack frame should be written to,
+    /// and its layout in the caller.
+    pub return_place: Option<PlaceTy<'tcx, Tag>>,
 
     /// The list of locals for this stack frame, stored in order as
     /// `[return_ptr, arguments..., variables..., temporaries...]`.
@@ -97,7 +98,8 @@ pub struct Frame<'mir, 'tcx: 'mir, Tag=()> {
 #[derive(Clone, Debug, Eq, PartialEq, Hash)]
 pub enum StackPopCleanup {
     /// Jump to the next block in the caller, or cause UB if None (that's a function
-    /// that may never return).
+    /// that may never return). Also store layout of return place so
+    /// we can validate it at that layout.
     Goto(Option<mir::BasicBlock>),
     /// Just do nohing: Used by Main and for the box_alloc hook in miri.
     /// `cleanup` says whether locals are deallocated.  Static computation
@@ -424,7 +426,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
         instance: ty::Instance<'tcx>,
         span: source_map::Span,
         mir: &'mir mir::Mir<'tcx>,
-        return_place: Place<M::PointerTag>,
+        return_place: Option<PlaceTy<'tcx, M::PointerTag>>,
         return_to_block: StackPopCleanup,
     ) -> EvalResult<'tcx> {
         ::log_settings::settings().indentation += 1;
@@ -509,15 +511,38 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
             }
             StackPopCleanup::None { cleanup } => {
                 if !cleanup {
-                    // Leak the locals
+                    // Leak the locals. Also skip validation, this is only used by
+                    // static/const computation which does its own (stronger) final
+                    // validation.
                     return Ok(());
                 }
             }
         }
-        // deallocate all locals that are backed by an allocation
+        // Deallocate all locals that are backed by an allocation.
         for local in frame.locals {
             self.deallocate_local(local)?;
         }
+        // Validate the return value.
+        if let Some(return_place) = frame.return_place {
+            if M::ENFORCE_VALIDITY {
+                // Data got changed, better make sure it matches the type!
+                // It is still possible that the return place held invalid data while
+                // the function is running, but that's okay because nobody could have
+                // accessed that same data from the "outside" to observe any broken
+                // invariant -- that is, unless a function somehow has a ptr to
+                // its return place... but the way MIR is currently generated, the
+                // return place is always a local and then this cannot happen.
+                self.validate_operand(
+                    self.place_to_op(return_place)?,
+                    &mut vec![],
+                    None,
+                    /*const_mode*/false,
+                )?;
+            }
+        } else {
+            // Uh, that shouln't happen... the function did not intend to return
+            return err!(Unreachable);
+        }
 
         Ok(())
     }

src/librustc_mir/interpret/place.rs

@@ -256,12 +256,6 @@ impl<'tcx, Tag: ::std::fmt::Debug> Place<Tag> {
 }
 
 impl<'tcx, Tag: ::std::fmt::Debug> PlaceTy<'tcx, Tag> {
-    /// Produces a Place that will error if attempted to be read from or written to
-    #[inline]
-    pub fn null(cx: impl HasDataLayout, layout: TyLayout<'tcx>) -> Self {
-        PlaceTy { place: Place::from_scalar_ptr(Scalar::ptr_null(cx), layout.align), layout }
-    }
-
     #[inline]
     pub fn to_mem_place(self) -> MPlaceTy<'tcx, Tag> {
         MPlaceTy { mplace: self.place.to_mem_place(), layout: self.layout }
@@ -565,9 +559,15 @@ where
     ) -> EvalResult<'tcx, PlaceTy<'tcx, M::PointerTag>> {
         use rustc::mir::Place::*;
         let place = match *mir_place {
-            Local(mir::RETURN_PLACE) => PlaceTy {
-                place: self.frame().return_place,
-                layout: self.layout_of_local(self.cur_frame(), mir::RETURN_PLACE)?,
+            Local(mir::RETURN_PLACE) => match self.frame().return_place {
+                Some(return_place) =>
+                    // We use our layout to verify our assumption; caller will validate
+                    // their layout on return.
+                    PlaceTy {
+                        place: *return_place,
+                        layout: self.layout_of_local(self.cur_frame(), mir::RETURN_PLACE)?,
+                    },
+                None => return err!(InvalidNullPointerUsage),
             },
             Local(local) => PlaceTy {
                 place: Place::Local {

src/librustc_mir/interpret/snapshot.rs

@@ -337,7 +337,7 @@ struct FrameSnapshot<'a, 'tcx: 'a> {
     instance: &'a ty::Instance<'tcx>,
     span: &'a Span,
     return_to_block: &'a StackPopCleanup,
-    return_place: Place<(), AllocIdSnapshot<'a>>,
+    return_place: Option<Place<(), AllocIdSnapshot<'a>>>,
     locals: IndexVec<mir::Local, LocalValue<(), AllocIdSnapshot<'a>>>,
     block: &'a mir::BasicBlock,
     stmt: usize,
@@ -362,7 +362,7 @@ impl<'a, 'mir, 'tcx: 'mir> HashStable<StableHashingContext<'a>> for Frame<'mir,
         } = self;
 
         (mir, instance, span, return_to_block).hash_stable(hcx, hasher);
-        (return_place, locals, block, stmt).hash_stable(hcx, hasher);
+        (return_place.as_ref().map(|r| &**r), locals, block, stmt).hash_stable(hcx, hasher);
     }
 }
 impl<'a, 'mir, 'tcx, Ctx> Snapshot<'a, Ctx> for &'a Frame<'mir, 'tcx>
@@ -388,7 +388,7 @@ impl<'a, 'mir, 'tcx, Ctx> Snapshot<'a, Ctx> for &'a Frame<'mir, 'tcx>
             return_to_block,
             block,
             stmt: *stmt,
-            return_place: return_place.snapshot(ctx),
+            return_place: return_place.map(|r| r.snapshot(ctx)),
             locals: locals.iter().map(|local| local.snapshot(ctx)).collect(),
         }
     }

src/librustc_mir/interpret/terminator.rs

@@ -17,7 +17,7 @@ use rustc_target::spec::abi::Abi;
 
 use rustc::mir::interpret::{EvalResult, PointerArithmetic, EvalErrorKind, Scalar};
 use super::{
-    EvalContext, Machine, Value, OpTy, Place, PlaceTy, Operand, StackPopCleanup
+    EvalContext, Machine, Value, OpTy, PlaceTy, MPlaceTy, Operand, StackPopCleanup
 };
 
 impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
@@ -39,7 +39,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
         use rustc::mir::TerminatorKind::*;
         match terminator.kind {
             Return => {
-                self.dump_place(self.frame().return_place);
+                self.frame().return_place.map(|r| self.dump_place(*r));
                 self.pop_stack_frame()?
             }
 
@@ -286,15 +286,11 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
                     None => return Ok(()),
                 };
 
-                let return_place = match dest {
-                    Some(place) => *place,
-                    None => Place::null(&self), // any access will error. good!
-                };
                 self.push_stack_frame(
                     instance,
                     span,
                     mir,
-                    return_place,
+                    dest,
                     StackPopCleanup::Goto(ret),
                 )?;
 
@@ -375,17 +371,19 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
                         return err!(FunctionArgCountMismatch);
                     }
                     // Don't forget to check the return type!
-                    let callee_ret = self.eval_place(&mir::Place::Local(mir::RETURN_PLACE))?;
                     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 {
-                        if !callee_ret.layout.abi.is_uninhabited() {
+                        let callee_layout =
+                            self.layout_of_local(self.cur_frame(), mir::RETURN_PLACE)?;
+                        if !callee_layout.abi.is_uninhabited() {
                             return err!(FunctionRetMismatch(
-                                self.tcx.types.never, callee_ret.layout.ty
+                                self.tcx.types.never, callee_layout.ty
                             ));
                         }
                     }
@@ -453,14 +451,14 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
         };
 
         let ty = self.tcx.mk_unit(); // return type is ()
-        let dest = PlaceTy::null(&self, self.layout_of(ty)?);
+        let dest = MPlaceTy::dangling(self.layout_of(ty)?, &self);
 
         self.eval_fn_call(
             instance,
             span,
             Abi::Rust,
             &[arg],
-            Some(dest),
+            Some(dest.into()),
             Some(target),
         )
     }