Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent foreign Rust exceptions from being caught #102721

Merged
merged 7 commits into from
Oct 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 28 additions & 12 deletions library/panic_unwind/src/emcc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,12 @@ static EXCEPTION_TYPE_INFO: TypeInfo = TypeInfo {
name: b"rust_panic\0".as_ptr(),
Copy link
Member

@bjorn3 bjorn3 Oct 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe change this to avoid unsoundness in combination with older rust versions? Same for the exception type for the other unwinding mechanisms.

Copy link
Contributor Author

@nbdd0121 nbdd0121 Oct 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth mentioning that catching a foreign unwind in Rust is UB. We are providing guards against this but it's not part of guarantee.

Currently there are no ways to hit this on stable, and if you hit this using nightly you are already in UB territory. So we can change this string (and the exception class), but we don't have to.

};

// NOTE(nbdd0121): The `canary` field will be part of stable ABI after `c_unwind` stabilization.
#[repr(C)]
struct Exception {
// See `gcc.rs` on why this is present. We already have a static here so just use it.
canary: *const TypeInfo,

// This is necessary because C++ code can capture our exception with
// std::exception_ptr and rethrow it multiple times, possibly even in
// another thread.
Expand All @@ -70,27 +75,38 @@ pub unsafe fn cleanup(ptr: *mut u8) -> Box<dyn Any + Send> {
let catch_data = &*(ptr as *mut CatchData);

let adjusted_ptr = __cxa_begin_catch(catch_data.ptr as *mut libc::c_void) as *mut Exception;
let out = if catch_data.is_rust_panic {
let was_caught = (*adjusted_ptr).caught.swap(true, Ordering::SeqCst);
if was_caught {
// Since cleanup() isn't allowed to panic, we just abort instead.
intrinsics::abort();
}
(*adjusted_ptr).data.take().unwrap()
} else {
if !catch_data.is_rust_panic {
super::__rust_foreign_exception();
};
}

let canary = ptr::addr_of!((*adjusted_ptr).canary).read();
if !ptr::eq(canary, &EXCEPTION_TYPE_INFO) {
super::__rust_foreign_exception();
}

let was_caught = (*adjusted_ptr).caught.swap(true, Ordering::SeqCst);
if was_caught {
// Since cleanup() isn't allowed to panic, we just abort instead.
intrinsics::abort();
}
let out = (*adjusted_ptr).data.take().unwrap();
__cxa_end_catch();
out
}

pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
let sz = mem::size_of_val(&data);
let exception = __cxa_allocate_exception(sz) as *mut Exception;
let exception = __cxa_allocate_exception(mem::size_of::<Exception>()) as *mut Exception;
if exception.is_null() {
return uw::_URC_FATAL_PHASE1_ERROR as u32;
}
ptr::write(exception, Exception { caught: AtomicBool::new(false), data: Some(data) });
ptr::write(
nbdd0121 marked this conversation as resolved.
Show resolved Hide resolved
exception,
Exception {
canary: &EXCEPTION_TYPE_INFO,
caught: AtomicBool::new(false),
data: Some(data),
},
);
__cxa_throw(exception as *mut _, &EXCEPTION_TYPE_INFO, exception_cleanup);
}

Expand Down
30 changes: 27 additions & 3 deletions library/panic_unwind/src/gcc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,23 @@

use alloc::boxed::Box;
use core::any::Any;
use core::ptr;

use unwind as uw;

// In case where multiple copies of std exist in a single process,
// we use address of this static variable to distinguish an exception raised by
// this copy and some other copy (which needs to be treated as foreign exception).
static CANARY: u8 = 0;

// NOTE(nbdd0121)
// Once `c_unwind` feature is stabilized, there will be ABI stability requirement
// on this struct. The first two field must be `_Unwind_Exception` and `canary`,
// as it may be accessed by a different version of the std with a different compiler.
#[repr(C)]
struct Exception {
_uwe: uw::_Unwind_Exception,
canary: *const u8,
bjorn3 marked this conversation as resolved.
Show resolved Hide resolved
bjorn3 marked this conversation as resolved.
Show resolved Hide resolved
cause: Box<dyn Any + Send>,
}

Expand All @@ -54,6 +65,7 @@ pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
exception_cleanup,
private: [0; uw::unwinder_private_data_size],
},
canary: &CANARY,
cause: data,
});
let exception_param = Box::into_raw(exception) as *mut uw::_Unwind_Exception;
Expand All @@ -75,10 +87,22 @@ pub unsafe fn cleanup(ptr: *mut u8) -> Box<dyn Any + Send> {
if (*exception).exception_class != rust_exception_class() {
uw::_Unwind_DeleteException(exception);
super::__rust_foreign_exception();
} else {
let exception = Box::from_raw(exception as *mut Exception);
exception.cause
}

let exception = exception.cast::<Exception>();
// Just access the canary field, avoid accessing the entire `Exception` as
// it can be a foreign Rust exception.
let canary = ptr::addr_of!((*exception).canary).read();
if !ptr::eq(canary, &CANARY) {
// A foreign Rust exception, treat it slightly differently from other
// foreign exceptions, because call into `_Unwind_DeleteException` will
// call into `__rust_drop_panic` which produces a confusing
// "Rust panic must be rethrown" message.
super::__rust_foreign_exception();
}

let exception = Box::from_raw(exception as *mut Exception);
exception.cause
}

// Rust's exception class identifier. This is used by personality routines to
Expand Down
20 changes: 15 additions & 5 deletions library/panic_unwind/src/seh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,15 @@
use alloc::boxed::Box;
use core::any::Any;
use core::mem::{self, ManuallyDrop};
use core::ptr;
use libc::{c_int, c_uint, c_void};

// NOTE(nbdd0121): The `canary` field will be part of stable ABI after `c_unwind` stabilization.
#[repr(C)]
struct Exception {
// See `gcc.rs` on why this is present. We already have a static here so just use it.
canary: *const _TypeDescriptor,

// This needs to be an Option because we catch the exception by reference
// and its destructor is executed by the C++ runtime. When we take the Box
// out of the exception, we need to leave the exception in a valid state
Expand Down Expand Up @@ -235,7 +241,7 @@ static mut TYPE_DESCRIPTOR: _TypeDescriptor = _TypeDescriptor {
macro_rules! define_cleanup {
($abi:tt $abi2:tt) => {
unsafe extern $abi fn exception_cleanup(e: *mut Exception) {
if let Exception { data: Some(b) } = e.read() {
if let Exception { data: Some(b), .. } = e.read() {
drop(b);
super::__rust_drop_panic();
}
Expand Down Expand Up @@ -265,7 +271,7 @@ pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
// The ManuallyDrop is needed here since we don't want Exception to be
// dropped when unwinding. Instead it will be dropped by exception_cleanup
// which is invoked by the C++ runtime.
let mut exception = ManuallyDrop::new(Exception { data: Some(data) });
let mut exception = ManuallyDrop::new(Exception { canary: &TYPE_DESCRIPTOR, data: Some(data) });
let throw_ptr = &mut exception as *mut _ as *mut _;

// This... may seems surprising, and justifiably so. On 32-bit MSVC the
Expand Down Expand Up @@ -321,8 +327,12 @@ pub unsafe fn cleanup(payload: *mut u8) -> Box<dyn Any + Send> {
// __rust_try. This happens when a non-Rust foreign exception is caught.
if payload.is_null() {
super::__rust_foreign_exception();
} else {
let exception = &mut *(payload as *mut Exception);
exception.data.take().unwrap()
}
let exception = payload as *mut Exception;
let canary = ptr::addr_of!((*exception).canary).read();
if !ptr::eq(canary, &TYPE_DESCRIPTOR) {
// A foreign Rust exception.
super::__rust_foreign_exception();
}
(*exception).data.take().unwrap()
}
11 changes: 11 additions & 0 deletions src/test/run-make-fulldeps/foreign-rust-exceptions/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# ignore-i686-pc-windows-gnu

# This test doesn't work on 32-bit MinGW as cdylib has its own copy of unwinder
# so cross-DLL unwinding does not work.

include ../tools.mk

all:
$(RUSTC) bar.rs --crate-type=cdylib
$(RUSTC) foo.rs
$(call RUN,foo) 2>&1 | $(CGREP) "Rust cannot catch foreign exceptions"
7 changes: 7 additions & 0 deletions src/test/run-make-fulldeps/foreign-rust-exceptions/bar.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#![crate_type = "cdylib"]
#![feature(c_unwind)]

#[no_mangle]
extern "C-unwind" fn panic() {
panic!();
}
13 changes: 13 additions & 0 deletions src/test/run-make-fulldeps/foreign-rust-exceptions/foo.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#![feature(c_unwind)]

#[cfg_attr(not(windows), link(name = "bar"))]
#[cfg_attr(windows, link(name = "bar.dll"))]
extern "C-unwind" {
fn panic();
}

fn main() {
let _ = std::panic::catch_unwind(|| {
unsafe { panic() };
});
}