validity in non-const mode relies on ref_to_mplace checking bounds; (de)reference hooks work on places

Author: RalfJung

src/librustc_mir/const_eval.rs

@@ -20,8 +20,8 @@ use rustc::hir::{self, def_id::DefId};
 use rustc::hir::def::Def;
 use rustc::mir::interpret::{ConstEvalErr, ErrorHandled};
 use rustc::mir;
-use rustc::ty::{self, Ty, TyCtxt, Instance, query::TyCtxtAt};
-use rustc::ty::layout::{self, Size, LayoutOf, TyLayout};
+use rustc::ty::{self, TyCtxt, Instance, query::TyCtxtAt};
+use rustc::ty::layout::{self, LayoutOf, TyLayout};
 use rustc::ty::subst::Subst;
 use rustc::traits::Reveal;
 use rustc_data_structures::indexed_vec::IndexVec;
@@ -32,7 +32,7 @@ use syntax::ast::Mutability;
 use syntax::source_map::{Span, DUMMY_SP};
 
 use interpret::{self,
-    PlaceTy, MemPlace, OpTy, Operand, Value, Pointer, Scalar, ConstValue,
+    PlaceTy, MemPlace, OpTy, Operand, Value, Scalar, ConstValue,
     EvalResult, EvalError, EvalErrorKind, GlobalId, EvalContext, StackPopCleanup,
     Allocation, AllocId, MemoryKind,
     snapshot, RefTracking,
@@ -465,28 +465,6 @@ impl<'a, 'mir, 'tcx> interpret::Machine<'a, 'mir, 'tcx>
             &ecx.stack[..],
         )
     }
-
-    #[inline(always)]
-    fn tag_reference(
-        _ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>,
-        _ptr: Pointer<Self::PointerTag>,
-        _pointee_ty: Ty<'tcx>,
-        _pointee_size: Size,
-        _borrow_kind: Option<hir::Mutability>,
-    ) -> EvalResult<'tcx, Self::PointerTag> {
-        Ok(())
-    }
-
-    #[inline(always)]
-    fn tag_dereference(
-        _ecx: &EvalContext<'a, 'mir, 'tcx, Self>,
-        _ptr: Pointer<Self::PointerTag>,
-        _pointee_ty: Ty<'tcx>,
-        _pointee_size: Size,
-        _borrow_kind: Option<hir::Mutability>,
-    ) -> EvalResult<'tcx, Self::PointerTag> {
-        Ok(())
-    }
 }
 
 /// Project to a field of a (variant of a) const

src/librustc_mir/interpret/machine.rs

@@ -21,7 +21,7 @@ use rustc::ty::{self, Ty, layout::{Size, TyLayout}, query::TyCtxtAt};
 
 use super::{
     Allocation, AllocId, EvalResult, Scalar,
-    EvalContext, PlaceTy, OpTy, Pointer, MemoryKind,
+    EvalContext, PlaceTy, OpTy, Pointer, MemPlace, MemoryKind,
 };
 
 /// Classifying memory accesses
@@ -205,26 +205,32 @@ pub trait Machine<'a, 'mir, 'tcx>: Sized {
     }
 
     /// Executed when evaluating the `&` operator: Creating a new reference.
-    /// This has the chance to adjust the tag.
+    /// This has the chance to adjust the tag.  It should not change anything else!
     /// `mutability` can be `None` in case a raw ptr is being created.
+    #[inline]
     fn tag_reference(
-        ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>,
-        ptr: Pointer<Self::PointerTag>,
-        pointee_ty: Ty<'tcx>,
-        pointee_size: Size,
-        mutability: Option<hir::Mutability>,
-    ) -> EvalResult<'tcx, Self::PointerTag>;
+        _ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>,
+        place: MemPlace<Self::PointerTag>,
+        _ty: Ty<'tcx>,
+        _size: Size,
+        _mutability: Option<hir::Mutability>,
+    ) -> EvalResult<'tcx, MemPlace<Self::PointerTag>> {
+        Ok(place)
+    }
 
     /// Executed when evaluating the `*` operator: Following a reference.
-    /// This has the change to adjust the tag.
+    /// This has the change to adjust the tag.  It should not change anything else!
     /// `mutability` can be `None` in case a raw ptr is being dereferenced.
+    #[inline]
     fn tag_dereference(
-        ecx: &EvalContext<'a, 'mir, 'tcx, Self>,
-        ptr: Pointer<Self::PointerTag>,
-        pointee_ty: Ty<'tcx>,
-        pointee_size: Size,
-        mutability: Option<hir::Mutability>,
-    ) -> EvalResult<'tcx, Self::PointerTag>;
+        _ecx: &EvalContext<'a, 'mir, 'tcx, Self>,
+        place: MemPlace<Self::PointerTag>,
+        _ty: Ty<'tcx>,
+        _size: Size,
+        _mutability: Option<hir::Mutability>,
+    ) -> EvalResult<'tcx, MemPlace<Self::PointerTag>> {
+        Ok(place)
+    }
 
     /// Execute a validation operation
     #[inline]

src/librustc_mir/interpret/place.rs

@@ -276,26 +276,25 @@ where
 
         let align = layout.align;
         let meta = val.to_meta()?;
-
-        let ptr = match val.to_scalar_ptr()? {
-            Scalar::Ptr(ptr) if M::ENABLE_PTR_TRACKING_HOOKS => {
-                // Machine might want to track the `*` operator
-                let (size, _) = self.size_and_align_of(meta, layout)?
-                    .expect("ref_to_mplace cannot determine size");
-                let mutbl = match val.layout.ty.sty {
-                    // `builtin_deref` considers boxes immutable, that's useless for our purposes
-                    ty::Ref(_, _, mutbl) => Some(mutbl),
-                    ty::Adt(def, _) if def.is_box() => Some(hir::MutMutable),
-                    ty::RawPtr(_) => None,
-                    _ => bug!("Unexpected pointer type {}", val.layout.ty.sty),
-                };
-                let tag = M::tag_dereference(self, ptr, pointee_type, size, mutbl)?;
-                Scalar::Ptr(Pointer::new_with_tag(ptr.alloc_id, ptr.offset, tag))
-            }
-            other => other,
+        let ptr = val.to_scalar_ptr()?;
+        let mplace = MemPlace { ptr, align, meta };
+        // Pointer tag tracking might want to adjust the tag.
+        let mplace = if M::ENABLE_PTR_TRACKING_HOOKS {
+            let (size, _) = self.size_and_align_of(meta, layout)?
+                // for extern types, just cover what we can
+                .unwrap_or_else(|| layout.size_and_align());
+            let mutbl = match val.layout.ty.sty {
+                // `builtin_deref` considers boxes immutable, that's useless for our purposes
+                ty::Ref(_, _, mutbl) => Some(mutbl),
+                ty::Adt(def, _) if def.is_box() => Some(hir::MutMutable),
+                ty::RawPtr(_) => None,
+                _ => bug!("Unexpected pointer type {}", val.layout.ty.sty),
+            };
+            M::tag_dereference(self, mplace, pointee_type, size, mutbl)?
+        } else {
+            mplace
         };
-
-        Ok(MPlaceTy { mplace: MemPlace { ptr, align, meta }, layout })
+        Ok(MPlaceTy { mplace, layout })
     }
 
     /// Turn a mplace into a (thin or fat) pointer, as a reference, pointing to the same space.
@@ -305,24 +304,25 @@ where
         place: MPlaceTy<'tcx, M::PointerTag>,
         borrow_kind: Option<mir::BorrowKind>,
     ) -> EvalResult<'tcx, Value<M::PointerTag>> {
-        let ptr = match place.ptr {
-            Scalar::Ptr(ptr) if M::ENABLE_PTR_TRACKING_HOOKS => {
-                // Machine might want to track the `&` operator
-                let (size, _) = self.size_and_align_of_mplace(place)?
-                    .expect("create_ref cannot determine size");
-                let mutbl = match borrow_kind {
-                    Some(mir::BorrowKind::Mut { .. }) => Some(hir::MutMutable),
-                    Some(_) => Some(hir::MutImmutable),
-                    None => None,
-                };
-                let tag = M::tag_reference(self, ptr, place.layout.ty, size, mutbl)?;
-                Scalar::Ptr(Pointer::new_with_tag(ptr.alloc_id, ptr.offset, tag))
-            },
-            other => other,
+        // Pointer tag tracking might want to adjust the tag
+        let place = if M::ENABLE_PTR_TRACKING_HOOKS {
+            let (size, _) = self.size_and_align_of_mplace(place)?
+                // for extern types, just cover what we can
+                .unwrap_or_else(|| place.layout.size_and_align());
+            let mutbl = match borrow_kind {
+                Some(mir::BorrowKind::Mut { .. }) |
+                Some(mir::BorrowKind::Unique) =>
+                    Some(hir::MutMutable),
+                Some(_) => Some(hir::MutImmutable),
+                None => None,
+            };
+            M::tag_reference(self, *place, place.layout.ty, size, mutbl)?
+        } else {
+            *place
         };
         Ok(match place.meta {
-            None => Value::Scalar(ptr.into()),
-            Some(meta) => Value::ScalarPair(ptr.into(), meta.into()),
+            None => Value::Scalar(place.ptr.into()),
+            Some(meta) => Value::ScalarPair(place.ptr.into(), meta.into()),
         })
     }
 

src/librustc_mir/interpret/validity.rs

@@ -12,7 +12,7 @@ use std::fmt::Write;
 use std::hash::Hash;
 
 use syntax_pos::symbol::Symbol;
-use rustc::ty::layout::{self, Size, Align, TyLayout};
+use rustc::ty::layout::{self, Size, Align, TyLayout, LayoutOf};
 use rustc::ty;
 use rustc_data_structures::fx::FxHashSet;
 use rustc::mir::interpret::{
@@ -176,19 +176,27 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
                     // undef. We should fix that, but let's start low.
                 }
             }
-            _ if ty.is_box() || ty.is_region_ptr() || ty.is_unsafe_ptr() => {
-                // Handle fat pointers. We also check fat raw pointers,
-                // their metadata must be valid!
-                // This also checks that the ptr itself is initialized, which
-                // seems reasonable even for raw pointers.
-                let place = try_validation!(self.ref_to_mplace(value),
-                    "undefined data in pointer", path);
+            ty::RawPtr(..) => {
+                // No undef allowed here.  Eventually this should be consistent with
+                // the integer types.
+                let _ptr = try_validation!(value.to_scalar_ptr(),
+                    "undefined address in pointer", path);
+                let _meta = try_validation!(value.to_meta(),
+                    "uninitialized data in fat pointer metadata", path);
+            }
+            _ if ty.is_box() || ty.is_region_ptr() => {
+                // Handle fat pointers.
                 // Check metadata early, for better diagnostics
-                if place.layout.is_unsized() {
-                    let tail = self.tcx.struct_tail(place.layout.ty);
+                let ptr = try_validation!(value.to_scalar_ptr(),
+                    "undefined address in pointer", path);
+                let meta = try_validation!(value.to_meta(),
+                    "uninitialized data in fat pointer metadata", path);
+                let layout = self.layout_of(value.layout.ty.builtin_deref(true).unwrap().ty)?;
+                if layout.is_unsized() {
+                    let tail = self.tcx.struct_tail(layout.ty);
                     match tail.sty {
                         ty::Dynamic(..) => {
-                            let vtable = try_validation!(place.meta.unwrap().to_ptr(),
+                            let vtable = try_validation!(meta.unwrap().to_ptr(),
                                 "non-pointer vtable in fat pointer", path);
                             try_validation!(self.read_drop_type_from_vtable(vtable),
                                 "invalid drop fn in vtable", path);
@@ -197,7 +205,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
                             // FIXME: More checks for the vtable.
                         }
                         ty::Slice(..) | ty::Str => {
-                            try_validation!(place.meta.unwrap().to_usize(self),
+                            try_validation!(meta.unwrap().to_usize(self),
                                 "non-integer slice length in fat pointer", path);
                         }
                         ty::Foreign(..) => {
@@ -207,59 +215,65 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
                             bug!("Unexpected unsized type tail: {:?}", tail),
                     }
                 }
-                // for safe ptrs, also check the ptr values itself
-                if !ty.is_unsafe_ptr() {
-                    // Make sure this is non-NULL and aligned
-                    let (size, align) = self.size_and_align_of(place.meta, place.layout)?
-                        // for the purpose of validity, consider foreign types to have
-                        // alignment and size determined by the layout (size will be 0,
-                        // alignment should take attributes into account).
-                        .unwrap_or_else(|| place.layout.size_and_align());
-                    match self.memory.check_align(place.ptr, align) {
-                        Ok(_) => {},
-                        Err(err) => match err.kind {
+                // Make sure this is non-NULL and aligned
+                let (size, align) = self.size_and_align_of(meta, layout)?
+                    // for the purpose of validity, consider foreign types to have
+                    // alignment and size determined by the layout (size will be 0,
+                    // alignment should take attributes into account).
+                    .unwrap_or_else(|| layout.size_and_align());
+                match self.memory.check_align(ptr, align) {
+                    Ok(_) => {},
+                    Err(err) => {
+                        error!("{:?} is not aligned to {:?}", ptr, align);
+                        match err.kind {
                             EvalErrorKind::InvalidNullPointerUsage =>
                                 return validation_failure!("NULL reference", path),
                             EvalErrorKind::AlignmentCheckFailed { .. } =>
                                 return validation_failure!("unaligned reference", path),
                             _ =>
                                 return validation_failure!(
                                     "dangling (out-of-bounds) reference (might be NULL at \
-                                     run-time)",
+                                        run-time)",
                                     path
                                 ),
                         }
                     }
-                    // non-ZST also have to be dereferenceable
+                }
+                // Turn ptr into place.
+                // `ref_to_mplace` also calls the machine hook for (re)activating the tag,
+                // which in turn will (in full miri) check if the pointer is dereferencable.
+                let place = self.ref_to_mplace(value)?;
+                // Recursive checking
+                if let Some(ref_tracking) = ref_tracking {
+                    assert!(const_mode, "We should only do recursie checking in const mode");
                     if size != Size::ZERO {
+                        // Non-ZST also have to be dereferencable
                         let ptr = try_validation!(place.ptr.to_ptr(),
                             "integer pointer in non-ZST reference", path);
-                        if const_mode {
-                            // Skip validation entirely for some external statics
-                            let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id);
-                            if let Some(AllocType::Static(did)) = alloc_kind {
-                                // `extern static` cannot be validated as they have no body.
-                                // FIXME: Statics from other crates are also skipped.
-                                // They might be checked at a different type, but for now we
-                                // want to avoid recursing too deeply.  This is not sound!
-                                if !did.is_local() || self.tcx.is_foreign_item(did) {
-                                    return Ok(());
-                                }
+                        // Skip validation entirely for some external statics
+                        let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id);
+                        if let Some(AllocType::Static(did)) = alloc_kind {
+                            // `extern static` cannot be validated as they have no body.
+                            // FIXME: Statics from other crates are also skipped.
+                            // They might be checked at a different type, but for now we
+                            // want to avoid recursing too deeply.  This is not sound!
+                            if !did.is_local() || self.tcx.is_foreign_item(did) {
+                                return Ok(());
                             }
                         }
+                        // Maintain the invariant that the place we are checking is
+                        // already verified to be in-bounds.
                         try_validation!(self.memory.check_bounds(ptr, size, false),
                             "dangling (not entirely in bounds) reference", path);
                     }
-                    if let Some(ref_tracking) = ref_tracking {
-                        // Check if we have encountered this pointer+layout combination
-                        // before.  Proceed recursively even for integer pointers, no
-                        // reason to skip them! They are (recursively) valid for some ZST,
-                        // but not for others (e.g. `!` is a ZST).
-                        let op = place.into();
-                        if ref_tracking.seen.insert(op) {
-                            trace!("Recursing below ptr {:#?}", *op);
-                            ref_tracking.todo.push((op, path_clone_and_deref(path)));
-                        }
+                    // Check if we have encountered this pointer+layout combination
+                    // before.  Proceed recursively even for integer pointers, no
+                    // reason to skip them! They are (recursively) valid for some ZST,
+                    // but not for others (e.g. `!` is a ZST).
+                    let op = place.into();
+                    if ref_tracking.seen.insert(op) {
+                        trace!("Recursing below ptr {:#?}", *op);
+                        ref_tracking.todo.push((op, path_clone_and_deref(path)));
                     }
                 }
             }