"C-unwind" ABI is unsound with "-Cpanic=abort"

[RalfJung]

In the following code, test should explicitly be allowed to unwind:

#![feature(c_unwind)]

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

fn main() {
    unsafe { test(); }
}

However, when building this with -Cpanic=abort, we generate LLVM IR as follows:

; Function Attrs: nounwind nonlazybind
declare void @test() unnamed_addr #1

This means unwinding of test is UB, i.e., this is a soundness problem.

This most likely also affects #[unwind(allowed)], but that attribute is slated for removal anyway.

[apiraino]

Assigning P-medium since still a nightly-only feature. Discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

@rustbot label -I-prioritize +P-medium

[cratelyn]

I'll look into a fix for this, as the author of #76570, which introduced this issue. Thank you for flagging this @RalfJung.

[nagisa]

This is probably pretty hard to fix in a way that affects least amount of code negatively.

Any function that may at any point, directly or indirectly, end up calling a C-unwind function must not be annotated with a nounwind. It can't be known in general case that this is true, this knowledge could only be available at binary load time.

So AFAICT the only viable fixes here are:

• just don't annotate anything with nounwind, ever, -Cpanic=abort or not (we can still replace invokes with calls though);
• annotate leaf functions (functions that don't call anything else) as nounwind and then any functions that only call other nounwind functions as nounwind.

The latter is something LLVM is already capable of inferring by itself, so there's probably little value in doing it in rustc. Maybe later, once or if MIR optimisations start caring about this, I guess?

[RalfJung]

There's another possible fix: wrap imported extern "C-unwind" functions in a wrapper that turns unwinding into aborts. Rust code calling the function all goes through that wrapper. Then the nounwind is actually correct.

[nagisa]

We cannot do that, I think. Rust has no idea what the correct personality function would be to "catch" the unwinds from the other side of FFI. Also, isn't the intent that in situations where "C calls Rust calls C" unwinding across POF Rust frames is a supported use-case?

[bjorn3]

There is no "correct" or "wrong" personality function to catch unwinds. Any personality function is allowed to catch any non-forced unwind. The panic_abort crate could define a personality function that always aborts when not generating a backtrace. If the wrapper function or any function directly calling "C-unwind" without wrapper doesn't use nounwind, any attempt at unwinding through rust code would abort.

[RalfJung]

Also, isn't the intent that in situations where "C calls Rust calls C" unwinding across POF Rust frames is a supported use-case?

I don't think this is a support use-case with -Cpanic=abort. At least, the fact that nounwind is emitted aggressively under -Cpanic=abort kind of implies that this is not a support use-case.

[nagisa]

Any personality function is allowed to catch any non-forced unwind.

It is allowed to, but we don't necessarily want to expend the effort necessary to implement the personality function in a way that would allow catching e.g. all sorts of different unwinds that arbitrary runtime on the other end could generate. But I guess you're right in that we could just abort inside of the personality function itself.

I don't think this is a support use-case *with -Cpanic=abort".

Okay, I think that's a fair limitation to have. All of the binding crates that rely on "C-unwind" for their functionality would then need to always build with -Cpanic=unwind, which right now cannot be done stably or straightforwardly in Cargo.

[RalfJung]

All of the binding crates that rely on "C-unwind" for their functionality would then need to always build with -Cpanic=unwind, which right now cannot be done stably or straightforwardly in Cargo.

Yeah, for now they'd have to tell users "you must build your final binary with -Cpanic=unwind".
Rust in principle supports linking -Cpanic=unwind rlibs into a -Cpanic=abort crate tree (that's what we do for the standard library).

[BatmanAoD]

There's a fair amount of back-and-forth in #86155 between @RalfJung and @alexcrichton about whether or not this issue was resolved there. It looks to me like it was, and we can now close this (though it did not fully resolve the issue of mixed panic modes between crates). Do either of you disagree?

[alexcrichton]

That sounds accurate to me, the issue here as-is in the OP at least is fixed.

[RalfJung]

Is there an issue tracking the mixed panic mode problem? That certainly needs to be tracked somewhere.

[BatmanAoD]

I suppose not. For stabilization I think we're fairly set on just making it (documented) UB to have an unwind cross a mixed-panic-mode boundary, but it does seem worth tracking as a potential future improvement.

…

On Sat, May 7, 2022, 8:35 AM Ralf Jung ***@***.***> wrote: Is there an issue tracking the mixed panic mode problem? That certainly needs to be tracked somewhere. — Reply to this email directly, view it on GitHub <#83116 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARU4TYYM5A252VBAXNOSVLVIZ5URANCNFSM4ZFDHXYA> . You are receiving this because you commented.Message ID: ***@***.***>

[RalfJung]

For stabilization I think we're fairly set on just making it
(documented) UB

Oh, I didn't know that. We don't have a lot of "language primitive" UB so we should be careful here. I don't have a strong opinion on this particular one, but I think we should ensure that

• Miri can detect it
• it gets added to https://doc.rust-lang.org/reference/behavior-considered-undefined.html (it already talks about unwinding but this specific case seems different)

Also, isn't this a soundness bug? This can be all in safe code...

[BatmanAoD]

I definitely intend to add it to the undefined behavior section of the reference, as part of the documentation needed for stabilization.

And yes, it's a soundness bug, but effectively a very narrow case of an existing soundness bug that is otherwise fixed by the c-unwind feature, i.e., that it's UB to unwind at all from an extern <not Rust> function. To be fair, you could consider it new unsoundness since the "C-unwind" ABI is new, but at least that is opt-in, and the extern "C" soundness hole should be closed. Part of the rationale for not solving this remaining edge case prior to stabilization is that it's impossible to mix-and-match panic modes in Cargo, so users need to explicitly combine binaries that disagree on what panic means in order to actually have UB.

Unfortunately, I have no idea what would be involved in adding support for MIRI to detect this issue.

[RalfJung]

Having an issue to track this might be a good start.

[BatmanAoD]

I've opened a new issue here: #96926