Abort instead of unwinding past FFI functions

[SimonSapin]

More updated description (2021-03-11)

With #76570 having landed, Rust now aborts when a panic leaves an extern "C" function, so the original soundness issue is fixed. However, there is an attribute to opt-out of this behavior, #[unwind(allowed)]. Using this attribute is currently unsound, so it should be fixed, removed, or require some form of unsafe.

Updated Description

This is a tracking issue for switching the compiler to abort the program whenever a Rust program panics across an FFI boundary, notably causing any non-Rust-ABI function to panic/unwind.

This is being implemented in #55982 and some regressions were found in the wild so this is also being repurposed as a tracking issue for any breakage that comes up and a place to discuss the breakage. If you're a crate author and you find your way here, don't hesitate to ask questions!

Original Description

Doing Rust unwinding into non-Rust stack frames is undefined behavior. This was fixed in 1.24.0, and then reverted (I think by #48380 ?) in 1.24.1 because of a regression that affected rlua.

The latter blog post said:

The solution here is to generate the abort handler, but in a way that longjmp won’t trigger it. It’s not 100% clear if this will make it into Rust 1.25; if the landing is smooth, we may backport, otherwise, this functionality will be back in 1.26.

The link PR has landed, but my understanding is that it does not change FFI functions back to aborting on unwind (though it looks like it does fix the issue that affected rlua).

This UB is not mentioned in any later release notes.

This issue has been assigned to @BatmanAoD via this comment.

[SimonSapin]

CC @alexcrichton, @nikomatsakis

[liigo]

cc me

[alexcrichton]

cc @diwic

I think @SimonSapin your description is accurate. The backport to stable and backport to beta were both minor patches, and the main one to nightly (at the time) was #48380 as you mentioned.

AFAIK we should be good to go again to do this.

[Mark-Simulacrum]

I think I would prefer that instead of immediately landing a change to the default unwind behavior we just stabilize the unwind attribute and wait for a few cycles to let the ecosystem migrate to unwind(allowed) or unwind(abort) across extern "C" functions. It would be nice to have some docs providing guidance here as well.

[SimonSapin]

Why is this attribute useful? If the issue that affected rlua is fixed, what can make the answer to "do you want to protect against some potential UB" be anything other than "yes, obviously"?

[Mark-Simulacrum]

There are cases where unwinding is fine, for example if you are using a extern "C" function to talk between different Rust crates or extern "Rust" functions to do the same. There are cases where you legitimately are fine with unwinding (and IMO it shouldn't be UB). I don't think the decision whether unwinding should abort or continue should be based on the extern-ness of the function. unwind(allowed) or unwind(aborts) may be useful in normal Rust as well if you want to do -Cpanic=abort on a smaller (non-library) scale, though it's less useful to an extent in that case due to catch_unwind existing.

[diwic]

Good to see some movement here.

For me it's bikeshedding whether we go with #[unwind], #[unwind(allowed)] or even extern "C-unwind" as long as we default to aborting for every non-Rust ABI. But it does seem it would be helpful for the libjpeg / libpng bindings.

[SimonSapin]

Alright, stabilizing some attribute (perhaps with unsafe in its name) to support this libjpeg use case sounds fine, but I don’t see a reason to wait several cycles to change the default back when the plan was to do it in 1.25.

[Mark-Simulacrum]

The primary point we reverted back then was because of breakage due to changing the default. I'm not convinced that the breakage would not happen again; there's currently no way to explicitly document your requirements on stable. I see this as the same situation as we normally have for library features, where the unstable/deprecated solution isn't removed until the stable alternative lands in stable.

I also am unclear why we would/should rush this. People have waited, people can continue to wait. Stabilizing unwind seems uncontroversial; we can do so immediately.

[diwic]

@Mark-Simulacrum

unwind(allowed) or unwind(aborts) may be useful in normal Rust as well if you want to do -Cpanic=abort on a smaller (non-library) scale, though it's less useful to an extent in that case due to catch_unwind existing.

I'm not sure what this means, but if -Cpanic=abort is specified I don't think you should be able to override that with a #[unwind(allowed)].

I also am unclear why we would/should rush this. People have waited, people can continue to wait.

Well, why should fixing I-unsound bugs have high priority in general? I think they should have high priority, but not everyone shares that sentiment.

Stabilizing unwind seems uncontroversial; we can do so immediately.

Are there some unresolved questions here w r t:

• Which ABIs should allow #[unwind]? Should we do this for all ABIs, or just "C"?
• In a -Cpanic=abort scenario should #[unwind] be able to override this for Rust ABI and nevertheless allow unwinding? (I prefer not.)

[Mark-Simulacrum]

I'm not sure what this means, but if -Cpanic=abort is specified I don't think you should be able to override that with a #[unwind(allowed)].

I mean the reverse; if you've specified -Cpanic=unwind but for whatever reason want a specific function to not unwind, you can today do so with catch_unwind but I could see us also supporting #[unwind(abort)] annotations as an alternative. I'm not sure there's a need, though.

Well, why should fixing I-unsound bugs have high priority in general? I think they should have high priority, but not everyone shares that sentiment.

This isn't an unsoundness bug though. It is currently UB to unwind through stack frames not generated by the same Rust compiler (and will always be, at least in the short term) but I don't see how that is unsound. I've removed that tag.

Are there some unresolved questions here w r t ...

I'd be happy stabilizing #[unwind] for all extern functions regardless of ABI; it seems like a feature orthogonal to the ABI. I don't think it's possible to make #[unwind] override -Cpanic=abort anyway, or at least not trivial. That is to say, it shouldn't be supported, but perhaps should also not be an error since there's not currently a way to cfg around -Cpanic...

[SimonSapin]

This isn't an unsoundness bug though

It is indeed not rustc or std that is unsound, but the current situation is that it is very difficult to write sound FFI code. You need to carefully audit every callback either to convince yourself that it cannot ever panic, or use catch_unwind (again in every callback) and then carefully audit the code that is outside of the catch_unwind closure that wraps stuff to be UnwindSafe or deals with the returned Result. (Don’t use .unwrap() !)

So I’d say this is a soundness-related issue.