Skip to content

Commit 6a10920

Browse files
committed
Auto merge of rust-lang#97235 - nbdd0121:unwind, r=Amanieu
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
2 parents 0075bb4 + 0cf28dc commit 6a10920

27 files changed

+366
-39
lines changed

compiler/rustc_interface/src/passes.rs

+1
Original file line numberDiff line numberDiff line change
@@ -972,6 +972,7 @@ fn analysis(tcx: TyCtxt<'_>, (): ()) -> Result<()> {
972972
if !tcx.sess.opts.debugging_opts.thir_unsafeck {
973973
rustc_mir_transform::check_unsafety::check_unsafety(tcx, def_id);
974974
}
975+
tcx.ensure().has_ffi_unwind_calls(def_id);
975976

976977
if tcx.hir().body_const_context(def_id).is_some() {
977978
tcx.ensure()

compiler/rustc_lint_defs/src/builtin.rs

+40
Original file line numberDiff line numberDiff line change
@@ -3231,6 +3231,7 @@ declare_lint_pass! {
32313231
UNEXPECTED_CFGS,
32323232
DEPRECATED_WHERE_CLAUSE_LOCATION,
32333233
TEST_UNSTABLE_LINT,
3234+
FFI_UNWIND_CALLS,
32343235
]
32353236
}
32363237

@@ -3896,3 +3897,42 @@ declare_lint! {
38963897
"this unstable lint is only for testing",
38973898
@feature_gate = sym::test_unstable_lint;
38983899
}
3900+
3901+
declare_lint! {
3902+
/// The `ffi_unwind_calls` lint detects calls to foreign functions or function pointers with
3903+
/// `C-unwind` or other FFI-unwind ABIs.
3904+
///
3905+
/// ### Example
3906+
///
3907+
/// ```rust,ignore (need FFI)
3908+
/// #![feature(ffi_unwind_calls)]
3909+
/// #![feature(c_unwind)]
3910+
///
3911+
/// # mod impl {
3912+
/// # #[no_mangle]
3913+
/// # pub fn "C-unwind" fn foo() {}
3914+
/// # }
3915+
///
3916+
/// extern "C-unwind" {
3917+
/// fn foo();
3918+
/// }
3919+
///
3920+
/// fn bar() {
3921+
/// unsafe { foo(); }
3922+
/// let ptr: unsafe extern "C-unwind" fn() = foo;
3923+
/// unsafe { ptr(); }
3924+
/// }
3925+
/// ```
3926+
///
3927+
/// {{produces}}
3928+
///
3929+
/// ### Explanation
3930+
///
3931+
/// For crates containing such calls, if they are compiled with `-C panic=unwind` then the
3932+
/// produced library cannot be linked with crates compiled with `-C panic=abort`. For crates
3933+
/// that desire this ability it is therefore necessary to avoid such calls.
3934+
pub FFI_UNWIND_CALLS,
3935+
Allow,
3936+
"call to foreign functions or function pointers with FFI-unwind ABI",
3937+
@feature_gate = sym::c_unwind;
3938+
}

compiler/rustc_metadata/src/creader.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -748,7 +748,7 @@ impl<'a> CrateLoader<'a> {
748748
if !data.is_panic_runtime() {
749749
self.sess.err(&format!("the crate `{}` is not a panic runtime", name));
750750
}
751-
if data.panic_strategy() != desired_strategy {
751+
if data.required_panic_strategy() != Some(desired_strategy) {
752752
self.sess.err(&format!(
753753
"the crate `{}` does not have the panic \
754754
strategy `{}`",

compiler/rustc_metadata/src/dependency_format.rs

+11-11
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,6 @@ use rustc_middle::ty::TyCtxt;
6060
use rustc_session::config::CrateType;
6161
use rustc_session::cstore::CrateDepKind;
6262
use rustc_session::cstore::LinkagePreference::{self, RequireDynamic, RequireStatic};
63-
use rustc_target::spec::PanicStrategy;
6463

6564
pub(crate) fn calculate(tcx: TyCtxt<'_>) -> Dependencies {
6665
tcx.sess
@@ -367,14 +366,19 @@ fn verify_ok(tcx: TyCtxt<'_>, list: &[Linkage]) {
367366
prev_name, cur_name
368367
));
369368
}
370-
panic_runtime = Some((cnum, tcx.panic_strategy(cnum)));
369+
panic_runtime = Some((
370+
cnum,
371+
tcx.required_panic_strategy(cnum).unwrap_or_else(|| {
372+
bug!("cannot determine panic strategy of a panic runtime");
373+
}),
374+
));
371375
}
372376
}
373377

374378
// If we found a panic runtime, then we know by this point that it's the
375379
// only one, but we perform validation here that all the panic strategy
376380
// compilation modes for the whole DAG are valid.
377-
if let Some((cnum, found_strategy)) = panic_runtime {
381+
if let Some((runtime_cnum, found_strategy)) = panic_runtime {
378382
let desired_strategy = sess.panic_strategy();
379383

380384
// First up, validate that our selected panic runtime is indeed exactly
@@ -384,7 +388,7 @@ fn verify_ok(tcx: TyCtxt<'_>, list: &[Linkage]) {
384388
"the linked panic runtime `{}` is \
385389
not compiled with this crate's \
386390
panic strategy `{}`",
387-
tcx.crate_name(cnum),
391+
tcx.crate_name(runtime_cnum),
388392
desired_strategy.desc()
389393
));
390394
}
@@ -397,18 +401,14 @@ fn verify_ok(tcx: TyCtxt<'_>, list: &[Linkage]) {
397401
if let Linkage::NotLinked = *linkage {
398402
continue;
399403
}
400-
if desired_strategy == PanicStrategy::Abort {
401-
continue;
402-
}
403404
let cnum = CrateNum::new(i + 1);
404-
if tcx.is_compiler_builtins(cnum) {
405+
if cnum == runtime_cnum || tcx.is_compiler_builtins(cnum) {
405406
continue;
406407
}
407408

408-
let found_strategy = tcx.panic_strategy(cnum);
409-
if desired_strategy != found_strategy {
409+
if let Some(found_strategy) = tcx.required_panic_strategy(cnum) && desired_strategy != found_strategy {
410410
sess.err(&format!(
411-
"the crate `{}` is compiled with the \
411+
"the crate `{}` requires \
412412
panic strategy `{}` which is \
413413
incompatible with this crate's \
414414
strategy of `{}`",

compiler/rustc_metadata/src/rmeta/decoder.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -1758,8 +1758,8 @@ impl CrateMetadata {
17581758
self.dep_kind.with_lock(|dep_kind| *dep_kind = f(*dep_kind))
17591759
}
17601760

1761-
pub(crate) fn panic_strategy(&self) -> PanicStrategy {
1762-
self.root.panic_strategy
1761+
pub(crate) fn required_panic_strategy(&self) -> Option<PanicStrategy> {
1762+
self.root.required_panic_strategy
17631763
}
17641764

17651765
pub(crate) fn needs_panic_runtime(&self) -> bool {

compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ provide! { <'tcx> tcx, def_id, other, cdata,
246246
has_global_allocator => { cdata.root.has_global_allocator }
247247
has_panic_handler => { cdata.root.has_panic_handler }
248248
is_profiler_runtime => { cdata.root.profiler_runtime }
249-
panic_strategy => { cdata.root.panic_strategy }
249+
required_panic_strategy => { cdata.root.required_panic_strategy }
250250
panic_in_drop_strategy => { cdata.root.panic_in_drop_strategy }
251251
extern_crate => {
252252
let r = *cdata.extern_crate.lock();

compiler/rustc_metadata/src/rmeta/encoder.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -665,7 +665,7 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
665665
triple: tcx.sess.opts.target_triple.clone(),
666666
hash: tcx.crate_hash(LOCAL_CRATE),
667667
stable_crate_id: tcx.def_path_hash(LOCAL_CRATE.as_def_id()).stable_crate_id(),
668-
panic_strategy: tcx.sess.panic_strategy(),
668+
required_panic_strategy: tcx.required_panic_strategy(LOCAL_CRATE),
669669
panic_in_drop_strategy: tcx.sess.opts.debugging_opts.panic_in_drop,
670670
edition: tcx.sess.edition(),
671671
has_global_allocator: tcx.has_global_allocator(LOCAL_CRATE),

compiler/rustc_metadata/src/rmeta/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ pub(crate) struct CrateRoot {
217217
extra_filename: String,
218218
hash: Svh,
219219
stable_crate_id: StableCrateId,
220-
panic_strategy: PanicStrategy,
220+
required_panic_strategy: Option<PanicStrategy>,
221221
panic_in_drop_strategy: PanicStrategy,
222222
edition: Edition,
223223
has_global_allocator: bool,

compiler/rustc_middle/src/query/mod.rs

+6-2
Original file line numberDiff line numberDiff line change
@@ -1356,9 +1356,13 @@ rustc_queries! {
13561356
desc { "query a crate is `#![profiler_runtime]`" }
13571357
separate_provide_extern
13581358
}
1359-
query panic_strategy(_: CrateNum) -> PanicStrategy {
1359+
query has_ffi_unwind_calls(key: LocalDefId) -> bool {
1360+
desc { |tcx| "check if `{}` contains FFI-unwind calls", tcx.def_path_str(key.to_def_id()) }
1361+
cache_on_disk_if { true }
1362+
}
1363+
query required_panic_strategy(_: CrateNum) -> Option<PanicStrategy> {
13601364
fatal_cycle
1361-
desc { "query a crate's configured panic strategy" }
1365+
desc { "query a crate's required panic strategy" }
13621366
separate_provide_extern
13631367
}
13641368
query panic_in_drop_strategy(_: CrateNum) -> PanicStrategy {

compiler/rustc_mir_transform/src/abort_unwinding_calls.rs

+1-6
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
use crate::MirPass;
22
use rustc_ast::InlineAsmOptions;
3-
use rustc_hir::def::DefKind;
43
use rustc_middle::mir::*;
54
use rustc_middle::ty::layout;
65
use rustc_middle::ty::{self, TyCtxt};
@@ -31,11 +30,7 @@ impl<'tcx> MirPass<'tcx> for AbortUnwindingCalls {
3130

3231
// We don't simplify the MIR of constants at this time because that
3332
// namely results in a cyclic query when we call `tcx.type_of` below.
34-
let is_function = match kind {
35-
DefKind::Fn | DefKind::AssocFn | DefKind::Ctor(..) => true,
36-
_ => tcx.is_closure(def_id),
37-
};
38-
if !is_function {
33+
if !kind.is_fn_like() {
3934
return;
4035
}
4136

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
use rustc_hir::def_id::{CrateNum, LocalDefId, LOCAL_CRATE};
2+
use rustc_middle::mir::*;
3+
use rustc_middle::ty::layout;
4+
use rustc_middle::ty::query::Providers;
5+
use rustc_middle::ty::{self, TyCtxt};
6+
use rustc_session::lint::builtin::FFI_UNWIND_CALLS;
7+
use rustc_target::spec::abi::Abi;
8+
use rustc_target::spec::PanicStrategy;
9+
10+
fn abi_can_unwind(abi: Abi) -> bool {
11+
use Abi::*;
12+
match abi {
13+
C { unwind }
14+
| System { unwind }
15+
| Cdecl { unwind }
16+
| Stdcall { unwind }
17+
| Fastcall { unwind }
18+
| Vectorcall { unwind }
19+
| Thiscall { unwind }
20+
| Aapcs { unwind }
21+
| Win64 { unwind }
22+
| SysV64 { unwind } => unwind,
23+
PtxKernel
24+
| Msp430Interrupt
25+
| X86Interrupt
26+
| AmdGpuKernel
27+
| EfiApi
28+
| AvrInterrupt
29+
| AvrNonBlockingInterrupt
30+
| CCmseNonSecureCall
31+
| Wasm
32+
| RustIntrinsic
33+
| PlatformIntrinsic
34+
| Unadjusted => false,
35+
Rust | RustCall | RustCold => true,
36+
}
37+
}
38+
39+
// Check if the body of this def_id can possibly leak a foreign unwind into Rust code.
40+
fn has_ffi_unwind_calls(tcx: TyCtxt<'_>, local_def_id: LocalDefId) -> bool {
41+
debug!("has_ffi_unwind_calls({local_def_id:?})");
42+
43+
// Only perform check on functions because constants cannot call FFI functions.
44+
let def_id = local_def_id.to_def_id();
45+
let kind = tcx.def_kind(def_id);
46+
if !kind.is_fn_like() {
47+
return false;
48+
}
49+
50+
let body = &*tcx.mir_built(ty::WithOptConstParam::unknown(local_def_id)).borrow();
51+
52+
let body_ty = tcx.type_of(def_id);
53+
let body_abi = match body_ty.kind() {
54+
ty::FnDef(..) => body_ty.fn_sig(tcx).abi(),
55+
ty::Closure(..) => Abi::RustCall,
56+
ty::Generator(..) => Abi::Rust,
57+
_ => span_bug!(body.span, "unexpected body ty: {:?}", body_ty),
58+
};
59+
let body_can_unwind = layout::fn_can_unwind(tcx, Some(def_id), body_abi);
60+
61+
// Foreign unwinds cannot leak past functions that themselves cannot unwind.
62+
if !body_can_unwind {
63+
return false;
64+
}
65+
66+
let mut tainted = false;
67+
68+
for block in body.basic_blocks() {
69+
if block.is_cleanup {
70+
continue;
71+
}
72+
let Some(terminator) = &block.terminator else { continue };
73+
let TerminatorKind::Call { func, .. } = &terminator.kind else { continue };
74+
75+
let ty = func.ty(body, tcx);
76+
let sig = ty.fn_sig(tcx);
77+
78+
// Rust calls cannot themselves create foreign unwinds.
79+
if let Abi::Rust | Abi::RustCall | Abi::RustCold = sig.abi() {
80+
continue;
81+
};
82+
83+
let fn_def_id = match ty.kind() {
84+
ty::FnPtr(_) => None,
85+
&ty::FnDef(def_id, _) => {
86+
// Rust calls cannot themselves create foreign unwinds.
87+
if !tcx.is_foreign_item(def_id) {
88+
continue;
89+
}
90+
Some(def_id)
91+
}
92+
_ => bug!("invalid callee of type {:?}", ty),
93+
};
94+
95+
if layout::fn_can_unwind(tcx, fn_def_id, sig.abi()) && abi_can_unwind(sig.abi()) {
96+
// We have detected a call that can possibly leak foreign unwind.
97+
//
98+
// Because the function body itself can unwind, we are not aborting this function call
99+
// upon unwind, so this call can possibly leak foreign unwind into Rust code if the
100+
// panic runtime linked is panic-abort.
101+
102+
let lint_root = body.source_scopes[terminator.source_info.scope]
103+
.local_data
104+
.as_ref()
105+
.assert_crate_local()
106+
.lint_root;
107+
let span = terminator.source_info.span;
108+
109+
tcx.struct_span_lint_hir(FFI_UNWIND_CALLS, lint_root, span, |lint| {
110+
let msg = match fn_def_id {
111+
Some(_) => "call to foreign function with FFI-unwind ABI",
112+
None => "call to function pointer with FFI-unwind ABI",
113+
};
114+
let mut db = lint.build(msg);
115+
db.span_label(span, msg);
116+
db.emit();
117+
});
118+
119+
tainted = true;
120+
}
121+
}
122+
123+
tainted
124+
}
125+
126+
fn required_panic_strategy(tcx: TyCtxt<'_>, cnum: CrateNum) -> Option<PanicStrategy> {
127+
assert_eq!(cnum, LOCAL_CRATE);
128+
129+
if tcx.is_panic_runtime(LOCAL_CRATE) {
130+
return Some(tcx.sess.panic_strategy());
131+
}
132+
133+
if tcx.sess.panic_strategy() == PanicStrategy::Abort {
134+
return Some(PanicStrategy::Abort);
135+
}
136+
137+
for def_id in tcx.hir().body_owners() {
138+
if tcx.has_ffi_unwind_calls(def_id) {
139+
// Given that this crate is compiled in `-C panic=unwind`, the `AbortUnwindingCalls`
140+
// MIR pass will not be run on FFI-unwind call sites, therefore a foreign exception
141+
// can enter Rust through these sites.
142+
//
143+
// On the other hand, crates compiled with `-C panic=abort` expects that all Rust
144+
// functions cannot unwind (whether it's caused by Rust panic or foreign exception),
145+
// and this expectation mismatch can cause unsoundness (#96926).
146+
//
147+
// To address this issue, we enforce that if FFI-unwind calls are used in a crate
148+
// compiled with `panic=unwind`, then the final panic strategy must be `panic=unwind`.
149+
// This will ensure that no crates will have wrong unwindability assumption.
150+
//
151+
// It should be noted that it is okay to link `panic=unwind` into a `panic=abort`
152+
// program if it contains no FFI-unwind calls. In such case foreign exception can only
153+
// enter Rust in a `panic=abort` crate, which will lead to an abort. There will also
154+
// be no exceptions generated from Rust, so the assumption which `panic=abort` crates
155+
// make, that no Rust function can unwind, indeed holds for crates compiled with
156+
// `panic=unwind` as well. In such case this function returns `None`, indicating that
157+
// the crate does not require a particular final panic strategy, and can be freely
158+
// linked to crates with either strategy (we need such ability for libstd and its
159+
// dependencies).
160+
return Some(PanicStrategy::Unwind);
161+
}
162+
}
163+
164+
// This crate can be linked with either runtime.
165+
None
166+
}
167+
168+
pub(crate) fn provide(providers: &mut Providers) {
169+
*providers = Providers { has_ffi_unwind_calls, required_panic_strategy, ..*providers };
170+
}

compiler/rustc_mir_transform/src/lib.rs

+5
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ pub mod dump_mir;
5959
mod early_otherwise_branch;
6060
mod elaborate_box_derefs;
6161
mod elaborate_drops;
62+
mod ffi_unwind_calls;
6263
mod function_item_references;
6364
mod generator;
6465
mod inline;
@@ -98,6 +99,7 @@ pub fn provide(providers: &mut Providers) {
9899
check_unsafety::provide(providers);
99100
check_packed_ref::provide(providers);
100101
coverage::query::provide(providers);
102+
ffi_unwind_calls::provide(providers);
101103
shim::provide(providers);
102104
*providers = Providers {
103105
mir_keys,
@@ -223,6 +225,9 @@ fn mir_const<'tcx>(
223225
}
224226
}
225227

228+
// has_ffi_unwind_calls query uses the raw mir, so make sure it is run.
229+
tcx.ensure().has_ffi_unwind_calls(def.did);
230+
226231
let mut body = tcx.mir_built(def).steal();
227232

228233
rustc_middle::mir::dump_mir(tcx, None, "mir_map", &0, &body, |_, _| Ok(()));

0 commit comments

Comments
 (0)