fix drop typing; use same machinery for validating (sanity checking) dyn trait ptrs and slices

Author: RalfJung

src/librustc_mir/interpret/eval_context.rs

@@ -10,7 +10,7 @@ use rustc::ty::layout::{
     self, Size, Align, HasDataLayout, LayoutOf, TyLayout, Primitive
 };
 use rustc::ty::subst::{Subst, Substs};
-use rustc::ty::{self, Ty, TyCtxt};
+use rustc::ty::{self, Ty, TyCtxt, TypeFoldable};
 use rustc::ty::query::TyCtxtAt;
 use rustc_data_structures::fx::{FxHashSet, FxHasher};
 use rustc_data_structures::indexed_vec::IndexVec;
@@ -24,7 +24,7 @@ use syntax::source_map::{self, Span};
 use syntax::ast::Mutability;
 
 use super::{
-    Value, ValTy, Operand, MemPlace, MPlaceTy, Place,
+    Value, ValTy, Operand, MemPlace, MPlaceTy, Place, PlaceExtra,
     Memory, Machine
 };
 
@@ -442,10 +442,14 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M
         }
     }
 
-    pub fn monomorphize(&self, ty: Ty<'tcx>, substs: &'tcx Substs<'tcx>) -> Ty<'tcx> {
+    pub fn monomorphize<T: TypeFoldable<'tcx> + Subst<'tcx>>(
+        &self,
+        t: T,
+        substs: &'tcx Substs<'tcx>
+    ) -> T {
         // miri doesn't care about lifetimes, and will choke on some crazy ones
         // let's simply get rid of them
-        let substituted = ty.subst(*self.tcx, substs);
+        let substituted = t.subst(*self.tcx, substs);
         self.tcx.normalize_erasing_regions(ty::ParamEnv::reveal_all(), substituted)
     }
 
@@ -844,35 +848,41 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M
                 match dest.layout.ty.builtin_deref(false).map(|tam| &tam.ty.sty) {
                     | Some(ty::TyStr)
                     | Some(ty::TySlice(_)) => {
-                        // check the length
+                        // check the length (for nicer error messages)
                         let len_mplace = self.mplace_field(dest, 1)?;
                         let len = self.read_scalar(len_mplace.into())?;
                         let len = match len.to_bits(len_mplace.layout.size) {
                             Err(_) => return validation_failure!("length is not a valid integer", path),
                             Ok(len) => len as u64,
                         };
-                        // get the fat ptr
+                        // get the fat ptr, and recursively check it
                         let ptr = self.ref_to_mplace(self.read_value(dest.into())?)?;
-                        let mut path = path.clone();
-                        self.dump_field_name(&mut path, dest.layout.ty, 0, variant).unwrap();
-                        // check all fields
-                        for i in 0..len {
+                        assert_eq!(ptr.extra, PlaceExtra::Length(len));
+                        let unpacked_ptr = self.unpack_unsized_mplace(ptr)?;
+                        if seen.insert(unpacked_ptr) {
                             let mut path = path.clone();
-                            self.dump_field_name(&mut path, ptr.layout.ty, i as usize, 0).unwrap();
-                            let field = self.mplace_field(ptr, i)?;
-                            self.validate_mplace(field, path, seen, todo)?;
+                            self.dump_field_name(&mut path, dest.layout.ty, 0, 0).unwrap();
+                            todo.push((unpacked_ptr, path))
                         }
-                        // FIXME: For a TyStr, check that this is valid UTF-8
                     },
                     Some(ty::TyDynamic(..)) => {
-                        let vtable_mplace = self.mplace_field(dest, 1)?;
-                        let vtable = self.read_scalar(vtable_mplace.into())?;
-                        if vtable.to_ptr().is_err() {
-                            return validation_failure!("vtable address is not a pointer", path);
+                        // check the vtable (for nicer error messages)
+                        let vtable = self.read_scalar(self.mplace_field(dest, 1)?.into())?;
+                        let vtable = match vtable.to_ptr() {
+                            Err(_) => return validation_failure!("vtable address is not a pointer", path),
+                            Ok(vtable) => vtable,
+                        };
+                        // get the fat ptr, and recursively check it
+                        let ptr = self.ref_to_mplace(self.read_value(dest.into())?)?;
+                        assert_eq!(ptr.extra, PlaceExtra::Vtable(vtable));
+                        let unpacked_ptr = self.unpack_unsized_mplace(ptr)?;
+                        if seen.insert(unpacked_ptr) {
+                            let mut path = path.clone();
+                            self.dump_field_name(&mut path, dest.layout.ty, 0, 0).unwrap();
+                            todo.push((unpacked_ptr, path))
                         }
-                        // get the fat ptr
-                        let _ptr = self.ref_to_mplace(self.read_value(dest.into())?)?;
-                        // FIXME: What can we verify about this?
+                        // FIXME: More checks for the vtable... making sure it is exactly
+                        // the one one would expect for this type.
                     },
                     Some(ty) =>
                         bug!("Unexpected fat pointer target type {:?}", ty),
@@ -884,6 +894,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M
                             let field = self.mplace_field(dest, i as u64)?;
                             self.validate_mplace(field, path, seen, todo)?;
                         }
+                        // FIXME: For a TyStr, check that this is valid UTF-8.
                     },
                 }
 

src/librustc_mir/interpret/place.rs

@@ -721,4 +721,39 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
         };
         Ok(OpTy { op, layout: place.layout })
     }
+
+    /// Turn a place that is a dyn trait (i.e., PlaceExtra::Vtable and the appropriate layout)
+    /// or a slice into the specific fixed-size place and layout that is given by the vtable/len.
+    /// This "unpacks" the existential quantifier, so to speak.
+    pub fn unpack_unsized_mplace(&self, mplace: MPlaceTy<'tcx>) -> EvalResult<'tcx, MPlaceTy<'tcx>> {
+        trace!("Unpacking {:?} ({:?})", *mplace, mplace.layout.ty);
+        let layout = match mplace.extra {
+            PlaceExtra::Vtable(vtable) => {
+                // the drop function signature
+                let drop_instance = self.read_drop_type_from_vtable(vtable)?;
+                trace!("Found drop fn: {:?}", drop_instance);
+                let fn_sig = drop_instance.ty(*self.tcx).fn_sig(*self.tcx);
+                let fn_sig = self.tcx.normalize_erasing_late_bound_regions(self.param_env, &fn_sig);
+                // the drop function takes *mut T where T is the type being dropped, so get that
+                let ty = fn_sig.inputs()[0].builtin_deref(true).unwrap().ty;
+                let layout = self.layout_of(ty)?;
+                // Sanity checks
+                let (size, align) = self.read_size_and_align_from_vtable(vtable)?;
+                assert_eq!(size, layout.size);
+                assert_eq!(align.abi(), layout.align.abi()); // only ABI alignment is preserved
+                // Done!
+                layout
+            },
+            PlaceExtra::Length(len) => {
+                let ty = self.tcx.mk_array(mplace.layout.field(self, 0)?.ty, len);
+                self.layout_of(ty)?
+            }
+            PlaceExtra::None => bug!("Expected a fat pointer"),
+        };
+        trace!("Unpacked type: {:?}", layout.ty);
+        Ok(MPlaceTy {
+            mplace: MemPlace { extra: PlaceExtra::None, ..*mplace },
+            layout
+        })
+    }
 }

src/librustc_mir/interpret/terminator/drop.rs

@@ -1,57 +1,45 @@
 use rustc::mir::BasicBlock;
-use rustc::ty::{self, Ty, layout::LayoutOf};
+use rustc::ty::{self, layout::LayoutOf};
 use syntax::source_map::Span;
 
 use rustc::mir::interpret::{EvalResult};
-use interpret::{Machine, EvalContext, PlaceTy, Value, OpTy, Operand};
+use interpret::{Machine, EvalContext, PlaceTy, PlaceExtra, OpTy, Operand};
 
 impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
-    pub(crate) fn drop_place(
+    pub(crate) fn drop_in_place(
         &mut self,
         place: PlaceTy<'tcx>,
         instance: ty::Instance<'tcx>,
         span: Span,
         target: BasicBlock,
     ) -> EvalResult<'tcx> {
+        trace!("drop_in_place: {:?},\n  {:?}, {:?}", *place, place.layout.ty, instance);
         // We take the address of the object.  This may well be unaligned, which is fine for us here.
         // However, unaligned accesses will probably make the actual drop implementation fail -- a problem shared
         // by rustc.
-        let val = self.force_allocation(place)?.to_ref(&self);
-        self.drop(val, instance, place.layout.ty, span, target)
-    }
-
-    fn drop(
-        &mut self,
-        arg: Value,
-        instance: ty::Instance<'tcx>,
-        ty: Ty<'tcx>,
-        span: Span,
-        target: BasicBlock,
-    ) -> EvalResult<'tcx> {
-        trace!("drop: {:?},\n  {:?}, {:?}", arg, ty.sty, instance.def);
+        let place = self.force_allocation(place)?;
 
-        let (instance, arg) = match ty.sty {
+        let (instance, place) = match place.layout.ty.sty {
             ty::TyDynamic(..) => {
-                if let Value::ScalarPair(ptr, vtable) = arg {
-                    // Figure out the specific drop function to call, and just pass along
-                    // the thin part of the pointer.
-                    let instance = self.read_drop_type_from_vtable(vtable.to_ptr()?)?;
-                    trace!("Dropping via vtable: {:?}", instance.def);
-                    (instance, Value::Scalar(ptr))
-                } else {
-                    bug!("expected fat ptr, got {:?}", arg);
-                }
+                // Dropping a trait object.
+                let vtable = match place.extra {
+                    PlaceExtra::Vtable(vtable) => vtable,
+                    _ => bug!("Expected vtable when dropping {:#?}", place),
+                };
+                let place = self.unpack_unsized_mplace(place)?;
+                let instance = self.read_drop_type_from_vtable(vtable)?;
+                (instance, place)
             }
-            _ => (instance, arg),
+            _ => (instance, place),
         };
 
-        // the drop function expects a reference to the value
-        let fn_sig = self.tcx.fn_sig(instance.def_id()).skip_binder().clone();
+        let fn_sig = instance.ty(*self.tcx).fn_sig(*self.tcx);
+        let fn_sig = self.tcx.normalize_erasing_late_bound_regions(self.param_env, &fn_sig);
+
         let arg = OpTy {
-            op: Operand::Immediate(arg),
-            layout: self.layout_of(fn_sig.output())?,
+            op: Operand::Immediate(place.to_ref(&self)),
+            layout: self.layout_of(self.tcx.mk_mut_ptr(place.layout.ty))?,
         };
-        trace!("Dropped type: {:?}", fn_sig.output());
 
         // This should always be (), but getting it from the sig seems
         // easier than creating a layout of ().

src/librustc_mir/interpret/terminator/mod.rs

@@ -129,7 +129,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                 trace!("TerminatorKind::drop: {:?}, type {}", location, ty);
 
                 let instance = ::monomorphize::resolve_drop_in_place(*self.tcx, ty);
-                self.drop_place(
+                self.drop_in_place(
                     place,
                     instance,
                     terminator.source_info.span,

src/test/ui/union-ub-fat-ptr.rs

@@ -66,16 +66,18 @@ trait Trait {}
 
 // OK
 const A: &str = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 1 } }.str};
-// should lint
+// bad
 const B: &str = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.str};
+//~^ ERROR this constant likely exhibits undefined behavior
 // bad
 const C: &str = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.str};
 //~^ ERROR this constant likely exhibits undefined behavior
 
 // OK
 const A2: &[u8] = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 1 } }.slice};
-// should lint
+// bad
 const B2: &[u8] = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.slice};
+//~^ ERROR this constant likely exhibits undefined behavior
 // bad
 const C2: &[u8] = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.slice};
 //~^ ERROR this constant likely exhibits undefined behavior

src/test/ui/union-ub-fat-ptr.stderr

@@ -1,43 +1,59 @@
 error[E0080]: this constant likely exhibits undefined behavior
-  --> $DIR/union-ub-fat-ptr.rs:72:1
+  --> $DIR/union-ub-fat-ptr.rs:70:1
+   |
+LL | const B: &str = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.str};
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access at offset N, outside bounds of allocation N which has size N
+   |
+   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
+
+error[E0080]: this constant likely exhibits undefined behavior
+  --> $DIR/union-ub-fat-ptr.rs:73:1
    |
 LL | const C: &str = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.str};
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered length is not a valid integer
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
 error[E0080]: this constant likely exhibits undefined behavior
-  --> $DIR/union-ub-fat-ptr.rs:80:1
+  --> $DIR/union-ub-fat-ptr.rs:79:1
+   |
+LL | const B2: &[u8] = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.slice};
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access at offset N, outside bounds of allocation N which has size N
+   |
+   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
+
+error[E0080]: this constant likely exhibits undefined behavior
+  --> $DIR/union-ub-fat-ptr.rs:82:1
    |
 LL | const C2: &[u8] = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.slice};
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered length is not a valid integer
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
 error[E0080]: this constant likely exhibits undefined behavior
-  --> $DIR/union-ub-fat-ptr.rs:84:1
+  --> $DIR/union-ub-fat-ptr.rs:86:1
    |
 LL | const D: &Trait = unsafe { DynTransmute { repr: DynRepr { ptr: &92, vtable: &3 } }.rust};
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ tried to access memory with alignment N, but alignment N is required
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
 error[E0080]: this constant likely exhibits undefined behavior
-  --> $DIR/union-ub-fat-ptr.rs:87:1
+  --> $DIR/union-ub-fat-ptr.rs:89:1
    |
 LL | const E: &Trait = unsafe { DynTransmute { repr2: DynRepr2 { ptr: &92, vtable: &3 } }.rust};
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access at offset N, outside bounds of allocation N which has size N
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ a memory access tried to interpret some bytes as a pointer
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
 error[E0080]: this constant likely exhibits undefined behavior
-  --> $DIR/union-ub-fat-ptr.rs:90:1
+  --> $DIR/union-ub-fat-ptr.rs:92:1
    |
 LL | const F: &Trait = unsafe { DynTransmute { bad: BadDynRepr { ptr: &92, vtable: 3 } }.rust};
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered vtable address is not a pointer
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
-error: aborting due to 5 previous errors
+error: aborting due to 7 previous errors
 
 For more information about this error, try `rustc --explain E0080`.