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

Well-defined unwinding through FFI boundaries #2699

Closed
wants to merge 23 commits into from
Closed
Changes from 7 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
191 changes: 191 additions & 0 deletions text/XXX-annotate-unwind-rust.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
- Feature Name: (fill me in with a unique ident, `my_awesome_feature`)
- Start Date: (fill me in with today's date, YYYY-MM-DD)
- RFC PR: ???
- Rust Issue: ???

# Summary
[summary]: #summary

A new function annotation, `#[unwind(Rust)]`, will be introduced; it will
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use present tense - this RFC introduces...

explicitly permit `extern` functions to unwind (`panic`) without aborting. No
Copy link
Contributor

Choose a reason for hiding this comment

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

Only extern "ABI" functions ? (all ABIs ? or which ones). Shouldn't this also be supported on extern function declarations (extern { fn-decl }) ?

guarantees are made regarding the specific unwinding implementation.
Copy link
Contributor

Choose a reason for hiding this comment

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

We do make some guarantees though. For example, when using a particular toolchain to compile Rust code, all extern functions and extern function declarations with this attribute are assumed to use the same panic implementation.


# Motivation
[motivation]: #motivation

This will enable resolving
[rust-lang/rust#58794](https://github.com/rust-lang/rust/issues/58794) without
breaking existing code.

Currently, unwinding through an FFI boundary is always undefined behavior. We
would like to make the behavior safe by aborting when the stack is unwound to
an FFI boundary. However, there are existing Rust crates (notably, wrappers
around the `libpng` and `libjpeg` C libraries) that make use of the current
implementation's behavior. The proposed annotation would act as an "opt out" of
the safety guarantee provided by aborting at an FFI boundary.

Copy link
Contributor

Choose a reason for hiding this comment

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

These library authors decided to rely on an implementation detail of undefined behavior, they knew about it, and have been actively blocking the fix of soundness hole in the language without putting in any work towards a solution.

If anything, this is probably the strongest argument against this RFC. It sends the message that it is ok to rely on UB, exploit implementation details, etc. knowingly as long as you complain loud enough when things break because then the language will be bent in your favor.

Calling it an ""opt out" of a safety guarantee" does not help.


Honestly I don't think even mentioning this would do this RFC a favor. From the use cases discussed, the Rust-Rust dynamic linking one felt like the only one that could motivate this. It might be better to focus on that instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't realize you felt this way about the specific impetus for the request and the proposal itself; I had thought, from you last comments in the issue thread, that you were in agreement that this was a reasonable request and a viable proposal.

I disagree with your perspective on the UB issue and am willing to have that conversation, but I'm now wondering if that discussion should be continued back on the internals thread so that it doesn't make this PR too noisy. If I had understood that we didn't yet have agreement on the way forward, I wouldn't have opened this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

(By "the UB issue" I don't mean expecting LLVM not to use nounwind for optimization; I agree that doing so is incorrect.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't realize you felt this way about the specific impetus for the request and the proposal itself; I had thought, from you last comments in the issue thread, that you were in agreement that this was a reasonable request and a viable proposal.

I thought this comment expressed what I believe would be an appropriate motivation for this attribute.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gnzlbg I agree that FFI Rust->Rust is an appropriate motivation for the attribute, but it felt disingenuous to me to advocate for it on that ground alone since it's not the real motivation for myself or @kornelski to be pushing for an annotation of some sort changing FFI unwind behavior. What I took from your phrasing in that comment about Rust->X->Rust was that you agreed it would be appropriate for @kornelski and anyone else to use the proposed annotation for Rust->X->Rust as long as they realize that they're relying on an implementation detail:

...by accident, it would allow those doing Rust->X->Rust to continue to "work" as long as their assumptions about implementation details still hold.

Knowledge about implementation details doesn't seem like an overly burdensome requirement for unsafe interactions between languages; that's how the computing world managed to function without a formally defined C ABI for several decades.

Copy link
Contributor

@gnzlbg gnzlbg May 20, 2019

Choose a reason for hiding this comment

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

What I took from your phrasing in that comment about Rust->X->Rust was that you agreed it would be appropriate for @kornelski and anyone else to use the proposed annotation for Rust->X->Rust as long as they realize that they're relying on an implementation detail:

I don't see where I agreed to that. My comment says that "panics in Rust->Rust FFI" is a simple extension to the language that solves an useful problem, and that such a feature would allow some of the people that are invoking undefined-behavior in Rust today to continue doing so after the soundness fix for panics in Rust FFI lands (which AFAICT is all they wanted).

I never suggested #[unwind(Rust)] as a solution to "panic in Rust->X", nor say that it would be appropriate for anyone to write code with undefined behavior. If anything, I showed multiple times how to write code without UB in stable Rust today, and argued that because that is possible "panics in Rust->X" might not be a problem worth solving.

# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

Consider an `extern "C"` function that may `panic`:

```rust
extern "C" fn may_panic(i: i32) {
if i < 0 {
panic!("Oops, I should have used u32.");
}
}
```

In a future (TBD) version of Rust, calling this function with an argument less
than zero will cause the program to be aborted. This is the only way the Rust
compiler can ensure that the function is safe, because there is no way for it
to know whether or not the calling code supports the same implementation of
stack-unwinding used for Rust.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the main issue.

The Rust language guarantees that extern "C" functions do not panic, and these functions are optimized accordingly by LLVM. If they were to actually panic, the optimizations would be incorrect, which results in undefined behavior. Therefore, the language requires these functions to abort if a panic were to escape from them, but due to a bug in the implementation, this does not happen today. This bugs allows safe Rust programs to trigger UB, which makes the implementation unsound.

None of this has anything to do with the unwinding implementation of other languages. In fact, even if Rust code compiled with the exact same toolchain was used to call this function, the unsoundness bug would still be there, because LLVM will still optimize this code under the assumption that it does not panic.


As a concrete example, Rust code marked `exern "C"` may be invoked from C code,
but C code on Linux or BSD may be compiled without support for native
stack-unwinding. This means that the runtime would lack the necessary metadata
to properly perform the unwinding operation.

However, there are cases in which a `panic` through an `extern` function can be
used safely. For instance, it is possible to invoke `extern` Rust functions
via Rust code built with the same toolchain, in which case it would be
irrelevant to the unwinding operation that the `panic`ing function is an
`extern` function:

```rust
fn main() {
let result = panic::catch_unwind(|| {
may_panic(-1);
}
assert!(result.is_err());
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

See above, this example is unsound.


In order to ensure that `may_panic` will not simply abort in a future version
of Rust, it must be marked `#[unwind(Rust)]`. This annotation can only be used
with an `unsafe` function, since the compiler is unable to make guarantees
about the behavior of the caller.

```rust
#[unwind(Rust)]
unsafe extern "C" fn may_panic(i: i32) {
if i < 0 {
panic!("Oops, I should have used u32.");
}
}
```

**PLEASE NOTE:** Using this annotation **does not** provide any guarantees
Copy link
Contributor

@gnzlbg gnzlbg May 13, 2019

Choose a reason for hiding this comment

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

The behavior of extern functions and extern function declarations that unwind (in any way - not only via Rust panics) is undefined. #[unwind(Rust)] allows these functions to let a Rust panic! escape, which is something that they cannot do without this attribute.

Explaining that does not really need mentioning anything about the panic implementation. If you want to mention that, just mentioning that the Rust panic implementation is unspecified, that is, it can change at any time, should be enough.

about the unwinding implementation. Therefore, using this feature to compile
functions intended to be called by C code **does not make the behavior
well-defined**; in particular, **the behavior may change in a future version of
Rust.**

The *only* well-defined behavior specified by this annotation is Rust-to-Rust
unwinding.

It is safe to call such a function from C or C++ code only if that code is
guaranteed to provide the same unwinding implementation as the Rust compiler
used to compile the function.

Since the behavior may be subject to change without notice as the Rust compiler
is updated, it is recommended that all projects that rely on unwinding from
Rust code into C code lock the project's `rustc` version and only update it
after ensuring that the behavior will remain correct.
Copy link
Contributor

Choose a reason for hiding this comment

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

If this feature were aimed at solving this problem, then I don't think that having to do this to be able to use this feature correctly would be good enough.

I don't think this feature can solve this problem well without major changes.


# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

Unwinding for functions marked `#[unwind(Rust)]` is performed as if the
function were not marked `extern`. This is identical to the behavior of `rustc`
for all versions prior to 1.35 except for 1.24.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

This reads a bit like this feature is an opt-out workaround for some language change. I don't think the RFC should frame that this way.


This annotation has no effect on functions not marked `extern`. It has no
observable effect unless the marked function `panic`s (e.g. it has no
observable effect when a function returns normally or enters an infinite loop).
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the attribute does have an effect from the point-of-view of how the generated code can be optimized, even if the marked function never panic!s in practice.


# Drawbacks
[drawbacks]: #drawbacks

Since the Rust unwinding implementation is not specified, this annotation is
explicitly designed to expose a potentially non-forward-compatible API. As
mentioned in (the guide-level explanation)[#guide-level-explanation], use of
BatmanAoD marked this conversation as resolved.
Show resolved Hide resolved
this annotation will make projects vulnerable to breakage (and specifically to
undefined behavior) simply by updating their Rust compiler.

Furthermore, this annotation will have different behaviors on different
platforms, and determining whether it is safe to use on a particular platform
is fairly difficult. So far, there are only two safe use cases identified:

* On Windows, C code built with MSVC always respects SEH, the unwinding
mechanism used by both (MSVC) Rust and C++, so it should always be safe to
invoke such a function when MSVC is the only toolchain used, as long as
`rustc` uses SEH as its `panic` implementation.
* In projects using LLVM or GCC exclusively, the `-fexceptions` flag ensures
that C code is compiled with C++ exception support, so the runtime behavior
should be safe as long as `rustc` uses the native C++ exception mechanism as
its `panic` implementation.

# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives

This is the minimum possible change to the language that will permit making
`panic` safe by default while permitting existing users of the current behavior
a way to keep their code working after
[rust-lang/rust#58794](https://github.com/rust-lang/rust/issues/58794) is
resolved.

The language team has twice attempted to stabilize the desired default behavior
of aborting when unwinding across an FFI boundary without providing a way to
opt-out of this behavior, but it has become clear that the community will not
accept such a change without a means of opting out because of the impact on
existing projects (particularly [mozjpeg](https://crates.io/crates/mozjpeg)).

Any alternatives that provide guarantees about the specific Rust unwinding
implementation would make implementation more difficult and would lock us in to
a specific annotation semantics for describing unwinding mechanisms. Suggested
notations include:

- `#[unwind(c)]`
- `#[unwind(c++)]`
- `#[unwind(c++-native)]`
- `#[unwind(seh)]`

This proposal does not exclude the possibility of introducing one or more of
these notations at a later date, and indeed it will almost certainly be
necessary to introduce a way to specify an unwinding implementation if the
`rustc` default unwinding mechnanism ever changes. However, introducing such
a notation would be a larger change to the language, and there is no consensus
yet on what the notation should be.

# Prior art
[prior-art]: #prior-art

The proposed behavior of this annotation is simply to maintain the existing
behavior for un-annotated functions.

As mentioned above, GCC and LLVM provide an `-fexceptions` flag that makes the
C++ exception mechanism interoperable with C stackframes.

# Unresolved questions
[unresolved-questions]: #unresolved-questions

As mentioned [above](#rationale-and-alternatives), further work will be
required to provide a means of specifying details of the unwinding
implementation to provide guarnateed-safe interoperability with (some) C and
C++ code. That work is out of scope for this RFC.

# Future possibilities
[future-possibilities]: #future-possibilities

As mentioned [above](#rationale-and-alternatives), further work will be
required to provide a means of specifying details of the unwinding
implementation to provide guarnateed-safe interoperability with (some) C and
C++ code. That work is out of scope for this RFC.

Note that this feature _does not_ commit the team to delivering future variants
of the `#[unwind(...)]` annotation. For instance, compatibility with C code
could be provided via a `rustc` flag specifying the (global) unwinding
implementation to use.