Auto merge of #85546 - hyd-dev:unwind, r=RalfJung

const-eval: disallow unwinding across functions that `!fn_can_unwind()`

Following rust-lang/miri#1776 (comment), so r? `@RalfJung`

This PR turns `unwind` in `StackPopCleanup::Goto` into a new enum `StackPopUnwind`, with a `NotAllowed` variant to indicate that unwinding is not allowed. This variant is chosen based on `rustc_middle::ty::layout::fn_can_unwind()` in `eval_fn_call()` when pushing the frame. A check is added in `unwind_to_block()` to report UB if unwinding happens across a `StackPopUnwind::NotAllowed` frame.

Tested with Miri `HEAD` with [minor changes](https://github.com/rust-lang/miri/compare/HEAD..9cf3c7f0d86325a586fbcbf2acdc9232b861f1d8) and the rust-lang/miri#1776 branch with [these changes](https://github.com/rust-lang/miri/compare/d866c1c52f48bf562720383455b75c257bb1ad92..626638fbfe2fff34648dda29a34d59db498a6e52).

compiler/rustc_middle/src/ty/layout.rs

@@ -2579,7 +2579,7 @@ where
     fn adjust_for_abi(&mut self, cx: &C, abi: SpecAbi);
 }
 
-fn fn_can_unwind(
+pub fn fn_can_unwind(
     panic_strategy: PanicStrategy,
     codegen_fn_attr_flags: CodegenFnAttrFlags,
     call_conv: Conv,
@@ -2641,6 +2641,43 @@ fn fn_can_unwind(
     }
 }
 
+pub fn conv_from_spec_abi(tcx: TyCtxt<'_>, abi: SpecAbi) -> Conv {
+    use rustc_target::spec::abi::Abi::*;
+    match tcx.sess.target.adjust_abi(abi) {
+        RustIntrinsic | PlatformIntrinsic | Rust | RustCall => Conv::Rust,
+
+        // It's the ABI's job to select this, not ours.
+        System { .. } => bug!("system abi should be selected elsewhere"),
+        EfiApi => bug!("eficall abi should be selected elsewhere"),
+
+        Stdcall { .. } => Conv::X86Stdcall,
+        Fastcall => Conv::X86Fastcall,
+        Vectorcall => Conv::X86VectorCall,
+        Thiscall { .. } => Conv::X86ThisCall,
+        C { .. } => Conv::C,
+        Unadjusted => Conv::C,
+        Win64 => Conv::X86_64Win64,
+        SysV64 => Conv::X86_64SysV,
+        Aapcs => Conv::ArmAapcs,
+        CCmseNonSecureCall => Conv::CCmseNonSecureCall,
+        PtxKernel => Conv::PtxKernel,
+        Msp430Interrupt => Conv::Msp430Intr,
+        X86Interrupt => Conv::X86Intr,
+        AmdGpuKernel => Conv::AmdGpuKernel,
+        AvrInterrupt => Conv::AvrInterrupt,
+        AvrNonBlockingInterrupt => Conv::AvrNonBlockingInterrupt,
+        Wasm => Conv::C,
+
+        // These API constants ought to be more specific...
+        Cdecl => Conv::C,
+    }
+}
+
+pub fn fn_ptr_codegen_fn_attr_flags() -> CodegenFnAttrFlags {
+    // Assume that fn pointers may always unwind
+    CodegenFnAttrFlags::UNWIND
+}
+
 impl<'tcx, C> FnAbiExt<'tcx, C> for call::FnAbi<'tcx, Ty<'tcx>>
 where
     C: LayoutOf<Ty = Ty<'tcx>, TyAndLayout = TyAndLayout<'tcx>>
@@ -2650,10 +2687,7 @@ where
         + HasParamEnv<'tcx>,
 {
     fn of_fn_ptr(cx: &C, sig: ty::PolyFnSig<'tcx>, extra_args: &[Ty<'tcx>]) -> Self {
-        // Assume that fn pointers may always unwind
-        let codegen_fn_attr_flags = CodegenFnAttrFlags::UNWIND;
-
-        call::FnAbi::new_internal(cx, sig, extra_args, None, codegen_fn_attr_flags, false)
+        call::FnAbi::new_internal(cx, sig, extra_args, None, fn_ptr_codegen_fn_attr_flags(), false)
     }
 
     fn of_instance(cx: &C, instance: ty::Instance<'tcx>, extra_args: &[Ty<'tcx>]) -> Self {
@@ -2689,35 +2723,7 @@ where
 
         let sig = cx.tcx().normalize_erasing_late_bound_regions(ty::ParamEnv::reveal_all(), sig);
 
-        use rustc_target::spec::abi::Abi::*;
-        let conv = match cx.tcx().sess.target.adjust_abi(sig.abi) {
-            RustIntrinsic | PlatformIntrinsic | Rust | RustCall => Conv::Rust,
-
-            // It's the ABI's job to select this, not ours.
-            System { .. } => bug!("system abi should be selected elsewhere"),
-            EfiApi => bug!("eficall abi should be selected elsewhere"),
-
-            Stdcall { .. } => Conv::X86Stdcall,
-            Fastcall => Conv::X86Fastcall,
-            Vectorcall => Conv::X86VectorCall,
-            Thiscall { .. } => Conv::X86ThisCall,
-            C { .. } => Conv::C,
-            Unadjusted => Conv::C,
-            Win64 => Conv::X86_64Win64,
-            SysV64 => Conv::X86_64SysV,
-            Aapcs => Conv::ArmAapcs,
-            CCmseNonSecureCall => Conv::CCmseNonSecureCall,
-            PtxKernel => Conv::PtxKernel,
-            Msp430Interrupt => Conv::Msp430Intr,
-            X86Interrupt => Conv::X86Intr,
-            AmdGpuKernel => Conv::AmdGpuKernel,
-            AvrInterrupt => Conv::AvrInterrupt,
-            AvrNonBlockingInterrupt => Conv::AvrNonBlockingInterrupt,
-            Wasm => Conv::C,
-
-            // These API constants ought to be more specific...
-            Cdecl => Conv::C,
-        };
+        let conv = conv_from_spec_abi(cx.tcx(), sig.abi);
 
         let mut inputs = sig.inputs();
         let extra_args = if sig.abi == RustCall {
@@ -2753,6 +2759,7 @@ where
             target.os == "linux" && target.arch == "sparc64" && target_env_gnu_like;
         let linux_powerpc_gnu_like =
             target.os == "linux" && target.arch == "powerpc" && target_env_gnu_like;
+        use SpecAbi::*;
         let rust_abi = matches!(sig.abi, RustIntrinsic | PlatformIntrinsic | Rust | RustCall);
 
         // Handle safe Rust thin and fat pointers.

compiler/rustc_mir/src/const_eval/machine.rs

@@ -17,7 +17,7 @@ use rustc_target::spec::abi::Abi;
 
 use crate::interpret::{
     self, compile_time_machine, AllocId, Allocation, Frame, ImmTy, InterpCx, InterpResult, Memory,
-    OpTy, PlaceTy, Pointer, Scalar,
+    OpTy, PlaceTy, Pointer, Scalar, StackPopUnwind,
 };
 
 use super::error::*;
@@ -223,7 +223,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
         _abi: Abi,
         args: &[OpTy<'tcx>],
         _ret: Option<(&PlaceTy<'tcx>, mir::BasicBlock)>,
-        _unwind: Option<mir::BasicBlock>, // unwinding is not supported in consts
+        _unwind: StackPopUnwind, // unwinding is not supported in consts
     ) -> InterpResult<'tcx, Option<&'mir mir::Body<'tcx>>> {
         debug!("find_mir_or_eval_fn: {:?}", instance);
 
@@ -263,7 +263,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
         instance: ty::Instance<'tcx>,
         args: &[OpTy<'tcx>],
         ret: Option<(&PlaceTy<'tcx>, mir::BasicBlock)>,
-        _unwind: Option<mir::BasicBlock>,
+        _unwind: StackPopUnwind,
     ) -> InterpResult<'tcx> {
         // Shared intrinsics.
         if ecx.emulate_intrinsic(instance, args, ret)? {

compiler/rustc_mir/src/interpret/eval_context.rs

@@ -134,14 +134,25 @@ pub struct FrameInfo<'tcx> {
     pub lint_root: Option<hir::HirId>,
 }
 
-#[derive(Clone, Eq, PartialEq, Debug, HashStable)] // Miri debug-prints these
+/// Unwind information.
+#[derive(Clone, Copy, Eq, PartialEq, Debug, HashStable)]
+pub enum StackPopUnwind {
+    /// The cleanup block.
+    Cleanup(mir::BasicBlock),
+    /// No cleanup needs to be done.
+    Skip,
+    /// Unwinding is not allowed (UB).
+    NotAllowed,
+}
+
+#[derive(Clone, Copy, Eq, PartialEq, Debug, HashStable)] // Miri debug-prints these
 pub enum StackPopCleanup {
     /// Jump to the next block in the caller, or cause UB if None (that's a function
     /// that may never return). Also store layout of return place so
     /// we can validate it at that layout.
     /// `ret` stores the block we jump to on a normal return, while `unwind`
     /// stores the block used for cleanup during unwinding.
-    Goto { ret: Option<mir::BasicBlock>, unwind: Option<mir::BasicBlock> },
+    Goto { ret: Option<mir::BasicBlock>, unwind: StackPopUnwind },
     /// Just do nothing: Used by Main and for the `box_alloc` hook in miri.
     /// `cleanup` says whether locals are deallocated. Static computation
     /// wants them leaked to intern what they need (and just throw away
@@ -746,13 +757,20 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     /// *Unwind* to the given `target` basic block.
     /// Do *not* use for returning! Use `return_to_block` instead.
     ///
-    /// If `target` is `None`, that indicates the function does not need cleanup during
-    /// unwinding, and we will just keep propagating that upwards.
-    pub fn unwind_to_block(&mut self, target: Option<mir::BasicBlock>) {
+    /// If `target` is `StackPopUnwind::Skip`, that indicates the function does not need cleanup
+    /// during unwinding, and we will just keep propagating that upwards.
+    ///
+    /// If `target` is `StackPopUnwind::NotAllowed`, that indicates the function does not allow
+    /// unwinding, and doing so is UB.
+    pub fn unwind_to_block(&mut self, target: StackPopUnwind) -> InterpResult<'tcx> {
         self.frame_mut().loc = match target {
-            Some(block) => Ok(mir::Location { block, statement_index: 0 }),
-            None => Err(self.frame_mut().body.span),
+            StackPopUnwind::Cleanup(block) => Ok(mir::Location { block, statement_index: 0 }),
+            StackPopUnwind::Skip => Err(self.frame_mut().body.span),
+            StackPopUnwind::NotAllowed => {
+                throw_ub_format!("unwinding past a stack frame that does not allow unwinding")
+            }
         };
+        Ok(())
     }
 
     /// Pops the current frame from the stack, deallocating the
@@ -801,21 +819,20 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             }
         }
 
+        let return_to_block = frame.return_to_block;
+
         // Now where do we jump next?
 
         // Usually we want to clean up (deallocate locals), but in a few rare cases we don't.
         // In that case, we return early. We also avoid validation in that case,
         // because this is CTFE and the final value will be thoroughly validated anyway.
-        let (cleanup, next_block) = match frame.return_to_block {
-            StackPopCleanup::Goto { ret, unwind } => {
-                (true, Some(if unwinding { unwind } else { ret }))
-            }
-            StackPopCleanup::None { cleanup, .. } => (cleanup, None),
+        let cleanup = match return_to_block {
+            StackPopCleanup::Goto { .. } => true,
+            StackPopCleanup::None { cleanup, .. } => cleanup,
         };
 
         if !cleanup {
             assert!(self.stack().is_empty(), "only the topmost frame should ever be leaked");
-            assert!(next_block.is_none(), "tried to skip cleanup when we have a next block!");
             assert!(!unwinding, "tried to skip cleanup during unwinding");
             // Leak the locals, skip validation, skip machine hook.
             return Ok(());
@@ -834,16 +851,20 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         // Normal return, figure out where to jump.
         if unwinding {
             // Follow the unwind edge.
-            let unwind = next_block.expect("Encountered StackPopCleanup::None when unwinding!");
-            self.unwind_to_block(unwind);
+            let unwind = match return_to_block {
+                StackPopCleanup::Goto { unwind, .. } => unwind,
+                StackPopCleanup::None { .. } => {
+                    panic!("Encountered StackPopCleanup::None when unwinding!")
+                }
+            };
+            self.unwind_to_block(unwind)
         } else {
             // Follow the normal return edge.
-            if let Some(ret) = next_block {
-                self.return_to_block(ret)?;
+            match return_to_block {
+                StackPopCleanup::Goto { ret, .. } => self.return_to_block(ret),
+                StackPopCleanup::None { .. } => Ok(()),
             }
         }
-
-        Ok(())
     }
 
     /// Mark a storage as live, killing the previous content.

compiler/rustc_mir/src/interpret/machine.rs

@@ -14,7 +14,7 @@ use rustc_target::spec::abi::Abi;
 
 use super::{
     AllocId, Allocation, CheckInAllocMsg, Frame, ImmTy, InterpCx, InterpResult, LocalValue,
-    MemPlace, Memory, MemoryKind, OpTy, Operand, PlaceTy, Pointer, Scalar,
+    MemPlace, Memory, MemoryKind, OpTy, Operand, PlaceTy, Pointer, Scalar, StackPopUnwind,
 };
 
 /// Data returned by Machine::stack_pop,
@@ -163,7 +163,7 @@ pub trait Machine<'mir, 'tcx>: Sized {
         abi: Abi,
         args: &[OpTy<'tcx, Self::PointerTag>],
         ret: Option<(&PlaceTy<'tcx, Self::PointerTag>, mir::BasicBlock)>,
-        unwind: Option<mir::BasicBlock>,
+        unwind: StackPopUnwind,
     ) -> InterpResult<'tcx, Option<&'mir mir::Body<'tcx>>>;
 
     /// Execute `fn_val`.  It is the hook's responsibility to advance the instruction
@@ -174,7 +174,7 @@ pub trait Machine<'mir, 'tcx>: Sized {
         abi: Abi,
         args: &[OpTy<'tcx, Self::PointerTag>],
         ret: Option<(&PlaceTy<'tcx, Self::PointerTag>, mir::BasicBlock)>,
-        unwind: Option<mir::BasicBlock>,
+        unwind: StackPopUnwind,
     ) -> InterpResult<'tcx>;
 
     /// Directly process an intrinsic without pushing a stack frame. It is the hook's
@@ -184,7 +184,7 @@ pub trait Machine<'mir, 'tcx>: Sized {
         instance: ty::Instance<'tcx>,
         args: &[OpTy<'tcx, Self::PointerTag>],
         ret: Option<(&PlaceTy<'tcx, Self::PointerTag>, mir::BasicBlock)>,
-        unwind: Option<mir::BasicBlock>,
+        unwind: StackPopUnwind,
     ) -> InterpResult<'tcx>;
 
     /// Called to evaluate `Assert` MIR terminators that trigger a panic.
@@ -456,7 +456,7 @@ pub macro compile_time_machine(<$mir: lifetime, $tcx: lifetime>) {
         _abi: Abi,
         _args: &[OpTy<$tcx>],
         _ret: Option<(&PlaceTy<$tcx>, mir::BasicBlock)>,
-        _unwind: Option<mir::BasicBlock>,
+        _unwind: StackPopUnwind,
     ) -> InterpResult<$tcx> {
         match fn_val {}
     }

compiler/rustc_mir/src/interpret/mod.rs

@@ -18,7 +18,9 @@ mod visitor;
 
 pub use rustc_middle::mir::interpret::*; // have all the `interpret` symbols in one place: here
 
-pub use self::eval_context::{Frame, FrameInfo, InterpCx, LocalState, LocalValue, StackPopCleanup};
+pub use self::eval_context::{
+    Frame, FrameInfo, InterpCx, LocalState, LocalValue, StackPopCleanup, StackPopUnwind,
+};
 pub use self::intern::{intern_const_alloc_recursive, InternKind};
 pub use self::machine::{compile_time_machine, AllocMap, Machine, MayLeak, StackPopJump};
 pub use self::memory::{AllocCheck, AllocRef, AllocRefMut, FnVal, Memory, MemoryKind};