improper_ctypes fires for &mut T, &T, *const T and *mut T (when T: Sized)

[CryZe]

I'm not sure if this is intentional, but I haven't found any discussion regarding this. The improper_ctypes lint fires on pointer types when T is not FFI safe. However pointers have defined layout that is always FFI safe, independent of what the pointer is pointing to. It certainly seems valid to point out the pointer won't be able to be dereferenced by C. This should probably be a clippy lint though. Rust itself shouldn't fire false positives like this.

[sfackler]

Is T: Sized?

[CryZe]

Yes, here's the playground example

pub struct Foo {
    _x: i32,
}

#[no_mangle]
pub extern "C" fn foo(_: *const Foo) {}

warning: `extern` fn uses type `Foo`, which is not FFI-safe
 --> src/lib.rs:6:26
  |
6 | pub extern "C" fn foo(_: *const Foo) {}
  |                          ^^^^^^^^^^ not FFI-safe
  |
  = note: `#[warn(improper_ctypes)]` on by default
  = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
  = note: this struct has unspecified layout
note: type defined here
 --> src/lib.rs:1:1
  |
1 | / pub struct Foo {
2 | |     _x: i32,
3 | | }
  | |_^

[hanna-kruppe]

I am pretty sure this is intentional (or was, when this part of the lint was implemented), but that doesn't mean it can't change. This is a scenario where reasonable interpretations of the lint disagree with each other:

• On the one hand, pointers to sized types are like C pointers, so they have defined memory layout and call ABI. If the intent of the code is that the other side of the FFI just passes the pointer around, then everything is fine and the warning is a false positive.
• On the other hand, the pointee type is not FFI-safe and code behind the FFI cannot interact with that type correctly. If the intent is that the other side of the FFI dereference the pointer, then most likely the author of the Rust side made a mistake (e.g., missed a repr(C) annotation) and the warning is a true positive.

It is not obvious to me which side the improper_ctypes lint is supposed to serve. It's true that rustc lints ought to err on the side of fewer false positives, but on the other hand there are a lot of potential bugs (and very subtle ones, as things may appear to work correctly) that the current behavior catches, and false positives can be avoided by using *mut c_void or pointers to extern types for APIs where the pointee type is supposed to be opaque for the other side of the FFI.

[CryZe]

I believe this could possibly be a separate lint (and with a different message too Pointer can't be dereferenced by C or so). Because otherwise improper_ctypes is likely to be disabled module or crate wide (as seen in quiche for example), which means actual calling convention mistakes won't show up anymore.

[hanna-kruppe]

Splitting up into two different lints seems interesting (as long as both stay in rustc and enabled by default).

It comes with an interesting backwards compatibility question, though. We probably wouldn't want projects that used allow(improper_ctypes) to see warnings they opted out of again just because we shuffled around where they're emitted. So would we turn improper_ctypes into a lint group encompassing both new lints? Of course, that means that any old code (and any new code that copy-pastes or is written by someone with ingrained habits) will automatically disable both lints at once even if it would benefit from the "ABI is unspecified" subset.

[Rantanen]

Is there any way to mark a type (instead of the method) as "I know this is improper but don't warn me about it"?

#[repr(transparent)]
#[allow(improper_ctypes)]
pub struct Opaque<T>(T);

pub extern "C" fn foo(_: *const Opaque<String>) {}

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=ccd4a6a4c205c17d84e17eb6d1ceac4f

The following does work, which surprised me because PhantomData alone is not FFI safe and this uses a tuple struct with two fields and claims to be repr(transparent) which I thought would hate structs with two fields, even if one of them was PhantomData...

#[repr(transparent)]
pub struct OpaquePtr<T>(*mut std::ffi::c_void, std::marker::PhantomData<T>);
pub extern "C" fn bar(_: OpaquePtr<String>) {}

I'm just looking for ways to describe the "FFI just passes the pointer around" intent in the type system. I realize the use of this might be noisy at scale given there's no automatic conversion between the types and would need manual wrapping and unwrapping.

[CryZe]

Well idea would be to mark it with #[allow(opaque_c_ptr)] or so. Just passing *const String should be entirely fine (as long as you truly treat it as opaque).

[hanna-kruppe]

Is there any way to mark a type (instead of the method) as "I know this is improper but don't warn me about it"?

I believe this doesn't work because the lint message is associated with the function rather than the type(s) which the lint finds objectionable. I don't know how difficult it would be to support this, I guess it would be something of a special case at best.

The following does work, which surprised me because PhantomData alone is not FFI safe and this uses a tuple struct with two fields and claims to be repr(transparent) which I thought would hate structs with two fields, even if one of them was PhantomData...

repr(transparent) explicitly allows extra fields that have size zero and alignment one, to enable the use of PhantomData and similar tricks for more type safety in FFI interfaces. PhantomData alone is not FFI-safe because it's a ZST which is not a standard C construct and it's not clear (or wasn't when the lint was written) to what degree we support the GNU extensions that introduce ZSTs to C. *const PhantomData<String> works, though it's a bit obscure, so your OpaquePtr<T> newtype seems best to me.

[SimonSapin]

#65134 adds thousands of new warnings to Servo. The pervasive pattern is that we have many unsafe extern fn functions that take *const Foo parameters. The functions are extern so that they can be called from C++. The Foo Rust type is not FFI-safe, but in this case the pointer to it is FFI-safe because it is an opaque pointer as far as C++ code is concerned.

Changing the parameter type to *const c_void and adding an as cast near the top of the function body “fixes” the warning, but I feel it’s really not an improvement. Type-safety is diminished for no benefit that I can see.

Now I’m tempted to add #![allow(improper_ctypes)] to the parts of the crate that do the most FFI in order to unblock the compiler upgrade. But that feels very unproductive.

Please consider reverting #65134, or making raw pointers unconditionally FFI-safe.

CC @rust-lang/lang

[shepmaster]

It comes with an interesting backwards compatibility question, though

Does it really? It’s nightly-only still, isn’t it? That level of churn (in the small timeframe present) seems acceptable in nightly.

[SimonSapin]

@rkruppe Since you filed #65867, what do you think is a good way to deal with pointers that point to a non-FFI-safe type but are opaque as far as the foreign code is concerned?

[hanna-kruppe]

I'm confused, #65867 is unrelated (as discussed there, probably wasn't even a legit issue, just a temporary issue with #65134, that I thought was pre-existing and independent from that PR). You might be thinking of #19834 that actually predates and motivated #65134? But I didn't file that, FWIW.

In any case, I think we should revert #65134 right now so we can discuss how to handle the unexpected fallout without pressure. Options raised so far include:

• considering pointers to sized types FFI-safe
• splitting up the lint in defined ABI vs fully usable from C code
• some wrapper types (of questionable ergonomics)

It's a messy subject and I'd rather not see people add allow(improper_ctypes) en masse while we discuss these and other options.

[SimonSapin]

Since the PR description for #65134 contains “Fixes #65867” I assumed it was an implementation of what that issue proposed.

I'd rather not see people add allow(improper_ctypes) en masse

I’d rather not add it either, but at this point it’s the only practical way (without changing all parameters to *mut c_void and adding casts everywhere, losing type safety in the process) to upgrade Servo to a Nightly more recent than #65134. I’ve asked the compiler team to wait for that upgrade before landing #66344.

[davidtwco]

Since the PR description for #65134 contains “Fixes #65867” I assumed it was an implementation of what that issue proposed.

I added this to the PR when @rkruppe filed #65867 since it was, as @rkruppe said, a temporary issue within #65134 that it resolved.

---

Just to clarify, #65134 didn't change the behaviour of the lint so that would trigger for *const Foo where it did not before, just that the lint, as it was before, would also apply to extern "C" fns.

Perhaps a good compromise would be to stop looking at the Foo in *const Foo if the lint is checking an extern "C" fn (and not just a forward declaration in a extern "C" { }). This way we'd still benefit from having the improper ctypes lint apply to the extern "C" fns, but for this problematic case it will behave as before, giving us time to decide how to proceed?

[Centril]

cc #66373

Perhaps a good compromise would be to stop looking at the Foo in *const Foo if the lint is checking an extern "C" fn (and not just a forward declaration in a extern "C" { }). This way we'd still benefit from having the improper ctypes lint apply to the extern "C" fns, but for this problematic case it will behave as before, giving us time to decide how to proceed?

@davidtwco Not sure how invasive that would be in the lint code but as an interim solution, that seems reasonable. Would you be up for implementing the tweak?

[pnkfelix]

Nominating for discussion at T-compiler meeting, largely to figure out whether we should ASAP revert PR #65134 as a whole, or if we should/can wait for a more nuanced solution/compromise.

[hanna-kruppe]

I think we should revert ASAP, and I say this as someone who was very eager to get the PR merged. It's likely quicker to revert than to land any tweaks, especially since we have to consider the possibility that whatever tweaks get done quickly/first will turn out to be insufficient. Plus, after the revert, a PR with the tweaked lint can get a try build either for a crater run or for affected projects like Servo to try out the tweaked rules. That will allow us to judge how effective the tweaks are without time pressure.

(Also feel free to @ me in the T-compiler meeting when discussing this, I'm free in that time slot.)

[SimonSapin]

@davidtwco Yes, that compromise would take care of Servo’s new warnings and unblock the compiler upgrade. However it effectively means there are two different definitions of "FFI-safe". I don’t know how desirable that is.

[SimonSapin]

(Reverting #65134 would also unblock Servo.)

[elichai]

I want to take the devil's advocate here.

I think that if someone is passing pointers to C with the point of being opaque the right thing to do is use void pointer.
I think the likely scenario in someone passing pointer to a non-FFI type to extern "C" is just that he forgot the repr(C) attribute, not that he's doing opaque magic.

But I would agree as a compromise that we can not lint this when it's a extern "C" implementation and not declaration (that way the declaration might use void pointers but to skip the casting the implementation might use the non-FFI pointers). But I really think that disabling this lint in extern declarations is a bad idea.

[CryZe]

Then you lose all type safety on Rust's side. I'm passing stuff like Option<&mut RustType> as parameters and return values in my FFI. In fact my C API comprised of 250 functions or so is almost entirely safe (only C strings are unsafe, and even those can be made safe). Also I'm automatically generating safe high level bindings from the C API, and that wouldn't really be possible anymore if everything was just *const c_void. I don't think throwing out all safety / type safety is a good long term solution.

[elichai]

@CryZe I edited my post.
why lose type safety?

#[repr(transparant)]
struct Opaque<T>(T);

extern "C" { fn c_func(ptr: *mut c_void) -> size_t};

pub fn wrapper<T>(ptr: &mut Opaque<T>) -> usize {
unsafe {c_func(ptr as *mut _ as *mut c_void)} as usize
}

Writing this code shows you know what you're doing, you know you're using it as an opaque pointer because you're using void pointer. if you don't use void pointer i'd argue it's more likely you just forgot putting a repr or you have no idea what you're doing.

[SimonSapin]

Boiling the ocean is not realistic. Many projects already exists with hundreds of FFI bindings. Migrating every extern function to have a wrapper function and every parameter to have a wrapper type would be an enormous amount of work.

And that proposal does lose type safety. If you use bindgen, all the other side sees is still void* everywhere. void* is not the only opaque pointer. C has struct Foo; syntax for this purpose.

[elichai]

Boiling the ocean is not realistic. Many projects already exits with hundreds of FFI bindings. Migrating every extern function to have a wrapper function and every parameter to have a wrapper type would be an enormous amount of work.

And that proposal does lose type safety. If you use bindgen, all the other side sees is still void* everywhere. void* is not the only opaque pointer. C has struct Foo; syntax for this purpose.

I understand, I'm just trying to also say that usually people pass stuff to C code by const pointers and not by value, so not checking the repr on those arguments means the lint lose most of it's usefulness for the regular user (who needs the lint the most)

[danielhenrymantilla]

Hence the need for two separate lints: it depends on the use case.

[danielhenrymantilla]

As a matter of fact, the following code triggers the lint:

#[no_mangle] pub
unsafe
extern "C"
fn foo (_: *const Foo) {}

pub
struct Foo {
    _private: [u8; 0],
}

Given that no members of Foo are public, and that it is Sized and being passed by pointer, this ought to be accepted: it is literally impossible to misuse.

[pnkfelix]

The T-lang design team discussed this matter in their weekly meeting about a week ago. (Sorry I didn't take the time to write a summary until now.)

• (some of the points I write here are redundant restatements of comments others have made. I just want to make sure I explicitly acknowledge the issues in this summary.)

Some cases caught by improper_ctypes are situations where the given types are not well-defined under the calling-convention protocol according to the ABI. We definitely want to catch those, regardless of whether one is talking about extern "C" definitions or declarations.

Other cases caught by the lint are just trying to help the developer avoid footguns. Specifically, as pointed out earlier by @rkruppe, the improper_ctypes lint is being made to serve multiple purposes that do not always agree in terms of their result. It can depend on the intent of the author: Are they just passing the pointer around as an opaque thing? Or is the C code on the other end going to attempt to actually dereference the pointer.

• Concretely, passing &Foo (where Foo is a sized non-repr-C struct) to foreign code declared via extern "C" { fn foreign(x: &Foo); } may or may not be correct depending on what the receiver is going to do with it.

A related point brought up during the lang team meeting: before, when the lint only covered extern "C" declarations, the conditions being checked were about data that was flowing out to foreign code. Now, with the lint covering extern "C" definitions that are themselves written in Rust, we are talking about data that is flowing in to Rust code.

• Receiving a &Foo in a locally defined function extern "C" fn riir_local(x: &Foo) { ... } makes total sense, since it is Rust code that will being doing the dereferencing.

---

Overall, there was a general consensus that we do not want to just reland PR # #65134 (not that anyone on this thread was currently suggesting doing that). A more nuanced approach seems justified here.

@rkruppe has laid out some options previously. I think the options the T-lang design team was most interested in exploring were either:

• Making the improper_ctypes lint behave differently depending on whether its looking at a extern "C" declaration or an extern "C" definition (which is a category that subsumes what @davidtwco has proposed here,
• And/or: Make two distinct lints, as @rkruppe has proposed informally, that correspond to whether the inputs simply meet requirements to well-defined by the ABI, versus whether the inputs are intended to be fully-usable from C code (and thus are subject to more invasive analysis). This presumably could allow developers to express their intent more precisely for specific extern "C" declarations.

---

The T-lang design team did not come to a conclusion as to which of the above two approaches we thought would be best. (And I have generalized the statements here to make it clear that its possible that we will want a mix of the two.)

Would @davidtwco and/or @rkruppe be interested in trying to explore the design space of the above two points, and come back with a concrete proposal after doing such exploration?

[hanna-kruppe]

FWIW, personally I have come to believe that we should definitely split up the lint (with or without the other option) since it's useful independently of this specific issue, but I don't think I can commit the time to explore this further or write up a more detailed proposal any time soon.

_{(If I get pinged to discuss that in Discord or Zulip, I probably won't be able to resist being nerd-sniped. Please use this knowledge responsibly :P)}

[davidtwco]

I’m happy to perform any implementation required here, but I don’t have any particular opinions on the semantics - I just spotted #19834 as work needing done.

I’m happy to proceed with making the lint behave differently (for this case) depending on extern “C” fn or extern “C” {}; or with a separate lint if someone has opinions on what each lint would do.

[pnkfelix]

triage: since PR #66378 has landed, I am reclassifying this as P-medium.

[pnkfelix]

removing nomination label. T-compiler does not need to discuss this, at least not until the follow-up work is done.

[davidtwco]

Closing, #72700 fixes this.