Fix FFI-unwind unsoundness with mixed panic mode #97235

nbdd0121 · 2022-05-20T22:15:12Z

UB maybe introduced when an FFI exception happens in a C-unwind foreign function and it propagates through a crate compiled with -C panic=unwind into a crate compiled with -C panic=abort (#96926).

To prevent this unsoundness from happening, we will disallow a crate compiled with -C panic=unwind to be linked into panic-abort if it contains a call to C-unwind foreign function or function pointer. If no such call exists, then we continue to allow such mixed panic mode linking because it's sound (and stable). In fact we still need the ability to do mixed panic mode linking for std, because we only compile std once with -C panic=unwind and link it regardless panic strategy.

For libraries that wish to remain compile-once-and-linkable-to-both-panic-runtimes, a ffi_unwind_calls lint is added (gated under c_unwind feature gate) to flag any FFI unwind calls that will cause the linkable panic runtime be restricted.

In summary:

#![warn(ffi_unwind_calls)]

mod foo {
    #[no_mangle]
    pub extern "C-unwind" fn foo() {}
}

extern "C-unwind" {
    fn foo();
}

fn main() {
    // Call to Rust function is fine regardless ABI.
    foo::foo();
    // Call to foreign function, will cause the crate to be unlinkable to panic-abort if compiled with `-Cpanic=unwind`.
    unsafe { foo(); }
    //~^ WARNING call to foreign function with FFI-unwind ABI
    let ptr: extern "C-unwind" fn() = foo::foo;
    // Call to function pointer, will cause the crate to be unlinkable to panic-abort if compiled with `-Cpanic=unwind`.
    ptr();
    //~^ WARNING call to function pointer with FFI-unwind ABI
}

Fix #96926

@rustbot label: T-compiler F-c_unwind

rust-highfive · 2022-05-20T22:15:15Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-05-20T22:15:16Z

r? @compiler-errors

(rust-highfive has picked a reviewer for you, use r? to override)

BatmanAoD · 2022-05-20T22:45:15Z

compiler/rustc_metadata/src/rmeta/mod.rs

@@ -198,7 +198,7 @@ crate struct CrateRoot<'tcx> {
    extra_filename: String,
    hash: Svh,
    stable_crate_id: StableCrateId,
-    panic_strategy: PanicStrategy,
+    panic_strategy: Option<PanicStrategy>,


What would cause this to be None?

If the crate is compiled with -C panic=unwind and contains no FFI unwind calls. I.e. it does not require a particular panic runtime to be linked.

Got it, thanks!

That seems a bit odd? The panic strategy still makes a difference for other things, doesn't it?

This field should definitely be renamed (perhaps requires_panic_runtime?), it is very confusing at the moment since "panic strategy" refers to how the current crate was compiled.

#![no_std] code doesn't require a panic runtime, but could still require a particular panic strategy for any dependent crates. For example a -Cpanic=abort no std crate requires all dependent crates to be compiled with -Cpanic=abort too, even though using #[panic_handler] rather than linking a panic runtime would work.

This field and related query (panic_strategy(_: CrateNum)) is only used for checking compatibility. I could rename this to required_panic_strategy.

compiler-errors · 2022-05-20T23:09:31Z

not sure if i'm the best person to review this

r? rust-lang/compiler

bors · 2022-05-21T09:37:03Z

☔ The latest upstream changes (presumably #97239) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2022-05-23T08:37:02Z

// Call to Rust function is fine regardless ABI.

Why is that okay?
I first thought the check would be "reject if there are any calls with C-unwind ABI", but clearly what actually happens is a bit more subtle, so I want to make sure we document precisely why this is okay. :)

(And that documentation should be somewhere more permanent that this PR, such as in the code and maybe even ameded to the RFC or added to the reference or so.)

compiler/rustc_mir_transform/src/ffi_unwind_calls.rs

RalfJung · 2022-05-23T08:40:46Z

compiler/rustc_metadata/src/dependency_format.rs

@@ -397,18 +401,14 @@ fn verify_ok(tcx: TyCtxt<'_>, list: &[Linkage]) {
            if let Linkage::NotLinked = *linkage {
                continue;
            }
-            if desired_strategy == PanicStrategy::Abort {
-                continue;
-            }


This is a key part of the PR, right? That's where previously we skipped a check that we do not skip any more now.

Sorts of. Previously any crate can be linked to panic-abort; this line only has to be removed because the required panic strategy becomes three-value. You can think of the all previous PanicStrategy::Unwind maps to None in this PR.

RalfJung · 2022-05-23T08:45:50Z

Why is that okay?

Trying to answer my own question... Is it correct that this PR ensures the following property?

If anywhere in the crate graph is a panic=abort crate, then all call sites where foreign exceptions could enter the Rust part of the program are in a panic=abort crate, and hence will lead to immediate abort.
These call sites are C-unwind call sites to FFI functions or function pointers -- a call site calling a Rust function cannot be where anything enters Rust.

There will also be no exceptions generated from Rust itself, and hence it is correct that a "Rust" ABI function can never unwind.

RalfJung · 2022-05-23T08:47:46Z

library/std/src/lib.rs

@@ -210,6 +210,8 @@
 #![allow(unused_lifetimes)]
 // Tell the compiler to link to either panic_abort or panic_unwind
 #![needs_panic_runtime]
+// Ensure that std can be linked against panic_abort despite compiled with `-C panic=unwind`
+#![cfg_attr(not(bootstrap), deny(ffi_unwind_calls))]


Don't core and alloc need the same?

I actually want to make it a default for compiling std and all its dependencies -- but not sure how.

@nbdd0121 was this followed up on? In the future, please feel free to reach out to #t-infra on Zulip for questions like this, we can help with the instrumentation necessary. In this case you'd likely add to the deny'd lints in bootstrap, around here, with a gate on Mode::Std.

I created a thread in t-libs. The lint should be denied for std and its dependencies but not denied for panic runtimes, and it seems that this isn't quite achievement at the moment.

RalfJung · 2022-05-23T08:52:31Z

Cc @Amanieu

Amanieu · 2022-05-23T10:02:41Z

compiler/rustc_interface/src/passes.rs

@@ -954,6 +954,7 @@ fn analysis(tcx: TyCtxt<'_>, (): ()) -> Result<()> {
            if !tcx.sess.opts.debugging_opts.thir_unsafeck {
                rustc_mir_transform::check_unsafety::check_unsafety(tcx, def_id);
            }
+            tcx.ensure().has_ffi_unwind_calls(def_id);


This seems to be called twice: here and in mir_const. Is this intentional?

Yes. This query uses mir_built, which will be stolen later. So I followed the same pattern as check_unsafety (which also uses mir_built, by making sure it's evaluated before a query that could steal it.

RalfJung · 2022-05-23T20:34:44Z

compiler/rustc_mir_transform/src/abort_unwinding_calls.rs

@@ -31,11 +31,7 @@ impl<'tcx> MirPass<'tcx> for AbortUnwindingCalls {

        // We don't simplify the MIR of constants at this time because that
        // namely results in a cyclic query when we call `tcx.type_of` below.
-        let is_function = match kind {
-            DefKind::Fn | DefKind::AssocFn | DefKind::Ctor(..) => true,


This is a change in behavior; is_fn_like returns false for a Ctor.

It's fine; ctor won't call other functions.

Fix FFI-unwind unsoundness with mixed panic mode UB maybe introduced when an FFI exception happens in a `C-unwind` foreign function and it propagates through a crate compiled with `-C panic=unwind` into a crate compiled with `-C panic=abort` (rust-lang#96926). To prevent this unsoundness from happening, we will disallow a crate compiled with `-C panic=unwind` to be linked into `panic-abort` *if* it contains a call to `C-unwind` foreign function or function pointer. If no such call exists, then we continue to allow such mixed panic mode linking because it's sound (and stable). In fact we still need the ability to do mixed panic mode linking for std, because we only compile std once with `-C panic=unwind` and link it regardless panic strategy. For libraries that wish to remain compile-once-and-linkable-to-both-panic-runtimes, a `ffi_unwind_calls` lint is added (gated under `c_unwind` feature gate) to flag any FFI unwind calls that will cause the linkable panic runtime be restricted. In summary: ```rust #![warn(ffi_unwind_calls)] mod foo { #[no_mangle] pub extern "C-unwind" fn foo() {} } extern "C-unwind" { fn foo(); } fn main() { // Call to Rust function is fine regardless ABI. foo::foo(); // Call to foreign function, will cause the crate to be unlinkable to panic-abort if compiled with `-Cpanic=unwind`. unsafe { foo(); } //~^ WARNING call to foreign function with FFI-unwind ABI let ptr: extern "C-unwind" fn() = foo::foo; // Call to function pointer, will cause the crate to be unlinkable to panic-abort if compiled with `-Cpanic=unwind`. ptr(); //~^ WARNING call to function pointer with FFI-unwind ABI } ``` Fix rust-lang#96926 ``@rustbot`` label: T-compiler F-c_unwind

Dylan-DPC · 2022-06-29T12:28:20Z

failed in rollup

@bors r-

RalfJung · 2022-06-29T12:57:52Z

@bors rollup=never

nbdd0121 · 2022-06-30T01:17:34Z

Currently the lint only fires when -C panic=unwind is used -- if -C panic=abort is used then the lint does not fire.

It's possible to have ffi_unwind_calls lint to fire regardless current panic statregy -- but doing so would require me to change fn_can_unwind from returning bool to returning a tristate (CanUnwind, NoUnwind, FollowPanicStrategy). If that behaviour is desirable I can do it in a follow up PR.

Amanieu · 2022-07-01T23:55:35Z

@bors r+

bors · 2022-07-01T23:55:37Z

📌 Commit 0cf28dc has been approved by Amanieu

bors · 2022-07-02T14:06:30Z

⌛ Testing commit 0cf28dc with merge 6a10920...

bors · 2022-07-02T16:34:48Z

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 6a10920 to master...

rust-timer · 2022-07-03T01:00:46Z

Finished benchmarking commit (6a10920): comparison url.

Instruction count

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	0.5%	1.2%	86
Regressions 😿 (secondary)	0.9%	2.5%	36
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	0.5%	1.2%	86

Max RSS (memory usage)

Results

Primary benchmarks: mixed results
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	2.9%	4.6%	4
Regressions 😿 (secondary)	5.8%	10.2%	8
Improvements 🎉 (primary)	-1.6%	-1.7%	3
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	1.0%	4.6%	7

Cycles

Results

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	2.9%	3.5%	4
Regressions 😿 (secondary)	4.8%	5.7%	4
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-2.4%	-2.4%	1
All 😿🎉 (primary)	2.9%	3.5%	4

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

Mark-Simulacrum · 2022-07-05T13:24:03Z

@nbdd0121 @Amanieu was a perf regression expected here? Do you have ideas on what caused it? It seems to me that it's pretty consistently a small regression across multiple benchmarks, so I suspect it's worth following up on to try and see if we can eliminate it or at least understand the cause.

nbdd0121 · 2022-07-05T13:30:28Z

There is a new query to be executed (and cached) for each MIR function body, so the regression is not unexpected.

Fix outdated comment of `fn_can_unwind` The first part is outdated since rust-lang#96473, and the second part is outdated since rust-lang#97235

rust-highfive assigned compiler-errors May 20, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 20, 2022

rustbot added the F-c_unwind `#![feature(c_unwind)]` label May 20, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 20, 2022

This comment has been minimized.

Sign in to view

nbdd0121 force-pushed the unwind branch from e9b1d33 to 4e3ee25 Compare May 20, 2022 22:29

This comment has been minimized.

Sign in to view

BatmanAoD reviewed May 20, 2022

View reviewed changes

rust-highfive assigned jackh726 and unassigned compiler-errors May 20, 2022

This comment has been minimized.

Sign in to view

BatmanAoD mentioned this pull request May 22, 2022

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

Merged

nbdd0121 force-pushed the unwind branch from 8f47f6d to cf46273 Compare May 22, 2022 23:28

RalfJung reviewed May 23, 2022

View reviewed changes

compiler/rustc_mir_transform/src/ffi_unwind_calls.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 23, 2022

View reviewed changes

RalfJung mentioned this pull request May 23, 2022

Remove libstd's calls to C-unwind foreign functions #97033

Merged

Amanieu reviewed May 23, 2022

View reviewed changes

RalfJung reviewed May 23, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

nbdd0121 force-pushed the unwind branch from b7d3a71 to cef1c54 Compare May 23, 2022 20:44

This comment has been minimized.

Sign in to view

Dylan-DPC mentioned this pull request Jun 29, 2022

Rollup of 7 pull requests #98667

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 29, 2022

Skip ffi-unwind lint test with -Cpanic=abort

0cf28dc

nbdd0121 force-pushed the unwind branch from 98c6042 to 0cf28dc Compare June 30, 2022 01:51

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 1, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 2, 2022

bors merged commit 6a10920 into rust-lang:master Jul 2, 2022

rustbot added this to the 1.64.0 milestone Jul 2, 2022

rustbot added the perf-regression Performance regression. label Jul 3, 2022

Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Jul 5, 2022

nbdd0121 mentioned this pull request Dec 7, 2022

Refactor unwind in MIR #102906

Merged

nbdd0121 mentioned this pull request Jun 30, 2023

Fix outdated comment of fn_can_unwind #113213

Merged

cuviper mentioned this pull request Apr 10, 2024

RUST_BACKTRACE=full loop with -Cpanic=abort on aarch64-unknown-linux-gnu #123733

Open

Fix FFI-unwind unsoundness with mixed panic mode #97235

Fix FFI-unwind unsoundness with mixed panic mode #97235

Conversation

nbdd0121 commented May 20, 2022

rust-highfive commented May 20, 2022

rust-highfive commented May 20, 2022

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung May 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented May 20, 2022

This comment has been minimized.

bors commented May 21, 2022

RalfJung commented May 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented May 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented May 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Dylan-DPC commented Jun 29, 2022

RalfJung commented Jun 29, 2022

nbdd0121 commented Jun 30, 2022

Amanieu commented Jul 1, 2022

bors commented Jul 1, 2022

bors commented Jul 2, 2022

bors commented Jul 2, 2022

rust-timer commented Jul 3, 2022

Footnotes

Mark-Simulacrum commented Jul 5, 2022

nbdd0121 commented Jul 5, 2022

RalfJung May 23, 2022 •

edited

RalfJung commented May 23, 2022 •

edited