Auto merge of #67711 - Amanieu:fix_unwind_leak, r=alexcrichton

Fix memory leak if C++ catches a Rust panic and discards it

If C++ catches a Rust panic using `catch (...)` and then chooses not to rethrow it, the `Box<dyn Any>` in the exception may be leaked. This PR fixes this by adding the necessary destructors to the exception object.

r? @Mark-Simulacrum

7 files changed, 139 insertions, 26 deletions

diff --git a/src/libpanic_unwind/emcc.rs b/src/libpanic_unwind/emcc.rs
index 9d3fe5254f8..9161d49959c 100644
--- a/src/libpanic_unwind/emcc.rs
+++ b/src/libpanic_unwind/emcc.rs
@@ -52,22 +52,49 @@ pub fn payload() -> *mut u8 {
     ptr::null_mut()
 }
 
+struct Exception {
+    // This needs to be an Option because the object's lifetime follows C++
+    // semantics: when catch_unwind moves the Box out of the exception it must
+    // still leave the exception object in a valid state because its destructor
+    // is still going to be called by __cxa_end_catch..
+    data: Option<Box<dyn Any + Send>>,
+}
+
 pub unsafe fn cleanup(ptr: *mut u8) -> Box<dyn Any + Send> {
     assert!(!ptr.is_null());
-    let adjusted_ptr = __cxa_begin_catch(ptr as *mut libc::c_void);
-    let ex = ptr::read(adjusted_ptr as *mut _);
+    let adjusted_ptr = __cxa_begin_catch(ptr as *mut libc::c_void) as *mut Exception;
+    let ex = (*adjusted_ptr).data.take();
     __cxa_end_catch();
-    ex
+    ex.unwrap()
 }
 
 pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
     let sz = mem::size_of_val(&data);
-    let exception = __cxa_allocate_exception(sz);
+    let exception = __cxa_allocate_exception(sz) as *mut Exception;
     if exception.is_null() {
         return uw::_URC_FATAL_PHASE1_ERROR as u32;
     }
-    ptr::write(exception as *mut _, data);
-    __cxa_throw(exception as *mut _, &EXCEPTION_TYPE_INFO, ptr::null_mut());
+    ptr::write(exception, Exception { data: Some(data) });
+    __cxa_throw(exception as *mut _, &EXCEPTION_TYPE_INFO, exception_cleanup);
+}
+
+// On WASM and ARM, the destructor returns the pointer to the object.
+cfg_if::cfg_if! {
+    if #[cfg(any(target_arch = "arm", target_arch = "wasm32"))] {
+        type DestructorRet = *mut libc::c_void;
+    } else {
+        type DestructorRet = ();
+    }
+}
+extern "C" fn exception_cleanup(ptr: *mut libc::c_void) -> DestructorRet {
+    unsafe {
+        if let Some(b) = (ptr as *mut Exception).read().data {
+            drop(b);
+            super::__rust_drop_panic();
+        }
+        #[cfg(any(target_arch = "arm", target_arch = "wasm32"))]
+        ptr
+    }
 }
 
 #[lang = "eh_personality"]
@@ -89,7 +116,7 @@ extern "C" {
     fn __cxa_throw(
         thrown_exception: *mut libc::c_void,
         tinfo: *const TypeInfo,
-        dest: *mut libc::c_void,
+        dest: extern "C" fn(*mut libc::c_void) -> DestructorRet,
     ) -> !;
     fn __gxx_personality_v0(
         version: c_int,
diff --git a/src/libpanic_unwind/gcc.rs b/src/libpanic_unwind/gcc.rs
index 6a48fa05f8d..6e04317d491 100644
--- a/src/libpanic_unwind/gcc.rs
+++ b/src/libpanic_unwind/gcc.rs
@@ -57,7 +57,7 @@ use unwind as uw;
 #[repr(C)]
 struct Exception {
     _uwe: uw::_Unwind_Exception,
-    cause: Option<Box<dyn Any + Send>>,
+    cause: Box<dyn Any + Send>,
 }
 
 pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
@@ -67,7 +67,7 @@ pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
             exception_cleanup,
             private: [0; uw::unwinder_private_data_size],
         },
-        cause: Some(data),
+        cause: data,
     });
     let exception_param = Box::into_raw(exception) as *mut uw::_Unwind_Exception;
     return uw::_Unwind_RaiseException(exception_param) as u32;
@@ -78,6 +78,7 @@ pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
     ) {
         unsafe {
             let _: Box<Exception> = Box::from_raw(exception as *mut Exception);
+            super::__rust_drop_panic();
         }
     }
 }
@@ -87,10 +88,8 @@ pub fn payload() -> *mut u8 {
 }
 
 pub unsafe fn cleanup(ptr: *mut u8) -> Box<dyn Any + Send> {
-    let my_ep = ptr as *mut Exception;
-    let cause = (*my_ep).cause.take();
-    uw::_Unwind_DeleteException(ptr as *mut _);
-    cause.unwrap()
+    let exception = Box::from_raw(ptr as *mut Exception);
+    exception.cause
 }
 
 // Rust's exception class identifier.  This is used by personality routines to
diff --git a/src/libpanic_unwind/lib.rs b/src/libpanic_unwind/lib.rs
index e721162edc0..6383ae39fb6 100644
--- a/src/libpanic_unwind/lib.rs
+++ b/src/libpanic_unwind/lib.rs
@@ -26,6 +26,7 @@
 #![feature(staged_api)]
 #![feature(std_internals)]
 #![feature(unwind_attributes)]
+#![feature(abi_thiscall)]
 #![panic_runtime]
 #![feature(panic_runtime)]
 
@@ -60,6 +61,12 @@ cfg_if::cfg_if! {
     }
 }
 
+extern "C" {
+    /// Handler in libstd called when a panic object is dropped outside of
+    /// `catch_unwind`.
+    fn __rust_drop_panic() -> !;
+}
+
 mod dwarf;
 
 // Entry point for catching an exception, implemented using the `try` intrinsic
diff --git a/src/libpanic_unwind/seh.rs b/src/libpanic_unwind/seh.rs
index e1907ec4e5f..d9dca2c0f4f 100644
--- a/src/libpanic_unwind/seh.rs
+++ b/src/libpanic_unwind/seh.rs
@@ -77,8 +77,11 @@ use libc::{c_int, c_uint, c_void};
 //      #include <stdint.h>
 //
 //      struct rust_panic {
+//          rust_panic(const rust_panic&);
+//          ~rust_panic();
+//
 //          uint64_t x[2];
-//      }
+//      };
 //
 //      void foo() {
 //          rust_panic a = {0, 1};
@@ -128,7 +131,7 @@ mod imp {
 #[repr(C)]
 pub struct _ThrowInfo {
     pub attributes: c_uint,
-    pub pnfnUnwind: imp::ptr_t,
+    pub pmfnUnwind: imp::ptr_t,
     pub pForwardCompat: imp::ptr_t,
     pub pCatchableTypeArray: imp::ptr_t,
 }
@@ -145,7 +148,7 @@ pub struct _CatchableType {
     pub pType: imp::ptr_t,
     pub thisDisplacement: _PMD,
     pub sizeOrOffset: c_int,
-    pub copy_function: imp::ptr_t,
+    pub copyFunction: imp::ptr_t,
 }
 
 #[repr(C)]
@@ -168,7 +171,7 @@ const TYPE_NAME: [u8; 11] = *b"rust_panic\0";
 
 static mut THROW_INFO: _ThrowInfo = _ThrowInfo {
     attributes: 0,
-    pnfnUnwind: ptr!(0),
+    pmfnUnwind: ptr!(0),
     pForwardCompat: ptr!(0),
     pCatchableTypeArray: ptr!(0),
 };
@@ -181,7 +184,7 @@ static mut CATCHABLE_TYPE: _CatchableType = _CatchableType {
     pType: ptr!(0),
     thisDisplacement: _PMD { mdisp: 0, pdisp: -1, vdisp: 0 },
     sizeOrOffset: mem::size_of::<[u64; 2]>() as c_int,
-    copy_function: ptr!(0),
+    copyFunction: ptr!(0),
 };
 
 extern "C" {
@@ -208,6 +211,43 @@ static mut TYPE_DESCRIPTOR: _TypeDescriptor = _TypeDescriptor {
     name: TYPE_NAME,
 };
 
+// Destructor used if the C++ code decides to capture the exception and drop it
+// without propagating it. The catch part of the try intrinsic will set the
+// first word of the exception object to 0 so that it is skipped by the
+// destructor.
+//
+// Note that x86 Windows uses the "thiscall" calling convention for C++ member
+// functions instead of the default "C" calling convention.
+//
+// The exception_copy function is a bit special here: it is invoked by the MSVC
+// runtime under a try/catch block and the panic that we generate here will be
+// used as the result of the exception copy. This is used by the C++ runtime to
+// support capturing exceptions with std::exception_ptr, which we can't support
+// because Box<dyn Any> isn't clonable.
+macro_rules! define_cleanup {
+    ($abi:tt) => {
+        unsafe extern $abi fn exception_cleanup(e: *mut [u64; 2]) {
+            if (*e)[0] != 0 {
+                cleanup(*e);
+                super::__rust_drop_panic();
+            }
+        }
+        #[unwind(allowed)]
+        unsafe extern $abi fn exception_copy(_dest: *mut [u64; 2],
+                                             _src: *mut [u64; 2])
+                                             -> *mut [u64; 2] {
+            panic!("Rust panics cannot be copied");
+        }
+    }
+}
+cfg_if::cfg_if! {
+   if #[cfg(target_arch = "x86")] {
+       define_cleanup!("thiscall");
+   } else {
+       define_cleanup!("C");
+   }
+}
+
 pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
     use core::intrinsics::atomic_store;
 
@@ -220,8 +260,7 @@ pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
     // exception (constructed above).
     let ptrs = mem::transmute::<_, raw::TraitObject>(data);
     let mut ptrs = [ptrs.data as u64, ptrs.vtable as u64];
-    let ptrs_ptr = ptrs.as_mut_ptr();
-    let throw_ptr = ptrs_ptr as *mut _;
+    let throw_ptr = ptrs.as_mut_ptr() as *mut _;
 
     // This... may seems surprising, and justifiably so. On 32-bit MSVC the
     // pointers between these structure are just that, pointers. On 64-bit MSVC,
@@ -243,6 +282,12 @@ pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
     //
     // In any case, we basically need to do something like this until we can
     // express more operations in statics (and we may never be able to).
+    if !cfg!(bootstrap) {
+        atomic_store(
+            &mut THROW_INFO.pmfnUnwind as *mut _ as *mut u32,
+            ptr!(exception_cleanup) as u32,
+        );
+    }
     atomic_store(
         &mut THROW_INFO.pCatchableTypeArray as *mut _ as *mut u32,
         ptr!(&CATCHABLE_TYPE_ARRAY as *const _) as u32,
@@ -255,6 +300,12 @@ pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
         &mut CATCHABLE_TYPE.pType as *mut _ as *mut u32,
         ptr!(&TYPE_DESCRIPTOR as *const _) as u32,
     );
+    if !cfg!(bootstrap) {
+        atomic_store(
+            &mut CATCHABLE_TYPE.copyFunction as *mut _ as *mut u32,
+            ptr!(exception_copy) as u32,
+        );
+    }
 
     extern "system" {
         #[unwind(allowed)]
diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs
index 8ea50a972ec..031837c1efb 100644
--- a/src/librustc_codegen_llvm/intrinsic.rs
+++ b/src/librustc_codegen_llvm/intrinsic.rs
@@ -922,6 +922,9 @@ fn codegen_msvc_try(
         //      #include <stdint.h>
         //
         //      struct rust_panic {
+        //          rust_panic(const rust_panic&);
+        //          ~rust_panic();
+        //
         //          uint64_t x[2];
         //      }
         //
@@ -929,17 +932,19 @@ fn codegen_msvc_try(
         //          try {
         //              foo();
         //              return 0;
-        //          } catch(rust_panic a) {
+        //          } catch(rust_panic& a) {
         //              ret[0] = a.x[0];
         //              ret[1] = a.x[1];
+        //              a.x[0] = 0;
         //              return 1;
         //          }
         //      }
         //
         // More information can be found in libstd's seh.rs implementation.
         let i64_2 = bx.type_array(bx.type_i64(), 2);
-        let i64_align = bx.tcx().data_layout.i64_align.abi;
-        let slot = bx.alloca(i64_2, i64_align);
+        let i64_2_ptr = bx.type_ptr_to(i64_2);
+        let ptr_align = bx.tcx().data_layout.pointer_align.abi;
+        let slot = bx.alloca(i64_2_ptr, ptr_align);
         bx.invoke(func, &[data], normal.llbb(), catchswitch.llbb(), None);
 
         normal.ret(bx.const_i32(0));
@@ -947,15 +952,31 @@ fn codegen_msvc_try(
         let cs = catchswitch.catch_switch(None, None, 1);
         catchswitch.add_handler(cs, catchpad.llbb());
 
+        // The flag value of 8 indicates that we are catching the exception by
+        // reference instead of by value. We can't use catch by value because
+        // that requires copying the exception object, which we don't support
+        // since our exception object effectively contains a Box.
+        //
+        // Source: MicrosoftCXXABI::getAddrOfCXXCatchHandlerType in clang
+        let flags = bx.const_i32(8);
         let tydesc = match bx.tcx().lang_items().eh_catch_typeinfo() {
             Some(did) => bx.get_static(did),
             None => bug!("eh_catch_typeinfo not defined, but needed for SEH unwinding"),
         };
-        let funclet = catchpad.catch_pad(cs, &[tydesc, bx.const_i32(0), slot]);
+        let funclet = catchpad.catch_pad(cs, &[tydesc, flags, slot]);
 
-        let payload = catchpad.load(slot, i64_align);
+        let i64_align = bx.tcx().data_layout.i64_align.abi;
+        let payload_ptr = catchpad.load(slot, ptr_align);
+        let payload = catchpad.load(payload_ptr, i64_align);
         let local_ptr = catchpad.bitcast(local_ptr, bx.type_ptr_to(i64_2));
         catchpad.store(payload, local_ptr, i64_align);
+
+        // Clear the first word of the exception so avoid double-dropping it.
+        // This will be read by the destructor which is implicitly called at the
+        // end of the catch block by the runtime.
+        let payload_0_ptr = catchpad.inbounds_gep(payload_ptr, &[bx.const_i32(0), bx.const_i32(0)]);
+        catchpad.store(bx.const_u64(0), payload_0_ptr, i64_align);
+
         catchpad.catch_ret(&funclet, caught.llbb());
 
         caught.ret(bx.const_i32(1));
diff --git a/src/libstd/panicking.rs b/src/libstd/panicking.rs
index 599ccc809be..bfadeafb7c7 100644
--- a/src/libstd/panicking.rs
+++ b/src/libstd/panicking.rs
@@ -55,6 +55,15 @@ extern "C" {
     fn __rust_start_panic(payload: usize) -> u32;
 }
 
+/// This function is called by the panic runtime if FFI code catches a Rust
+/// panic but doesn't rethrow it. We don't support this case since it messes
+/// with our panic count.
+#[cfg(not(test))]
+#[rustc_std_internal_symbol]
+extern "C" fn __rust_drop_panic() -> ! {
+    rtabort!("Rust panics must be rethrown");
+}
+
 #[derive(Copy, Clone)]
 enum Hook {
     Default,
diff --git a/src/test/run-make-fulldeps/foreign-exceptions/foo.rs b/src/test/run-make-fulldeps/foreign-exceptions/foo.rs
index 399c78f8d2d..9c2045c8c89 100644
--- a/src/test/run-make-fulldeps/foreign-exceptions/foo.rs
+++ b/src/test/run-make-fulldeps/foreign-exceptions/foo.rs
@@ -4,7 +4,6 @@
 
 // For linking libstdc++ on MinGW
 #![cfg_attr(all(windows, target_env = "gnu"), feature(static_nobundle))]
-
 #![feature(unwind_attributes)]
 
 use std::panic::{catch_unwind, AssertUnwindSafe};