c_unwind full stabilization request: change in extern "C" behavior

[BatmanAoD]

Tracking issue: #74990

This is a separate issue to enable using the FCP bot a second time. The request-for-stabilization comment is duplicated below, without modification.

RFC "C-unwind ABI": rust-lang/rfcs#2945

Feature gate to be stabilized: #![feature(c_unwind)]

Stabilization PR: #116088

[BatmanAoD]

Request for Stabilization

This is a request for full stabilization of the c_unwind feature, RFC-2945. The behavior of non-"Rust", non-unwind ABIs (such as extern "C") will be modified to close soundness holes caused by permitting stack-unwinding to cross FFI boundaries that do not support unwinding.

Summary

When using panic=unwind, if a Rust function marked extern "C" panics (and that panic is not caught), the runtime will now abort.

Previously, the runtime would simply attempt to unwind the caller's stack, but the behavior when doing so was undefined, because extern "C" functions are optimized with the assumption that they cannot unwind (i.e. in rustc, they are given the LLVM nounwind annotation).

This affects existing programs. If a program relies on a Rust panic "escaping" from extern "C":

• It is currently unsound.
• Once this feature is stabilized, the program will crash when this occurs, whereas previously it may have appeared to work as expected.
• Replacing extern "x" with extern "x-unwind" will produce the intended behavior without the unsoundness.

The behavior of function calls using extern "C" is unchanged; thus, it is still undefined behavior to call a C++ function that throws an exception using the extern "C" ABI, even when compiling with panic=unwind.

Changes since the RFC

None.

Unresolved questions

This feature does not affect the behavior of pthread_exit or longjmp, so stabilization will neither break existing code using these features nor provide any soundness guarantee.

Tests

"C" guaranteed to abort on panic

This test verifies that an extern "C" function that panic!s will always abort if the panic would otherwise "escape".

Documentation

The documentation PRs for this feature did not distinguish between the initial partially-stabilized behavior and the fully-stabilized behavior, so they already include the modifications to non-unwind ABIs.

• The Rustonomicon PR has already been merged.
• The Reference PR is still awaiting approval.

I've also opened an issue to discuss adding a brief note about this feature in the Book.

[joshtriplett]

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[nbdd0121]

cc @rust-lang/wg-ffi-unwind

[tmandry]

• Once this feature is stabilized, the program will crash when this occurs, whereas previously it may have appeared to work as expected.

Do we have a crater run showing the impact of this? I'd prefer to give the ecosystem time to update their code ahead of crashing everyone's programs (even if they're technically already broken).

That said, we will need some way of alerting all the broken libraries.

[nbdd0121]

We could do the crater run on the PR (still under preparation, sorry!)

[BatmanAoD]

I do think it makes sense to do a crater run before actually approving the stabilization.

[traviscross]

@rustbot labels +I-lang-nominated

Nominating so we can discuss what needs to happen to move this forward.

[traviscross]

@rustbot labels -I-lang-nominated

[nikomatsakis]

I'm in favor of doing this. At worst we can tie it to the edition.

[traviscross]

The situation seems to be that we're waiting on #116088 for the crater run, and that PR is waiting on #113923 to land.

[BatmanAoD]

@traviscross That's correct; sorry for not linking to those myself.

[nbdd0121]

Crater run is completed. I have triaged all crater failures and determined they are either flaky test, or broken crate, or are expected failure due to the change to enforce extern "C" to be nounwind.

[traviscross]

@rustbot labels +I-lang-nominated

Nominating for discussion as it seems we had been waiting on a crater run and analysis that now has been completed.

[RalfJung]

However, #113923 has also been reverted, so currently we can't stabilize this yet AFAIK.

[nbdd0121]

I think that's should only be blocking the PR, not the FCP?

[BatmanAoD]

expected failure due to the change to enforce extern "C" to be nounwind

I think this is what we were specifically worried about, isn't it? How widespread is this?

From your comment, it looks like it's these crates?

• ad9361-rs
• cashweb-secp256k1
• consistenttime
• lv2-worker
• mujs
• quest_bind
• sawp-ffi
• near_sdk
• zcash

Any idea if the maintainers of these crates know about the upcoming change?

[cratelyn]

the zcash crate looks to be https://crates.io/crates/zcash a namesquatting crate. it is version 0.0.0, hasn't been updated in 5 years, and the associated repository has since been deleted.

p.s. so excited to see this work in the final comment period! thank you all again for helping mentor my work on #76570. 💟

[bjorn3]

The crater report contains regressions for several zcash related git repos: https://crater-reports.s3.amazonaws.com/pr-116088-2/index.html Several at least are forks of the upstream zcash, for which no regressions are reported. So it may well be that they were forked before it was fixed in upstream.

[traviscross]

@rustbot labels -I-lang-nominated

We discussed this in triage today with the result that this in now entering FCP. Thanks to @BatmanAoD for working to push this forward.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[CAD97]

Small thing: I find it somewhat surprising the exact manner in which unwinds become aborts in extern "C" fn. Specifically, the current behavior is that each unwinding edge in the extern "C" fn is set to terminate(abi), resulting in that any locals of the extern "C" fn itself are not dropped. The behavior that I expected is achievable by immediately delegating to a function with an unwinding ABI — that unwinding continues as normal up until it reaches the extern "C" boundary, at which point it aborts. (In MIR terms, generate cleanup bbs as normal, only replacing unwind continue with unwind terminate(abi), leaving any unwind: bbN as-is.)

Further refining, it surprises me that wrapping a function body in an immediately invoked closure changes semantics. I do not expect the changed ABI in the signature to change the semantics of the contained body.

I could live with this, as the developer actually technically has more control in this setup, and it's lower overhead to abort more eagerly, but it still feels a bit wrong, and performance of the about-to-abort edge isn't particularly important. Any optimization barriers to the happy path are mostly still present due to the unwind edge existing at all.

(Sorry for just missing FCP on this observation... I said something similar in the discussion around drop unwinds when I determined this is how it works, but didn't think to mention it here as well for some reason.)

nbdd0121 notes that while this is the case for the Itanium unwind ABI, MSVC+SEH produces an immediate abort at the unwind source instead. This doesn't exactly match the behavior my testing gets, but it is at least different for SEH than Itanium.

[RalfJung]

I agree that's surprising, and opened an issue: #123231.

Not sure if that's something we need to resolve before stabilization. However if we want to keep the current behavior we definitely need to document somewhere that calling these destructors is not guaranteed.

[daira]

The crater report contains regressions for several zcash related git repos: https://crater-reports.s3.amazonaws.com/pr-116088-2/index.html Several at least are forks of the upstream zcash, for which no regressions are reported. So it may well be that they were forked before it was fixed in upstream.

Speaking as ECC's engineering manager responsible for zcashd and librustzcash, I strongly support stabilization of this change. In fact I have been bitterly complaining, with this issue as one of the examples, about the time it takes to fix soundness bugs in Rust. So it would be rather ironic if Zcash-related code were any part of the reason you were not doing so in this case (although I cannot see why it would be).

zcashd pins a specific Rust version, so there is no significant risk. If it breaks us, we will fix it before changing that pin — or not, because zcashd is being deprecated anyway.

We also have an issue to change all our uses of "C" to "C-unwind" in librustzcash; the only reason why that was not prioritized is that there appeared to be no significant progress on the full stabilization, which is precisely the thing that would make "C-unwind" useful to us.

[BatmanAoD]

@daira

Believe me, I sympathize with the pain of how long this feature/fix has taken.

We also have an issue to change all our uses of "C" to "C-unwind" in librustzcash; the only reason why that was not prioritized is that there appeared to be no significant progress on the full stabilization, which is precisely the thing that would make "C-unwind" useful to us.

Sorry, I don't understand this dilemma; the "full stabilization" will only affect "C". The "C-unwind" behavior, as described in the stabilization request and RFC, is already in place.