Skip to content

Fix FFI-unwind unsoundness with mixed panic mode #97235

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 9 commits into from
Jul 2, 2022
1 change: 1 addition & 0 deletions compiler/rustc_interface/src/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if tcx.hir().body_const_context(def_id).is_some() {
tcx.ensure()
Expand Down
40 changes: 40 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3230,6 +3230,7 @@ declare_lint_pass! {
UNEXPECTED_CFGS,
DEPRECATED_WHERE_CLAUSE_LOCATION,
TEST_UNSTABLE_LINT,
FFI_UNWIND_CALLS,
]
}

Expand Down Expand Up @@ -3895,3 +3896,42 @@ declare_lint! {
"this unstable lint is only for testing",
@feature_gate = sym::test_unstable_lint;
}

declare_lint! {
/// The `ffi_unwind_calls` lint detects calls to foreign functions or function pointers with
/// `C-unwind` or other FFI-unwind ABIs.
///
/// ### Example
///
/// ```rust,ignore (need FFI)
/// #![feature(ffi_unwind_calls)]
/// #![feature(c_unwind)]
///
/// # mod impl {
/// # #[no_mangle]
/// # pub fn "C-unwind" fn foo() {}
/// # }
///
/// extern "C-unwind" {
/// fn foo();
/// }
///
/// fn bar() {
/// unsafe { foo(); }
/// let ptr: unsafe extern "C-unwind" fn() = foo;
/// unsafe { ptr(); }
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// For crates containing such calls, if they are compiled with `-C panic=unwind` then the
/// produced library cannot be linked with crates compiled with `-C panic=abort`. For crates
/// that desire this ability it is therefore necessary to avoid such calls.
pub FFI_UNWIND_CALLS,
Allow,
"call to foreign functions or function pointers with FFI-unwind ABI",
@feature_gate = sym::c_unwind;
}
2 changes: 1 addition & 1 deletion compiler/rustc_metadata/src/creader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,7 @@ impl<'a> CrateLoader<'a> {
if !data.is_panic_runtime() {
self.sess.err(&format!("the crate `{}` is not a panic runtime", name));
}
if data.panic_strategy() != desired_strategy {
if data.required_panic_strategy() != Some(desired_strategy) {
self.sess.err(&format!(
"the crate `{}` does not have the panic \
strategy `{}`",
Expand Down
22 changes: 11 additions & 11 deletions compiler/rustc_metadata/src/dependency_format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ use rustc_middle::ty::TyCtxt;
use rustc_session::config::CrateType;
use rustc_session::cstore::CrateDepKind;
use rustc_session::cstore::LinkagePreference::{self, RequireDynamic, RequireStatic};
use rustc_target::spec::PanicStrategy;

pub(crate) fn calculate(tcx: TyCtxt<'_>) -> Dependencies {
tcx.sess
Expand Down Expand Up @@ -367,14 +366,19 @@ fn verify_ok(tcx: TyCtxt<'_>, list: &[Linkage]) {
prev_name, cur_name
));
}
panic_runtime = Some((cnum, tcx.panic_strategy(cnum)));
panic_runtime = Some((
cnum,
tcx.required_panic_strategy(cnum).unwrap_or_else(|| {
bug!("cannot determine panic strategy of a panic runtime");
}),
));
}
}

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

// First up, validate that our selected panic runtime is indeed exactly
Expand All @@ -384,7 +388,7 @@ fn verify_ok(tcx: TyCtxt<'_>, list: &[Linkage]) {
"the linked panic runtime `{}` is \
not compiled with this crate's \
panic strategy `{}`",
tcx.crate_name(cnum),
tcx.crate_name(runtime_cnum),
desired_strategy.desc()
));
}
Expand All @@ -397,18 +401,14 @@ fn verify_ok(tcx: TyCtxt<'_>, list: &[Linkage]) {
if let Linkage::NotLinked = *linkage {
continue;
}
if desired_strategy == PanicStrategy::Abort {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let cnum = CrateNum::new(i + 1);
if tcx.is_compiler_builtins(cnum) {
if cnum == runtime_cnum || tcx.is_compiler_builtins(cnum) {
continue;
}

let found_strategy = tcx.panic_strategy(cnum);
if desired_strategy != found_strategy {
if let Some(found_strategy) = tcx.required_panic_strategy(cnum) && desired_strategy != found_strategy {
sess.err(&format!(
"the crate `{}` is compiled with the \
"the crate `{}` requires \
panic strategy `{}` which is \
incompatible with this crate's \
strategy of `{}`",
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_metadata/src/rmeta/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1759,8 +1759,8 @@ impl CrateMetadata {
self.dep_kind.with_lock(|dep_kind| *dep_kind = f(*dep_kind))
}

pub(crate) fn panic_strategy(&self) -> PanicStrategy {
self.root.panic_strategy
pub(crate) fn required_panic_strategy(&self) -> Option<PanicStrategy> {
self.root.required_panic_strategy
}

pub(crate) fn needs_panic_runtime(&self) -> bool {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ provide! { <'tcx> tcx, def_id, other, cdata,
has_global_allocator => { cdata.root.has_global_allocator }
has_panic_handler => { cdata.root.has_panic_handler }
is_profiler_runtime => { cdata.root.profiler_runtime }
panic_strategy => { cdata.root.panic_strategy }
required_panic_strategy => { cdata.root.required_panic_strategy }
panic_in_drop_strategy => { cdata.root.panic_in_drop_strategy }
extern_crate => {
let r = *cdata.extern_crate.lock();
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_metadata/src/rmeta/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,7 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
triple: tcx.sess.opts.target_triple.clone(),
hash: tcx.crate_hash(LOCAL_CRATE),
stable_crate_id: tcx.def_path_hash(LOCAL_CRATE.as_def_id()).stable_crate_id(),
panic_strategy: tcx.sess.panic_strategy(),
required_panic_strategy: tcx.required_panic_strategy(LOCAL_CRATE),
panic_in_drop_strategy: tcx.sess.opts.debugging_opts.panic_in_drop,
edition: tcx.sess.edition(),
has_global_allocator: tcx.has_global_allocator(LOCAL_CRATE),
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_metadata/src/rmeta/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ pub(crate) struct CrateRoot {
extra_filename: String,
hash: Svh,
stable_crate_id: StableCrateId,
panic_strategy: PanicStrategy,
required_panic_strategy: Option<PanicStrategy>,
panic_in_drop_strategy: PanicStrategy,
edition: Edition,
has_global_allocator: bool,
Expand Down
8 changes: 6 additions & 2 deletions compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1365,9 +1365,13 @@ rustc_queries! {
desc { "query a crate is `#![profiler_runtime]`" }
separate_provide_extern
}
query panic_strategy(_: CrateNum) -> PanicStrategy {
query has_ffi_unwind_calls(key: LocalDefId) -> bool {
desc { |tcx| "check if `{}` contains FFI-unwind calls", tcx.def_path_str(key.to_def_id()) }
cache_on_disk_if { true }
}
query required_panic_strategy(_: CrateNum) -> Option<PanicStrategy> {
fatal_cycle
desc { "query a crate's configured panic strategy" }
desc { "query a crate's required panic strategy" }
separate_provide_extern
}
query panic_in_drop_strategy(_: CrateNum) -> PanicStrategy {
Expand Down
7 changes: 1 addition & 6 deletions compiler/rustc_mir_transform/src/abort_unwinding_calls.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use crate::MirPass;
use rustc_ast::InlineAsmOptions;
use rustc_hir::def::DefKind;
use rustc_middle::mir::*;
use rustc_middle::ty::layout;
use rustc_middle::ty::{self, TyCtxt};
Expand Down Expand Up @@ -31,11 +30,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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_ => tcx.is_closure(def_id),
};
if !is_function {
if !kind.is_fn_like() {
return;
}

Expand Down
170 changes: 170 additions & 0 deletions compiler/rustc_mir_transform/src/ffi_unwind_calls.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
use rustc_hir::def_id::{CrateNum, LocalDefId, LOCAL_CRATE};
use rustc_middle::mir::*;
use rustc_middle::ty::layout;
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::{self, TyCtxt};
use rustc_session::lint::builtin::FFI_UNWIND_CALLS;
use rustc_target::spec::abi::Abi;
use rustc_target::spec::PanicStrategy;

fn abi_can_unwind(abi: Abi) -> bool {
use Abi::*;
match abi {
C { unwind }
| System { unwind }
| Cdecl { unwind }
| Stdcall { unwind }
| Fastcall { unwind }
| Vectorcall { unwind }
| Thiscall { unwind }
| Aapcs { unwind }
| Win64 { unwind }
| SysV64 { unwind } => unwind,
PtxKernel
| Msp430Interrupt
| X86Interrupt
| AmdGpuKernel
| EfiApi
| AvrInterrupt
| AvrNonBlockingInterrupt
| CCmseNonSecureCall
| Wasm
| RustIntrinsic
| PlatformIntrinsic
| Unadjusted => false,
Rust | RustCall | RustCold => true,
}
}

// Check if the body of this def_id can possibly leak a foreign unwind into Rust code.
fn has_ffi_unwind_calls(tcx: TyCtxt<'_>, local_def_id: LocalDefId) -> bool {
debug!("has_ffi_unwind_calls({local_def_id:?})");

// Only perform check on functions because constants cannot call FFI functions.
let def_id = local_def_id.to_def_id();
let kind = tcx.def_kind(def_id);
if !kind.is_fn_like() {
return false;
}

let body = &*tcx.mir_built(ty::WithOptConstParam::unknown(local_def_id)).borrow();

let body_ty = tcx.type_of(def_id);
let body_abi = match body_ty.kind() {
ty::FnDef(..) => body_ty.fn_sig(tcx).abi(),
ty::Closure(..) => Abi::RustCall,
ty::Generator(..) => Abi::Rust,
_ => span_bug!(body.span, "unexpected body ty: {:?}", body_ty),
};
let body_can_unwind = layout::fn_can_unwind(tcx, Some(def_id), body_abi);

// Foreign unwinds cannot leak past functions that themselves cannot unwind.
if !body_can_unwind {
return false;
}

let mut tainted = false;

for block in body.basic_blocks() {
if block.is_cleanup {
continue;
}
let Some(terminator) = &block.terminator else { continue };
let TerminatorKind::Call { func, .. } = &terminator.kind else { continue };

let ty = func.ty(body, tcx);
let sig = ty.fn_sig(tcx);

// Rust calls cannot themselves create foreign unwinds.
if let Abi::Rust | Abi::RustCall | Abi::RustCold = sig.abi() {
continue;
};

let fn_def_id = match ty.kind() {
ty::FnPtr(_) => None,
&ty::FnDef(def_id, _) => {
// Rust calls cannot themselves create foreign unwinds.
if !tcx.is_foreign_item(def_id) {
continue;
}
Some(def_id)
}
_ => bug!("invalid callee of type {:?}", ty),
};

if layout::fn_can_unwind(tcx, fn_def_id, sig.abi()) && abi_can_unwind(sig.abi()) {
// We have detected a call that can possibly leak foreign unwind.
//
// Because the function body itself can unwind, we are not aborting this function call
// upon unwind, so this call can possibly leak foreign unwind into Rust code if the
// panic runtime linked is panic-abort.

let lint_root = body.source_scopes[terminator.source_info.scope]
.local_data
.as_ref()
.assert_crate_local()
.lint_root;
let span = terminator.source_info.span;

tcx.struct_span_lint_hir(FFI_UNWIND_CALLS, lint_root, span, |lint| {
let msg = match fn_def_id {
Some(_) => "call to foreign function with FFI-unwind ABI",
None => "call to function pointer with FFI-unwind ABI",
};
let mut db = lint.build(msg);
db.span_label(span, msg);
db.emit();
});

tainted = true;
}
}

tainted
}

fn required_panic_strategy(tcx: TyCtxt<'_>, cnum: CrateNum) -> Option<PanicStrategy> {
assert_eq!(cnum, LOCAL_CRATE);

if tcx.is_panic_runtime(LOCAL_CRATE) {
return Some(tcx.sess.panic_strategy());
}

if tcx.sess.panic_strategy() == PanicStrategy::Abort {
return Some(PanicStrategy::Abort);
}

for def_id in tcx.hir().body_owners() {
if tcx.has_ffi_unwind_calls(def_id) {
// Given that this crate is compiled in `-C panic=unwind`, the `AbortUnwindingCalls`
// MIR pass will not be run on FFI-unwind call sites, therefore a foreign exception
// can enter Rust through these sites.
//
// On the other hand, crates compiled with `-C panic=abort` expects that all Rust
// functions cannot unwind (whether it's caused by Rust panic or foreign exception),
// and this expectation mismatch can cause unsoundness (#96926).
//
// To address this issue, we enforce that if FFI-unwind calls are used in a crate
// compiled with `panic=unwind`, then the final panic strategy must be `panic=unwind`.
// This will ensure that no crates will have wrong unwindability assumption.
//
// It should be noted that it is okay to link `panic=unwind` into a `panic=abort`
// program if it contains no FFI-unwind calls. In such case foreign exception can only
// enter Rust in a `panic=abort` crate, which will lead to an abort. There will also
// be no exceptions generated from Rust, so the assumption which `panic=abort` crates
// make, that no Rust function can unwind, indeed holds for crates compiled with
// `panic=unwind` as well. In such case this function returns `None`, indicating that
// the crate does not require a particular final panic strategy, and can be freely
// linked to crates with either strategy (we need such ability for libstd and its
// dependencies).
return Some(PanicStrategy::Unwind);
}
}

// This crate can be linked with either runtime.
None
}

pub(crate) fn provide(providers: &mut Providers) {
*providers = Providers { has_ffi_unwind_calls, required_panic_strategy, ..*providers };
}
5 changes: 5 additions & 0 deletions compiler/rustc_mir_transform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ mod dest_prop;
pub mod dump_mir;
mod early_otherwise_branch;
mod elaborate_drops;
mod ffi_unwind_calls;
mod function_item_references;
mod generator;
mod inline;
Expand Down Expand Up @@ -96,6 +97,7 @@ pub fn provide(providers: &mut Providers) {
check_unsafety::provide(providers);
check_packed_ref::provide(providers);
coverage::query::provide(providers);
ffi_unwind_calls::provide(providers);
shim::provide(providers);
*providers = Providers {
mir_keys,
Expand Down Expand Up @@ -221,6 +223,9 @@ fn mir_const<'tcx>(
}
}

// has_ffi_unwind_calls query uses the raw mir, so make sure it is run.
tcx.ensure().has_ffi_unwind_calls(def.did);

let mut body = tcx.mir_built(def).steal();

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