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

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

Open
gnzlbg opened this issue Oct 11, 2019 · 18 comments
Open

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

gnzlbg opened this issue Oct 11, 2019 · 18 comments

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Oct 11, 2019

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.).

  • How would we detect a double panic?
  • What error message would be printed?
@BatmanAoD
Copy link
Member

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
Copy link
Contributor Author

gnzlbg commented Oct 12, 2019

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
Copy link
Member

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
Copy link
Contributor Author

gnzlbg commented Oct 15, 2019

@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
Copy link
Contributor Author

gnzlbg commented Oct 15, 2019

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
Copy link
Member

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
Copy link
Contributor Author

gnzlbg commented Oct 15, 2019

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.

bors added a commit to rust-lang-ci/rust that referenced this issue Feb 19, 2022
Guard against unwinding in cleanup code

Currently the only safe guard we have against double unwind is the panic count (which is local to Rust). When double unwinds indeed happen (e.g. C++ exception + Rust panic, or two C++ exceptions), then the second unwind actually goes through and the first unwind is leaked. This can cause UB. cc rust-lang/project-ffi-unwind#6

E.g. given the following C++ code:
```c++
extern "C" void foo() {
    throw "A";
}

extern "C" void execute(void (*fn)()) {
    try {
        fn();
    } catch(...) {
    }
}
```

This program is well-defined to terminate:
```c++
struct dtor {
    ~dtor() noexcept(false) {
        foo();
    }
};

void a() {
    dtor a;
    dtor b;
}

int main() {
    execute(a);
    return 0;
}
```

But this Rust code doesn't catch the double unwind:
```rust
extern "C-unwind" {
    fn foo();
    fn execute(f: unsafe extern "C-unwind" fn());
}

struct Dtor;

impl Drop for Dtor {
    fn drop(&mut self) {
        unsafe { foo(); }
    }
}

extern "C-unwind" fn a() {
    let _a = Dtor;
    let _b = Dtor;
}

fn main() {
    unsafe { execute(a) };
}
```

To address this issue, this PR adds an unwind edge to an abort block, so that the Rust example aborts. This is similar to how clang guards against double unwind (except clang calls terminate per C++ spec and we abort).

The cost should be very small; it's an additional trap instruction (well, two for now, since we use TrapUnreachable, but that's a different issue) for each function with landing pads; if LLVM gains support to encode "abort/terminate" info directly in LSDA like GCC does, then it'll be free. It's an additional basic block though so compile time may be worse, so I'd like a perf run.

r? `@ghost`
`@rustbot` label: F-c_unwind
@BatmanAoD
Copy link
Member

Now that rust-lang/rust#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
Copy link

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

  • Do checks before crossing FFI boundary: when calling Rust from C++, you need to check if exception is pending before calling; when calling C++ from Rust, you need to check if panicking is in process before calling.
  • Add a panic handler in Rust that throws C++ exception instead.

Neither sounds ideal.

@Amanieu
Copy link
Member

Amanieu commented Mar 13, 2022

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
Copy link
Member

BatmanAoD commented Mar 14, 2022

@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
Copy link
Member

Amanieu commented Mar 14, 2022

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
Copy link
Member

BatmanAoD commented Mar 15, 2022

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
Copy link

shinmao commented Dec 28, 2022

} 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
Copy link
Member

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

@Amanieu
Copy link
Member

Amanieu commented Dec 30, 2022

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
Copy link

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
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants