rust-lang / project-ffi-unwind

Apache License 2.0
36 stars 13 forks source link

Double-panic behavior on C++->Rust unwinding #6

Open gnzlbg opened 5 years ago

gnzlbg commented 5 years ago

This code:

fn main() {
    foo();
}

fn foo() {
    let _x = Foo;
    unsafe { bar() }
}

// note: the `unsafe` allows replacing this with 
// an `extern "C unwind"` below:
unsafe fn bar() { panic!("bar"); }

struct Foo;
impl Drop for Foo {
    fn drop(&mut self) {
        panic!("Foo");
    }
}

is guaranteed to panic twice, once printing "bar" and once again printing "Foo", and then, the program aborts due to the double panic printing "thread panicked while panicking. aborting".

I wonder what would happen if we replace bar above with an extern "C unwind" { fn bar(); } function implemented in C++ as extern "C" void bar() { throw "bar"; } or similar (e.g. throwing a std::string("bar"), std::runtime_exception("bar"), etc.).

BatmanAoD commented 5 years ago

Initially, I think it's reasonable to consider this undefined behavior.

We will need to look at how the double panic is currently detected and determine whether this will already detect panic-while-unwinding caused by foreign exceptions. If it can, perhaps we should just change the error message to "thread panicked while unwinding. aborting". (I don't think it would be terribly valuable to make the mechanism detect why the unwind is occurring and explain this to the user.)

gnzlbg commented 5 years ago

I think we probably want to extend the UB initially to the opposite case as well, that is, when a Rust "C unwind" function panics and in C++ a noexcept(false) destructor throws.

This means that a safe extern "C unwind" Rust function can cause UB when called from C++, but that's ok I think.

BatmanAoD commented 5 years ago

I think we probably want to extend the UB initially to the opposite case as well, that is, when a Rust "C unwind" function panics and in C++ a noexcept(false) destructor throws.

This is what I was referring to on Zulip about "defining the behavior of native (non-Rust) code when attempting to throw while a Rust panic is already in-flight."

I don't understand how Rust could specify this behavior for C++.

gnzlbg commented 5 years ago

@BatmanAoD ah, I completely misunderstood you then, since I left very similar comments in the PR. So yes, I agree this is wrong:

I think we probably want to extend the UB initially to the opposite case as well, that is, when a Rust "C unwind" function panics and in C++ a noexcept(false) destructor throws.

We can't specify this. What we can do is document that, if this happens, the C++ standard doesn't make any guarantees about this behavior, and document what guarantees each of the target implementations makes. For example, Itanium-C++ says that if a foreign exception, e.g., thrown by Rust, causes C++ to throw while unwinding, the behavior is undefined.

gnzlbg commented 5 years ago

For future reference (https://itanium-cxx-abi.github.io/cxx-abi/abi-eh.html#base-personality):

The behavior is undefined in the following cases:

[...] A __foreign_exception is active at the same time as another exception (either there is a nested exception while catching the foreign exception, or the foreign exception was itself nested).

EDIT: this is the behavior that C++ on targets with the Itanium ABI has, when a foreign exception, such as the one caused from unwinding from Rust into C++, triggers a double-drop in C++.

BatmanAoD commented 5 years ago

Okay, thanks for the clarifications.

Would you mind submitting a PR adding some verbiage to the roadmap to the effect that panic-while-unwinding-foreign-exceptions is undefined?

gnzlbg commented 5 years ago

Would you mind submitting a PR adding some verbiage to the roadmap to the effect that panic-while-unwinding-foreign-exceptions is undefined?

So I made the behavior of this implementation defined in #9 , which on x86_64-apple-darwin and x86_64-unknown-linux-gnu (and probably all other Itanium targets, like FreeBSD) can be defined to abort, since on such targets, panic! can just check if we are already unwinding, and if so, whether the current on flight exception originated in Rust or in some other programming language, and act accordingly and abort. For the case in which both panics originate in Rust the error message will probably be nicer than when one of the panics is foreign, because we can print the error messages of both panics. When the panic is foreign, we should be able to at least print a stack trace.

BatmanAoD commented 2 years ago

Now that https://github.com/rust-lang/rust/pull/92911 has been merged, I believe the only remaining issue here (discussed in Zulip) is a Rust panic escaping into the C++ runtime while a C++ exception is in-flight. In practice, it should usually be safe, but the Itanium ABI specifies that it's UB; I think it's therefore reasonable to formally specify that this is UB for Rust as well.

CC @nikomatsakis @gnzlbg @nbdd0121

nbdd0121 commented 2 years ago

There are usability problems if this is UB. To avoid UB, you have to either:

Neither sounds ideal.

Amanieu commented 2 years ago

In practice, both libc++ (Clang) and libstdc++ (GCC) will call std::terminate when catch (...) catches a foreign exception if there is an active C++ exception (execution is already inside an outer catch block). Unwinding through destructors with a Rust panic works fine since it doesn't interact with the C++ exception machinery (the landing pads just call _Unwind_Resume).

For the reverse case, Rust panics don't have any thread-local state while they are running and are effectively invisible to the C++ exception machinery. So nesting a C++ exception while a Rust panic is unwinding "just works".

BatmanAoD commented 2 years ago

@nbdd0121 I'm not sure I see why this is particularly problematic "sharp edge", though. Cross-language exception handling is a fairly niche use case; panicking from within a drop or catch_unwind seems fairly niche as well (and in the C++ world is heavily discouraged, even if it technically "works"); and panicking in such a way that a drop is expected to unwind into a different language seems extremely niche and generally not likely to be a "good" approach to any problem I can imagine.

I think ideally we'd want to be able to guarantee an abort in this case. But the set of circumstances needed to trigger it seem complex enough that I'm not yet convinced that we need to do that, or even that the implementation effort and possible runtime cost would be worth it.

Amanieu commented 2 years ago

The specific case to trigger the UB is:

#[no_mangle]
extern "C-unwind" fn rust_panic() {
    panic!();
}
int main() {
    try {
        rust_panic();
    } catch (...) {
        // The Rust panic is caught here.
        try {
            // Throw a C++ exception
            throw exception();
        } catch (exception&) {
            // UB happens here: you can't have a live foreign exception (in the catch block) at the same time as a C++ exception.
            // This is because internally C++ keeps a linked list of live exceptions, but this doesn't work with foreign exception objects.
        }
    }
}
BatmanAoD commented 2 years ago

Oh, I thought we were talking about UB that only occurs when the rust_panic() type function happens within drop or catch_unwind; so you're saying the reverse, where C++ creates a new exception and tries to catch it while the Rust panic is still live, is UB?

That still seems...not that bad, to be honest.

Or, more precisely: it seems like it's not our problem. AFAIK C++ doesn't (and probably never will) attempt to define behavior involving "foreign exceptions". We've done our due diligence on the Rust side, I think, of trying to make Rust a well-behaved citizen; I'm not sure it makes much sense to try to ensure that C++ is also a well-behaved neighbor.

shinmao commented 1 year ago
} catch (...) {
        // The Rust panic is caught here.
        try {
            // Throw a C++ exception
            throw exception();
        } catch (exception&) {
           // UB?
        }
    }

The case @Amanieu mentioned above. In my test case, Rust will still give fatal runtime error: Rust panics must be rethrown, isn't it? The catch(exception&) block will be executed, but at the end, rustc will still abort the program.

Basically, the result is same as:

// foreign exception from Rust
} catch(...) {
     // code
}

The code will also be executed but fatal runtime error at the end.

So not so bad... just like @BatmanAoD said?

BatmanAoD commented 1 year ago

@nbdd0121 @Amanieu do either of you still believe this is a problem worth solving?

Amanieu commented 1 year ago

I don't think this is worth solving. In practice it's going to abort, even though the spec technically says it's UB.

nbdd0121 commented 1 year ago

Ideally I think we should specify that this is not UB for Rust code. The fact that it's technically defined as UB per C++ Itanium ABI for C++ code is beyond our control.

BatmanAoD commented 1 year ago

@nbdd0121 Doesn't that mean that the behavior might actually be different on different Itanium boards, regardless of what code we generate?

I'd be okay with a note in the reference saying something like "in practice, this should cause the process to abort, but is formally undefined," but I don't want to say that Rust guarantees this isn't UB if we don't actually control it.