From 14ee66acecc353c0b3045d74fbe3addd66f96d79 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 5 Nov 2019 23:04:17 +0100 Subject: [PATCH 1/4] miri cast: avoid unnecessary to_scalar_ptr --- src/librustc_mir/interpret/cast.rs | 4 ++-- src/librustc_mir/interpret/place.rs | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/interpret/cast.rs b/src/librustc_mir/interpret/cast.rs index 9ab347957f97a..2037305580ad3 100644 --- a/src/librustc_mir/interpret/cast.rs +++ b/src/librustc_mir/interpret/cast.rs @@ -260,7 +260,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { match (&src_pointee_ty.kind, &dest_pointee_ty.kind) { (&ty::Array(_, length), &ty::Slice(_)) => { - let ptr = self.read_immediate(src)?.to_scalar_ptr()?; + let ptr = self.read_immediate(src)?.to_scalar()?; // u64 cast is from usize to u64, which is always good let val = Immediate::new_slice( ptr, @@ -279,7 +279,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { (_, &ty::Dynamic(ref data, _)) => { // Initial cast from sized to dyn trait let vtable = self.get_vtable(src_pointee_ty, data.principal())?; - let ptr = self.read_immediate(src)?.to_scalar_ptr()?; + let ptr = self.read_immediate(src)?.to_scalar()?; let val = Immediate::new_dyn_trait(ptr, vtable); self.write_immediate(val, dest) } diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 0289c52fd3744..d696a595777a4 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -287,7 +287,9 @@ where &self, val: ImmTy<'tcx, M::PointerTag>, ) -> InterpResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> { - let pointee_type = val.layout.ty.builtin_deref(true).unwrap().ty; + let pointee_type = val.layout.ty.builtin_deref(true) + .expect("`ref_to_mplace` called on non-ptr type") + .ty; let layout = self.layout_of(pointee_type)?; let mplace = MemPlace { From 32453ce488909ec12b893ceb9a204718eae724e4 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 6 Nov 2019 08:44:15 +0100 Subject: [PATCH 2/4] remvoe to_scalar_ptr and use ref_to_mplace everywhere --- src/librustc_mir/interpret/intern.rs | 22 ++++++-------- src/librustc_mir/interpret/operand.rs | 20 ------------ src/librustc_mir/interpret/place.rs | 8 +++-- src/librustc_mir/interpret/validity.rs | 42 +++++++++----------------- 4 files changed, 30 insertions(+), 62 deletions(-) diff --git a/src/librustc_mir/interpret/intern.rs b/src/librustc_mir/interpret/intern.rs index 924529d7f5579..2171ceaa452c8 100644 --- a/src/librustc_mir/interpret/intern.rs +++ b/src/librustc_mir/interpret/intern.rs @@ -192,20 +192,18 @@ for let ty = mplace.layout.ty; if let ty::Ref(_, referenced_ty, mutability) = ty.kind { let value = self.ecx.read_immediate(mplace.into())?; + let mplace = self.ecx.ref_to_mplace(value)?; // Handle trait object vtables - if let Ok(meta) = value.to_meta() { - if let ty::Dynamic(..) = - self.ecx.tcx.struct_tail_erasing_lifetimes( - referenced_ty, self.ecx.param_env).kind - { - if let Ok(vtable) = meta.unwrap().to_ptr() { - // explitly choose `Immutable` here, since vtables are immutable, even - // if the reference of the fat pointer is mutable - self.intern_shallow(vtable.alloc_id, Mutability::Immutable, None)?; - } + if let ty::Dynamic(..) = + self.ecx.tcx.struct_tail_erasing_lifetimes( + referenced_ty, self.ecx.param_env).kind + { + if let Ok(vtable) = mplace.meta.unwrap().to_ptr() { + // explitly choose `Immutable` here, since vtables are immutable, even + // if the reference of the fat pointer is mutable + self.intern_shallow(vtable.alloc_id, Mutability::Immutable, None)?; } } - let mplace = self.ecx.ref_to_mplace(value)?; // Check if we have encountered this pointer+layout combination before. // Only recurse for allocation-backed pointers. if let Scalar::Ptr(ptr) = mplace.ptr { @@ -230,7 +228,7 @@ for ty::Array(_, n) if n.eval_usize(self.ecx.tcx.tcx, self.ecx.param_env) == 0 => {} ty::Slice(_) - if value.to_meta().unwrap().unwrap().to_usize(self.ecx)? == 0 => {} + if mplace.meta.unwrap().to_usize(self.ecx)? == 0 => {} _ => bug!("const qualif failed to prevent mutable references"), } }, diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index ae23971849eea..d80ad3848d20a 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -82,26 +82,6 @@ impl<'tcx, Tag> Immediate { Immediate::ScalarPair(a, b) => Ok((a.not_undef()?, b.not_undef()?)) } } - - /// Converts the immediate into a pointer (or a pointer-sized integer). - /// Throws away the second half of a ScalarPair! - #[inline] - pub fn to_scalar_ptr(self) -> InterpResult<'tcx, Scalar> { - match self { - Immediate::Scalar(ptr) | - Immediate::ScalarPair(ptr, _) => ptr.not_undef(), - } - } - - /// Converts the value into its metadata. - /// Throws away the first half of a ScalarPair! - #[inline] - pub fn to_meta(self) -> InterpResult<'tcx, Option>> { - Ok(match self { - Immediate::Scalar(_) => None, - Immediate::ScalarPair(_, meta) => Some(meta.not_undef()?), - }) - } } // ScalarPair needs a type to interpret, so we often have an immediate and a type together diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index d696a595777a4..36e58d356d100 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -291,15 +291,19 @@ where .expect("`ref_to_mplace` called on non-ptr type") .ty; let layout = self.layout_of(pointee_type)?; + let (ptr, meta) = match *val { + Immediate::Scalar(ptr) => (ptr.not_undef()?, None), + Immediate::ScalarPair(ptr, meta) => (ptr.not_undef()?, Some(meta.not_undef()?)), + }; let mplace = MemPlace { - ptr: val.to_scalar_ptr()?, + ptr, // We could use the run-time alignment here. For now, we do not, because // the point of tracking the alignment here is to make sure that the *static* // alignment information emitted with the loads is correct. The run-time // alignment can only be more restrictive. align: layout.align.abi, - meta: val.to_meta()?, + meta, }; Ok(MPlaceTy { mplace, layout }) } diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 82b8b28d72b7b..929425afc9e91 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -388,44 +388,31 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> } } ty::RawPtr(..) => { - // Check pointer part. - if self.ref_tracking_for_consts.is_some() { - // Integers/floats in CTFE: For consistency with integers, we do not - // accept undef. - let _ptr = try_validation!(value.to_scalar_ptr(), - "undefined address in raw pointer", self.path); - } else { - // Remain consistent with `usize`: Accept anything. - } - - // Check metadata. - let meta = try_validation!(value.to_meta(), - "uninitialized data in wide pointer metadata", self.path); - let layout = self.ecx.layout_of(value.layout.ty.builtin_deref(true).unwrap().ty)?; - if layout.is_unsized() { - self.check_wide_ptr_meta(meta, layout)?; + // We are conservative with undef for integers, but try to + // actually enforce our current rules for raw pointers. + let place = try_validation!(self.ecx.ref_to_mplace(value), + "undefined pointer", self.path); + if place.layout.is_unsized() { + self.check_wide_ptr_meta(place.meta, place.layout)?; } } _ if ty.is_box() || ty.is_region_ptr() => { // Handle wide pointers. // Check metadata early, for better diagnostics - let ptr = try_validation!(value.to_scalar_ptr(), - "undefined address in pointer", self.path); - let meta = try_validation!(value.to_meta(), - "uninitialized data in wide pointer metadata", self.path); - let layout = self.ecx.layout_of(value.layout.ty.builtin_deref(true).unwrap().ty)?; - if layout.is_unsized() { - self.check_wide_ptr_meta(meta, layout)?; + let place = try_validation!(self.ecx.ref_to_mplace(value), + "undefined pointer", self.path); + if place.layout.is_unsized() { + self.check_wide_ptr_meta(place.meta, place.layout)?; } // Make sure this is dereferencable and all. - let (size, align) = self.ecx.size_and_align_of(meta, layout)? + let (size, align) = self.ecx.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(|| (layout.size, layout.align.abi)); + .unwrap_or_else(|| (place.layout.size, place.layout.align.abi)); let ptr: Option<_> = match self.ecx.memory.check_ptr_access_align( - ptr, + place.ptr, size, Some(align), CheckInAllocMsg::InboundsTest, @@ -435,7 +422,7 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> Err(err) => { info!( "{:?} did not pass access check for size {:?}, align {:?}", - ptr, size, align + place.ptr, size, align ); match err.kind { err_unsup!(InvalidNullPointerUsage) => @@ -459,7 +446,6 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> }; // Recursive checking if let Some(ref mut ref_tracking) = self.ref_tracking_for_consts { - let place = self.ecx.ref_to_mplace(value)?; if let Some(ptr) = ptr { // not a ZST // Skip validation entirely for some external statics let alloc_kind = self.ecx.tcx.alloc_map.lock().get(ptr.alloc_id); From 87edcf095da6218d05639dd62daeda0d0642d0c5 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 6 Nov 2019 08:45:50 +0100 Subject: [PATCH 3/4] improve a comment --- src/librustc_mir/interpret/validity.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 929425afc9e91..8cb2f6c3462cc 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -613,7 +613,7 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'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. - // We also accept undef, for consistency with the type-based checks. + // We also accept undef, for consistency with the slow path. match self.ecx.memory.get(ptr.alloc_id)?.check_bytes( self.ecx, ptr, From 2312a56f5c4e65df4b6d8128489bc58a79555718 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 6 Nov 2019 09:52:11 +0100 Subject: [PATCH 4/4] --bless --- src/test/ui/consts/const-eval/ub-wide-ptr.stderr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/ui/consts/const-eval/ub-wide-ptr.stderr b/src/test/ui/consts/const-eval/ub-wide-ptr.stderr index 9134ef5a31ad9..85fb8ac2a4a36 100644 --- a/src/test/ui/consts/const-eval/ub-wide-ptr.stderr +++ b/src/test/ui/consts/const-eval/ub-wide-ptr.stderr @@ -42,7 +42,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-wide-ptr.rs:107:1 | LL | const SLICE_LENGTH_UNINIT: &[u8] = unsafe { SliceTransmute { addr: 42 }.slice}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized data in wide pointer metadata + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered undefined 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 rustc repository if you believe it should not be considered undefined behavior. @@ -90,7 +90,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-wide-ptr.rs:133:1 | LL | const RAW_SLICE_LENGTH_UNINIT: *const [u8] = unsafe { SliceTransmute { addr: 42 }.raw_slice}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized data in wide pointer metadata + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered undefined 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 rustc repository if you believe it should not be considered undefined behavior.