panic when an interpreter error gets unintentionally discarded

Author: RalfJung

compiler/rustc_const_eval/src/interpret/place.rs

@@ -13,9 +13,9 @@ use rustc_target::abi::{Abi, Align, HasDataLayout, Size};
 use tracing::{instrument, trace};
 
 use super::{
-    AllocRef, AllocRefMut, CheckAlignMsg, CtfeProvenance, ImmTy, Immediate, InterpCx, InterpResult,
-    Machine, MemoryKind, Misalignment, OffsetMode, OpTy, Operand, Pointer, Projectable, Provenance,
-    Scalar, alloc_range, mir_assign_valid_types,
+    AllocRef, AllocRefMut, CheckAlignMsg, CtfeProvenance, DiscardInterpError, ImmTy, Immediate,
+    InterpCx, InterpResult, Machine, MemoryKind, Misalignment, OffsetMode, OpTy, Operand, Pointer,
+    Projectable, Provenance, Scalar, alloc_range, mir_assign_valid_types,
 };
 
 #[derive(Copy, Clone, Hash, PartialEq, Eq, Debug)]
@@ -490,9 +490,16 @@ where
         // If an access is both OOB and misaligned, we want to see the bounds error.
         // However we have to call `check_misalign` first to make the borrow checker happy.
         let misalign_err = self.check_misalign(mplace.mplace.misaligned, CheckAlignMsg::BasedOn);
-        let a = self.get_ptr_alloc_mut(mplace.ptr(), size)?;
-        misalign_err?;
-        Ok(a)
+        match self.get_ptr_alloc_mut(mplace.ptr(), size) {
+            Ok(a) => {
+                misalign_err?;
+                Ok(a)
+            }
+            Err(e) => {
+                misalign_err.discard_interp_error();
+                Err(e)
+            }
+        }
     }
 
     /// Turn a local in the current frame into a place.

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -17,8 +17,8 @@ use rustc_hir as hir;
 use rustc_middle::bug;
 use rustc_middle::mir::interpret::ValidationErrorKind::{self, *};
 use rustc_middle::mir::interpret::{
-    ExpectedKind, InterpError, InvalidMetaKind, Misalignment, PointerKind, Provenance,
-    UnsupportedOpInfo, ValidationErrorInfo, alloc_range,
+    ExpectedKind, InterpError, InterpErrorInfo, InvalidMetaKind, Misalignment, PointerKind,
+    Provenance, UnsupportedOpInfo, ValidationErrorInfo, alloc_range,
 };
 use rustc_middle::ty::layout::{LayoutCx, LayoutOf, TyAndLayout};
 use rustc_middle::ty::{self, Ty};
@@ -95,16 +95,19 @@ macro_rules! try_validation {
             Ok(x) => x,
             // We catch the error and turn it into a validation failure. We are okay with
             // allocation here as this can only slow down builds that fail anyway.
-            Err(e) => match e.kind() {
-                $(
-                    $($p)|+ =>
-                       throw_validation_failure!(
-                            $where,
-                            $kind
-                        )
-                ),+,
-                #[allow(unreachable_patterns)]
-                _ => Err::<!, _>(e)?,
+            Err(e) => {
+                let (kind, backtrace) = e.into_parts();
+                match kind {
+                    $(
+                        $($p)|+ => {
+                            throw_validation_failure!(
+                                $where,
+                                $kind
+                            )
+                        }
+                    ),+,
+                    _ => Err::<!, _>(InterpErrorInfo::from_parts(kind, backtrace))?,
+                }
             }
         }
     }};
@@ -510,7 +513,7 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValidityVisitor<'rt, 'tcx, M> {
             Ub(DanglingIntPointer { addr: i, .. }) => DanglingPtrNoProvenance {
                 ptr_kind,
                 // FIXME this says "null pointer" when null but we need translate
-                pointer: format!("{}", Pointer::<Option<AllocId>>::from_addr_invalid(*i))
+                pointer: format!("{}", Pointer::<Option<AllocId>>::from_addr_invalid(i))
             },
             Ub(PointerOutOfBounds { .. }) => DanglingPtrOutOfBounds {
                 ptr_kind
@@ -1231,7 +1234,8 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValueVisitor<'tcx, M> for ValidityVisitor<'rt,
                     Err(err) => {
                         // For some errors we might be able to provide extra information.
                         // (This custom logic does not fit the `try_validation!` macro.)
-                        match err.kind() {
+                        let (kind, backtrace) = err.into_parts();
+                        match kind {
                             Ub(InvalidUninitBytes(Some((_alloc_id, access)))) | Unsup(ReadPointerAsInt(Some((_alloc_id, access)))) => {
                                 // Some byte was uninitialized, determine which
                                 // element that byte belongs to so we can
@@ -1242,15 +1246,15 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValueVisitor<'tcx, M> for ValidityVisitor<'rt,
                                 .unwrap();
                                 self.path.push(PathElem::ArrayElem(i));
 
-                                if matches!(err.kind(), Ub(InvalidUninitBytes(_))) {
+                                if matches!(kind, Ub(InvalidUninitBytes(_))) {
                                     throw_validation_failure!(self.path, Uninit { expected })
                                 } else {
                                     throw_validation_failure!(self.path, PointerAsInt { expected })
                                 }
                             }
 
                             // Propagate upwards (that will also check for unexpected errors).
-                            _ => return Err(err),
+                            _ => return Err(InterpErrorInfo::from_parts(kind, backtrace)),
                         }
                     }
                 }
@@ -1282,7 +1286,7 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValueVisitor<'tcx, M> for ValidityVisitor<'rt,
                     // It's not great to catch errors here, since we can't give a very good path,
                     // but it's better than ICEing.
                     Ub(InvalidVTableTrait { vtable_dyn_type, expected_dyn_type }) => {
-                        InvalidMetaWrongTrait { vtable_dyn_type, expected_dyn_type: *expected_dyn_type }
+                        InvalidMetaWrongTrait { vtable_dyn_type, expected_dyn_type }
                     },
                 );
             }

compiler/rustc_middle/src/mir/interpret/error.rs

@@ -1,7 +1,7 @@
 use std::any::Any;
 use std::backtrace::Backtrace;
 use std::borrow::Cow;
-use std::fmt;
+use std::{fmt, mem};
 
 use either::Either;
 use rustc_ast_ir::Mutability;
@@ -104,13 +104,57 @@ rustc_data_structures::static_assert_size!(InterpErrorInfo<'_>, 8);
 /// These should always be constructed by calling `.into()` on
 /// an `InterpError`. In `rustc_mir::interpret`, we have `throw_err_*`
 /// macros for this.
+///
+/// Interpreter errors must *not* be silently discarded (that will lead to a panic). Instead,
+/// explicitly call `discard_interp_error` if this is really the right thing to do. Note that if
+/// this happens during const-eval or in Miri, it could lead to a UB error being lost!
 #[derive(Debug)]
 pub struct InterpErrorInfo<'tcx>(Box<InterpErrorInfoInner<'tcx>>);
 
+/// Calling `.ok()` on an `InterpResult` leads to a panic because of the guard.
+/// To still let people opt-in to discarding interpreter errors, we have this extension trait.
+pub trait DiscardInterpError {
+    type Output;
+
+    fn discard_interp_error(self) -> Option<Self::Output>;
+}
+
+impl<'tcx, T> DiscardInterpError for InterpResult<'tcx, T> {
+    type Output = T;
+
+    fn discard_interp_error(self) -> Option<Self::Output> {
+        match self {
+            Ok(v) => Some(v),
+            Err(e) => {
+                // Disarm the guard.
+                mem::forget(e.0.guard);
+                None
+            }
+        }
+    }
+}
+
+#[derive(Debug)]
+struct Guard;
+
+impl Drop for Guard {
+    fn drop(&mut self) {
+        // We silence the guard if we are already panicking, to avoid double-panics.
+        if !std::thread::panicking() {
+            panic!(
+                "an interpreter error got improperly discarded; use `discard_interp_error()` instead of `ok()` if this is intentional"
+            );
+        }
+    }
+}
+
 #[derive(Debug)]
 struct InterpErrorInfoInner<'tcx> {
     kind: InterpError<'tcx>,
     backtrace: InterpErrorBacktrace,
+    /// This makes us panic on drop. This is used to catch
+    /// accidentally discarding an interpreter error.
+    guard: Guard,
 }
 
 #[derive(Debug)]
@@ -151,15 +195,25 @@ impl InterpErrorBacktrace {
 
 impl<'tcx> InterpErrorInfo<'tcx> {
     pub fn into_parts(self) -> (InterpError<'tcx>, InterpErrorBacktrace) {
-        let InterpErrorInfo(box InterpErrorInfoInner { kind, backtrace }) = self;
+        let InterpErrorInfo(box InterpErrorInfoInner { kind, backtrace, guard }) = self;
+        mem::forget(guard); // The error got explicitly discarded right here.
         (kind, backtrace)
     }
 
     pub fn into_kind(self) -> InterpError<'tcx> {
-        let InterpErrorInfo(box InterpErrorInfoInner { kind, .. }) = self;
+        let InterpErrorInfo(box InterpErrorInfoInner { kind, guard, .. }) = self;
+        mem::forget(guard); // The error got explicitly discarded right here.
         kind
     }
 
+    pub fn discard_interp_error(self) {
+        mem::forget(self.0.guard); // The error got explicitly discarded right here.
+    }
+
+    pub fn from_parts(kind: InterpError<'tcx>, backtrace: InterpErrorBacktrace) -> Self {
+        Self(Box::new(InterpErrorInfoInner { kind, backtrace, guard: Guard }))
+    }
+
     #[inline]
     pub fn kind(&self) -> &InterpError<'tcx> {
         &self.0.kind
@@ -191,6 +245,7 @@ impl<'tcx> From<InterpError<'tcx>> for InterpErrorInfo<'tcx> {
         InterpErrorInfo(Box::new(InterpErrorInfoInner {
             kind,
             backtrace: InterpErrorBacktrace::new(),
+            guard: Guard,
         }))
     }
 }

compiler/rustc_middle/src/mir/interpret/mod.rs

@@ -34,12 +34,12 @@ pub use self::allocation::{
     InitChunkIter, alloc_range,
 };
 pub use self::error::{
-    BadBytesAccess, CheckAlignMsg, CheckInAllocMsg, ErrorHandled, EvalStaticInitializerRawResult,
-    EvalToAllocationRawResult, EvalToConstValueResult, EvalToValTreeResult, ExpectedKind,
-    InterpError, InterpErrorInfo, InterpResult, InvalidMetaKind, InvalidProgramInfo,
-    MachineStopType, Misalignment, PointerKind, ReportedErrorInfo, ResourceExhaustionInfo,
-    ScalarSizeMismatch, UndefinedBehaviorInfo, UnsupportedOpInfo, ValidationErrorInfo,
-    ValidationErrorKind,
+    BadBytesAccess, CheckAlignMsg, CheckInAllocMsg, DiscardInterpError, ErrorHandled,
+    EvalStaticInitializerRawResult, EvalToAllocationRawResult, EvalToConstValueResult,
+    EvalToValTreeResult, ExpectedKind, InterpError, InterpErrorInfo, InterpResult, InvalidMetaKind,
+    InvalidProgramInfo, MachineStopType, Misalignment, PointerKind, ReportedErrorInfo,
+    ResourceExhaustionInfo, ScalarSizeMismatch, UndefinedBehaviorInfo, UnsupportedOpInfo,
+    ValidationErrorInfo, ValidationErrorKind,
 };
 pub use self::pointer::{CtfeProvenance, Pointer, PointerArithmetic, Provenance};
 pub use self::value::Scalar;

compiler/rustc_mir_transform/src/dataflow_const_prop.rs

@@ -3,7 +3,9 @@
 //! Currently, this pass only propagates scalar values.
 
 use rustc_const_eval::const_eval::{DummyMachine, throw_machine_stop_str};
-use rustc_const_eval::interpret::{ImmTy, Immediate, InterpCx, OpTy, PlaceTy, Projectable};
+use rustc_const_eval::interpret::{
+    DiscardInterpError, ImmTy, Immediate, InterpCx, OpTy, PlaceTy, Projectable,
+};
 use rustc_data_structures::fx::FxHashMap;
 use rustc_hir::def::DefKind;
 use rustc_middle::bug;
@@ -364,8 +366,10 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> {
                 }
             }
             Operand::Constant(box constant) => {
-                if let Ok(constant) =
-                    self.ecx.eval_mir_constant(&constant.const_, constant.span, None)
+                if let Some(constant) = self
+                    .ecx
+                    .eval_mir_constant(&constant.const_, constant.span, None)
+                    .discard_interp_error()
                 {
                     self.assign_constant(state, place, constant, &[]);
                 }
@@ -387,15 +391,17 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> {
         for &(mut proj_elem) in projection {
             if let PlaceElem::Index(index) = proj_elem {
                 if let FlatSet::Elem(index) = state.get(index.into(), &self.map)
-                    && let Ok(offset) = index.to_target_usize(&self.tcx)
+                    && let Some(offset) = index.to_target_usize(&self.tcx).discard_interp_error()
                     && let Some(min_length) = offset.checked_add(1)
                 {
                     proj_elem = PlaceElem::ConstantIndex { offset, min_length, from_end: false };
                 } else {
                     return;
                 }
             }
-            operand = if let Ok(operand) = self.ecx.project(&operand, proj_elem) {
+            operand = if let Some(operand) =
+                self.ecx.project(&operand, proj_elem).discard_interp_error()
+            {
                 operand
             } else {
                 return;
@@ -406,24 +412,30 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> {
             place,
             operand,
             &mut |elem, op| match elem {
-                TrackElem::Field(idx) => self.ecx.project_field(op, idx.as_usize()).ok(),
-                TrackElem::Variant(idx) => self.ecx.project_downcast(op, idx).ok(),
+                TrackElem::Field(idx) => {
+                    self.ecx.project_field(op, idx.as_usize()).discard_interp_error()
+                }
+                TrackElem::Variant(idx) => {
+                    self.ecx.project_downcast(op, idx).discard_interp_error()
+                }
                 TrackElem::Discriminant => {
-                    let variant = self.ecx.read_discriminant(op).ok()?;
-                    let discr_value =
-                        self.ecx.discriminant_for_variant(op.layout.ty, variant).ok()?;
+                    let variant = self.ecx.read_discriminant(op).discard_interp_error()?;
+                    let discr_value = self
+                        .ecx
+                        .discriminant_for_variant(op.layout.ty, variant)
+                        .discard_interp_error()?;
                     Some(discr_value.into())
                 }
                 TrackElem::DerefLen => {
-                    let op: OpTy<'_> = self.ecx.deref_pointer(op).ok()?.into();
-                    let len_usize = op.len(&self.ecx).ok()?;
+                    let op: OpTy<'_> = self.ecx.deref_pointer(op).discard_interp_error()?.into();
+                    let len_usize = op.len(&self.ecx).discard_interp_error()?;
                     let layout =
                         self.tcx.layout_of(self.param_env.and(self.tcx.types.usize)).unwrap();
                     Some(ImmTy::from_uint(len_usize, layout).into())
                 }
             },
             &mut |place, op| {
-                if let Ok(imm) = self.ecx.read_immediate_raw(op)
+                if let Some(imm) = self.ecx.read_immediate_raw(op).discard_interp_error()
                     && let Some(imm) = imm.right()
                 {
                     let elem = self.wrap_immediate(*imm);
@@ -447,11 +459,11 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> {
             (FlatSet::Bottom, _) | (_, FlatSet::Bottom) => (FlatSet::Bottom, FlatSet::Bottom),
             // Both sides are known, do the actual computation.
             (FlatSet::Elem(left), FlatSet::Elem(right)) => {
-                match self.ecx.binary_op(op, &left, &right) {
+                match self.ecx.binary_op(op, &left, &right).discard_interp_error() {
                     // Ideally this would return an Immediate, since it's sometimes
                     // a pair and sometimes not. But as a hack we always return a pair
                     // and just make the 2nd component `Bottom` when it does not exist.
-                    Ok(val) => {
+                    Some(val) => {
                         if matches!(val.layout.abi, Abi::ScalarPair(..)) {
                             let (val, overflow) = val.to_scalar_pair();
                             (FlatSet::Elem(val), FlatSet::Elem(overflow))
@@ -470,7 +482,7 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> {
                 }
 
                 let arg_scalar = const_arg.to_scalar();
-                let Ok(arg_value) = arg_scalar.to_bits(layout.size) else {
+                let Some(arg_value) = arg_scalar.to_bits(layout.size).discard_interp_error() else {
                     return (FlatSet::Top, FlatSet::Top);
                 };
 
@@ -518,8 +530,10 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> {
             return None;
         }
         let enum_ty_layout = self.tcx.layout_of(self.param_env.and(enum_ty)).ok()?;
-        let discr_value =
-            self.ecx.discriminant_for_variant(enum_ty_layout.ty, variant_index).ok()?;
+        let discr_value = self
+            .ecx
+            .discriminant_for_variant(enum_ty_layout.ty, variant_index)
+            .discard_interp_error()?;
         Some(discr_value.to_scalar())
     }
 
@@ -573,7 +587,7 @@ impl<'a, 'tcx> Collector<'a, 'tcx> {
         map: &Map<'tcx>,
     ) -> Option<Const<'tcx>> {
         let ty = place.ty(self.local_decls, self.patch.tcx).ty;
-        let layout = ecx.layout_of(ty).ok()?;
+        let layout = ecx.layout_of(ty).discard_interp_error()?;
 
         if layout.is_zst() {
             return Some(Const::zero_sized(ty));
@@ -595,7 +609,7 @@ impl<'a, 'tcx> Collector<'a, 'tcx> {
                 .intern_with_temp_alloc(layout, |ecx, dest| {
                     try_write_constant(ecx, dest, place, ty, state, map)
                 })
-                .ok()?;
+                .discard_interp_error()?;
             return Some(Const::Val(ConstValue::Indirect { alloc_id, offset: Size::ZERO }, ty));
         }
 
@@ -830,7 +844,7 @@ impl<'tcx> MutVisitor<'tcx> for Patch<'tcx> {
         if let PlaceElem::Index(local) = elem {
             let offset = self.before_effect.get(&(location, local.into()))?;
             let offset = offset.try_to_scalar()?;
-            let offset = offset.to_target_usize(&self.tcx).ok()?;
+            let offset = offset.to_target_usize(&self.tcx).discard_interp_error()?;
             let min_length = offset.checked_add(1)?;
             Some(PlaceElem::ConstantIndex { offset, min_length, from_end: false })
         } else {