catch_unwind incorrectly tries to stop ForceUnwinds

[gnzlbg]

MWE:

use std::os::unix::thread::JoinHandleExt;
use libc::pthread_cancel;

struct DropGuard;
impl Drop for DropGuard {
    fn drop(&mut self) {
        println!("unwinding foo");
    }
}

fn foo() {
    let _x = DropGuard;
    println!("thread started");
    std::thread::sleep(std::time::Duration::new(1, 0));
    println!("thread finished");
}

fn main() {
    let handle0 = std::thread::spawn(foo);
    std::thread::sleep(std::time::Duration::new(0, 10_000));
    unsafe { 
        let x = pthread_cancel(handle0.as_pthread_t());
        assert_eq!(x, 0);
    }
    handle0.join();
}

(Playground)

Output:

thread started
unwinding foo

Errors:

   Compiling playground v0.0.1 (/playground)
warning: unused `std::result::Result` that must be used
  --> src/main.rs:25:5
   |
25 |     handle0.join();
   |     ^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_must_use)]` on by default
   = note: this `Result` may be an `Err` variant, which should be handled

    Finished release [optimized] target(s) in 0.86s
     Running `target/release/playground`
FATAL: exception not rethrown
timeout: the monitored command dumped core
/root/entrypoint.sh: line 8:     7 Aborted                 timeout --signal=KILL ${timeout} "$@"

According to the Itanium ABI:

• "forced" unwinding (such as caused by longjmp or thread termination).
  [...]
  During "forced unwinding", on the other hand, an external agent is driving the unwinding. For instance, this can be the longjmp routine. This external agent, not each personality routine, knows when to stop unwinding. The fact that a personality routine is not given a choice about whether unwinding will proceed is indicated by the _UA_FORCE_UNWIND flag.

So catch_unwind definitely needs to always catch Rust exceptions, and at the thread boundary it probably also want to catch foreign exceptions and abort since whatever that should do probably won't work. When not on a thread boundary, aborting on foreign exceptions might be a conservative step until a better solution is discussed (an alternative solution would be not to catch them and just let them escape, or to catch them and have the Any be some ForeignPanic type defined somewhere in std::panic that users can use to detect that a foreign exception was raised). What it probably shouldn't do is catch "ForceUnwind" otherwise longjmp might not work across it and threads will not be canceled, but... maybe the conservative thing to do would be for this to abort for the time being?

[gnzlbg]

cc @Amanieu @alexcrichton

[Amanieu]

I feel that we should simply let all foreign exceptions unwind through catch_unwind without catching them. This roughly matches the behavior of a C++ catch clause which only catches the rust_unwind exception type. This should of course be documented.

[gnzlbg]

This roughly matches the behavior of a C++ catch clause which only catches the rust_unwind exception type.

Note that C++ with Itanium ABI also catches foreign exceptions if a catch(...) clause is used. That is, in C++, if your implementations supports it (which AFAIK none does right now), you can use catch(__rust_exception const& e) to catch a Rust panic (you can't modify it though). According to the Itanium ABI, the __rust_exception type should be defined in the <exception> header.

[Amanieu]

Actually, looking at the implementation of catch_unwind, there is no check that the exception object that was caught is actually a panic generated by Rust. The code just blindly casts the exception pointer to a pointer to struct Exception. This will fail horribly if the exception was not generated by Rust.

[gnzlbg]

Yes, I think that's correct at the language spec level, because right now, letting a foreign exception (or any kind of exception actually) unwind into Rust is UB, so we do not provide any kind of guarantees about what catch_unwind does.

As you mention, what we currently do for foreign exceptions is horrible. Since there are some platforms where we can easily do better (e.g. just aborting), I think we should do so as a "quality-of-implementation" issue at least, even though it is still UB.