fix handling of unsized types in validation; validate str to be UTF-8

Author: RalfJung

src/librustc_mir/const_eval.rs

@@ -29,7 +29,7 @@ use rustc::mir::interpret::{
     Scalar, AllocId, Allocation, ConstValue, AllocType,
 };
 use interpret::{self,
-    Place, PlaceExtra, PlaceTy, MemPlace, OpTy, Operand, Value,
+    Place, PlaceTy, MemPlace, OpTy, Operand, Value,
     EvalContext, StackPopCleanup, MemoryKind, Memory,
 };
 
@@ -103,7 +103,7 @@ pub fn op_to_const<'tcx>(
     let val = match normalized_op {
         Err(MemPlace { ptr, align, extra }) => {
             // extract alloc-offset pair
-            assert_eq!(extra, PlaceExtra::None);
+            assert!(extra.is_none());
             let ptr = ptr.to_ptr()?;
             let alloc = ecx.memory.get(ptr.alloc_id)?;
             assert!(alloc.align.abi() >= align.abi());

src/librustc_mir/interpret/eval_context.rs

@@ -34,7 +34,7 @@ use rustc::mir::interpret::{
 use syntax::source_map::{self, Span};
 
 use super::{
-    Value, Operand, MemPlace, MPlaceTy, Place, PlaceExtra,
+    Value, Operand, MemPlace, MPlaceTy, Place,
     Memory, Machine
 };
 
@@ -462,90 +462,94 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M
     }
 
     /// Return the actual dynamic size and alignment of the place at the given type.
-    /// Note that the value does not matter if the type is sized. For unsized types,
-    /// the value has to be a fat pointer, and we only care about the "extra" data in it.
-    pub fn size_and_align_of_mplace(
+    /// Only the "extra" (metadata) part of the place matters.
+    pub(super) fn size_and_align_of(
         &self,
-        mplace: MPlaceTy<'tcx>,
+        metadata: Option<Scalar>,
+        layout: TyLayout<'tcx>,
     ) -> EvalResult<'tcx, (Size, Align)> {
-        if let PlaceExtra::None = mplace.extra {
-            assert!(!mplace.layout.is_unsized());
-            Ok(mplace.layout.size_and_align())
-        } else {
-            let layout = mplace.layout;
-            assert!(layout.is_unsized());
-            match layout.ty.sty {
-                ty::Adt(..) | ty::Tuple(..) => {
-                    // First get the size of all statically known fields.
-                    // Don't use type_of::sizing_type_of because that expects t to be sized,
-                    // and it also rounds up to alignment, which we want to avoid,
-                    // as the unsized field's alignment could be smaller.
-                    assert!(!layout.ty.is_simd());
-                    debug!("DST layout: {:?}", layout);
-
-                    let sized_size = layout.fields.offset(layout.fields.count() - 1);
-                    let sized_align = layout.align;
-                    debug!(
-                        "DST {} statically sized prefix size: {:?} align: {:?}",
-                        layout.ty,
-                        sized_size,
-                        sized_align
-                    );
-
-                    // Recurse to get the size of the dynamically sized field (must be
-                    // the last field).
-                    let field = self.mplace_field(mplace, layout.fields.count() as u64 - 1)?;
-                    let (unsized_size, unsized_align) = self.size_and_align_of_mplace(field)?;
-
-                    // FIXME (#26403, #27023): We should be adding padding
-                    // to `sized_size` (to accommodate the `unsized_align`
-                    // required of the unsized field that follows) before
-                    // summing it with `sized_size`. (Note that since #26403
-                    // is unfixed, we do not yet add the necessary padding
-                    // here. But this is where the add would go.)
-
-                    // Return the sum of sizes and max of aligns.
-                    let size = sized_size + unsized_size;
-
-                    // Choose max of two known alignments (combined value must
-                    // be aligned according to more restrictive of the two).
-                    let align = sized_align.max(unsized_align);
-
-                    // Issue #27023: must add any necessary padding to `size`
-                    // (to make it a multiple of `align`) before returning it.
-                    //
-                    // Namely, the returned size should be, in C notation:
-                    //
-                    //   `size + ((size & (align-1)) ? align : 0)`
-                    //
-                    // emulated via the semi-standard fast bit trick:
-                    //
-                    //   `(size + (align-1)) & -align`
-
-                    Ok((size.abi_align(align), align))
-                }
-                ty::Dynamic(..) => {
-                    let vtable = match mplace.extra {
-                        PlaceExtra::Vtable(vtable) => vtable,
-                        _ => bug!("Expected vtable"),
-                    };
-                    // the second entry in the vtable is the dynamic size of the object.
-                    self.read_size_and_align_from_vtable(vtable)
-                }
-
-                ty::Slice(_) | ty::Str => {
-                    let len = match mplace.extra {
-                        PlaceExtra::Length(len) => len,
-                        _ => bug!("Expected length"),
-                    };
-                    let (elem_size, align) = layout.field(self, 0)?.size_and_align();
-                    Ok((elem_size * len, align))
-                }
+        let metadata = match metadata {
+            None => {
+                assert!(!layout.is_unsized());
+                return Ok(layout.size_and_align())
+            }
+            Some(metadata) => {
+                assert!(layout.is_unsized());
+                metadata
+            }
+        };
+        match layout.ty.sty {
+            ty::Adt(..) | ty::Tuple(..) => {
+                // First get the size of all statically known fields.
+                // Don't use type_of::sizing_type_of because that expects t to be sized,
+                // and it also rounds up to alignment, which we want to avoid,
+                // as the unsized field's alignment could be smaller.
+                assert!(!layout.ty.is_simd());
+                debug!("DST layout: {:?}", layout);
+
+                let sized_size = layout.fields.offset(layout.fields.count() - 1);
+                let sized_align = layout.align;
+                debug!(
+                    "DST {} statically sized prefix size: {:?} align: {:?}",
+                    layout.ty,
+                    sized_size,
+                    sized_align
+                );
+
+                // Recurse to get the size of the dynamically sized field (must be
+                // the last field).
+                let field = layout.field(self, layout.fields.count() - 1)?;
+                let (unsized_size, unsized_align) = self.size_and_align_of(Some(metadata), field)?;
+
+                // FIXME (#26403, #27023): We should be adding padding
+                // to `sized_size` (to accommodate the `unsized_align`
+                // required of the unsized field that follows) before
+                // summing it with `sized_size`. (Note that since #26403
+                // is unfixed, we do not yet add the necessary padding
+                // here. But this is where the add would go.)
+
+                // Return the sum of sizes and max of aligns.
+                let size = sized_size + unsized_size;
+
+                // Choose max of two known alignments (combined value must
+                // be aligned according to more restrictive of the two).
+                let align = sized_align.max(unsized_align);
+
+                // Issue #27023: must add any necessary padding to `size`
+                // (to make it a multiple of `align`) before returning it.
+                //
+                // Namely, the returned size should be, in C notation:
+                //
+                //   `size + ((size & (align-1)) ? align : 0)`
+                //
+                // emulated via the semi-standard fast bit trick:
+                //
+                //   `(size + (align-1)) & -align`
+
+                Ok((size.abi_align(align), align))
+            }
+            ty::Dynamic(..) => {
+                let vtable = metadata.to_ptr()?;
+                // the second entry in the vtable is the dynamic size of the object.
+                self.read_size_and_align_from_vtable(vtable)
+            }
 
-                _ => bug!("size_of_val::<{:?}> not supported", layout.ty),
+            ty::Slice(_) | ty::Str => {
+                let len = metadata.to_usize(self)?;
+                let (elem_size, align) = layout.field(self, 0)?.size_and_align();
+                Ok((elem_size * len, align))
             }
+
+            _ => bug!("size_and_align_of::<{:?}> not supported", layout.ty),
         }
     }
+    #[inline]
+    pub fn size_and_align_of_mplace(
+        &self,
+        mplace: MPlaceTy<'tcx>
+    ) -> EvalResult<'tcx, (Size, Align)> {
+        self.size_and_align_of(mplace.extra, mplace.layout)
+    }
 
     pub fn push_stack_frame(
         &mut self,

src/librustc_mir/interpret/intrinsics.rs

@@ -142,8 +142,10 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                 self.mplace_field(place, 3)?,
             );
 
-            let msg = Symbol::intern(self.read_str(msg.into())?);
-            let file = Symbol::intern(self.read_str(file.into())?);
+            let msg_place = self.ref_to_mplace(self.read_value(msg.into())?)?;
+            let msg = Symbol::intern(self.read_str(msg_place)?);
+            let file_place = self.ref_to_mplace(self.read_value(file.into())?)?;
+            let file = Symbol::intern(self.read_str(file_place)?);
             let line = self.read_scalar(line.into())?.to_u32()?;
             let col = self.read_scalar(col.into())?.to_u32()?;
             return Err(EvalErrorKind::Panic { msg, file, line, col }.into());
@@ -159,8 +161,10 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                 self.mplace_field(place, 2)?,
             );
 
-            let msg = Symbol::intern(self.read_str(msg)?);
-            let file = Symbol::intern(self.read_str(file.into())?);
+            let msg_place = self.ref_to_mplace(self.read_value(msg.into())?)?;
+            let msg = Symbol::intern(self.read_str(msg_place)?);
+            let file_place = self.ref_to_mplace(self.read_value(file.into())?)?;
+            let file = Symbol::intern(self.read_str(file_place)?);
             let line = self.read_scalar(line.into())?.to_u32()?;
             let col = self.read_scalar(col.into())?.to_u32()?;
             return Err(EvalErrorKind::Panic { msg, file, line, col }.into());

src/librustc_mir/interpret/mod.rs

@@ -27,7 +27,7 @@ pub use self::eval_context::{
     EvalContext, Frame, StackPopCleanup, LocalValue,
 };
 
-pub use self::place::{Place, PlaceExtra, PlaceTy, MemPlace, MPlaceTy};
+pub use self::place::{Place, PlaceTy, MemPlace, MPlaceTy};
 
 pub use self::memory::{Memory, MemoryKind};
 

src/librustc_mir/interpret/operand.rs

@@ -21,7 +21,7 @@ use rustc_data_structures::indexed_vec::Idx;
 use rustc::mir::interpret::{
     GlobalId, ConstValue, Scalar, EvalResult, Pointer, ScalarMaybeUndef, EvalErrorKind
 };
-use super::{EvalContext, Machine, MemPlace, MPlaceTy, PlaceExtra, MemoryKind};
+use super::{EvalContext, Machine, MemPlace, MPlaceTy, MemoryKind};
 
 /// A `Value` represents a single immediate self-contained Rust value.
 ///
@@ -65,6 +65,14 @@ impl<'tcx> Value {
         self.to_scalar_or_undef().not_undef()
     }
 
+    #[inline]
+    pub fn to_scalar_pair(self) -> EvalResult<'tcx, (Scalar, Scalar)> {
+        match self {
+            Value::Scalar(..) => bug!("Got a thin pointer where a scalar pair was expected"),
+            Value::ScalarPair(a, b) => Ok((a.not_undef()?, b.not_undef()?))
+        }
+    }
+
     /// Convert the value into a pointer (or a pointer-sized integer).
     /// Throws away the second half of a ScalarPair!
     #[inline]
@@ -74,24 +82,6 @@ impl<'tcx> Value {
             Value::ScalarPair(ptr, _) => ptr.not_undef(),
         }
     }
-
-    pub fn to_scalar_dyn_trait(self) -> EvalResult<'tcx, (Scalar, Pointer)> {
-        match self {
-            Value::ScalarPair(ptr, vtable) =>
-                Ok((ptr.not_undef()?, vtable.to_ptr()?)),
-            _ => bug!("expected ptr and vtable, got {:?}", self),
-        }
-    }
-
-    pub fn to_scalar_slice(self, cx: impl HasDataLayout) -> EvalResult<'tcx, (Scalar, u64)> {
-        match self {
-            Value::ScalarPair(ptr, val) => {
-                let len = val.to_bits(cx.data_layout().pointer_size)?;
-                Ok((ptr.not_undef()?, len as u64))
-            }
-            _ => bug!("expected ptr and length, got {:?}", self),
-        }
-    }
 }
 
 // ScalarPair needs a type to interpret, so we often have a value and a type together
@@ -242,7 +232,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
         &self,
         mplace: MPlaceTy<'tcx>,
     ) -> EvalResult<'tcx, Option<Value>> {
-        if mplace.extra != PlaceExtra::None {
+        debug_assert_eq!(mplace.extra.is_some(), mplace.layout.is_unsized());
+        if mplace.extra.is_some() {
+            // Dont touch unsized
             return Ok(None);
         }
         let (ptr, ptr_align) = mplace.to_scalar_ptr_align();
@@ -315,20 +307,16 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
         }
     }
 
-    // operand must be a &str or compatible layout
+    // Turn the MPlace into a string (must already be dereferenced!)
     pub fn read_str(
         &self,
-        op: OpTy<'tcx>,
+        mplace: MPlaceTy<'tcx>,
     ) -> EvalResult<'tcx, &str> {
-        let val = self.read_value(op)?;
-        if let Value::ScalarPair(ptr, len) = *val {
-            let len = len.not_undef()?.to_bits(self.memory.pointer_size())?;
-            let bytes = self.memory.read_bytes(ptr.not_undef()?, Size::from_bytes(len as u64))?;
-            let str = ::std::str::from_utf8(bytes).map_err(|err| EvalErrorKind::ValidationFailure(err.to_string()))?;
-            Ok(str)
-        } else {
-            bug!("read_str: not a str")
-        }
+        let len = mplace.len(self)?;
+        let bytes = self.memory.read_bytes(mplace.ptr, Size::from_bytes(len as u64))?;
+        let str = ::std::str::from_utf8(bytes)
+            .map_err(|err| EvalErrorKind::ValidationFailure(err.to_string()))?;
+        Ok(str)
     }
 
     pub fn uninit_operand(&mut self, layout: TyLayout<'tcx>) -> EvalResult<'tcx, Operand> {