Rollup merge of #69762 - RalfJung:validity-errors, r=oli-obk

Ensure that validity only raises validity errors

For now, only as a debug-assertion (similar to const-prop detecting errors that allocate).

Now includes #69646.
[Relative diff](RalfJung/rust@layout-visitor...RalfJung:validity-errors).

r? @oli-obk

Author: Centril

src/librustc/mir/interpret/error.rs

@@ -245,7 +245,7 @@ fn print_backtrace(backtrace: &mut Backtrace) {
     eprintln!("\n\nAn error occurred in miri:\n{:?}", backtrace);
 }
 
-impl From<ErrorHandled> for InterpErrorInfo<'tcx> {
+impl From<ErrorHandled> for InterpErrorInfo<'_> {
     fn from(err: ErrorHandled) -> Self {
         match err {
             ErrorHandled::Reported => err_inval!(ReferencedConstant),
@@ -291,7 +291,7 @@ pub enum InvalidProgramInfo<'tcx> {
     Layout(layout::LayoutError<'tcx>),
 }
 
-impl fmt::Debug for InvalidProgramInfo<'tcx> {
+impl fmt::Debug for InvalidProgramInfo<'_> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         use InvalidProgramInfo::*;
         match self {
@@ -321,6 +321,8 @@ pub enum UndefinedBehaviorInfo {
     RemainderByZero,
     /// Overflowing inbounds pointer arithmetic.
     PointerArithOverflow,
+    /// Invalid metadata in a wide pointer (using `str` to avoid allocations).
+    InvalidMeta(&'static str),
 }
 
 impl fmt::Debug for UndefinedBehaviorInfo {
@@ -338,6 +340,7 @@ impl fmt::Debug for UndefinedBehaviorInfo {
             DivisionByZero => write!(f, "dividing by zero"),
             RemainderByZero => write!(f, "calculating the remainder with a divisor of zero"),
             PointerArithOverflow => write!(f, "overflowing in-bounds pointer arithmetic"),
+            InvalidMeta(msg) => write!(f, "invalid metadata in wide pointer: {}", msg),
         }
     }
 }
@@ -354,8 +357,8 @@ pub enum UnsupportedOpInfo<'tcx> {
     Unsupported(String),
 
     /// When const-prop encounters a situation it does not support, it raises this error.
-    /// This must not allocate for performance reasons.
-    ConstPropUnsupported(&'tcx str),
+    /// This must not allocate for performance reasons (hence `str`, not `String`).
+    ConstPropUnsupported(&'static str),
 
     // -- Everything below is not categorized yet --
     FunctionAbiMismatch(Abi, Abi),
@@ -612,3 +615,19 @@ impl fmt::Debug for InterpError<'_> {
         }
     }
 }
+
+impl InterpError<'_> {
+    /// Some errors allocate to be created as they contain free-form strings.
+    /// And sometimes we want to be sure that did not happen as it is a
+    /// waste of resources.
+    pub fn allocates(&self) -> bool {
+        match self {
+            InterpError::MachineStop(_)
+            | InterpError::Unsupported(UnsupportedOpInfo::Unsupported(_))
+            | InterpError::Unsupported(UnsupportedOpInfo::ValidationFailure(_))
+            | InterpError::UndefinedBehavior(UndefinedBehaviorInfo::Ub(_))
+            | InterpError::UndefinedBehavior(UndefinedBehaviorInfo::UbExperimental(_)) => true,
+            _ => false,
+        }
+    }
+}

src/librustc_mir/const_eval/eval_queries.rs

@@ -186,7 +186,12 @@ fn validate_and_turn_into_const<'tcx>(
         if cid.promoted.is_none() {
             let mut ref_tracking = RefTracking::new(mplace);
             while let Some((mplace, path)) = ref_tracking.todo.pop() {
-                ecx.validate_operand(mplace.into(), path, Some(&mut ref_tracking))?;
+                ecx.const_validate_operand(
+                    mplace.into(),
+                    path,
+                    &mut ref_tracking,
+                    /*may_ref_to_static*/ is_static,
+                )?;
             }
         }
         // Now that we validated, turn this into a proper constant.

src/librustc_mir/interpret/eval_context.rs

@@ -457,10 +457,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
 
                 // Check if this brought us over the size limit.
                 if size.bytes() >= self.tcx.data_layout().obj_size_bound() {
-                    throw_ub_format!(
-                        "wide pointer metadata contains invalid information: \
-                        total size is bigger than largest supported object"
-                    );
+                    throw_ub!(InvalidMeta("total size is bigger than largest supported object"));
                 }
                 Ok(Some((size, align)))
             }
@@ -476,10 +473,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
 
                 // Make sure the slice is not too big.
                 let size = elem.size.checked_mul(len, &*self.tcx).ok_or_else(|| {
-                    err_ub_format!(
-                        "invalid slice: \
-                        total size is bigger than largest supported object"
-                    )
+                    err_ub!(InvalidMeta("slice is bigger than largest supported object"))
                 })?;
                 Ok(Some((size, elem.align.abi)))
             }
@@ -685,7 +679,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     // 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)?, vec![], None)?;
+                    self.validate_operand(self.place_to_op(return_place)?)?;
                 }
             } else {
                 // Uh, that shouldn't happen... the function did not intend to return

src/librustc_mir/interpret/place.rs

@@ -689,7 +689,7 @@ where
 
         if M::enforce_validity(self) {
             // Data got changed, better make sure it matches the type!
-            self.validate_operand(self.place_to_op(dest)?, vec![], None)?;
+            self.validate_operand(self.place_to_op(dest)?)?;
         }
 
         Ok(())
@@ -706,7 +706,7 @@ where
 
         if M::enforce_validity(self) {
             // Data got changed, better make sure it matches the type!
-            self.validate_operand(dest.into(), vec![], None)?;
+            self.validate_operand(dest.into())?;
         }
 
         Ok(())
@@ -843,7 +843,7 @@ where
 
         if M::enforce_validity(self) {
             // Data got changed, better make sure it matches the type!
-            self.validate_operand(self.place_to_op(dest)?, vec![], None)?;
+            self.validate_operand(self.place_to_op(dest)?)?;
         }
 
         Ok(())
@@ -951,7 +951,7 @@ where
 
         if M::enforce_validity(self) {
             // Data got changed, better make sure it matches the type!
-            self.validate_operand(dest.into(), vec![], None)?;
+            self.validate_operand(dest.into())?;
         }
 
         Ok(())

src/librustc_mir/interpret/validity.rs

@@ -46,13 +46,17 @@ macro_rules! try_validation {
     ($e:expr, $what:expr, $where:expr, $details:expr) => {{
         match $e {
             Ok(x) => x,
+            // We re-throw the error, so we are okay with allocation:
+            // this can only slow down builds that fail anyway.
             Err(_) => throw_validation_failure!($what, $where, $details),
         }
     }};
 
     ($e:expr, $what:expr, $where:expr) => {{
         match $e {
             Ok(x) => x,
+            // We re-throw the error, so we are okay with allocation:
+            // this can only slow down builds that fail anyway.
             Err(_) => throw_validation_failure!($what, $where),
         }
     }};
@@ -167,6 +171,7 @@ struct ValidityVisitor<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> {
     path: Vec<PathElem>,
     ref_tracking_for_consts:
         Option<&'rt mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>>,
+    may_ref_to_static: bool,
     ecx: &'rt InterpCx<'mir, 'tcx, M>,
 }
 
@@ -320,9 +325,17 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M
             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)?
+        let size_and_align = match self.ecx.size_and_align_of(place.meta, place.layout) {
+            Ok(res) => res,
+            Err(err) => match err.kind {
+                err_ub!(InvalidMeta(msg)) => throw_validation_failure!(
+                    format_args!("invalid {} metadata: {}", kind, msg),
+                    self.path
+                ),
+                _ => bug!("Unexpected error during ptr size_and_align_of: {}", err),
+            },
+        };
+        let (size, align) = size_and_align
             // 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).
@@ -359,10 +372,13 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M
                         format_args!("a dangling {} (created from integer)", kind),
                         self.path
                     ),
-                    _ => throw_validation_failure!(
-                        format_args!("a dangling {} (not entirely in bounds)", kind),
-                        self.path
-                    ),
+                    err_unsup!(PointerOutOfBounds { .. }) | err_unsup!(DanglingPointerDeref) => {
+                        throw_validation_failure!(
+                            format_args!("a dangling {} (not entirely in bounds)", kind),
+                            self.path
+                        )
+                    }
+                    _ => bug!("Unexpected error during ptr inbounds test: {}", err),
                 }
             }
         };
@@ -380,6 +396,12 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M
                     if !did.is_local() || self.ecx.tcx.is_foreign_item(did) {
                         return Ok(());
                     }
+                    if !self.may_ref_to_static && self.ecx.tcx.is_static(did) {
+                        throw_validation_failure!(
+                            format_args!("a {} pointing to a static variable", kind),
+                            self.path
+                        );
+                    }
                 }
             }
             // Proceed recursively even for ZST, no reason to skip them!
@@ -638,6 +660,7 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
                 err_unsup!(ReadPointerAsBytes) => {
                     throw_validation_failure!("a pointer", self.path, "plain (non-pointer) bytes")
                 }
+                // Propagate upwards (that will also check for unexpected errors).
                 _ => return Err(err),
             },
         }
@@ -773,31 +796,59 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
 }
 
 impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
-    /// This function checks the data at `op`. `op` is assumed to cover valid memory if it
-    /// is an indirect operand.
-    /// It will error if the bits at the destination do not match the ones described by the layout.
-    ///
-    /// `ref_tracking_for_consts` can be `None` to avoid recursive checking below references.
-    /// This also toggles between "run-time" (no recursion) and "compile-time" (with recursion)
-    /// validation (e.g., pointer values are fine in integers at runtime) and various other const
-    /// specific validation checks.
-    pub fn validate_operand(
+    fn validate_operand_internal(
         &self,
         op: OpTy<'tcx, M::PointerTag>,
         path: Vec<PathElem>,
         ref_tracking_for_consts: Option<
             &mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>,
         >,
+        may_ref_to_static: bool,
     ) -> InterpResult<'tcx> {
-        trace!("validate_operand: {:?}, {:?}", *op, op.layout.ty);
+        trace!("validate_operand_internal: {:?}, {:?}", *op, op.layout.ty);
 
         // Construct a visitor
-        let mut visitor = ValidityVisitor { path, ref_tracking_for_consts, ecx: self };
+        let mut visitor =
+            ValidityVisitor { path, ref_tracking_for_consts, may_ref_to_static, ecx: self };
 
         // Try to cast to ptr *once* instead of all the time.
         let op = self.force_op_ptr(op).unwrap_or(op);
 
-        // Run it
-        visitor.visit_value(op)
+        // Run it.
+        match visitor.visit_value(op) {
+            Ok(()) => Ok(()),
+            Err(err) if matches!(err.kind, err_unsup!(ValidationFailure { .. })) => Err(err),
+            Err(err) if cfg!(debug_assertions) => {
+                bug!("Unexpected error during validation: {}", err)
+            }
+            Err(err) => Err(err),
+        }
+    }
+
+    /// This function checks the data at `op` to be const-valid.
+    /// `op` is assumed to cover valid memory if it is an indirect operand.
+    /// It will error if the bits at the destination do not match the ones described by the layout.
+    ///
+    /// `ref_tracking` is used to record references that we encounter so that they
+    /// can be checked recursively by an outside driving loop.
+    ///
+    /// `may_ref_to_static` controls whether references are allowed to point to statics.
+    #[inline(always)]
+    pub fn const_validate_operand(
+        &self,
+        op: OpTy<'tcx, M::PointerTag>,
+        path: Vec<PathElem>,
+        ref_tracking: &mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>,
+        may_ref_to_static: bool,
+    ) -> InterpResult<'tcx> {
+        self.validate_operand_internal(op, path, Some(ref_tracking), may_ref_to_static)
+    }
+
+    /// This function checks the data at `op` to be runtime-valid.
+    /// `op` is assumed to cover valid memory if it is an indirect operand.
+    /// It will error if the bits at the destination do not match the ones described by the layout.
+    #[inline(always)]
+    pub fn validate_operand(&self, op: OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx> {
+        self.validate_operand_internal(op, vec![], None, false)
     }
 }

src/librustc_mir/transform/const_prop.rs

@@ -404,32 +404,15 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
         let r = match f(self) {
             Ok(val) => Some(val),
             Err(error) => {
-                use rustc::mir::interpret::{
-                    InterpError::*, UndefinedBehaviorInfo, UnsupportedOpInfo,
-                };
-                match error.kind {
-                    MachineStop(_) => bug!("ConstProp does not stop"),
-
-                    // Some error shouldn't come up because creating them causes
-                    // an allocation, which we should avoid. When that happens,
-                    // dedicated error variants should be introduced instead.
-                    // Only test this in debug builds though to avoid disruptions.
-                    Unsupported(UnsupportedOpInfo::Unsupported(_))
-                    | Unsupported(UnsupportedOpInfo::ValidationFailure(_))
-                    | UndefinedBehavior(UndefinedBehaviorInfo::Ub(_))
-                    | UndefinedBehavior(UndefinedBehaviorInfo::UbExperimental(_))
-                        if cfg!(debug_assertions) =>
-                    {
-                        bug!("const-prop encountered allocating error: {:?}", error.kind);
-                    }
-
-                    Unsupported(_)
-                    | UndefinedBehavior(_)
-                    | InvalidProgram(_)
-                    | ResourceExhaustion(_) => {
-                        // Ignore these errors.
-                    }
-                }
+                // Some errors shouldn't come up because creating them causes
+                // an allocation, which we should avoid. When that happens,
+                // dedicated error variants should be introduced instead.
+                // Only test this in debug builds though to avoid disruptions.
+                debug_assert!(
+                    !error.kind.allocates(),
+                    "const-prop encountered allocating error: {}",
+                    error
+                );
                 None
             }
         };
@@ -654,11 +637,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
         source_info: SourceInfo,
     ) {
         trace!("attepting to replace {:?} with {:?}", rval, value);
-        if let Err(e) = self.ecx.validate_operand(
+        if let Err(e) = self.ecx.const_validate_operand(
             value,
             vec![],
             // FIXME: is ref tracking too expensive?
-            Some(&mut interpret::RefTracking::empty()),
+            &mut interpret::RefTracking::empty(),
+            /*may_ref_to_static*/ true,
         ) {
             trace!("validation error, attempt failed: {:?}", e);
             return;

src/test/ui/consts/const-eval/dangling.rs

@@ -6,7 +6,7 @@ use std::{mem, usize};
 const TEST: () = { unsafe { //~ NOTE
     let slice: *const [u8] = mem::transmute((1usize, usize::MAX));
     let _val = &*slice; //~ ERROR: any use of this value will cause an error
-    //~^ NOTE: total size is bigger than largest supported object
+    //~^ NOTE: slice is bigger than largest supported object
     //~^^ on by default
 } };
 

src/test/ui/consts/const-eval/dangling.stderr

@@ -4,7 +4,7 @@ error: any use of this value will cause an error
 LL | / const TEST: () = { unsafe {
 LL | |     let slice: *const [u8] = mem::transmute((1usize, usize::MAX));
 LL | |     let _val = &*slice;
-   | |                ^^^^^^^ invalid slice: total size is bigger than largest supported object
+   | |                ^^^^^^^ invalid metadata in wide pointer: slice is bigger than largest supported object
 LL | |
 LL | |
 LL | | } };