Rollup merge of #130885 - RalfJung:interp-error-discard, r=oli-obk

panic when an interpreter error gets unintentionally discarded

One important invariant of Miri is that when an interpreter error is raised (*in particular* a UB error), those must not be discarded: it's not okay to just check `foo().is_err()` and then continue executing.

This seems to catch new contributors by surprise fairly regularly, so this PR tries to make it so that *if* this ever happens, we get a panic rather than a silent missed UB bug. The interpreter error type now contains a "guard" that panics on drop, and that is explicitly passed to `mem::forget` when an error is deliberately discarded.

Fixes https://github.com/rust-lang/miri/issues/3855

compiler/rustc_middle/src/lib.rs

@@ -61,6 +61,8 @@
 #![feature(trait_upcasting)]
 #![feature(trusted_len)]
 #![feature(try_blocks)]
+#![feature(try_trait_v2)]
+#![feature(try_trait_v2_yeet)]
 #![feature(type_alias_impl_trait)]
 #![feature(yeet_expr)]
 #![warn(unreachable_pub)]

compiler/rustc_middle/src/mir/consts.rs

@@ -148,7 +148,7 @@ impl<'tcx> ConstValue<'tcx> {
                         /* read_provenance */ true,
                     )
                     .ok()?;
-                let ptr = ptr.to_pointer(&tcx).ok()?;
+                let ptr = ptr.to_pointer(&tcx).discard_err()?;
                 let len = a
                     .read_scalar(
                         &tcx,
@@ -156,7 +156,7 @@ impl<'tcx> ConstValue<'tcx> {
                         /* read_provenance */ false,
                     )
                     .ok()?;
-                let len = len.to_target_usize(&tcx).ok()?;
+                let len = len.to_target_usize(&tcx).discard_err()?;
                 if len == 0 {
                     return Some(&[]);
                 }

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

@@ -20,7 +20,7 @@ use rustc_target::abi::{Align, HasDataLayout, Size};
 use super::{
     AllocId, BadBytesAccess, CtfeProvenance, InterpError, InterpResult, Pointer, PointerArithmetic,
     Provenance, ResourceExhaustionInfo, Scalar, ScalarSizeMismatch, UndefinedBehaviorInfo,
-    UnsupportedOpInfo, read_target_uint, write_target_uint,
+    UnsupportedOpInfo, interp_ok, read_target_uint, write_target_uint,
 };
 use crate::ty;
 
@@ -318,8 +318,9 @@ impl<Prov: Provenance, Bytes: AllocBytes> Allocation<Prov, (), Bytes> {
     pub fn try_uninit<'tcx>(size: Size, align: Align) -> InterpResult<'tcx, Self> {
         Self::uninit_inner(size, align, || {
             ty::tls::with(|tcx| tcx.dcx().delayed_bug("exhausted memory during interpretation"));
-            InterpError::ResourceExhaustion(ResourceExhaustionInfo::MemoryExhausted).into()
+            InterpError::ResourceExhaustion(ResourceExhaustionInfo::MemoryExhausted)
         })
+        .into()
     }
 
     /// Try to create an Allocation of `size` bytes, panics if there is not enough memory
@@ -355,12 +356,12 @@ impl<Prov: Provenance, Bytes: AllocBytes> Allocation<Prov, (), Bytes> {
 impl Allocation {
     /// Adjust allocation from the ones in `tcx` to a custom Machine instance
     /// with a different `Provenance` and `Byte` type.
-    pub fn adjust_from_tcx<Prov: Provenance, Bytes: AllocBytes, Err>(
+    pub fn adjust_from_tcx<'tcx, Prov: Provenance, Bytes: AllocBytes>(
         &self,
         cx: &impl HasDataLayout,
-        mut alloc_bytes: impl FnMut(&[u8], Align) -> Result<Bytes, Err>,
-        mut adjust_ptr: impl FnMut(Pointer<CtfeProvenance>) -> Result<Pointer<Prov>, Err>,
-    ) -> Result<Allocation<Prov, (), Bytes>, Err> {
+        mut alloc_bytes: impl FnMut(&[u8], Align) -> InterpResult<'tcx, Bytes>,
+        mut adjust_ptr: impl FnMut(Pointer<CtfeProvenance>) -> InterpResult<'tcx, Pointer<Prov>>,
+    ) -> InterpResult<'tcx, Allocation<Prov, (), Bytes>> {
         // Copy the data.
         let mut bytes = alloc_bytes(&*self.bytes, self.align)?;
         // Adjust provenance of pointers stored in this allocation.
@@ -377,7 +378,7 @@ impl Allocation {
             new_provenance.push((offset, ptr_prov));
         }
         // Create allocation.
-        Ok(Allocation {
+        interp_ok(Allocation {
             bytes,
             provenance: ProvenanceMap::from_presorted_ptrs(new_provenance),
             init_mask: self.init_mask.clone(),

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::{convert, fmt, mem, ops};
 
 use either::Either;
 use rustc_ast_ir::Mutability;
@@ -104,6 +104,10 @@ 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_err` 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>>);
 
@@ -156,8 +160,11 @@ impl<'tcx> InterpErrorInfo<'tcx> {
     }
 
     pub fn into_kind(self) -> InterpError<'tcx> {
-        let InterpErrorInfo(box InterpErrorInfoInner { kind, .. }) = self;
-        kind
+        self.0.kind
+    }
+
+    pub fn from_parts(kind: InterpError<'tcx>, backtrace: InterpErrorBacktrace) -> Self {
+        Self(Box::new(InterpErrorInfoInner { kind, backtrace }))
     }
 
     #[inline]
@@ -599,8 +606,6 @@ pub enum InterpError<'tcx> {
     MachineStop(Box<dyn MachineStopType>),
 }
 
-pub type InterpResult<'tcx, T = ()> = Result<T, InterpErrorInfo<'tcx>>;
-
 impl InterpError<'_> {
     /// Some errors do string formatting even if the error is never printed.
     /// To avoid performance issues, there are places where we want to be sure to never raise these formatting errors,
@@ -728,3 +733,182 @@ macro_rules! throw_exhaust {
 macro_rules! throw_machine_stop {
     ($($tt:tt)*) => { do yeet $crate::err_machine_stop!($($tt)*) };
 }
+
+/// Guard type that panics on drop.
+#[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_err()` if this is intentional"
+            );
+        }
+    }
+}
+
+/// The result type used by the interpreter. This is a newtype around `Result`
+/// to block access to operations like `ok()` that discard UB errors.
+///
+/// We also make things panic if this type is ever implicitly dropped.
+#[derive(Debug)]
+pub struct InterpResult_<'tcx, T> {
+    res: Result<T, InterpErrorInfo<'tcx>>,
+    guard: Guard,
+}
+
+// Type alias to be able to set a default type argument.
+pub type InterpResult<'tcx, T = ()> = InterpResult_<'tcx, T>;
+
+impl<'tcx, T> ops::Try for InterpResult_<'tcx, T> {
+    type Output = T;
+    type Residual = InterpResult_<'tcx, convert::Infallible>;
+
+    #[inline]
+    fn from_output(output: Self::Output) -> Self {
+        InterpResult_::new(Ok(output))
+    }
+
+    #[inline]
+    fn branch(self) -> ops::ControlFlow<Self::Residual, Self::Output> {
+        match self.disarm() {
+            Ok(v) => ops::ControlFlow::Continue(v),
+            Err(e) => ops::ControlFlow::Break(InterpResult_::new(Err(e))),
+        }
+    }
+}
+
+impl<'tcx, T> ops::FromResidual for InterpResult_<'tcx, T> {
+    #[inline]
+    #[track_caller]
+    fn from_residual(residual: InterpResult_<'tcx, convert::Infallible>) -> Self {
+        match residual.disarm() {
+            Err(e) => Self::new(Err(e)),
+        }
+    }
+}
+
+// Allow `yeet`ing `InterpError` in functions returning `InterpResult_`.
+impl<'tcx, T> ops::FromResidual<ops::Yeet<InterpError<'tcx>>> for InterpResult_<'tcx, T> {
+    #[inline]
+    fn from_residual(ops::Yeet(e): ops::Yeet<InterpError<'tcx>>) -> Self {
+        Self::new(Err(e.into()))
+    }
+}
+
+// Allow `?` on `Result<_, InterpError>` in functions returning `InterpResult_`.
+// This is useful e.g. for `option.ok_or_else(|| err_ub!(...))`.
+impl<'tcx, T, E: Into<InterpErrorInfo<'tcx>>> ops::FromResidual<Result<convert::Infallible, E>>
+    for InterpResult_<'tcx, T>
+{
+    #[inline]
+    fn from_residual(residual: Result<convert::Infallible, E>) -> Self {
+        match residual {
+            Err(e) => Self::new(Err(e.into())),
+        }
+    }
+}
+
+impl<'tcx, T, E: Into<InterpErrorInfo<'tcx>>> From<Result<T, E>> for InterpResult<'tcx, T> {
+    #[inline]
+    fn from(value: Result<T, E>) -> Self {
+        Self::new(value.map_err(|e| e.into()))
+    }
+}
+
+impl<'tcx, T, V: FromIterator<T>> FromIterator<InterpResult<'tcx, T>> for InterpResult<'tcx, V> {
+    fn from_iter<I: IntoIterator<Item = InterpResult<'tcx, T>>>(iter: I) -> Self {
+        Self::new(iter.into_iter().map(|x| x.disarm()).collect())
+    }
+}
+
+impl<'tcx, T> InterpResult_<'tcx, T> {
+    #[inline(always)]
+    fn new(res: Result<T, InterpErrorInfo<'tcx>>) -> Self {
+        Self { res, guard: Guard }
+    }
+
+    #[inline(always)]
+    fn disarm(self) -> Result<T, InterpErrorInfo<'tcx>> {
+        mem::forget(self.guard);
+        self.res
+    }
+
+    /// Discard the error information in this result. Only use this if ignoring Undefined Behavior is okay!
+    #[inline]
+    pub fn discard_err(self) -> Option<T> {
+        self.disarm().ok()
+    }
+
+    /// Look at the `Result` wrapped inside of this.
+    /// Must only be used to report the error!
+    #[inline]
+    pub fn report_err(self) -> Result<T, InterpErrorInfo<'tcx>> {
+        self.disarm()
+    }
+
+    #[inline]
+    pub fn map<U>(self, f: impl FnOnce(T) -> U) -> InterpResult<'tcx, U> {
+        InterpResult_::new(self.disarm().map(f))
+    }
+
+    #[inline]
+    pub fn map_err(
+        self,
+        f: impl FnOnce(InterpErrorInfo<'tcx>) -> InterpErrorInfo<'tcx>,
+    ) -> InterpResult<'tcx, T> {
+        InterpResult_::new(self.disarm().map_err(f))
+    }
+
+    #[inline]
+    pub fn inspect_err(self, f: impl FnOnce(&InterpErrorInfo<'tcx>)) -> InterpResult<'tcx, T> {
+        InterpResult_::new(self.disarm().inspect_err(f))
+    }
+
+    #[inline]
+    #[track_caller]
+    pub fn unwrap(self) -> T {
+        self.disarm().unwrap()
+    }
+
+    #[inline]
+    #[track_caller]
+    pub fn unwrap_or_else(self, f: impl FnOnce(InterpErrorInfo<'tcx>) -> T) -> T {
+        self.disarm().unwrap_or_else(f)
+    }
+
+    #[inline]
+    #[track_caller]
+    pub fn expect(self, msg: &str) -> T {
+        self.disarm().expect(msg)
+    }
+
+    #[inline]
+    pub fn and_then<U>(self, f: impl FnOnce(T) -> InterpResult<'tcx, U>) -> InterpResult<'tcx, U> {
+        InterpResult_::new(self.disarm().and_then(|t| f(t).disarm()))
+    }
+
+    /// Returns success if both `self` and `other` succeed, while ensuring we don't
+    /// accidentally drop an error.
+    ///
+    /// If both are an error, `self` will be reported.
+    #[inline]
+    pub fn and<U>(self, other: InterpResult<'tcx, U>) -> InterpResult<'tcx, (T, U)> {
+        match self.disarm() {
+            Ok(t) => interp_ok((t, other?)),
+            Err(e) => {
+                // Discard the other error.
+                drop(other.disarm());
+                // Return `self`.
+                InterpResult_::new(Err(e))
+            }
+        }
+    }
+}
+
+#[inline(always)]
+pub fn interp_ok<'tcx, T>(x: T) -> InterpResult<'tcx, T> {
+    InterpResult_::new(Ok(x))
+}

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

@@ -39,7 +39,7 @@ pub use self::error::{
     InterpError, InterpErrorInfo, InterpResult, InvalidMetaKind, InvalidProgramInfo,
     MachineStopType, Misalignment, PointerKind, ReportedErrorInfo, ResourceExhaustionInfo,
     ScalarSizeMismatch, UndefinedBehaviorInfo, UnsupportedOpInfo, ValidationErrorInfo,
-    ValidationErrorKind,
+    ValidationErrorKind, interp_ok,
 };
 pub use self::pointer::{CtfeProvenance, Pointer, PointerArithmetic, Provenance};
 pub use self::value::Scalar;

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

@@ -8,7 +8,7 @@ use rustc_target::abi::{HasDataLayout, Size};
 
 use super::{
     AllocId, CtfeProvenance, InterpResult, Pointer, PointerArithmetic, Provenance,
-    ScalarSizeMismatch,
+    ScalarSizeMismatch, interp_ok,
 };
 use crate::ty::ScalarInt;
 
@@ -273,10 +273,10 @@ impl<'tcx, Prov: Provenance> Scalar<Prov> {
             .to_bits_or_ptr_internal(cx.pointer_size())
             .map_err(|s| err_ub!(ScalarSizeMismatch(s)))?
         {
-            Right(ptr) => Ok(ptr.into()),
+            Right(ptr) => interp_ok(ptr.into()),
             Left(bits) => {
                 let addr = u64::try_from(bits).unwrap();
-                Ok(Pointer::from_addr_invalid(addr))
+                interp_ok(Pointer::from_addr_invalid(addr))
             }
         }
     }
@@ -311,12 +311,12 @@ impl<'tcx, Prov: Provenance> Scalar<Prov> {
         if matches!(self, Scalar::Ptr(..)) {
             *self = self.to_scalar_int()?.into();
         }
-        Ok(())
+        interp_ok(())
     }
 
     #[inline(always)]
     pub fn to_scalar_int(self) -> InterpResult<'tcx, ScalarInt> {
-        self.try_to_scalar_int().map_err(|_| err_unsup!(ReadPointerAsInt(None)).into())
+        self.try_to_scalar_int().map_err(|_| err_unsup!(ReadPointerAsInt(None))).into()
     }
 
     #[inline(always)]
@@ -330,20 +330,22 @@ impl<'tcx, Prov: Provenance> Scalar<Prov> {
     #[inline]
     pub fn to_bits(self, target_size: Size) -> InterpResult<'tcx, u128> {
         assert_ne!(target_size.bytes(), 0, "you should never look at the bits of a ZST");
-        self.to_scalar_int()?.try_to_bits(target_size).map_err(|size| {
-            err_ub!(ScalarSizeMismatch(ScalarSizeMismatch {
-                target_size: target_size.bytes(),
-                data_size: size.bytes(),
-            }))
+        self.to_scalar_int()?
+            .try_to_bits(target_size)
+            .map_err(|size| {
+                err_ub!(ScalarSizeMismatch(ScalarSizeMismatch {
+                    target_size: target_size.bytes(),
+                    data_size: size.bytes(),
+                }))
+            })
             .into()
-        })
     }
 
     pub fn to_bool(self) -> InterpResult<'tcx, bool> {
         let val = self.to_u8()?;
         match val {
-            0 => Ok(false),
-            1 => Ok(true),
+            0 => interp_ok(false),
+            1 => interp_ok(true),
             _ => throw_ub!(InvalidBool(val)),
         }
     }
@@ -351,7 +353,7 @@ impl<'tcx, Prov: Provenance> Scalar<Prov> {
     pub fn to_char(self) -> InterpResult<'tcx, char> {
         let val = self.to_u32()?;
         match std::char::from_u32(val) {
-            Some(c) => Ok(c),
+            Some(c) => interp_ok(c),
             None => throw_ub!(InvalidChar(val)),
         }
     }
@@ -392,7 +394,7 @@ impl<'tcx, Prov: Provenance> Scalar<Prov> {
     /// Fails if the scalar is a pointer.
     pub fn to_target_usize(self, cx: &impl HasDataLayout) -> InterpResult<'tcx, u64> {
         let b = self.to_uint(cx.data_layout().pointer_size)?;
-        Ok(u64::try_from(b).unwrap())
+        interp_ok(u64::try_from(b).unwrap())
     }
 
     /// Converts the scalar to produce a signed integer of the given size.
@@ -400,7 +402,7 @@ impl<'tcx, Prov: Provenance> Scalar<Prov> {
     #[inline]
     pub fn to_int(self, size: Size) -> InterpResult<'tcx, i128> {
         let b = self.to_bits(size)?;
-        Ok(size.sign_extend(b))
+        interp_ok(size.sign_extend(b))
     }
 
     /// Converts the scalar to produce an `i8`. Fails if the scalar is a pointer.
@@ -432,13 +434,13 @@ impl<'tcx, Prov: Provenance> Scalar<Prov> {
     /// Fails if the scalar is a pointer.
     pub fn to_target_isize(self, cx: &impl HasDataLayout) -> InterpResult<'tcx, i64> {
         let b = self.to_int(cx.data_layout().pointer_size)?;
-        Ok(i64::try_from(b).unwrap())
+        interp_ok(i64::try_from(b).unwrap())
     }
 
     #[inline]
     pub fn to_float<F: Float>(self) -> InterpResult<'tcx, F> {
         // Going through `to_bits` to check size and truncation.
-        Ok(F::from_bits(self.to_bits(Size::from_bits(F::BITS))?))
+        interp_ok(F::from_bits(self.to_bits(Size::from_bits(F::BITS))?))
     }
 
     #[inline]

compiler/rustc_middle/src/ty/consts.rs

@@ -519,7 +519,7 @@ impl<'tcx> Const<'tcx> {
     }
 
     pub fn try_to_bool(self) -> Option<bool> {
-        self.try_to_scalar()?.to_bool().ok()
+        self.try_to_valtree()?.try_to_scalar_int()?.try_to_bool().ok()
     }
 
     #[inline]