Skip to content

Commit

Permalink
Rollup merge of rust-lang#128735 - jieyouxu:pr-120176-revive, r=cjgillot
Browse files Browse the repository at this point in the history
Add a special case for `CStr`/`CString` in the `improper_ctypes` lint

Revives rust-lang#120176. Just needed to bless a test and fix an argument, but seemed reasonable to me otherwise.

Instead of saying to "consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct", we now tell users to "Use `*const ffi::c_char` instead, and pass the value from `CStr::as_ptr()`" when the type involved is a `CStr` or a `CString`.

The suggestion is not made for `&mut CString` or `*mut CString`.

r? ```@cjgillot``` (since you were the reviewer of the original PR rust-lang#120176, but feel free to reroll)
  • Loading branch information
workingjubilee authored Aug 24, 2024
2 parents 1a70ad1 + b335ec9 commit 8423a2a
Show file tree
Hide file tree
Showing 7 changed files with 172 additions and 21 deletions.
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 @@ -985,6 +985,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 @@ -1213,7 +1221,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 @@ -1223,13 +1231,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 @@ -1239,7 +1247,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 @@ -1257,7 +1265,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 @@ -1277,7 +1285,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 @@ -1286,7 +1298,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 @@ -1308,6 +1320,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 @@ -1354,7 +1377,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 @@ -1378,7 +1401,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 @@ -1399,7 +1422,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 @@ -1469,9 +1492,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_tys, hdr) => {
let sig = sig_tys.with(hdr);
Expand All @@ -1485,7 +1508,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 @@ -1496,7 +1519,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 @@ -1619,7 +1642,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 @@ -673,6 +673,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

0 comments on commit 8423a2a

Please sign in to comment.