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.
Fixesrust-lang/miri#3855

Author: workingjubilee

compiler/rustc_const_eval/src/const_eval/dummy_machine.rs

@@ -6,7 +6,8 @@ use rustc_middle::{bug, span_bug, ty};
 use rustc_span::def_id::DefId;
 
 usecrate::interpret::{
-self,HasStaticRootDefId,ImmTy,Immediate,InterpCx,PointerArithmetic, throw_machine_stop,
+self,HasStaticRootDefId,ImmTy,Immediate,InterpCx,PointerArithmetic, interp_ok,
+ throw_machine_stop,
 };
 
 /// Macro for machine-specific `InterpError` without allocation.
@@ -79,7 +80,7 @@ impl<'tcx> interpret::Machine<'tcx> for DummyMachine {
 throw_machine_stop_str!("can't access mutable globals in ConstProp");
 }
 
-Ok(())
+interp_ok(())
 }
 
 fnfind_mir_or_eval_fn(
@@ -127,7 +128,7 @@ impl<'tcx> interpret::Machine<'tcx> for DummyMachine {
 right:&interpret::ImmTy<'tcx,Self::Provenance>,
 ) -> interpret::InterpResult<'tcx,ImmTy<'tcx,Self::Provenance>>{
 use rustc_middle::mir::BinOp::*;
-Ok(match bin_op {
+interp_ok(match bin_op {
 Eq | Ne | Lt | Le | Gt | Ge => {
 // Types can differ, e.g. fn ptrs with different `for`.
 assert_eq!(left.layout.abi, right.layout.abi);

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

@@ -20,7 +20,7 @@ use crate::const_eval::CheckAlignment;
 usecrate::interpret::{
 CtfeValidationMode,GlobalId,Immediate,InternKind,InternResult,InterpCx,InterpError,
 InterpResult,MPlaceTy,MemoryKind,OpTy,RefTracking,StackPopCleanup, create_static_alloc,
- eval_nullary_intrinsic, intern_const_alloc_recursive, throw_exhaust,
+ eval_nullary_intrinsic, intern_const_alloc_recursive,interp_ok,throw_exhaust,
 };
 usecrate::{CTRL_C_RECEIVED, errors};
 
@@ -98,19 +98,19 @@ fn eval_body_using_ecx<'tcx, R: InterpretationResult<'tcx>>(
 returnErr(ecx
 .tcx
 .dcx()
-.emit_err(errors::DanglingPtrInFinal{span: ecx.tcx.span,kind: intern_kind })
-.into());
+.emit_err(errors::DanglingPtrInFinal{span: ecx.tcx.span,kind: intern_kind }))
+.into();
 }
 Err(InternResult::FoundBadMutablePointer) => {
 returnErr(ecx
 .tcx
 .dcx()
-.emit_err(errors::MutablePtrInFinal{span: ecx.tcx.span,kind: intern_kind })
-.into());
+.emit_err(errors::MutablePtrInFinal{span: ecx.tcx.span,kind: intern_kind }))
+.into();
 }
 }
 
-Ok(R::make_result(ret, ecx))
+interp_ok(R::make_result(ret, ecx))
 }
 
 /// The `InterpCx` is only meant to be used to do field and index projections into constants for
@@ -147,7 +147,8 @@ pub fn mk_eval_cx_for_const_val<'tcx>(
 ty:Ty<'tcx>,
 ) -> Option<(CompileTimeInterpCx<'tcx>,OpTy<'tcx>)>{
 let ecx = mk_eval_cx_to_read_const_val(tcx.tcx, tcx.span, param_env,CanAccessMutGlobal::No);
-let op = ecx.const_val_to_op(val, ty,None).ok()?;
+// FIXME: is it a problem to discard the error here?
+let op = ecx.const_val_to_op(val, ty,None).discard_err()?;
 Some((ecx, op))
 }
 
@@ -185,12 +186,16 @@ pub(super) fn op_to_const<'tcx>(
  _ => false,
 };
 let immediate = if force_as_immediate {
-match ecx.read_immediate(op){
+match ecx.read_immediate(op).report_err(){
 Ok(imm) => Right(imm),
-Err(err)if !for_diagnostics => {
-panic!("normalization works on validated constants: {err:?}")
+Err(err) => {
+if for_diagnostics {
+// This discard the error, but for diagnostics that's okay.
+ op.as_mplace_or_imm()
+}else{
+panic!("normalization works on validated constants: {err:?}")
+}
 }
- _ => op.as_mplace_or_imm(),
 }
 }else{
  op.as_mplace_or_imm()
@@ -283,17 +288,19 @@ pub fn eval_to_const_value_raw_provider<'tcx>(
 let ty::FnDef(_, args) = ty.kind()else{
 bug!("intrinsic with type {:?}", ty);
 };
-returneval_nullary_intrinsic(tcx, key.param_env, def_id, args).map_err(|error| {
-let span = tcx.def_span(def_id);
-
-super::report(
- tcx,
- error.into_kind(),
- span,
- || (span,vec![]),
- |span, _| errors::NullaryIntrinsicError{ span },
-)
-});
+returneval_nullary_intrinsic(tcx, key.param_env, def_id, args).report_err().map_err(
+ |error| {
+let span = tcx.def_span(def_id);
+
+super::report(
+ tcx,
+ error.into_kind(),
+ span,
+ || (span,vec![]),
+ |span, _| errors::NullaryIntrinsicError{ span },
+)
+},
+);
 }
 
  tcx.eval_to_allocation_raw(key).map(|val| turn_into_const_value(tcx, val, key))
@@ -376,6 +383,7 @@ fn eval_in_interpreter<'tcx, R: InterpretationResult<'tcx>>(
 );
 let res = ecx.load_mir(cid.instance.def, cid.promoted);
  res.and_then(|body| eval_body_using_ecx(&mut ecx, cid, body))
+.report_err()
 .map_err(|error| report_eval_error(&ecx, cid, error))
 }
 
@@ -400,6 +408,7 @@ fn const_validate_mplace<'tcx>(
 }
 };
  ecx.const_validate_operand(&mplace.into(), path,&mut ref_tracking, mode)
+.report_err()
 // Instead of just reporting the `InterpError` via the usual machinery, we give a more targeted
 // error about the validation failure.
 .map_err(|error| report_validation_error(&ecx, cid, error, alloc_id))?;

compiler/rustc_const_eval/src/const_eval/machine.rs

@@ -24,8 +24,8 @@ use crate::fluent_generated as fluent;
 usecrate::interpret::{
 self,AllocId,AllocRange,ConstAllocation,CtfeProvenance,FnArg,Frame,GlobalAlloc,ImmTy,
 InterpCx,InterpResult,MPlaceTy,OpTy,Pointer,PointerArithmetic,RangeSet,Scalar,
-StackPopCleanup, compile_time_machine,err_ub, throw_exhaust, throw_inval,throw_ub_custom,
- throw_unsup, throw_unsup_format,
+StackPopCleanup, compile_time_machine,interp_ok, throw_exhaust, throw_inval,throw_ub,
+throw_ub_custom,throw_unsup, throw_unsup_format,
 };
 
 /// When hitting this many interpreted terminators we emit a deny by default lint
@@ -247,7 +247,7 @@ impl<'tcx> CompileTimeInterpCx<'tcx> {
 let msg = Symbol::intern(self.read_str(&msg_place)?);
 let span = self.find_closest_untracked_caller_location();
 let(file, line, col) = self.location_triple_for_span(span);
-returnErr(ConstEvalErrKind::Panic{ msg, file, line, col }.into());
+returnErr(ConstEvalErrKind::Panic{ msg, file, line, col }).into();
 }elseifself.tcx.is_lang_item(def_id,LangItem::PanicFmt){
 // For panic_fmt, call const_panic_fmt instead.
 let const_def_id = self.tcx.require_lang_item(LangItem::ConstPanicFmt,None);
@@ -259,16 +259,16 @@ impl<'tcx> CompileTimeInterpCx<'tcx> {
 self.cur_span(),
 );
 
-returnOk(Some(new_instance));
+returninterp_ok(Some(new_instance));
 }elseifself.tcx.is_lang_item(def_id,LangItem::AlignOffset){
 let args = self.copy_fn_args(args);
 // For align_offset, we replace the function call if the pointer has no address.
 matchself.align_offset(instance,&args, dest, ret)? {
-ControlFlow::Continue(()) => returnOk(Some(instance)),
-ControlFlow::Break(()) => returnOk(None),
+ControlFlow::Continue(()) => returninterp_ok(Some(instance)),
+ControlFlow::Break(()) => returninterp_ok(None),
 }
 }
-Ok(Some(instance))
+interp_ok(Some(instance))
 }
 
 /// `align_offset(ptr, target_align)` needs special handling in const eval, because the pointer
@@ -323,25 +323,25 @@ impl<'tcx> CompileTimeInterpCx<'tcx> {
  dest,
 StackPopCleanup::Goto{ ret,unwind: mir::UnwindAction::Unreachable},
 )?;
-Ok(ControlFlow::Break(()))
+interp_ok(ControlFlow::Break(()))
 }else{
 // Not alignable in const, return `usize::MAX`.
 let usize_max = Scalar::from_target_usize(self.target_usize_max(),self);
 self.write_scalar(usize_max, dest)?;
 self.return_to_block(ret)?;
-Ok(ControlFlow::Break(()))
+interp_ok(ControlFlow::Break(()))
 }
 }
 Err(_addr) => {
 // The pointer has an address, continue with function call.
-Ok(ControlFlow::Continue(()))
+interp_ok(ControlFlow::Continue(()))
 }
 }
 }
 
 /// See documentation on the `ptr_guaranteed_cmp` intrinsic.
 fnguaranteed_cmp(&mutself,a:Scalar,b:Scalar) -> InterpResult<'tcx,u8>{
-Ok(match(a, b){
+interp_ok(match(a, b){
 // Comparisons between integers are always known.
 (Scalar::Int{ .. },Scalar::Int{ .. }) => {
 if a == b {
@@ -403,8 +403,8 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
 instance: ty::InstanceKind<'tcx>,
 ) -> InterpResult<'tcx,&'tcx mir::Body<'tcx>>{
 match instance {
- ty::InstanceKind::Item(def) => Ok(ecx.tcx.mir_for_ctfe(def)),
- _ => Ok(ecx.tcx.instance_mir(instance)),
+ ty::InstanceKind::Item(def) => interp_ok(ecx.tcx.mir_for_ctfe(def)),
+ _ => interp_ok(ecx.tcx.instance_mir(instance)),
 }
 }
 
@@ -422,7 +422,7 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
 // Replace some functions.
 letSome(instance) = ecx.hook_special_const_fn(orig_instance, args, dest, ret)? else{
 // Call has already been handled.
-returnOk(None);
+returninterp_ok(None);
 };
 
 // Only check non-glue functions
@@ -444,14 +444,14 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
 // This is a const fn. Call it.
 // In case of replacement, we return the *original* instance to make backtraces work out
 // (and we hope this does not confuse the FnAbi checks too much).
-Ok(Some((ecx.load_mir(instance.def,None)?, orig_instance)))
+interp_ok(Some((ecx.load_mir(instance.def,None)?, orig_instance)))
 }
 
 fnpanic_nounwind(ecx:&mutInterpCx<'tcx,Self>,msg:&str) -> InterpResult<'tcx>{
 let msg = Symbol::intern(msg);
 let span = ecx.find_closest_untracked_caller_location();
 let(file, line, col) = ecx.location_triple_for_span(span);
-Err(ConstEvalErrKind::Panic{ msg, file, line, col }.into())
+Err(ConstEvalErrKind::Panic{ msg, file, line, col }).into()
 }
 
 fncall_intrinsic(
@@ -464,7 +464,7 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
 ) -> InterpResult<'tcx,Option<ty::Instance<'tcx>>>{
 // Shared intrinsics.
 if ecx.eval_intrinsic(instance, args, dest, target)? {
-returnOk(None);
+returninterp_ok(None);
 }
 let intrinsic_name = ecx.tcx.item_name(instance.def_id());
 
@@ -541,7 +541,7 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
 "intrinsic `{intrinsic_name}` is not supported at compile-time"
 );
 }
-returnOk(Some(ty::Instance{
+returninterp_ok(Some(ty::Instance{
 def: ty::InstanceKind::Item(instance.def_id()),
 args: instance.args,
 }));
@@ -550,7 +550,7 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
 
 // Intrinsic is done, jump to next block.
  ecx.return_to_block(target)?;
-Ok(None)
+interp_ok(None)
 }
 
 fnassert_panic(
@@ -581,7 +581,7 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
 }
 }
 };
-Err(ConstEvalErrKind::AssertFailure(err).into())
+Err(ConstEvalErrKind::AssertFailure(err)).into()
 }
 
 fnbinary_ptr_op(
@@ -652,7 +652,7 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
 }
 }
 
-Ok(())
+interp_ok(())
 }
 
 #[inline(always)]
@@ -670,7 +670,7 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
 if !ecx.recursion_limit.value_within_limit(ecx.stack().len() + 1){
 throw_exhaust!(StackFrameLimitReached)
 }else{
-Ok(frame)
+interp_ok(frame)
 }
 }
 
@@ -700,22 +700,22 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
 if is_write {
 // Write access. These are never allowed, but we give a targeted error message.
 match alloc.mutability{
-Mutability::Not => Err(err_ub!(WriteToReadOnly(alloc_id)).into()),
-Mutability::Mut => Err(ConstEvalErrKind::ModifiedGlobal.into()),
+Mutability::Not => throw_ub!(WriteToReadOnly(alloc_id)),
+Mutability::Mut => Err(ConstEvalErrKind::ModifiedGlobal).into(),
 }
 }else{
 // Read access. These are usually allowed, with some exceptions.
 if machine.can_access_mut_global == CanAccessMutGlobal::Yes{
 // Machine configuration allows us read from anything (e.g., `static` initializer).
-Ok(())
+interp_ok(())
 }elseif alloc.mutability == Mutability::Mut{
 // Machine configuration does not allow us to read statics (e.g., `const`
 // initializer).
-Err(ConstEvalErrKind::ConstAccessesMutGlobal.into())
+Err(ConstEvalErrKind::ConstAccessesMutGlobal).into()
 }else{
 // Immutable global, this read is fine.
 assert_eq!(alloc.mutability,Mutability::Not);
-Ok(())
+interp_ok(())
 }
 }
 }
@@ -748,9 +748,9 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
 // even when there is interior mutability.)
  place.map_provenance(CtfeProvenance::as_shared_ref)
 };
-Ok(ImmTy::from_immediate(new_place.to_ref(ecx), val.layout))
+interp_ok(ImmTy::from_immediate(new_place.to_ref(ecx), val.layout))
 }else{
-Ok(val.clone())
+interp_ok(val.clone())
 }
 }
 
@@ -763,20 +763,20 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
 ) -> InterpResult<'tcx>{
 if range.size == Size::ZERO{
 // Nothing to check.
-returnOk(());
+returninterp_ok(());
 }
 // Reject writes through immutable pointers.
 if immutable {
-returnErr(ConstEvalErrKind::WriteThroughImmutablePointer.into());
+returnErr(ConstEvalErrKind::WriteThroughImmutablePointer).into();
 }
 // Everything else is fine.
-Ok(())
+interp_ok(())
 }
 
 fnbefore_alloc_read(ecx:&InterpCx<'tcx,Self>,alloc_id:AllocId) -> InterpResult<'tcx>{
 // Check if this is the currently evaluated static.
 ifSome(alloc_id) == ecx.machine.static_root_ids.map(|(id, _)| id){
-returnErr(ConstEvalErrKind::RecursiveStatic.into());
+returnErr(ConstEvalErrKind::RecursiveStatic).into();
 }
 // If this is another static, make sure we fire off the query to detect cycles.
 // But only do that when checks for static recursion are enabled.
@@ -788,7 +788,7 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
  ecx.ctfe_query(|tcx| tcx.eval_static_initializer(def_id))?;
 }
 }
-Ok(())
+interp_ok(())
 }
 
 fncached_union_data_range<'e>(