miri value visitor: detect primitives by type, not layout

RalfJung · RalfJung · commit d47196b2ec23 · 2020-03-06T22:41:49.000+01:00
diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs
@@ -306,23 +306,119 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M
         Ok(())
     }
 
-    fn visit_primitive(&mut self, value: OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx> {
+    /// Check a reference or `Box`.
+    fn check_safe_pointer(&mut self, value: OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx> {
         let value = self.ecx.read_immediate(value)?;
+        // Handle wide pointers.
+        // Check metadata early, for better diagnostics
+        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 dereferenceable and all.
+        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(|| (place.layout.size, place.layout.align.abi));
+        let ptr: Option<_> = match self.ecx.memory.check_ptr_access_align(
+            place.ptr,
+            size,
+            Some(align),
+            CheckInAllocMsg::InboundsTest,
+        ) {
+            Ok(ptr) => ptr,
+            Err(err) => {
+                info!(
+                    "{:?} did not pass access check for size {:?}, align {:?}",
+                    place.ptr, size, align
+                );
+                match err.kind {
+                    err_unsup!(InvalidNullPointerUsage) => {
+                        throw_validation_failure!("a NULL reference", self.path)
+                    }
+                    err_unsup!(AlignmentCheckFailed { required, has }) => {
+                        throw_validation_failure!(
+                            format_args!(
+                                "an unaligned reference \
+                                    (required {} byte alignment but found {})",
+                                required.bytes(),
+                                has.bytes()
+                            ),
+                            self.path
+                        )
+                    }
+                    err_unsup!(ReadBytesAsPointer) => throw_validation_failure!(
+                        "a dangling reference (created from integer)",
+                        self.path
+                    ),
+                    _ => throw_validation_failure!(
+                        "a dangling reference (not entirely in bounds)",
+                        self.path
+                    ),
+                }
+            }
+        };
+        // Recursive checking
+        if let Some(ref mut ref_tracking) = self.ref_tracking_for_consts {
+            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);
+                if let Some(GlobalAlloc::Static(did)) = alloc_kind {
+                    // `extern static` cannot be validated as they have no body.
+                    // FIXME: Statics from other crates are also skipped.
+                    // They might be checked at a different type, but for now we
+                    // want to avoid recursing too deeply.  This is not sound!
+                    if !did.is_local() || self.ecx.tcx.is_foreign_item(did) {
+                        return Ok(());
+                    }
+                }
+            }
+            // Proceed recursively even for ZST, no reason to skip them!
+            // `!` is a ZST and we want to validate it.
+            // Normalize before handing `place` to tracking because that will
+            // check for duplicates.
+            let place = if size.bytes() > 0 {
+                self.ecx.force_mplace_ptr(place).expect("we already bounds-checked")
+            } else {
+                place
+            };
+            let path = &self.path;
+            ref_tracking.track(place, || {
+                // We need to clone the path anyway, make sure it gets created
+                // with enough space for the additional `Deref`.
+                let mut new_path = Vec::with_capacity(path.len() + 1);
+                new_path.clone_from(path);
+                new_path.push(PathElem::Deref);
+                new_path
+            });
+        }
+        Ok(())
+    }
+
+    /// Check if this is a value of primitive type, and if yes check the validity of the value
+    /// at that type.  Return `true` if the type is indeed primitive.
+    fn visit_primitive(&mut self, value: OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx, bool> {
         // Go over all the primitive types
         let ty = value.layout.ty;
         match ty.kind {
             ty::Bool => {
-                let value = value.to_scalar_or_undef();
+                let value = self.ecx.read_scalar(value)?;
                 try_validation!(value.to_bool(), value, self.path, "a boolean");
+                Ok(true)
             }
             ty::Char => {
-                let value = value.to_scalar_or_undef();
+                let value = self.ecx.read_scalar(value)?;
                 try_validation!(value.to_char(), value, self.path, "a valid unicode codepoint");
+                Ok(true)
             }
             ty::Float(_) | ty::Int(_) | ty::Uint(_) => {
+                let value = self.ecx.read_scalar(value)?;
                 // NOTE: Keep this in sync with the array optimization for int/float
                 // types below!
-                let value = value.to_scalar_or_undef();
                 if self.ref_tracking_for_consts.is_some() {
                     // Integers/floats in CTFE: Must be scalar bits, pointers are dangerous
                     let is_bits = value.not_undef().map_or(false, |v| v.is_bits());
@@ -337,120 +433,68 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M
                     // At run-time, for now, we accept *anything* for these types, including
                     // undef. We should fix that, but let's start low.
                 }
+                Ok(true)
             }
             ty::RawPtr(..) => {
                 // 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);
+                let place = try_validation!(
+                    self.ecx.ref_to_mplace(self.ecx.read_immediate(value)?),
+                    "undefined pointer",
+                    self.path
+                );
                 if place.layout.is_unsized() {
                     self.check_wide_ptr_meta(place.meta, place.layout)?;
                 }
+                Ok(true)
             }
-            _ if ty.is_box() || ty.is_region_ptr() => {
-                // Handle wide pointers.
-                // Check metadata early, for better diagnostics
-                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 dereferenceable and all.
-                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(|| (place.layout.size, place.layout.align.abi));
-                let ptr: Option<_> = match self.ecx.memory.check_ptr_access_align(
-                    place.ptr,
-                    size,
-                    Some(align),
-                    CheckInAllocMsg::InboundsTest,
-                ) {
-                    Ok(ptr) => ptr,
-                    Err(err) => {
-                        info!(
-                            "{:?} did not pass access check for size {:?}, align {:?}",
-                            place.ptr, size, align
-                        );
-                        match err.kind {
-                            err_unsup!(InvalidNullPointerUsage) => {
-                                throw_validation_failure!("a NULL reference", self.path)
-                            }
-                            err_unsup!(AlignmentCheckFailed { required, has }) => {
-                                throw_validation_failure!(
-                                    format_args!(
-                                        "an unaligned reference \
-                                         (required {} byte alignment but found {})",
-                                        required.bytes(),
-                                        has.bytes()
-                                    ),
-                                    self.path
-                                )
-                            }
-                            err_unsup!(ReadBytesAsPointer) => throw_validation_failure!(
-                                "a dangling reference (created from integer)",
-                                self.path
-                            ),
-                            _ => throw_validation_failure!(
-                                "a dangling reference (not entirely in bounds)",
-                                self.path
-                            ),
-                        }
-                    }
-                };
-                // Recursive checking
-                if let Some(ref mut ref_tracking) = self.ref_tracking_for_consts {
-                    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);
-                        if let Some(GlobalAlloc::Static(did)) = alloc_kind {
-                            // `extern static` cannot be validated as they have no body.
-                            // FIXME: Statics from other crates are also skipped.
-                            // They might be checked at a different type, but for now we
-                            // want to avoid recursing too deeply.  This is not sound!
-                            if !did.is_local() || self.ecx.tcx.is_foreign_item(did) {
-                                return Ok(());
-                            }
-                        }
-                    }
-                    // Proceed recursively even for ZST, no reason to skip them!
-                    // `!` is a ZST and we want to validate it.
-                    // Normalize before handing `place` to tracking because that will
-                    // check for duplicates.
-                    let place = if size.bytes() > 0 {
-                        self.ecx.force_mplace_ptr(place).expect("we already bounds-checked")
-                    } else {
-                        place
-                    };
-                    let path = &self.path;
-                    ref_tracking.track(place, || {
-                        // We need to clone the path anyway, make sure it gets created
-                        // with enough space for the additional `Deref`.
-                        let mut new_path = Vec::with_capacity(path.len() + 1);
-                        new_path.clone_from(path);
-                        new_path.push(PathElem::Deref);
-                        new_path
-                    });
-                }
+            ty::Ref(..) => {
+                self.check_safe_pointer(value)?;
+                Ok(true)
+            }
+            ty::Adt(def, ..) if def.is_box() => {
+                // FIXME make sure we have a test for `Box`!
+                self.check_safe_pointer(value)?;
+                Ok(true)
             }
             ty::FnPtr(_sig) => {
-                let value = value.to_scalar_or_undef();
+                let value = self.ecx.read_scalar(value)?;
                 let _fn = try_validation!(
                     value.not_undef().and_then(|ptr| self.ecx.memory.get_fn(ptr)),
                     value,
                     self.path,
                     "a function pointer"
                 );
                 // FIXME: Check if the signature matches
+                Ok(true)
             }
-            // This should be all the (inhabited) primitive types
-            _ => bug!("Unexpected primitive type {}", value.layout.ty),
+            ty::Never => throw_validation_failure!("a value of the never type `!`", self.path),
+            ty::Foreign(..) | ty::FnDef(..) => {
+                // Nothing to check.
+                Ok(true)
+            }
+            // This should be all the (inhabited) primitive types. The rest is compound, we
+            // check them by visiting their fields/variants.
+            // (`Str` UTF-8 check happens in `visit_aggregate`, too.)
+            ty::Adt(..)
+            | ty::Tuple(..)
+            | ty::Array(..)
+            | ty::Slice(..)
+            | ty::Str
+            | ty::Dynamic(..)
+            | ty::Closure(..)
+            | ty::Generator(..)
+            | ty::GeneratorWitness(..) => Ok(false),
+            // Some types only occur during inference, we should not see them here.
+            ty::Error
+            | ty::Infer(..)
+            | ty::Placeholder(..)
+            | ty::Bound(..)
+            | ty::Param(..)
+            | ty::Opaque(..)
+            | ty::UnnormalizedProjection(..)
+            | ty::Projection(..) => bug!("Encountered invalid type {:?}", ty),
         }
-        Ok(())
     }
 
     fn visit_scalar(
@@ -558,40 +602,23 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
     }
 
     #[inline(always)]
-    fn visit_union(&mut self, _v: Self::V, fields: usize) -> InterpResult<'tcx> {
-        // Empty unions are not accepted by rustc. That's great, it means we can
-        // use that as a signal for detecting primitives.  Make sure
-        // we did not miss any primitive.
-        assert!(fields > 0);
+    fn visit_union(&mut self, op: OpTy<'tcx, M::PointerTag>, fields: usize) -> InterpResult<'tcx> {
+        // Empty unions are not accepted by rustc. But uninhabited enums
+        // claim to be unions, so allow them, too.
+        assert!(op.layout.abi.is_uninhabited() || fields > 0);
         Ok(())
     }
 
     #[inline]
     fn visit_value(&mut self, op: OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx> {
         trace!("visit_value: {:?}, {:?}", *op, op.layout);
 
-        if op.layout.abi.is_uninhabited() {
-            // Uninhabited types do not have sensible layout, stop right here.
-            throw_validation_failure!(
-                format_args!("a value of uninhabited type {:?}", op.layout.ty),
-                self.path
-            )
-        }
-
-        // Check primitive types.  We do this after checking for uninhabited types,
-        // to exclude uninhabited enums (that also appear as fieldless unions here).
-        // Primitives can have varying layout, so we check them separately and before aggregate
-        // handling.
-        // It is CRITICAL that we get this check right, or we might be validating the wrong thing!
-        let primitive = match op.layout.fields {
-            // Primitives appear as Union with 0 fields - except for Boxes and fat pointers.
-            layout::FieldPlacement::Union(0) => true,
-            _ => op.layout.ty.builtin_deref(true).is_some(),
-        };
-        if primitive {
-            // No need to recurse further or check scalar layout, this is a leaf type.
-            return self.visit_primitive(op);
+        // Check primitive types -- the leafs of our recursive descend.
+        if self.visit_primitive(op)? {
+            return Ok(());
         }
+        // Sanity check: `builtin_deref` does not know any pointers that are not primitive.
+        assert!(op.layout.ty.builtin_deref(true).is_none());
 
         // Recursively walk the type. Translate some possible errors to something nicer.
         match self.walk_value(op) {
@@ -618,7 +645,12 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
         // scalars, we do the same check on every "level" (e.g., first we check
         // MyNewtype and then the scalar in there).
         match op.layout.abi {
-            layout::Abi::Uninhabited => unreachable!(), // checked above
+            layout::Abi::Uninhabited => {
+                throw_validation_failure!(
+                    format_args!("a value of uninhabited type {:?}", op.layout.ty),
+                    self.path
+                );
+            }
             layout::Abi::Scalar(ref scalar_layout) => {
                 self.visit_scalar(op, scalar_layout)?;
             }
diff --git a/src/test/ui/consts/const-eval/ub-uninhabit.stderr b/src/test/ui/consts/const-eval/ub-uninhabit.stderr
@@ -18,7 +18,7 @@ error[E0080]: it is undefined behavior to use this value
   --> $DIR/ub-uninhabit.rs:21:1
    |
 LL | const BAD_BAD_ARRAY: [Bar; 1] = unsafe { (TransmuteUnion::<(), [Bar; 1]> { a: () }).b };
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of uninhabited type [Bar; 1]
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of uninhabited type Bar at [0]
    |
    = 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.
 
diff --git a/src/test/ui/consts/const-eval/validate_uninhabited_zsts.stderr b/src/test/ui/consts/const-eval/validate_uninhabited_zsts.stderr
@@ -20,7 +20,7 @@ error[E0080]: it is undefined behavior to use this value
   --> $DIR/validate_uninhabited_zsts.rs:17:1
    |
 LL | const BAR: [Empty; 3] = [unsafe { std::mem::transmute(()) }; 3];
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of uninhabited type [Empty; 3]
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of uninhabited type Empty at [0]
    |
    = 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.
 
diff --git a/src/test/ui/consts/validate_never_arrays.stderr b/src/test/ui/consts/validate_never_arrays.stderr

Original file line number	Diff line number	Diff line change
`@@ -18,7 +18,7 @@ error[E0080]: it is undefined behavior to use this value`
`18`	`18`	`--> $DIR/ub-uninhabit.rs:21:1`
`19`	`19`	`\|`
`20`	`20`	`LL \| const BAD_BAD_ARRAY: [Bar; 1] = unsafe { (TransmuteUnion::<(), [Bar; 1]> { a: () }).b };`
`21`		`- \| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of uninhabited type [Bar; 1]`
	`21`	`+ \| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of uninhabited type Bar at [0]`
`22`	`22`	`\|`
`23`	`23`	`= 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.`
`24`	`24`
Original file line number	Diff line number	Diff line change
`@@ -20,7 +20,7 @@ error[E0080]: it is undefined behavior to use this value`
`20`	`20`	`--> $DIR/validate_uninhabited_zsts.rs:17:1`
`21`	`21`	`\|`
`22`	`22`	`LL \| const BAR: [Empty; 3] = [unsafe { std::mem::transmute(()) }; 3];`
`23`		`- \| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of uninhabited type [Empty; 3]`
	`23`	`+ \| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of uninhabited type Empty at [0]`
`24`	`24`	`\|`
`25`	`25`	`= 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.`
`26`	`26`