Auto merge of rust-lang#92911 - nbdd0121:unwind, r=Amanieu

Guard against unwinding in cleanup code

Currently the only safe guard we have against double unwind is the panic count (which is local to Rust). When double unwinds indeed happen (e.g. C++ exception + Rust panic, or two C++ exceptions), then the second unwind actually goes through and the first unwind is leaked. This can cause UB. cc rust-lang/project-ffi-unwind#6

E.g. given the following C++ code:
```c++
extern "C" void foo() {
    throw "A";
}

extern "C" void execute(void (*fn)()) {
    try {
        fn();
    } catch(...) {
    }
}
```

This program is well-defined to terminate:
```c++
struct dtor {
    ~dtor() noexcept(false) {
        foo();
    }
};

void a() {
    dtor a;
    dtor b;
}

int main() {
    execute(a);
    return 0;
}
```

But this Rust code doesn't catch the double unwind:
```rust
extern "C-unwind" {
    fn foo();
    fn execute(f: unsafe extern "C-unwind" fn());
}

struct Dtor;

impl Drop for Dtor {
    fn drop(&mut self) {
        unsafe { foo(); }
    }
}

extern "C-unwind" fn a() {
    let _a = Dtor;
    let _b = Dtor;
}

fn main() {
    unsafe { execute(a) };
}
```

To address this issue, this PR adds an unwind edge to an abort block, so that the Rust example aborts. This is similar to how clang guards against double unwind (except clang calls terminate per C++ spec and we abort).

The cost should be very small; it's an additional trap instruction (well, two for now, since we use TrapUnreachable, but that's a different issue) for each function with landing pads; if LLVM gains support to encode "abort/terminate" info directly in LSDA like GCC does, then it'll be free. It's an additional basic block though so compile time may be worse, so I'd like a perf run.

r? `@ghost`
`@rustbot` label: F-c_unwind

compiler/rustc_codegen_ssa/src/mir/block.rs

@@ -135,21 +135,38 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
         // If there is a cleanup block and the function we're calling can unwind, then
         // do an invoke, otherwise do a call.
         let fn_ty = bx.fn_decl_backend_type(&fn_abi);
-        if let Some(cleanup) = cleanup.filter(|_| fn_abi.can_unwind) {
+
+        let unwind_block = if let Some(cleanup) = cleanup.filter(|_| fn_abi.can_unwind) {
+            Some(self.llblock(fx, cleanup))
+        } else if fx.mir[self.bb].is_cleanup
+            && fn_abi.can_unwind
+            && !base::wants_msvc_seh(fx.cx.tcx().sess)
+        {
+            // Exception must not propagate out of the execution of a cleanup (doing so
+            // can cause undefined behaviour). We insert a double unwind guard for
+            // functions that can potentially unwind to protect against this.
+            //
+            // This is not necessary for SEH which does not use successive unwinding
+            // like Itanium EH. EH frames in SEH are different from normal function
+            // frames and SEH will abort automatically if an exception tries to
+            // propagate out from cleanup.
+            Some(fx.double_unwind_guard())
+        } else {
+            None
+        };
+
+        if let Some(unwind_block) = unwind_block {
             let ret_llbb = if let Some((_, target)) = destination {
                 fx.llbb(target)
             } else {
                 fx.unreachable_block()
             };
-            let invokeret = bx.invoke(
-                fn_ty,
-                fn_ptr,
-                &llargs,
-                ret_llbb,
-                self.llblock(fx, cleanup),
-                self.funclet(fx),
-            );
+            let invokeret =
+                bx.invoke(fn_ty, fn_ptr, &llargs, ret_llbb, unwind_block, self.funclet(fx));
             bx.apply_attrs_callsite(&fn_abi, invokeret);
+            if fx.mir[self.bb].is_cleanup {
+                bx.apply_attrs_to_cleanup_callsite(invokeret);
+            }
 
             if let Some((ret_dest, target)) = destination {
                 let mut ret_bx = fx.build_block(target);
@@ -486,17 +503,14 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
         let span = terminator.source_info.span;
         self.set_debug_loc(&mut bx, terminator.source_info);
 
-        // Get the location information.
-        let location = self.get_caller_location(&mut bx, terminator.source_info).immediate();
-
         // Obtain the panic entry point.
         let def_id = common::langcall(bx.tcx(), Some(span), "", LangItem::PanicNoUnwind);
         let instance = ty::Instance::mono(bx.tcx(), def_id);
         let fn_abi = bx.fn_abi_of_instance(instance, ty::List::empty());
         let llfn = bx.get_fn_addr(instance);
 
         // Codegen the actual panic invoke/call.
-        helper.do_call(self, &mut bx, fn_abi, llfn, &[location], None, None);
+        helper.do_call(self, &mut bx, fn_abi, llfn, &[], None, None);
     }
 
     /// Returns `true` if this is indeed a panic intrinsic and codegen is done.
@@ -1398,6 +1412,35 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
         })
     }
 
+    fn double_unwind_guard(&mut self) -> Bx::BasicBlock {
+        self.double_unwind_guard.unwrap_or_else(|| {
+            assert!(!base::wants_msvc_seh(self.cx.sess()));
+
+            let mut bx = self.new_block("abort");
+            self.set_debug_loc(&mut bx, mir::SourceInfo::outermost(self.mir.span));
+
+            let llpersonality = self.cx.eh_personality();
+            let llretty = self.landing_pad_type();
+            bx.cleanup_landing_pad(llretty, llpersonality);
+
+            let def_id = common::langcall(bx.tcx(), None, "", LangItem::PanicNoUnwind);
+            let instance = ty::Instance::mono(bx.tcx(), def_id);
+            let fn_abi = bx.fn_abi_of_instance(instance, ty::List::empty());
+            let fn_ptr = bx.get_fn_addr(instance);
+            let fn_ty = bx.fn_decl_backend_type(&fn_abi);
+
+            let llret = bx.call(fn_ty, fn_ptr, &[], None);
+            bx.apply_attrs_callsite(&fn_abi, llret);
+            bx.apply_attrs_to_cleanup_callsite(llret);
+
+            bx.unreachable();
+            let llbb = bx.llbb();
+
+            self.double_unwind_guard = Some(llbb);
+            llbb
+        })
+    }
+
     // FIXME(eddyb) replace with `build_sibling_block`/`append_sibling_block`
     // (which requires having a `Bx` already, and not all callers do).
     fn new_block(&self, name: &str) -> Bx {

compiler/rustc_codegen_ssa/src/mir/mod.rs

@@ -62,6 +62,9 @@ pub struct FunctionCx<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> {
     /// Cached unreachable block
     unreachable_block: Option<Bx::BasicBlock>,
 
+    /// Cached double unwind guarding block
+    double_unwind_guard: Option<Bx::BasicBlock>,
+
     /// The location where each MIR arg/var/tmp/ret is stored. This is
     /// usually an `PlaceRef` representing an alloca, but not always:
     /// sometimes we can skip the alloca and just store the value
@@ -169,6 +172,7 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
         personality_slot: None,
         cached_llbbs,
         unreachable_block: None,
+        double_unwind_guard: None,
         cleanup_kinds,
         landing_pads: IndexVec::from_elem(None, mir.basic_blocks()),
         funclets: IndexVec::from_fn_n(|_| None, mir.basic_blocks().len()),

library/core/src/panicking.rs

@@ -87,8 +87,7 @@ fn panic_bounds_check(index: usize, len: usize) -> ! {
 
 #[cfg(not(bootstrap))]
 #[cold]
-#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))]
-#[track_caller]
+#[inline(never)]
 #[lang = "panic_no_unwind"] // needed by codegen for panic in nounwind function
 fn panic_no_unwind() -> ! {
     if cfg!(feature = "panic_immediate_abort") {

src/test/codegen/drop.rs

@@ -23,17 +23,7 @@ pub fn droppy() {
 // FIXME(eddyb) the `void @` forces a match on the instruction, instead of the
 // comment, that's `; call core::ptr::drop_in_place::<drop::SomeUniqueName>`
 // for the `v0` mangling, should switch to matching on that once `legacy` is gone.
-// CHECK-NOT: invoke void @{{.*}}drop_in_place{{.*}}SomeUniqueName
-// CHECK: call void @{{.*}}drop_in_place{{.*}}SomeUniqueName
-// CHECK: call void @{{.*}}drop_in_place{{.*}}SomeUniqueName
-// CHECK-NOT: call void @{{.*}}drop_in_place{{.*}}SomeUniqueName
-// CHECK: invoke void @{{.*}}drop_in_place{{.*}}SomeUniqueName
-// CHECK-NOT: invoke void @{{.*}}drop_in_place{{.*}}SomeUniqueName
-// CHECK: call void @{{.*}}drop_in_place{{.*}}SomeUniqueName
-// CHECK-NOT: call void @{{.*}}drop_in_place{{.*}}SomeUniqueName
-// CHECK: invoke void @{{.*}}drop_in_place{{.*}}SomeUniqueName
-// CHECK-NOT: invoke void @{{.*}}drop_in_place{{.*}}SomeUniqueName
-// CHECK: call void @{{.*}}drop_in_place{{.*}}SomeUniqueName
+// CHECK-COUNT-6: {{(call|invoke) void @.*}}drop_in_place{{.*}}SomeUniqueName
 // CHECK-NOT: {{(call|invoke) void @.*}}drop_in_place{{.*}}SomeUniqueName
 // The next line checks for the } that ends the function definition
 // CHECK-LABEL: {{^[}]}}

src/test/codegen/unwind-landingpad-cold.rs

@@ -6,7 +6,7 @@
 // get the `cold` attribute.
 
 // CHECK-LABEL: @check_cold
-// CHECK: call void {{.+}}drop_in_place{{.+}} [[ATTRIBUTES:#[0-9]+]]
+// CHECK: {{(call|invoke) void .+}}drop_in_place{{.+}} [[ATTRIBUTES:#[0-9]+]]
 // CHECK: attributes [[ATTRIBUTES]] = { cold }
 #[no_mangle]
 pub fn check_cold(f: fn(), x: Box<u32>) {

src/test/run-make-fulldeps/foreign-double-unwind/Makefile

@@ -0,0 +1,10 @@
+-include ../tools.mk
+
+all: foo
+	$(call RUN,foo) | $(CGREP) -v unreachable
+
+foo: foo.rs $(call NATIVE_STATICLIB,foo)
+	$(RUSTC) $< -lfoo $(EXTRARSCXXFLAGS)
+
+$(TMPDIR)/libfoo.o: foo.cpp
+	$(call COMPILE_OBJ_CXX,$@,$<)

src/test/run-make-fulldeps/foreign-double-unwind/foo.cpp

@@ -0,0 +1,33 @@
+#include <cstdio>
+#include <exception>
+
+void println(const char* s) {
+    puts(s);
+    fflush(stdout);
+}
+
+struct outer_exception {};
+struct inner_exception {};
+
+extern "C" {
+    void throw_cxx_exception() {
+        if (std::uncaught_exception()) {
+            println("throwing inner C++ exception");
+            throw inner_exception();
+        } else {
+            println("throwing outer C++ exception");
+            throw outer_exception();
+        }
+    }
+
+    void cxx_catch_callback(void (*cb)()) {
+        try {
+            cb();
+            println("unreachable: callback returns");
+        } catch (outer_exception) {
+            println("unreachable: caught outer exception in catch (...)");
+        } catch (inner_exception) {
+            println("unreachable: caught inner exception in catch (...)");
+        }
+    }
+}

src/test/run-make-fulldeps/foreign-double-unwind/foo.rs

@@ -0,0 +1,26 @@
+// Tests that C++ double unwinding through Rust code will be properly guarded
+// against instead of exhibiting undefined behaviour.
+
+#![feature(c_unwind)]
+
+extern "C-unwind" {
+    fn throw_cxx_exception();
+    fn cxx_catch_callback(cb: extern "C-unwind" fn());
+}
+
+struct ThrowOnDrop;
+
+impl Drop for ThrowOnDrop {
+    fn drop(&mut self) {
+        unsafe { throw_cxx_exception() };
+    }
+}
+
+extern "C-unwind" fn test_double_unwind() {
+    let _a = ThrowOnDrop;
+    let _b = ThrowOnDrop;
+}
+
+fn main() {
+    unsafe { cxx_catch_callback(test_double_unwind) };
+}