Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add a special case for CStr/CString in the improper_ctypes lint #128735

Merged
merged 1 commit into from
Aug 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,11 @@ lint_improper_ctypes_box = box cannot be represented as a single pointer
lint_improper_ctypes_char_help = consider using `u32` or `libc::wchar_t` instead

lint_improper_ctypes_char_reason = the `char` type has no C equivalent

lint_improper_ctypes_cstr_help =
consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
lint_improper_ctypes_cstr_reason = `CStr`/`CString` do not have a guaranteed layout

lint_improper_ctypes_dyn = trait objects have no C equivalent

lint_improper_ctypes_enum_repr_help =
Expand Down
54 changes: 39 additions & 15 deletions compiler/rustc_lint/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -984,6 +984,14 @@ struct ImproperCTypesVisitor<'a, 'tcx> {
mode: CItemKind,
}

/// Accumulator for recursive ffi type checking
struct CTypesVisitorState<'tcx> {
cache: FxHashSet<Ty<'tcx>>,
/// The original type being checked, before we recursed
/// to any other types it contains.
base_ty: Ty<'tcx>,
}

enum FfiResult<'tcx> {
FfiSafe,
FfiPhantom(Ty<'tcx>),
Expand Down Expand Up @@ -1212,7 +1220,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
/// Checks if the given field's type is "ffi-safe".
fn check_field_type_for_ffi(
&self,
cache: &mut FxHashSet<Ty<'tcx>>,
acc: &mut CTypesVisitorState<'tcx>,
field: &ty::FieldDef,
args: GenericArgsRef<'tcx>,
) -> FfiResult<'tcx> {
Expand All @@ -1222,13 +1230,13 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
.tcx
.try_normalize_erasing_regions(self.cx.param_env, field_ty)
.unwrap_or(field_ty);
self.check_type_for_ffi(cache, field_ty)
self.check_type_for_ffi(acc, field_ty)
}

/// Checks if the given `VariantDef`'s field types are "ffi-safe".
fn check_variant_for_ffi(
&self,
cache: &mut FxHashSet<Ty<'tcx>>,
acc: &mut CTypesVisitorState<'tcx>,
ty: Ty<'tcx>,
def: ty::AdtDef<'tcx>,
variant: &ty::VariantDef,
Expand All @@ -1238,7 +1246,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
let transparent_with_all_zst_fields = if def.repr().transparent() {
if let Some(field) = transparent_newtype_field(self.cx.tcx, variant) {
// Transparent newtypes have at most one non-ZST field which needs to be checked..
match self.check_field_type_for_ffi(cache, field, args) {
match self.check_field_type_for_ffi(acc, field, args) {
FfiUnsafe { ty, .. } if ty.is_unit() => (),
r => return r,
}
Expand All @@ -1256,7 +1264,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
// We can't completely trust `repr(C)` markings, so make sure the fields are actually safe.
let mut all_phantom = !variant.fields.is_empty();
for field in &variant.fields {
all_phantom &= match self.check_field_type_for_ffi(cache, field, args) {
all_phantom &= match self.check_field_type_for_ffi(acc, field, args) {
FfiSafe => false,
// `()` fields are FFI-safe!
FfiUnsafe { ty, .. } if ty.is_unit() => false,
Expand All @@ -1276,7 +1284,11 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {

/// Checks if the given type is "ffi-safe" (has a stable, well-defined
/// representation which can be exported to C code).
fn check_type_for_ffi(&self, cache: &mut FxHashSet<Ty<'tcx>>, ty: Ty<'tcx>) -> FfiResult<'tcx> {
fn check_type_for_ffi(
&self,
acc: &mut CTypesVisitorState<'tcx>,
ty: Ty<'tcx>,
) -> FfiResult<'tcx> {
use FfiResult::*;

let tcx = self.cx.tcx;
Expand All @@ -1285,7 +1297,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
// `struct S(*mut S);`.
// FIXME: A recursion limit is necessary as well, for irregular
// recursive types.
if !cache.insert(ty) {
if !acc.cache.insert(ty) {
return FfiSafe;
}

Expand All @@ -1307,6 +1319,17 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
}
match def.adt_kind() {
AdtKind::Struct | AdtKind::Union => {
if let Some(sym::cstring_type | sym::cstr_type) =
tcx.get_diagnostic_name(def.did())
&& !acc.base_ty.is_mutable_ptr()
{
return FfiUnsafe {
ty,
reason: fluent::lint_improper_ctypes_cstr_reason,
help: Some(fluent::lint_improper_ctypes_cstr_help),
};
}

if !def.repr().c() && !def.repr().transparent() {
return FfiUnsafe {
ty,
Expand Down Expand Up @@ -1353,7 +1376,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
};
}

self.check_variant_for_ffi(cache, ty, def, def.non_enum_variant(), args)
self.check_variant_for_ffi(acc, ty, def, def.non_enum_variant(), args)
}
AdtKind::Enum => {
if def.variants().is_empty() {
Expand All @@ -1377,7 +1400,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
if let Some(ty) =
repr_nullable_ptr(self.cx.tcx, self.cx.param_env, ty, self.mode)
{
return self.check_type_for_ffi(cache, ty);
return self.check_type_for_ffi(acc, ty);
}

return FfiUnsafe {
Expand All @@ -1398,7 +1421,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
};
}

match self.check_variant_for_ffi(cache, ty, def, variant, args) {
match self.check_variant_for_ffi(acc, ty, def, variant, args) {
FfiSafe => (),
r => return r,
}
Expand Down Expand Up @@ -1468,9 +1491,9 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
FfiSafe
}

ty::RawPtr(ty, _) | ty::Ref(_, ty, _) => self.check_type_for_ffi(cache, ty),
ty::RawPtr(ty, _) | ty::Ref(_, ty, _) => self.check_type_for_ffi(acc, ty),

ty::Array(inner_ty, _) => self.check_type_for_ffi(cache, inner_ty),
ty::Array(inner_ty, _) => self.check_type_for_ffi(acc, inner_ty),

ty::FnPtr(sig) => {
if self.is_internal_abi(sig.abi()) {
Expand All @@ -1483,7 +1506,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {

let sig = tcx.instantiate_bound_regions_with_erased(sig);
for arg in sig.inputs() {
match self.check_type_for_ffi(cache, *arg) {
match self.check_type_for_ffi(acc, *arg) {
FfiSafe => {}
r => return r,
}
Expand All @@ -1494,7 +1517,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
return FfiSafe;
}

self.check_type_for_ffi(cache, ret_ty)
self.check_type_for_ffi(acc, ret_ty)
}

ty::Foreign(..) => FfiSafe,
Expand Down Expand Up @@ -1617,7 +1640,8 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
return;
}

match self.check_type_for_ffi(&mut FxHashSet::default(), ty) {
let mut acc = CTypesVisitorState { cache: FxHashSet::default(), base_ty: ty };
match self.check_type_for_ffi(&mut acc, ty) {
FfiResult::FfiSafe => {}
FfiResult::FfiPhantom(ty) => {
self.emit_ffi_unsafe_type_lint(
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,7 @@ symbols! {
crate_visibility_modifier,
crt_dash_static: "crt-static",
csky_target_feature,
cstr_type,
cstring_type,
ctlz,
ctlz_nonzero,
Expand Down
1 change: 1 addition & 0 deletions library/core/src/ffi/c_str.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ use crate::{fmt, intrinsics, ops, slice, str};
/// [str]: prim@str "str"
#[derive(PartialEq, Eq, Hash)]
#[stable(feature = "core_c_str", since = "1.64.0")]
#[rustc_diagnostic_item = "cstr_type"]
#[rustc_has_incoherent_inherent_impls]
#[lang = "CStr"]
// `fn from` in `impl From<&CStr> for Box<CStr>` current implementation relies
Expand Down
12 changes: 6 additions & 6 deletions tests/ui/extern/extern-C-non-FFI-safe-arg-ice-52334.stderr
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
warning: `extern` fn uses type `[i8 or u8 (arch dependant)]`, which is not FFI-safe
warning: `extern` fn uses type `CStr`, which is not FFI-safe
--> $DIR/extern-C-non-FFI-safe-arg-ice-52334.rs:7:12
|
LL | type Foo = extern "C" fn(::std::ffi::CStr);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
|
= help: consider using a raw pointer instead
= note: slices have no C equivalent
= help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
= note: `CStr`/`CString` do not have a guaranteed layout
= note: `#[warn(improper_ctypes_definitions)]` on by default

warning: `extern` block uses type `[i8 or u8 (arch dependant)]`, which is not FFI-safe
warning: `extern` block uses type `CStr`, which is not FFI-safe
--> $DIR/extern-C-non-FFI-safe-arg-ice-52334.rs:10:18
|
LL | fn meh(blah: Foo);
| ^^^ not FFI-safe
|
= help: consider using a raw pointer instead
= note: slices have no C equivalent
= help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
= note: `CStr`/`CString` do not have a guaranteed layout
= note: `#[warn(improper_ctypes)]` on by default

warning: 2 warnings emitted
Expand Down
36 changes: 36 additions & 0 deletions tests/ui/lint/lint-ctypes-cstr.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#![crate_type = "lib"]
#![deny(improper_ctypes, improper_ctypes_definitions)]

use std::ffi::{CStr, CString};

extern "C" {
fn take_cstr(s: CStr);
//~^ ERROR `extern` block uses type `CStr`, which is not FFI-safe
//~| HELP consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
fn take_cstr_ref(s: &CStr);
//~^ ERROR `extern` block uses type `CStr`, which is not FFI-safe
//~| HELP consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
fn take_cstring(s: CString);
//~^ ERROR `extern` block uses type `CString`, which is not FFI-safe
//~| HELP consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
fn take_cstring_ref(s: &CString);
//~^ ERROR `extern` block uses type `CString`, which is not FFI-safe
//~| HELP consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`

fn no_special_help_for_mut_cstring(s: *mut CString);
//~^ ERROR `extern` block uses type `CString`, which is not FFI-safe
//~| HELP consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct

fn no_special_help_for_mut_cstring_ref(s: &mut CString);
//~^ ERROR `extern` block uses type `CString`, which is not FFI-safe
//~| HELP consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
}

extern "C" fn rust_take_cstr_ref(s: &CStr) {}
//~^ ERROR `extern` fn uses type `CStr`, which is not FFI-safe
//~| HELP consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
extern "C" fn rust_take_cstring(s: CString) {}
//~^ ERROR `extern` fn uses type `CString`, which is not FFI-safe
//~| HELP consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
extern "C" fn rust_no_special_help_for_mut_cstring(s: *mut CString) {}
extern "C" fn rust_no_special_help_for_mut_cstring_ref(s: &mut CString) {}
84 changes: 84 additions & 0 deletions tests/ui/lint/lint-ctypes-cstr.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
error: `extern` block uses type `CStr`, which is not FFI-safe
--> $DIR/lint-ctypes-cstr.rs:7:21
|
LL | fn take_cstr(s: CStr);
| ^^^^ not FFI-safe
|
= help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
= note: `CStr`/`CString` do not have a guaranteed layout
note: the lint level is defined here
--> $DIR/lint-ctypes-cstr.rs:2:9
|
LL | #![deny(improper_ctypes, improper_ctypes_definitions)]
| ^^^^^^^^^^^^^^^

error: `extern` block uses type `CStr`, which is not FFI-safe
--> $DIR/lint-ctypes-cstr.rs:10:25
|
LL | fn take_cstr_ref(s: &CStr);
| ^^^^^ not FFI-safe
|
= help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
= note: `CStr`/`CString` do not have a guaranteed layout

error: `extern` block uses type `CString`, which is not FFI-safe
--> $DIR/lint-ctypes-cstr.rs:13:24
|
LL | fn take_cstring(s: CString);
| ^^^^^^^ not FFI-safe
|
= help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
= note: `CStr`/`CString` do not have a guaranteed layout

error: `extern` block uses type `CString`, which is not FFI-safe
--> $DIR/lint-ctypes-cstr.rs:16:28
|
LL | fn take_cstring_ref(s: &CString);
| ^^^^^^^^ not FFI-safe
|
= help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
= note: `CStr`/`CString` do not have a guaranteed layout

error: `extern` block uses type `CString`, which is not FFI-safe
--> $DIR/lint-ctypes-cstr.rs:20:43
|
LL | fn no_special_help_for_mut_cstring(s: *mut CString);
| ^^^^^^^^^^^^ not FFI-safe
|
= help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
= note: this struct has unspecified layout

error: `extern` block uses type `CString`, which is not FFI-safe
--> $DIR/lint-ctypes-cstr.rs:24:47
|
LL | fn no_special_help_for_mut_cstring_ref(s: &mut CString);
| ^^^^^^^^^^^^ not FFI-safe
|
= help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
= note: this struct has unspecified layout

error: `extern` fn uses type `CStr`, which is not FFI-safe
--> $DIR/lint-ctypes-cstr.rs:29:37
|
LL | extern "C" fn rust_take_cstr_ref(s: &CStr) {}
| ^^^^^ not FFI-safe
|
= help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
= note: `CStr`/`CString` do not have a guaranteed layout
note: the lint level is defined here
--> $DIR/lint-ctypes-cstr.rs:2:26
|
LL | #![deny(improper_ctypes, improper_ctypes_definitions)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: `extern` fn uses type `CString`, which is not FFI-safe
--> $DIR/lint-ctypes-cstr.rs:32:36
|
LL | extern "C" fn rust_take_cstring(s: CString) {}
| ^^^^^^^ not FFI-safe
|
= help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
= note: `CStr`/`CString` do not have a guaranteed layout

error: aborting due to 8 previous errors

Loading