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

Unsoundness when a panic Rust code is caught by separetely compiled Rust code through FFI-unwind #102715

Closed
nbdd0121 opened this issue Oct 5, 2022 · 7 comments · Fixed by #102721
Labels
A-FFI Area: Foreign function interface (FFI) A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows C-bug Category: This is a bug. F-c_unwind `#![feature(c_unwind)]` I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness requires-nightly This issue requires a nightly compiler in some way. WG-ffi-unwind Working group: FFI unwind

Comments

@nbdd0121
Copy link
Contributor

nbdd0121 commented Oct 5, 2022

Rust code might be able to catch foreign Rust code through FFI unwind.

a.rs:

#![crate_type = "cdylib"]
#![feature(c_unwind)]

#[no_mangle]
extern "C-unwind" fn panic() {
    panic!();
}

b.rs:

#![feature(c_unwind)]

#[link(name = "a")]
extern "C-unwind" {
    fn panic();
}

fn main() {
    let err = std::panic::catch_unwind(|| {
        unsafe { panic() };
    });
    match err {
        Err(v) => {
            // Able to access `Box<dyn Any>` generated by another
            // compiler; we can't guarantee that typeid does not conflict
            // across Rust versions, nor that the vtable format is
            // stable.
			// EDIT: Also this will result a `Box` allocated in one allocator
			// from being deallocated in another, which is more obviously unsound.
        }
        _ => (),
    }
}

These two crates could be compiled with different Rust versions, or same version with different flags (e.g. struct layout randomisation), and this will create unsoundness because we couldn't guarantee the ABI for separate compilations.

Currently we just use the exception class in the unwind runtime ("MOZ\0RUST") to tell apart Rust exceptions from foreign exceptions, but for soundness we need to treat Rust exception from another compilation as foreign exception as well.

@nbdd0121 nbdd0121 added the C-bug Category: This is a bug. label Oct 5, 2022
@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Oct 5, 2022

@rustbot label: +F-c_unwind +WG-project-ffi-unwind +I-unsound +requires-nightly +A-ffi +A-runtime

@rustbot rustbot added A-FFI Area: Foreign function interface (FFI) A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows F-c_unwind `#![feature(c_unwind)]` I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness requires-nightly This issue requires a nightly compiler in some way. WG-ffi-unwind Working group: FFI unwind labels Oct 5, 2022
@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Oct 6, 2022

I think it's worth mentioning that catching a foreign unwind in Rust is UB. I mark this as I-unsound because:

  1. We don't have a clear documentation about the situation of Rust code being foreign code.
  2. We are currently not guarding against it.
  3. It might work by mistake without anyone noticing.

While guarding against it is favorable, this issue could be considered solved if we simply document clearly that this kind of situation is same as catching a non-Rust (e.g. C++) foreign unwind, which is UB.

@DemiMarie
Copy link
Contributor

I think it's worth mentioning that catching a foreign unwind in Rust is UB.

I would like to see this somewhat changed in the future. Specifically, I would like:

  • Throwing e.g. a C++ exception across Rust stack frames causes Rust destructors to be called.
  • A Rust panic that unwinds into C++ causes C++ destructors to be called.
  • Cross-language exception objects are completely opaque: one can drop them (which will call its destructor, if applicable) or rethrow them, but (without directly interacting with the foreign language’s runtime) one cannot obtain any information about the exception.

That would likely require a separate RFC, though.

@bjorn3
Copy link
Member

bjorn3 commented Oct 7, 2022

or rethrow them

You can't rethrow foreign exceptions without hooking directly into the runtime of the respective language when catching them and when rethrowing them. libstd can't hook into the C++ standard library, so rethrowing after std::panic::catch_unwind is out of the question.

Throwing e.g. a C++ exception across Rust stack frames causes Rust destructors to be called.
A Rust panic that unwinds into C++ causes C++ destructors to be called.

Unwinding through C++ or Rust already causes destructors to run. It is only the catching that isn't allowed.

@RalfJung
Copy link
Member

RalfJung commented Oct 7, 2022

, this issue could be considered solved if we simply document clearly that this kind of situation is same as catching a non-Rust (e.g. C++) foreign unwind, which is UB.

Yeah this makes sense to me. Do we have any documentation talking about linking together Rust code compiled with different toolchains? We already obviously don't allow passing repr(Rust) types between them, they have to use FFI-safe types, so arguably it is only consistent to say the same about unwinding.

@bjorn3
Copy link
Member

bjorn3 commented Oct 7, 2022

Rust panics pass a Box<dyn Any + Send> around (passed to panic_any and resume_unwind and returned from catch_unwind) which is not FFI safe, so we shouldn't need to have an extra rule specifically for panics.

@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Oct 7, 2022

The problem is that there are no explicit use of Box<dyn Any + Send>. The example code in the OP exhibits UB in a quite subtle way.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 11, 2022
Prevent foreign Rust exceptions from being caught

Fix rust-lang#102715

Use the address of a static variable (which is guaranteed to be unique per copy of std) to tell apart if a Rust exception comes from local or foreign Rust code, and abort for the latter.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 25, 2022
Prevent foreign Rust exceptions from being caught

Fix rust-lang#102715

Use the address of a static variable (which is guaranteed to be unique per copy of std) to tell apart if a Rust exception comes from local or foreign Rust code, and abort for the latter.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 25, 2022
Prevent foreign Rust exceptions from being caught

Fix rust-lang#102715

Use the address of a static variable (which is guaranteed to be unique per copy of std) to tell apart if a Rust exception comes from local or foreign Rust code, and abort for the latter.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 28, 2022
Prevent foreign Rust exceptions from being caught

Fix rust-lang#102715

Use the address of a static variable (which is guaranteed to be unique per copy of std) to tell apart if a Rust exception comes from local or foreign Rust code, and abort for the latter.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 28, 2022
Prevent foreign Rust exceptions from being caught

Fix rust-lang#102715

Use the address of a static variable (which is guaranteed to be unique per copy of std) to tell apart if a Rust exception comes from local or foreign Rust code, and abort for the latter.
@bors bors closed this as completed in 6dd64d3 Oct 29, 2022
Aaron1011 pushed a commit to Aaron1011/rust that referenced this issue Jan 6, 2023
Prevent foreign Rust exceptions from being caught

Fix rust-lang#102715

Use the address of a static variable (which is guaranteed to be unique per copy of std) to tell apart if a Rust exception comes from local or foreign Rust code, and abort for the latter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-FFI Area: Foreign function interface (FFI) A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows C-bug Category: This is a bug. F-c_unwind `#![feature(c_unwind)]` I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness requires-nightly This issue requires a nightly compiler in some way. WG-ffi-unwind Working group: FFI unwind
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants