Skip to content

Commit

Permalink
Rollup merge of rust-lang#108611 - davidtwco:issue-94223-external-abi…
Browse files Browse the repository at this point in the history
…-fn-ptr-in-internal-abi-fn, r=jackh726

lint/ctypes: ext. abi fn-ptr in internal abi fn

Fixes rust-lang#94223.

- In the improper ctypes lint, instead of skipping functions with internal ABIs, check that the signature doesn't contain any fn-ptr types with external ABIs that aren't FFI-safe.
- When computing the ABI for fn-ptr types, remove an `unwrap` that assumed FFI-safe types in foreign fn-ptr types.
  - I'm not certain that this is the correct approach.
  • Loading branch information
Dylan-DPC authored May 2, 2023
2 parents 7b99493 + 3e7ca3e commit b3bc5f8
Show file tree
Hide file tree
Showing 7 changed files with 333 additions and 22 deletions.
166 changes: 149 additions & 17 deletions compiler/rustc_lint/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1250,17 +1250,45 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
}
}

fn check_foreign_fn(&mut self, def_id: LocalDefId, decl: &hir::FnDecl<'_>) {
/// Check if a function's argument types and result type are "ffi-safe".
///
/// Argument types and the result type are checked for functions with external ABIs.
/// For functions with internal ABIs, argument types and the result type are walked to find
/// fn-ptr types that have external ABIs, as these still need checked.
fn check_maybe_foreign_fn(
&mut self,
abi: SpecAbi,
def_id: LocalDefId,
decl: &'tcx hir::FnDecl<'_>,
) {
let sig = self.cx.tcx.fn_sig(def_id).subst_identity();
let sig = self.cx.tcx.erase_late_bound_regions(sig);

let is_internal_abi = self.is_internal_abi(abi);
let check_ty = |this: &mut ImproperCTypesVisitor<'a, 'tcx>,
hir_ty: &'tcx hir::Ty<'_>,
ty: Ty<'tcx>,
is_return_type: bool| {
// If this function has an external ABI, then its arguments and return type should be
// checked..
if !is_internal_abi {
this.check_type_for_ffi_and_report_errors(hir_ty.span, ty, false, is_return_type);
return;
}

// ..but if this function has an internal ABI, then search the argument or return type
// for any fn-ptr types with external ABI, which should be checked..
for (fn_ptr_ty, span) in this.find_fn_ptr_ty_with_external_abi(hir_ty, ty) {
this.check_type_for_ffi_and_report_errors(span, fn_ptr_ty, false, is_return_type);
}
};

for (input_ty, input_hir) in iter::zip(sig.inputs(), decl.inputs) {
self.check_type_for_ffi_and_report_errors(input_hir.span, *input_ty, false, false);
check_ty(self, input_hir, *input_ty, false);
}

if let hir::FnRetTy::Return(ref ret_hir) = decl.output {
let ret_ty = sig.output();
self.check_type_for_ffi_and_report_errors(ret_hir.span, ret_ty, false, true);
check_ty(self, ret_hir, sig.output(), true);
}
}

Expand All @@ -1275,28 +1303,134 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
SpecAbi::Rust | SpecAbi::RustCall | SpecAbi::RustIntrinsic | SpecAbi::PlatformIntrinsic
)
}

/// Find any fn-ptr types with external ABIs in `ty`.
///
/// For example, `Option<extern "C" fn()>` returns `extern "C" fn()`
fn find_fn_ptr_ty_with_external_abi(
&self,
hir_ty: &hir::Ty<'tcx>,
ty: Ty<'tcx>,
) -> Vec<(Ty<'tcx>, Span)> {
struct FnPtrFinder<'parent, 'a, 'tcx> {
visitor: &'parent ImproperCTypesVisitor<'a, 'tcx>,
spans: Vec<Span>,
tys: Vec<Ty<'tcx>>,
}

impl<'parent, 'a, 'tcx> hir::intravisit::Visitor<'_> for FnPtrFinder<'parent, 'a, 'tcx> {
fn visit_ty(&mut self, ty: &'_ hir::Ty<'_>) {
debug!(?ty);
if let hir::TyKind::BareFn(hir::BareFnTy { abi, .. }) = ty.kind
&& !self.visitor.is_internal_abi(*abi)
{
self.spans.push(ty.span);
}

hir::intravisit::walk_ty(self, ty)
}
}

impl<'vis, 'a, 'tcx> ty::visit::TypeVisitor<TyCtxt<'tcx>> for FnPtrFinder<'vis, 'a, 'tcx> {
type BreakTy = Ty<'tcx>;

fn visit_ty(&mut self, ty: Ty<'tcx>) -> ControlFlow<Self::BreakTy> {
if let ty::FnPtr(sig) = ty.kind() && !self.visitor.is_internal_abi(sig.abi()) {
self.tys.push(ty);
}

ty.super_visit_with(self)
}
}

let mut visitor = FnPtrFinder { visitor: &*self, spans: Vec::new(), tys: Vec::new() };
self.cx
.tcx
.try_normalize_erasing_regions(self.cx.param_env, ty)
.unwrap_or(ty)
.visit_with(&mut visitor);
hir::intravisit::Visitor::visit_ty(&mut visitor, hir_ty);

iter::zip(visitor.tys.drain(..), visitor.spans.drain(..)).collect()
}
}

impl<'tcx> LateLintPass<'tcx> for ImproperCTypesDeclarations {
fn check_foreign_item(&mut self, cx: &LateContext<'_>, it: &hir::ForeignItem<'_>) {
fn check_foreign_item(&mut self, cx: &LateContext<'tcx>, it: &hir::ForeignItem<'tcx>) {
let mut vis = ImproperCTypesVisitor { cx, mode: CItemKind::Declaration };
let abi = cx.tcx.hir().get_foreign_abi(it.hir_id());

if !vis.is_internal_abi(abi) {
match it.kind {
hir::ForeignItemKind::Fn(ref decl, _, _) => {
vis.check_foreign_fn(it.owner_id.def_id, decl);
}
hir::ForeignItemKind::Static(ref ty, _) => {
vis.check_foreign_static(it.owner_id, ty.span);
}
hir::ForeignItemKind::Type => (),
match it.kind {
hir::ForeignItemKind::Fn(ref decl, _, _) => {
vis.check_maybe_foreign_fn(abi, it.owner_id.def_id, decl);
}
hir::ForeignItemKind::Static(ref ty, _) if !vis.is_internal_abi(abi) => {
vis.check_foreign_static(it.owner_id, ty.span);
}
hir::ForeignItemKind::Static(..) | hir::ForeignItemKind::Type => (),
}
}
}

impl ImproperCTypesDefinitions {
fn check_ty_maybe_containing_foreign_fnptr<'tcx>(
&mut self,
cx: &LateContext<'tcx>,
hir_ty: &'tcx hir::Ty<'_>,
ty: Ty<'tcx>,
) {
let mut vis = ImproperCTypesVisitor { cx, mode: CItemKind::Definition };
for (fn_ptr_ty, span) in vis.find_fn_ptr_ty_with_external_abi(hir_ty, ty) {
vis.check_type_for_ffi_and_report_errors(span, fn_ptr_ty, true, false);
}
}
}

/// `ImproperCTypesDefinitions` checks items outside of foreign items (e.g. stuff that isn't in
/// `extern "C" { }` blocks):
///
/// - `extern "<abi>" fn` definitions are checked in the same way as the
/// `ImproperCtypesDeclarations` visitor checks functions if `<abi>` is external (e.g. "C").
/// - All other items which contain types (e.g. other functions, struct definitions, etc) are
/// checked for extern fn-ptrs with external ABIs.
impl<'tcx> LateLintPass<'tcx> for ImproperCTypesDefinitions {
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) {
match item.kind {
hir::ItemKind::Static(ty, ..)
| hir::ItemKind::Const(ty, ..)
| hir::ItemKind::TyAlias(ty, ..) => {
self.check_ty_maybe_containing_foreign_fnptr(
cx,
ty,
cx.tcx.type_of(item.owner_id).subst_identity(),
);
}
// See `check_fn`..
hir::ItemKind::Fn(..) => {}
// See `check_field_def`..
hir::ItemKind::Union(..) | hir::ItemKind::Struct(..) | hir::ItemKind::Enum(..) => {}
// Doesn't define something that can contain a external type to be checked.
hir::ItemKind::Impl(..)
| hir::ItemKind::TraitAlias(..)
| hir::ItemKind::Trait(..)
| hir::ItemKind::OpaqueTy(..)
| hir::ItemKind::GlobalAsm(..)
| hir::ItemKind::ForeignMod { .. }
| hir::ItemKind::Mod(..)
| hir::ItemKind::Macro(..)
| hir::ItemKind::Use(..)
| hir::ItemKind::ExternCrate(..) => {}
}
}

fn check_field_def(&mut self, cx: &LateContext<'tcx>, field: &'tcx hir::FieldDef<'tcx>) {
self.check_ty_maybe_containing_foreign_fnptr(
cx,
field.ty,
cx.tcx.type_of(field.def_id).subst_identity(),
);
}

fn check_fn(
&mut self,
cx: &LateContext<'tcx>,
Expand All @@ -1315,9 +1449,7 @@ impl<'tcx> LateLintPass<'tcx> for ImproperCTypesDefinitions {
};

let mut vis = ImproperCTypesVisitor { cx, mode: CItemKind::Definition };
if !vis.is_internal_abi(abi) {
vis.check_foreign_fn(id, decl);
}
vis.check_maybe_foreign_fn(abi, id, decl);
}
}

Expand Down
10 changes: 6 additions & 4 deletions compiler/rustc_target/src/abi/call/x86_64.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,9 @@ fn reg_component(cls: &[Option<Class>], i: &mut usize, size: Size) -> Option<Reg
}
}

fn cast_target(cls: &[Option<Class>], size: Size) -> CastTarget {
fn cast_target(cls: &[Option<Class>], size: Size) -> Option<CastTarget> {
let mut i = 0;
let lo = reg_component(cls, &mut i, size).unwrap();
let lo = reg_component(cls, &mut i, size)?;
let offset = Size::from_bytes(8) * (i as u64);
let mut target = CastTarget::from(lo);
if size > offset {
Expand All @@ -164,7 +164,7 @@ fn cast_target(cls: &[Option<Class>], size: Size) -> CastTarget {
}
}
assert_eq!(reg_component(cls, &mut i, Size::ZERO), None);
target
Some(target)
}

const MAX_INT_REGS: usize = 6; // RDI, RSI, RDX, RCX, R8, R9
Expand Down Expand Up @@ -227,7 +227,9 @@ where
// split into sized chunks passed individually
if arg.layout.is_aggregate() {
let size = arg.layout.size;
arg.cast_to(cast_target(cls, size))
if let Some(cast_target) = cast_target(cls, size) {
arg.cast_to(cast_target);
}
} else {
arg.extend_integer_width_to(32);
}
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/abi/foreign/foreign-fn-with-byval.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// run-pass
#![allow(improper_ctypes)]
#![allow(improper_ctypes, improper_ctypes_definitions)]

// ignore-wasm32-bare no libc to test ffi with

Expand Down
8 changes: 8 additions & 0 deletions tests/ui/abi/issue-94223.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// check-pass
#![allow(improper_ctypes_definitions)]
#![crate_type = "lib"]

// Check that computing the fn abi for `bad`, with a external ABI fn ptr that is not FFI-safe, does
// not ICE.

pub fn bad(f: extern "C" fn([u8])) {}
1 change: 1 addition & 0 deletions tests/ui/hashmap/hashmap-memory.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// run-pass

#![allow(improper_ctypes_definitions)]
#![allow(non_camel_case_types)]
#![allow(dead_code)]
#![allow(unused_mut)]
Expand Down
42 changes: 42 additions & 0 deletions tests/ui/lint/lint-ctypes-94223.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#![crate_type = "lib"]
#![deny(improper_ctypes_definitions)]

pub fn bad(f: extern "C" fn([u8])) {}
//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe

pub fn bad_twice(f: Result<extern "C" fn([u8]), extern "C" fn([u8])>) {}
//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe
//~^^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe

struct BadStruct(extern "C" fn([u8]));
//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe

enum BadEnum {
A(extern "C" fn([u8])),
//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe
}

enum BadUnion {
A(extern "C" fn([u8])),
//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe
}

type Foo = extern "C" fn([u8]);
//~^ ERROR `extern` fn uses type `[u8]`, which is not FFI-safe

pub struct FfiUnsafe;

#[allow(improper_ctypes_definitions)]
extern "C" fn f(_: FfiUnsafe) {
unimplemented!()
}

pub static BAD: extern "C" fn(FfiUnsafe) = f;
//~^ ERROR `extern` fn uses type `FfiUnsafe`, which is not FFI-safe

pub static BAD_TWICE: Result<extern "C" fn(FfiUnsafe), extern "C" fn(FfiUnsafe)> = Ok(f);
//~^ ERROR `extern` fn uses type `FfiUnsafe`, which is not FFI-safe
//~^^ ERROR `extern` fn uses type `FfiUnsafe`, which is not FFI-safe

pub const BAD_CONST: extern "C" fn(FfiUnsafe) = f;
//~^ ERROR `extern` fn uses type `FfiUnsafe`, which is not FFI-safe
Loading

0 comments on commit b3bc5f8

Please sign in to comment.