From 8ea1a53e19951444befeb3832b94bcd31a78068e Mon Sep 17 00:00:00 2001 From: Gurinder Singh Date: Fri, 20 Sep 2024 13:38:19 +0530 Subject: [PATCH] Add recursion limit to FFI safety lint Fixes stack overflow in the case of recursive types --- compiler/rustc_lint/messages.ftl | 3 +++ compiler/rustc_lint/src/errors.rs | 9 ++++++++ compiler/rustc_lint/src/types.rs | 23 +++++++++++++++---- tests/crashes/130310.rs | 15 ------------ .../improper-types-stack-overflow-130310.rs | 19 +++++++++++++++ ...mproper-types-stack-overflow-130310.stderr | 8 +++++++ 6 files changed, 58 insertions(+), 19 deletions(-) delete mode 100644 tests/crashes/130310.rs create mode 100644 tests/ui/lint/improper-types-stack-overflow-130310.rs create mode 100644 tests/ui/lint/improper-types-stack-overflow-130310.stderr diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl index 4aa86944a0b5c..5b96f0eb4e201 100644 --- a/compiler/rustc_lint/messages.ftl +++ b/compiler/rustc_lint/messages.ftl @@ -276,6 +276,9 @@ lint_extern_without_abi = extern declarations without an explicit ABI are deprec .label = ABI should be specified here .help = the default ABI is {$default_abi} +lint_ffi_safety_recursion_limit_reached = + reached recursion limit while checking type `{$ty}` for FFI safety + lint_for_loops_over_fallibles = for loop over {$article} `{$ref_prefix}{$ty}`. This is more readably written as an `if let` statement .suggestion = consider using `if let` to clear intent diff --git a/compiler/rustc_lint/src/errors.rs b/compiler/rustc_lint/src/errors.rs index d109a5c90305e..fa27fabc80f1d 100644 --- a/compiler/rustc_lint/src/errors.rs +++ b/compiler/rustc_lint/src/errors.rs @@ -1,6 +1,7 @@ use rustc_errors::codes::*; use rustc_errors::{Diag, EmissionGuarantee, SubdiagMessageOp, Subdiagnostic}; use rustc_macros::{Diagnostic, Subdiagnostic}; +use rustc_middle::ty::Ty; use rustc_session::lint::Level; use rustc_span::{Span, Symbol}; @@ -110,3 +111,11 @@ pub(crate) struct CheckNameUnknownTool<'a> { #[subdiagnostic] pub sub: RequestedLevel<'a>, } + +#[derive(Diagnostic)] +#[diag(lint_ffi_safety_recursion_limit_reached)] +pub(crate) struct RecursionLimitReached<'tcx> { + pub ty: Ty<'tcx>, + #[primary_span] + pub span: Span, +} diff --git a/compiler/rustc_lint/src/types.rs b/compiler/rustc_lint/src/types.rs index 900c496e0330d..99747fa08c2fb 100644 --- a/compiler/rustc_lint/src/types.rs +++ b/compiler/rustc_lint/src/types.rs @@ -18,6 +18,7 @@ use rustc_target::spec::abi::Abi as SpecAbi; use tracing::debug; use {rustc_ast as ast, rustc_attr as attr, rustc_hir as hir}; +use crate::errors::RecursionLimitReached; use crate::lints::{ AmbiguousWidePointerComparisons, AmbiguousWidePointerComparisonsAddrMetadataSuggestion, AmbiguousWidePointerComparisonsAddrSuggestion, AtomicOrderingFence, AtomicOrderingLoad, @@ -991,12 +992,15 @@ struct CTypesVisitorState<'tcx> { /// The original type being checked, before we recursed /// to any other types it contains. base_ty: Ty<'tcx>, + /// Number of times we recursed while checking the type + recursion_depth: usize, } enum FfiResult<'tcx> { FfiSafe, FfiPhantom(Ty<'tcx>), FfiUnsafe { ty: Ty<'tcx>, reason: DiagMessage, help: Option }, + RecursionLimitReached, } pub(crate) fn nonnull_optimization_guaranteed<'tcx>( @@ -1270,7 +1274,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { // `()` fields are FFI-safe! FfiUnsafe { ty, .. } if ty.is_unit() => false, FfiPhantom(..) => true, - r @ FfiUnsafe { .. } => return r, + r => return r, } } @@ -1296,12 +1300,19 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { // Protect against infinite recursion, for example // `struct S(*mut S);`. - // FIXME: A recursion limit is necessary as well, for irregular - // recursive types. if !acc.cache.insert(ty) { return FfiSafe; } + // Additional recursion check for more complex types like + // `struct A { v: *const A>, ... }` for which the + // cache check above won't be enough (fixes #130310) + if !tcx.recursion_limit().value_within_limit(acc.recursion_depth) { + return RecursionLimitReached; + } + + acc.recursion_depth += 1; + match *ty.kind() { ty::Adt(def, args) => { if let Some(boxed) = ty.boxed_ty() @@ -1644,7 +1655,8 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { return; } - let mut acc = CTypesVisitorState { cache: FxHashSet::default(), base_ty: ty }; + let mut acc = + CTypesVisitorState { cache: FxHashSet::default(), base_ty: ty, recursion_depth: 0 }; match self.check_type_for_ffi(&mut acc, ty) { FfiResult::FfiSafe => {} FfiResult::FfiPhantom(ty) => { @@ -1658,6 +1670,9 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { FfiResult::FfiUnsafe { ty, reason, help } => { self.emit_ffi_unsafe_type_lint(ty, sp, reason, help); } + FfiResult::RecursionLimitReached => { + self.cx.tcx.dcx().emit_err(RecursionLimitReached { ty, span: sp }); + } } } diff --git a/tests/crashes/130310.rs b/tests/crashes/130310.rs deleted file mode 100644 index d59dd39983c78..0000000000000 --- a/tests/crashes/130310.rs +++ /dev/null @@ -1,15 +0,0 @@ -//@ known-bug: rust-lang/rust#130310 - -use std::marker::PhantomData; - -#[repr(C)] -struct A { - a: *const A>, - p: PhantomData, -} - -extern "C" { - fn f(a: *const A<()>); -} - -fn main() {} diff --git a/tests/ui/lint/improper-types-stack-overflow-130310.rs b/tests/ui/lint/improper-types-stack-overflow-130310.rs new file mode 100644 index 0000000000000..bf44a28144605 --- /dev/null +++ b/tests/ui/lint/improper-types-stack-overflow-130310.rs @@ -0,0 +1,19 @@ +// Regression test for #130310 +// Tests that we do not fall into infinite +// recursion while checking FFI safety of +// recursive types like `A` below + +use std::marker::PhantomData; + +#[repr(C)] +struct A { + a: *const A>, // Recursive because of this field + p: PhantomData, +} + +extern "C" { + fn f(a: *const A<()>); + //~^ ERROR reached recursion limit while checking type `*const A<()>` for FFI safety +} + +fn main() {} diff --git a/tests/ui/lint/improper-types-stack-overflow-130310.stderr b/tests/ui/lint/improper-types-stack-overflow-130310.stderr new file mode 100644 index 0000000000000..e78cc93964a3a --- /dev/null +++ b/tests/ui/lint/improper-types-stack-overflow-130310.stderr @@ -0,0 +1,8 @@ +error: reached recursion limit while checking type `*const A<()>` for FFI safety + --> $DIR/improper-types-stack-overflow-130310.rs:15:13 + | +LL | fn f(a: *const A<()>); + | ^^^^^^^^^^^^ + +error: aborting due to 1 previous error +