From 5b75ec0a91f548a5a37afc050474ce47f3e74a40 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 9 Oct 2018 17:06:57 +0200 Subject: [PATCH 01/12] fix validation around transmuting copy_op --- src/librustc_mir/interpret/cast.rs | 3 +- src/librustc_mir/interpret/intrinsics.rs | 6 +- src/librustc_mir/interpret/place.rs | 153 +++++++++++++++++++---- src/librustc_mir/interpret/terminator.rs | 3 +- src/librustc_mir/interpret/validity.rs | 4 +- 5 files changed, 136 insertions(+), 33 deletions(-) diff --git a/src/librustc_mir/interpret/cast.rs b/src/librustc_mir/interpret/cast.rs index bfc7e6801fc46..b5137e914dc91 100644 --- a/src/librustc_mir/interpret/cast.rs +++ b/src/librustc_mir/interpret/cast.rs @@ -322,7 +322,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // For now, upcasts are limited to changes in marker // traits, and hence never actually require an actual // change to the vtable. - self.copy_op(src, dest) + let val = self.read_value(src)?; + self.write_value(*val, dest) } (_, &ty::Dynamic(ref data, _)) => { // Initial cast from sized to dyn trait diff --git a/src/librustc_mir/interpret/intrinsics.rs b/src/librustc_mir/interpret/intrinsics.rs index a669b2aafc2b8..5fa0fef36935d 100644 --- a/src/librustc_mir/interpret/intrinsics.rs +++ b/src/librustc_mir/interpret/intrinsics.rs @@ -151,11 +151,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> self.write_scalar(val, dest)?; } "transmute" => { - // Go through an allocation, to make sure the completely different layouts - // do not pose a problem. (When the user transmutes through a union, - // there will not be a layout mismatch.) - let dest = self.force_allocation(dest)?; - self.copy_op(args[0], dest.into())?; + self.copy_op_transmute(args[0], dest)?; } _ => return Ok(false), diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 8b9c6a5a27053..707857c809b2a 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -599,18 +599,47 @@ where } /// Write a value to a place + #[inline(always)] pub fn write_value( &mut self, src_val: Value, dest: PlaceTy<'tcx, M::PointerTag>, ) -> EvalResult<'tcx> { - trace!("write_value: {:?} <- {:?}", *dest, src_val); - // Check that the value actually is okay for that type + self.write_value_no_validate(src_val, dest)?; + 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)?; + // Data got changed, better make sure it matches the type! + self.validate_operand(self.place_to_op(dest)?, &mut vec![], None, /*const_mode*/false)?; + } + + Ok(()) + } + + /// Write a value to a place. + /// If you use this you are responsible for validating that things got copied at the + /// right type. + fn write_value_no_validate( + &mut self, + src_val: Value, + dest: PlaceTy<'tcx, M::PointerTag>, + ) -> EvalResult<'tcx> { + if cfg!(debug_assertions) { + // This is a very common path, avoid some checks in release mode + assert!(!dest.layout.is_unsized(), "Cannot write unsized data"); + match src_val { + Value::Scalar(ScalarMaybeUndef::Scalar(Scalar::Ptr(_))) => + assert_eq!(self.pointer_size(), dest.layout.size, + "Size mismatch when writing pointer"), + Value::Scalar(ScalarMaybeUndef::Scalar(Scalar::Bits { size, .. })) => + assert_eq!(Size::from_bytes(size.into()), dest.layout.size, + "Size mismatch when writing bits"), + Value::Scalar(ScalarMaybeUndef::Undef) => {}, // undef can have any size + Value::ScalarPair(_, _) => { + // FIXME: Can we check anything here? + } + } } + trace!("write_value: {:?} <- {:?}: {}", *dest, src_val, dest.layout.ty); // See if we can avoid an allocation. This is the counterpart to `try_read_value`, // but not factored as a separate function. @@ -627,15 +656,16 @@ where }, Place::Ptr(mplace) => mplace, // already in memory }; + let dest = MPlaceTy { mplace, layout: dest.layout }; // This is already in memory, write there. - let dest = MPlaceTy { mplace, layout: dest.layout }; - self.write_value_to_mplace(src_val, dest) + self.write_value_to_mplace_no_validate(src_val, dest) } - /// 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( + /// Write a value to memory. + /// If you use this you are responsible for validating that things git copied at the + /// right type. + fn write_value_to_mplace_no_validate( &mut self, value: Value, dest: MPlaceTy<'tcx, M::PointerTag>, @@ -653,8 +683,17 @@ where } let ptr = ptr.to_ptr()?; + // FIXME: We should check that there are dest.layout.size many bytes available in + // memory. The code below is not sufficient, with enough padding it might not + // cover all the bytes! match value { Value::Scalar(scalar) => { + match dest.layout.abi { + layout::Abi::Scalar(_) => {}, // fine + _ => bug!("write_value_to_mplace: invalid Scalar layout: {:#?}", + dest.layout) + } + self.memory.write_scalar( ptr, ptr_align.min(dest.layout.align), scalar, dest.layout.size ) @@ -670,45 +709,109 @@ where let b_offset = a_size.abi_align(b_align); let b_ptr = ptr.offset(b_offset, &self)?.into(); + // It is tempting to verify `b_offset` against `layout.fields.offset(1)`, + // but that does not work: We could be a newtype around a pair, then the + // fields do not match the `ScalarPair` components. + self.memory.write_scalar(ptr, ptr_align.min(a_align), a_val, a_size)?; self.memory.write_scalar(b_ptr, ptr_align.min(b_align), b_val, b_size) } } } - /// Copy the data from an operand to a place + /// Copy the data from an operand to a place. This does not support transmuting! + /// Use `copy_op_transmute` if the layouts could disagree. + #[inline(always)] pub fn copy_op( &mut self, src: OpTy<'tcx, M::PointerTag>, dest: PlaceTy<'tcx, M::PointerTag>, ) -> EvalResult<'tcx> { - assert!(!src.layout.is_unsized() && !dest.layout.is_unsized(), + self.copy_op_no_validate(src, dest)?; + + if M::ENFORCE_VALIDITY { + // Data got changed, better make sure it matches the type! + self.validate_operand(self.place_to_op(dest)?, &mut vec![], None, /*const_mode*/false)?; + } + + Ok(()) + } + + /// Copy the data from an operand to a place. This does not support transmuting! + /// Use `copy_op_transmute` if the layouts could disagree. + /// Also, if you use this you are responsible for validating that things git copied at the + /// right type. + fn copy_op_no_validate( + &mut self, + src: OpTy<'tcx, M::PointerTag>, + dest: PlaceTy<'tcx, M::PointerTag>, + ) -> EvalResult<'tcx> { + debug_assert!(!src.layout.is_unsized() && !dest.layout.is_unsized(), "Cannot copy unsized data"); - assert_eq!(src.layout.size, dest.layout.size, - "Size mismatch when copying!\nsrc: {:#?}\ndest: {:#?}", src, dest); + // We do NOT compare the types for equality, because well-typed code can + // actually "transmute" `&mut T` to `&T` in an assignment without a cast. + assert!(src.layout.details == dest.layout.details, + "Layout mismatch when copying!\nsrc: {:#?}\ndest: {:#?}", src, dest); // Let us see if the layout is simple so we take a shortcut, avoid force_allocation. - let (src_ptr, src_align) = match self.try_read_value(src)? { - Ok(src_val) => - // Yay, we got a value that we can write directly. We write with the - // *source layout*, because that was used to load, and if they do not match - // this is a transmute we want to support. - return self.write_value(src_val, PlaceTy { place: *dest, layout: src.layout }), - Err(mplace) => mplace.to_scalar_ptr_align(), + let src = match self.try_read_value(src)? { + Ok(src_val) => { + // Yay, we got a value that we can write directly. + return self.write_value_no_validate(src_val, dest); + } + Err(mplace) => mplace, }; // Slow path, this does not fit into an immediate. Just memcpy. - trace!("copy_op: {:?} <- {:?}", *dest, *src); + trace!("copy_op: {:?} <- {:?}: {}", *dest, src, dest.layout.ty); + let dest = self.force_allocation(dest)?; + let (src_ptr, src_align) = src.to_scalar_ptr_align(); 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 + dest.layout.size, false )?; + + Ok(()) + } + + /// Copy the data from an operand to a place. The layouts may disagree, but they must + /// have the same size. + pub fn copy_op_transmute( + &mut self, + src: OpTy<'tcx, M::PointerTag>, + dest: PlaceTy<'tcx, M::PointerTag>, + ) -> EvalResult<'tcx> { + if src.layout.details == dest.layout.details { + // Fast path: Just use normal `copy_op` + return self.copy_op(src, dest); + } + // We still require the sizes to match + debug_assert!(!src.layout.is_unsized() && !dest.layout.is_unsized(), + "Cannot copy unsized data"); + assert!(src.layout.size == dest.layout.size, + "Size mismatch when transmuting!\nsrc: {:#?}\ndest: {:#?}", src, dest); + + // The hard case is `ScalarPair`. `src` is already read from memory in this case, + // using `src.layout` to figure out which bytes to use for the 1st and 2nd field. + // We have to write them to `dest` at the offsets they were *read at*, which is + // not necessarily the same as the offsets in `dest.layout`! + // Hence we do the copy with the source layout on both sides. We also make sure to write + // into memory, because if `dest` is a local we would not even have a way to write + // at the `src` offsets; the fact that we came from a different layout would + // just be lost. + let dest = self.force_allocation(dest)?; + self.copy_op_no_validate( + src, + PlaceTy::from(MPlaceTy { mplace: *dest, layout: src.layout }), + )?; + if M::ENFORCE_VALIDITY { - // Something changed somewhere, better make sure it matches the type! + // Data got changed, better make sure it matches the type! self.validate_operand(dest.into(), &mut vec![], None, /*const_mode*/false)?; } + Ok(()) } @@ -734,7 +837,7 @@ where 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)?; + self.write_value_to_mplace_no_validate(value, ptr)?; let mplace = ptr.mplace; // Update the local *self.stack[frame].locals[local].access_mut()? = diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index e599608b2dac9..0ff18b0dd0b20 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -222,7 +222,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> if !Self::check_argument_compat(caller_arg.layout, callee_arg.layout) { return err!(FunctionArgMismatch(caller_arg.layout.ty, callee_arg.layout.ty)); } - self.copy_op(caller_arg, callee_arg) + // We allow some transmutes here + self.copy_op_transmute(caller_arg, callee_arg) } /// Call this function -- pushing the stack frame and initializing the arguments. diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 9dc035a3e20b8..cff4a0a323e0b 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -218,7 +218,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> return validation_failure!("unaligned reference", path), _ => return validation_failure!( - "dangling (deallocated) reference", path + "dangling (out-of-bounds) reference (might be NULL at \ + run-time)", + path ), } } From a05914e2dcf196c226a9a93c6609e57970be7e23 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 9 Oct 2018 18:16:27 +0200 Subject: [PATCH 02/12] check return type even for uninhabited case --- src/librustc_mir/interpret/terminator.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index 0ff18b0dd0b20..d8ec014b852f8 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -375,18 +375,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 { - // 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). + if !callee_ret.layout.abi.is_uninhabited() { + return err!(FunctionRetMismatch( + self.tcx.types.never, callee_ret.layout.ty + )); + } } Ok(()) })(); From f79a22c3d5a6ab20e896df6a0473755db37b96f1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 9 Oct 2018 21:05:53 +0200 Subject: [PATCH 03/12] validate return value on stack pop --- src/librustc_mir/const_eval.rs | 9 ++--- src/librustc_mir/interpret/eval_context.rs | 39 ++++++++++++++++++---- src/librustc_mir/interpret/place.rs | 18 +++++----- src/librustc_mir/interpret/snapshot.rs | 6 ++-- src/librustc_mir/interpret/terminator.rs | 22 ++++++------ 5 files changed, 59 insertions(+), 35 deletions(-) diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs index fd18d9feeea91..1ce32f8c0a858 100644 --- a/src/librustc_mir/const_eval.rs +++ b/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 }, )?; diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index f6944b2a9ae85..b0b8962b76ab1 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/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, + /// The location where the result of the current stack frame should be written to, + /// and its layout in the caller. + pub return_place: Option>, /// 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), /// 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, + return_place: Option>, 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(()) } diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 707857c809b2a..55077c8b6f9dd 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -256,12 +256,6 @@ impl<'tcx, Tag: ::std::fmt::Debug> Place { } 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 { diff --git a/src/librustc_mir/interpret/snapshot.rs b/src/librustc_mir/interpret/snapshot.rs index 06aee8605c6e1..11d5785bc565d 100644 --- a/src/librustc_mir/interpret/snapshot.rs +++ b/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>>, locals: IndexVec>>, block: &'a mir::BasicBlock, stmt: usize, @@ -362,7 +362,7 @@ impl<'a, 'mir, 'tcx: 'mir> HashStable> 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(), } } diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index d8ec014b852f8..a339fa34ae1f6 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/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), ) } From 93f53e51135e37f7e8328d31d72b6233049539e3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 9 Oct 2018 22:41:41 +0200 Subject: [PATCH 04/12] size_and_align_of can return no result for extern types --- src/librustc_mir/interpret/eval_context.rs | 40 +++++++++++----------- src/librustc_mir/interpret/place.rs | 4 +-- src/librustc_mir/interpret/validity.rs | 5 ++- 3 files changed, 26 insertions(+), 23 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index b0b8962b76ab1..bc613358152be 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -332,22 +332,16 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc } /// Return the actual dynamic size and alignment of the place at the given type. - /// Only the `meta` part of the place matters. + /// Only the "meta" (metadata) part of the place matters. + /// This can fail to provide an answer for extern types. pub(super) fn size_and_align_of( &self, metadata: Option>, layout: TyLayout<'tcx>, - ) -> EvalResult<'tcx, (Size, Align)> { - let metadata = match metadata { - None => { - assert!(!layout.is_unsized()); - return Ok(layout.size_and_align()) - } - Some(metadata) => { - assert!(layout.is_unsized()); - metadata - } - }; + ) -> EvalResult<'tcx, Option<(Size, Align)>> { + if !layout.is_unsized() { + return Ok(Some(layout.size_and_align())); + } match layout.ty.sty { ty::Adt(..) | ty::Tuple(..) => { // First get the size of all statically known fields. @@ -367,9 +361,11 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc ); // Recurse to get the size of the dynamically sized field (must be - // the last field). + // the last field). Can't have foreign types here, how would we + // adjust alignment and size for them? let field = layout.field(self, layout.fields.count() - 1)?; - let (unsized_size, unsized_align) = self.size_and_align_of(Some(metadata), field)?; + let (unsized_size, unsized_align) = self.size_and_align_of(metadata, field)? + .expect("Fields cannot be extern types"); // FIXME (#26403, #27023): We should be adding padding // to `sized_size` (to accommodate the `unsized_align` @@ -396,18 +392,22 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc // // `(size + (align-1)) & -align` - Ok((size.abi_align(align), align)) + Ok(Some((size.abi_align(align), align))) } ty::Dynamic(..) => { - let vtable = metadata.to_ptr()?; + let vtable = metadata.expect("dyn trait fat ptr must have vtable").to_ptr()?; // the second entry in the vtable is the dynamic size of the object. - self.read_size_and_align_from_vtable(vtable) + Ok(Some(self.read_size_and_align_from_vtable(vtable)?)) } ty::Slice(_) | ty::Str => { - let len = metadata.to_usize(self)?; + let len = metadata.expect("slice fat ptr must have vtable").to_usize(self)?; let (elem_size, align) = layout.field(self, 0)?.size_and_align(); - Ok((elem_size * len, align)) + Ok(Some((elem_size * len, align))) + } + + ty::Foreign(_) => { + Ok(None) } _ => bug!("size_and_align_of::<{:?}> not supported", layout.ty), @@ -417,7 +417,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc pub fn size_and_align_of_mplace( &self, mplace: MPlaceTy<'tcx, M::PointerTag> - ) -> EvalResult<'tcx, (Size, Align)> { + ) -> EvalResult<'tcx, Option<(Size, Align)>> { self.size_and_align_of(mplace.meta, mplace.layout) } diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 55077c8b6f9dd..923f0dc4c291a 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -319,9 +319,9 @@ where // Offset may need adjustment for unsized fields let (meta, offset) = if field_layout.is_unsized() { // re-use parent metadata to determine dynamic field layout - let (_, align) = self.size_and_align_of(base.meta, field_layout)?; + let (_, align) = self.size_and_align_of(base.meta, field_layout)? + .expect("Fields cannot be extern types"); (base.meta, offset.abi_align(align)) - } else { // base.meta could be present; we might be accessing a sized field of an unsized // struct. diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index cff4a0a323e0b..0c1dc092ce147 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -208,7 +208,10 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // 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)?; + let (size, align) = self.size_and_align_of(place.meta, place.layout)? + // for the purpose of validity, consider foreign types to have + // alignment 1 and size 0. + .unwrap_or_else(|| (Size::ZERO, Align::from_bytes(1, 1).unwrap())); match self.memory.check_align(place.ptr, align) { Ok(_) => {}, Err(err) => match err.kind { From d0c585c52582d79dc83ac180521ccf1d8c249b57 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 9 Oct 2018 22:44:20 +0200 Subject: [PATCH 05/12] seems like for generators we cannot access the freevars --- src/librustc_mir/interpret/validity.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 0c1dc092ce147..e2fa4b15f819d 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -559,12 +559,16 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // generators and closures. ty::Closure(def_id, _) | ty::Generator(def_id, _, _) => { if let Some(node_id) = self.tcx.hir.as_local_node_id(def_id) { - let freevar = self.tcx.with_freevars(node_id, |fv| fv[field]); - PathElem::ClosureVar(self.tcx.hir.name(freevar.var_id())) - } else { - // The closure is not local, so we cannot get the name - PathElem::ClosureVar(Symbol::intern(&field.to_string())) + if let Some(freevar) = self.tcx.with_freevars( + node_id, + |fv| fv.get(field).map(|field| *field)) + { + return PathElem::ClosureVar(self.tcx.hir.name(freevar.var_id())); + } } + // The closure is not local, or the freevars don't match up (seen for a generator!), + // so we cannot get the name. + PathElem::ClosureVar(Symbol::intern(&field.to_string())) } // tuples From 3272c9845cb6fcd8c15bcd0cb4c229de42f583c7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 10 Oct 2018 09:11:38 +0200 Subject: [PATCH 06/12] foreign types: use size and align from layout --- src/librustc_mir/interpret/validity.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index e2fa4b15f819d..59df61f2a6f93 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -210,8 +210,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // 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 1 and size 0. - .unwrap_or_else(|| (Size::ZERO, Align::from_bytes(1, 1).unwrap())); + // 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 { From 69576fcdee00b7e820c2faee258e24bb50db266a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 11 Oct 2018 08:48:15 +0200 Subject: [PATCH 07/12] make ENFORCE_VALIDITY a function miri needs this extra flexibility --- src/librustc_mir/const_eval.rs | 6 +++++- src/librustc_mir/interpret/eval_context.rs | 2 +- src/librustc_mir/interpret/machine.rs | 2 +- src/librustc_mir/interpret/place.rs | 6 +++--- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs index 1ce32f8c0a858..2cfd058831f05 100644 --- a/src/librustc_mir/const_eval.rs +++ b/src/librustc_mir/const_eval.rs @@ -343,7 +343,11 @@ impl<'a, 'mir, 'tcx> interpret::Machine<'a, 'mir, 'tcx> type MemoryMap = FxHashMap, Allocation<()>)>; const STATIC_KIND: Option = None; // no copying of statics allowed - const ENFORCE_VALIDITY: bool = false; // for now, we don't + + #[inline(always)] + fn enforce_validity(_ecx: &EvalContext<'a, 'mir, 'tcx, Self>) -> bool { + false // for now, we don't enforce validity + } fn find_fn( ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>, diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index bc613358152be..85a8376134aa4 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -524,7 +524,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc } // Validate the return value. if let Some(return_place) = frame.return_place { - if M::ENFORCE_VALIDITY { + if M::enforce_validity(self) { // 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 diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index a444f0bafd23c..560698f3f57a2 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -86,7 +86,7 @@ pub trait Machine<'a, 'mir, 'tcx>: Sized { const STATIC_KIND: Option; /// Whether to enforce the validity invariant - const ENFORCE_VALIDITY: bool; + fn enforce_validity(ecx: &EvalContext<'a, 'mir, 'tcx, Self>) -> bool; /// Called before a basic block terminator is executed. /// You can use this to detect endlessly running programs. diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 923f0dc4c291a..e4055947b6421 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -607,7 +607,7 @@ where ) -> EvalResult<'tcx> { self.write_value_no_validate(src_val, dest)?; - if M::ENFORCE_VALIDITY { + if M::enforce_validity(self) { // Data got changed, better make sure it matches the type! self.validate_operand(self.place_to_op(dest)?, &mut vec![], None, /*const_mode*/false)?; } @@ -729,7 +729,7 @@ where ) -> EvalResult<'tcx> { self.copy_op_no_validate(src, dest)?; - if M::ENFORCE_VALIDITY { + if M::enforce_validity(self) { // Data got changed, better make sure it matches the type! self.validate_operand(self.place_to_op(dest)?, &mut vec![], None, /*const_mode*/false)?; } @@ -807,7 +807,7 @@ where PlaceTy::from(MPlaceTy { mplace: *dest, layout: src.layout }), )?; - if M::ENFORCE_VALIDITY { + if M::enforce_validity(self) { // Data got changed, better make sure it matches the type! self.validate_operand(dest.into(), &mut vec![], None, /*const_mode*/false)?; } From c96eb706f0ddee1357f5b1d81683746390ef1d01 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 11 Oct 2018 10:16:51 +0200 Subject: [PATCH 08/12] Fix and test upvar name printing for validity --- src/librustc_mir/interpret/validity.rs | 16 ++++++-------- src/test/ui/consts/const-eval/ub-upvars.rs | 21 +++++++++++++++++++ .../ui/consts/const-eval/ub-upvars.stderr | 15 +++++++++++++ 3 files changed, 42 insertions(+), 10 deletions(-) create mode 100644 src/test/ui/consts/const-eval/ub-upvars.rs create mode 100644 src/test/ui/consts/const-eval/ub-upvars.stderr diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 59df61f2a6f93..fb81ab4e8c49d 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -559,17 +559,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> match layout.ty.sty { // generators and closures. ty::Closure(def_id, _) | ty::Generator(def_id, _, _) => { - if let Some(node_id) = self.tcx.hir.as_local_node_id(def_id) { - if let Some(freevar) = self.tcx.with_freevars( - node_id, - |fv| fv.get(field).map(|field| *field)) - { - return PathElem::ClosureVar(self.tcx.hir.name(freevar.var_id())); - } + if let Some(upvar) = self.tcx.optimized_mir(def_id).upvar_decls.get(field) { + PathElem::ClosureVar(upvar.debug_name) + } else { + // Sometimes the index is beyond the number of freevars (seen + // for a generator). + PathElem::ClosureVar(Symbol::intern(&field.to_string())) } - // The closure is not local, or the freevars don't match up (seen for a generator!), - // so we cannot get the name. - PathElem::ClosureVar(Symbol::intern(&field.to_string())) } // tuples diff --git a/src/test/ui/consts/const-eval/ub-upvars.rs b/src/test/ui/consts/const-eval/ub-upvars.rs new file mode 100644 index 0000000000000..309211d19d461 --- /dev/null +++ b/src/test/ui/consts/const-eval/ub-upvars.rs @@ -0,0 +1,21 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(const_transmute,const_let)] + +use std::mem; + +const BAD_UPVAR: &FnOnce() = &{ //~ ERROR this constant likely exhibits undefined behavior + let bad_ref: &'static u16 = unsafe { mem::transmute(0usize) }; + let another_var = 13; + move || { let _ = bad_ref; let _ = another_var; } +}; + +fn main() {} diff --git a/src/test/ui/consts/const-eval/ub-upvars.stderr b/src/test/ui/consts/const-eval/ub-upvars.stderr new file mode 100644 index 0000000000000..3ae140d6e1c20 --- /dev/null +++ b/src/test/ui/consts/const-eval/ub-upvars.stderr @@ -0,0 +1,15 @@ +error[E0080]: this constant likely exhibits undefined behavior + --> $DIR/ub-upvars.rs:15:1 + | +LL | / const BAD_UPVAR: &FnOnce() = &{ //~ ERROR this constant likely exhibits undefined behavior +LL | | let bad_ref: &'static u16 = unsafe { mem::transmute(0usize) }; +LL | | let another_var = 13; +LL | | move || { let _ = bad_ref; let _ = another_var; } +LL | | }; + | |__^ type validation failed: encountered 0 at .., but expected something greater or equal to 1 + | + = 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 previous error + +For more information about this error, try `rustc --explain E0080`. From dc4b2771f828103624a3109f38bd0ca1a9e924f8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 11 Oct 2018 11:15:30 +0200 Subject: [PATCH 09/12] validation: accept pointers in integer arrays --- src/librustc_mir/interpret/memory.rs | 24 ++++++++++++++++++++- src/librustc_mir/interpret/validity.rs | 15 +++++++++---- src/test/ui/consts/const-eval/ub-ref.rs | 3 +++ src/test/ui/consts/const-eval/ub-ref.stderr | 10 ++++++++- 4 files changed, 46 insertions(+), 6 deletions(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 7d3ae19e1a30c..781673a991472 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -650,7 +650,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { } /// It is the caller's responsibility to handle undefined and pointer bytes. - /// However, this still checks that there are no relocations on the egdes. + /// However, this still checks that there are no relocations on the *egdes*. #[inline] fn get_bytes_with_undef_and_ptr( &self, @@ -842,6 +842,28 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { } } + pub fn check_bytes( + &self, + ptr: Scalar, + size: Size, + allow_ptr: bool, + ) -> EvalResult<'tcx> { + // Empty accesses don't need to be valid pointers, but they should still be non-NULL + let align = Align::from_bytes(1, 1).unwrap(); + if size.bytes() == 0 { + self.check_align(ptr, align)?; + return Ok(()); + } + let ptr = ptr.to_ptr()?; + self.get_bytes_with_undef_and_ptr(ptr, size, align)?; + // Check undef, and maybe ptr + self.check_defined(ptr, size)?; + if !allow_ptr { + self.check_relocations(ptr, size)?; + } + Ok(()) + } + pub fn read_bytes(&self, ptr: Scalar, size: Size) -> EvalResult<'tcx, &[u8]> { // Empty accesses don't need to be valid pointers, but they should still be non-NULL let align = Align::from_bytes(1, 1).unwrap(); diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index fb81ab4e8c49d..2c5a3a1812820 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -496,9 +496,10 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> } // Special handling for arrays/slices of builtin integer types ty::Array(tys, ..) | ty::Slice(tys) if { - // This optimization applies only for integer types + // This optimization applies only for integer and floating point types + // (i.e., types that can hold arbitrary bytes). match tys.sty { - ty::Int(..) | ty::Uint(..) => true, + ty::Int(..) | ty::Uint(..) | ty::Float(..) => true, _ => false, } } => { @@ -510,9 +511,15 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // This is the size in bytes of the whole array. let size = Size::from_bytes(ty_size * len); - match self.memory.read_bytes(dest.ptr, size) { + // In run-time mode, we accept points in here. This is actually more + // permissive than a per-element check would be, e.g. we accept + // an &[u8] that contains a pointer even though bytewise checking would + // reject it. However, that's good: We don't inherently want + // to reject those pointers, we just do not have the machinery to + // talk about parts of a pointer. + match self.memory.check_bytes(dest.ptr, size, /*allow_ptr*/!const_mode) { // In the happy case, we needn't check anything else. - Ok(_) => {}, + Ok(()) => {}, // Some error happened, try to provide a more detailed description. Err(err) => { // For some errors we might be able to provide extra information diff --git a/src/test/ui/consts/const-eval/ub-ref.rs b/src/test/ui/consts/const-eval/ub-ref.rs index 7ee13f20dd2d9..584dc0691698a 100644 --- a/src/test/ui/consts/const-eval/ub-ref.rs +++ b/src/test/ui/consts/const-eval/ub-ref.rs @@ -21,6 +21,9 @@ const NULL: &u16 = unsafe { mem::transmute(0usize) }; const REF_AS_USIZE: usize = unsafe { mem::transmute(&0) }; //~^ ERROR this constant likely exhibits undefined behavior +const REF_AS_USIZE_SLICE: &[usize] = &[unsafe { mem::transmute(&0) }]; +//~^ ERROR this constant likely exhibits undefined behavior + const USIZE_AS_REF: &'static u8 = unsafe { mem::transmute(1337usize) }; //~^ ERROR this constant likely exhibits undefined behavior diff --git a/src/test/ui/consts/const-eval/ub-ref.stderr b/src/test/ui/consts/const-eval/ub-ref.stderr index 9907c780d2ccb..8bcb6d190b89a 100644 --- a/src/test/ui/consts/const-eval/ub-ref.stderr +++ b/src/test/ui/consts/const-eval/ub-ref.stderr @@ -25,11 +25,19 @@ LL | const REF_AS_USIZE: usize = unsafe { mem::transmute(&0) }; error[E0080]: this constant likely exhibits undefined behavior --> $DIR/ub-ref.rs:24:1 | +LL | const REF_AS_USIZE_SLICE: &[usize] = &[unsafe { mem::transmute(&0) }]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ a raw memory access tried to access part of a pointer value as raw bytes + | + = 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/ub-ref.rs:27:1 + | LL | const USIZE_AS_REF: &'static u8 = unsafe { mem::transmute(1337usize) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered integer pointer in non-ZST reference | = 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 4 previous errors +error: aborting due to 5 previous errors For more information about this error, try `rustc --explain E0080`. From b2ddd27c2e9de8e7ad75c1f7a276aff22d8a2c52 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 11 Oct 2018 13:13:14 +0200 Subject: [PATCH 10/12] address nits --- src/librustc_mir/interpret/memory.rs | 1 + src/librustc_mir/interpret/validity.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 781673a991472..222bef4ff136b 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -855,6 +855,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { return Ok(()); } let ptr = ptr.to_ptr()?; + // Check bounds, align and relocations on the edges self.get_bytes_with_undef_and_ptr(ptr, size, align)?; // Check undef, and maybe ptr self.check_defined(ptr, size)?; diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 2c5a3a1812820..aadb5cc871e1a 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -511,7 +511,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // This is the size in bytes of the whole array. let size = Size::from_bytes(ty_size * len); - // In run-time mode, we accept points in here. This is actually more + // In run-time mode, we accept pointers in here. This is actually more // permissive than a per-element check would be, e.g. we accept // an &[u8] that contains a pointer even though bytewise checking would // reject it. However, that's good: We don't inherently want From 06a4911ce1c36e961b6638a77abcbeb93807b96e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 12 Oct 2018 10:56:47 +0200 Subject: [PATCH 11/12] run-time validation: accept undef in int arrays, as we do for ints --- src/librustc_mir/interpret/memory.rs | 8 ++++---- src/librustc_mir/interpret/validity.rs | 7 ++++++- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 222bef4ff136b..4b0c0c3ee6173 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -846,7 +846,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { &self, ptr: Scalar, size: Size, - allow_ptr: bool, + allow_ptr_and_undef: bool, ) -> EvalResult<'tcx> { // Empty accesses don't need to be valid pointers, but they should still be non-NULL let align = Align::from_bytes(1, 1).unwrap(); @@ -857,9 +857,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { let ptr = ptr.to_ptr()?; // Check bounds, align and relocations on the edges self.get_bytes_with_undef_and_ptr(ptr, size, align)?; - // Check undef, and maybe ptr - self.check_defined(ptr, size)?; - if !allow_ptr { + // Check undef and ptr + if !allow_ptr_and_undef { + self.check_defined(ptr, size)?; self.check_relocations(ptr, size)?; } Ok(()) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index aadb5cc871e1a..c446980d04995 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -517,7 +517,12 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // reject it. However, that's good: We don't inherently want // to reject those pointers, we just do not have the machinery to // talk about parts of a pointer. - match self.memory.check_bytes(dest.ptr, size, /*allow_ptr*/!const_mode) { + // We also accept undef, for consistency with the type-based checks. + match self.memory.check_bytes( + dest.ptr, + size, + /*allow_ptr_and_undef*/!const_mode, + ) { // In the happy case, we needn't check anything else. Ok(()) => {}, // Some error happened, try to provide a more detailed description. From 6426cbe382f0379fe5c2ab987c94c4ad9dcc4c42 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 11 Oct 2018 10:18:31 +0200 Subject: [PATCH 12/12] update miri --- src/Cargo.lock | 20 ++++---------------- src/tools/miri | 2 +- 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/src/Cargo.lock b/src/Cargo.lock index c01086b3f6f2b..abe736a4fe613 100644 --- a/src/Cargo.lock +++ b/src/Cargo.lock @@ -825,16 +825,6 @@ name = "getopts" version = "0.2.17" source = "registry+https://github.com/rust-lang/crates.io-index" -[[package]] -name = "getset" -version = "0.0.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -dependencies = [ - "proc-macro2 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)", - "quote 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)", - "syn 0.13.11 (registry+https://github.com/rust-lang/crates.io-index)", -] - [[package]] name = "git2" version = "0.7.5" @@ -1303,7 +1293,7 @@ dependencies = [ "compiletest_rs 0.3.13 (registry+https://github.com/rust-lang/crates.io-index)", "env_logger 0.5.12 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.4 (registry+https://github.com/rust-lang/crates.io-index)", - "vergen 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)", + "vergen 3.0.3 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -3072,13 +3062,12 @@ source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] name = "vergen" -version = "2.0.0" +version = "3.0.3" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "bitflags 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)", "chrono 0.4.4 (registry+https://github.com/rust-lang/crates.io-index)", - "error-chain 0.12.0 (registry+https://github.com/rust-lang/crates.io-index)", - "getset 0.0.6 (registry+https://github.com/rust-lang/crates.io-index)", + "failure 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -3245,7 +3234,6 @@ source = "registry+https://github.com/rust-lang/crates.io-index" "checksum futures 0.1.21 (registry+https://github.com/rust-lang/crates.io-index)" = "1a70b146671de62ec8c8ed572219ca5d594d9b06c0b364d5e67b722fc559b48c" "checksum fwdansi 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "34dd4c507af68d37ffef962063dfa1944ce0dd4d5b82043dbab1dabe088610c3" "checksum getopts 0.2.17 (registry+https://github.com/rust-lang/crates.io-index)" = "b900c08c1939860ce8b54dc6a89e26e00c04c380fd0e09796799bd7f12861e05" -"checksum getset 0.0.6 (registry+https://github.com/rust-lang/crates.io-index)" = "54c7f36a235738bb25904d6a2b3dbb28f6f5736cd3918c4bf80d6bb236200782" "checksum git2 0.7.5 (registry+https://github.com/rust-lang/crates.io-index)" = "591f8be1674b421644b6c030969520bc3fa12114d2eb467471982ed3e9584e71" "checksum git2-curl 0.8.1 (registry+https://github.com/rust-lang/crates.io-index)" = "b502f6b1b467957403d168f0039e0c46fa6a1220efa2adaef25d5b267b5fe024" "checksum glob 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)" = "8be18de09a56b60ed0edf84bc9df007e30040691af7acd1c41874faac5895bfb" @@ -3419,7 +3407,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" "checksum utf8-ranges 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "fd70f467df6810094968e2fce0ee1bd0e87157aceb026a8c083bcf5e25b9efe4" "checksum vcpkg 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)" = "def296d3eb3b12371b2c7d0e83bfe1403e4db2d7a0bba324a12b21c4ee13143d" "checksum vec_map 0.8.1 (registry+https://github.com/rust-lang/crates.io-index)" = "05c78687fb1a80548ae3250346c3db86a80a7cdd77bda190189f2d0a0987c81a" -"checksum vergen 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "9a16834fc61e1492c07dae49b6c14b55f8b1d43a5f5f9e9a2ecc063f47b9f93c" +"checksum vergen 3.0.3 (registry+https://github.com/rust-lang/crates.io-index)" = "1b9696d96ec5d68984d060af80d7ba0ed4eb533978a0efb05ed4b8465f20d04f" "checksum version_check 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)" = "7716c242968ee87e5542f8021178248f267f295a5c4803beae8b8b7fd9bc6051" "checksum void 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)" = "6a02e4885ed3bc0f2de90ea6dd45ebcbb66dacffe03547fadbb0eeae2770887d" "checksum wait-timeout 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)" = "b9f3bf741a801531993db6478b95682117471f76916f5e690dd8d45395b09349" diff --git a/src/tools/miri b/src/tools/miri index cc275c63a90d4..26f9d617c3471 160000 --- a/src/tools/miri +++ b/src/tools/miri @@ -1 +1 @@ -Subproject commit cc275c63a90d4bea394e76607b2e10611eb1be36 +Subproject commit 26f9d617c347185433b77c481a5c50c55d9b72ce