From 1e137a796685c149903fcaa52444ab0d6a949b00 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 16 Aug 2018 00:18:09 +0200 Subject: [PATCH] fix drop typing; use same machinery for validating (sanity checking) dyn trait ptrs and slices --- src/librustc_mir/interpret/eval_context.rs | 53 +++++++++++-------- src/librustc_mir/interpret/place.rs | 35 ++++++++++++ src/librustc_mir/interpret/terminator/drop.rs | 52 +++++++----------- src/librustc_mir/interpret/terminator/mod.rs | 2 +- src/test/ui/union-ub-fat-ptr.rs | 6 ++- src/test/ui/union-ub-fat-ptr.stderr | 30 ++++++++--- 6 files changed, 115 insertions(+), 63 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 06e015cc4e6..82ff36d65ac 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -10,7 +10,7 @@ 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::ast::Mutability; use super::{ - Value, ValTy, Operand, MemPlace, MPlaceTy, Place, + Value, ValTy, Operand, MemPlace, MPlaceTy, Place, PlaceExtra, Memory, Machine }; @@ -442,10 +442,14 @@ pub fn load_mir( } } - pub fn monomorphize(&self, ty: Ty<'tcx>, substs: &'tcx Substs<'tcx>) -> Ty<'tcx> { + pub fn monomorphize + 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 @@ pub fn validate_mplace( 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 @@ pub fn validate_mplace( 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. }, } diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 555bc797f6c..b53aa4e2e65 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -721,4 +721,39 @@ pub fn place_to_op(&self, place: PlaceTy<'tcx>) -> EvalResult<'tcx, OpTy<'tcx>> }; 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 + }) + } } diff --git a/src/librustc_mir/interpret/terminator/drop.rs b/src/librustc_mir/interpret/terminator/drop.rs index 11fd7373284..598ee74fbfc 100644 --- a/src/librustc_mir/interpret/terminator/drop.rs +++ b/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 (). diff --git a/src/librustc_mir/interpret/terminator/mod.rs b/src/librustc_mir/interpret/terminator/mod.rs index e0980a4657e..5ceff57bfee 100644 --- a/src/librustc_mir/interpret/terminator/mod.rs +++ b/src/librustc_mir/interpret/terminator/mod.rs @@ -129,7 +129,7 @@ pub(super) fn eval_terminator( 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, diff --git a/src/test/ui/union-ub-fat-ptr.rs b/src/test/ui/union-ub-fat-ptr.rs index cfce92ece7a..45e70a9dbc9 100644 --- a/src/test/ui/union-ub-fat-ptr.rs +++ b/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 diff --git a/src/test/ui/union-ub-fat-ptr.stderr b/src/test/ui/union-ub-fat-ptr.stderr index f0298d9013c..924d0d667f2 100644 --- a/src/test/ui/union-ub-fat-ptr.stderr +++ b/src/test/ui/union-ub-fat-ptr.stderr @@ -1,5 +1,13 @@ 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 @@ -7,7 +15,15 @@ LL | const C: &str = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len = 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 @@ -15,7 +31,7 @@ LL | const C2: &[u8] = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, l = 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 @@ -23,21 +39,21 @@ LL | const D: &Trait = unsafe { DynTransmute { repr: DynRepr { ptr: &92, vtable: = 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`. -- GitLab