Result / Option of non-null / non-zero types are not FFI-safe

[gnzlbg]

I am writing an FFI wrapper for a library that has a function returning an integer error code:

extern "C" fn foo() -> libc::c_int;

The error code of foo is 0 on success and non-zero on error. As suggested by @kennytm , I can do the following dance to obtain a NonZeroU{width} type that's suitable to hold a c_int (note: NonZero<libc::c_int> does not work anymore, and the NonZeroI{width} variants have been removed, so more extra dance is then necessary to convert the unsigned integers to a libc::c_int to be able to compare this with with std error codes like libc::EINVAL):

pub trait NonZeroUInt { type NonZero; }
impl NonZeroUInt for i32 { type NonZero = core::num::NonZeroU32; }
impl NonZeroUInt for i64 { type NonZero = core::num::NonZeroU64; }
impl NonZeroUInt for isize { type NonZero = core::num::NonZeroUsize; }

pub type NonZeroCInt = <libc::c_int as NonZeroUInt>::NonZero;

However, when I then try to use Result in FFI:

extern "C" fn foo() -> Result<(), NonZeroCInt>;

I get an error stating that Result is not FFI-safe. This type is really similar to an Option<&'static 32> which is FFI-safe: an enum with 2 variants, one is zero sized, and the other has niches.

Option<NonNull<*mut i32>>, Option<NonZeroU32>, and Result<(), NonZeroU32> are not, however, FFI safe.

Is this an oversight ?
cc @eddyb @SimonSapin

[steveklabnik]

I think this isn't an oversight; Option has been special-cased here to be identical in representation to the original type, but non-repr-c enums, like Result, have no layout guarantees whatsoever.

I guess in theory we could do this for Result<(), T> where T is a NonZero type, but I imagine that's an RFC.

[gnzlbg]

You are right, I've posted this in the appropriate UCG thread: rust-lang/unsafe-code-guidelines#10 (comment)

[eddyb]

@steveklabnik that's not exactly the case, the shape is what's checked, so if you have your own Option-like type, the lint will allow it.
We could also allow variants with ZST fields.

But regardless of Result, I think the Option examples should all work.
What's missing is recursing through transparent newtypes, or just using layout to tell that the inner type is non-null.

cc @RalfJung @nikomatsakis @rkruppe @nagisa

[steveklabnik]

Sure, I was talking about the spec, not the lint. Unless I’m mistaken and the lint is the spec. I thought only Option was truly guaranteed even if we do always do the optimization for any option-like type. I could be wrong!

…

On Nov 3, 2018, at 10:51 AM, Eduard-Mihai Burtescu ***@***.***> wrote: Reopened #55615. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[eddyb]

@steveklabnik Ah, alright, I agree then, we should spec this (via UCG, I guess).
That would inform the lint, but the lint doesn't need to be perfect.

[RalfJung]

This kind of stuff was being discussed recently at rust-lang/unsafe-code-guidelines#10

EDIT: Oh, you already got that link. Well never mind then.^^

[gnzlbg]

That would inform the lint, but the lint doesn't need to be perfect.

So there are two orthogonal issues convoluted in my OP. First, is whether the niche optimizations for these types of enums are guaranteed or not. If they are not guaranteed, then using these in C FFI is pretty much undefined behavior independently of what Rust currently does. That should be clarified in the UCG and the current discussion seems to suggest that these should be guaranteed. What does Rust currently do here? Does this unconditionally perform these optimizations?

Second, is whether the warning raised by the improper_ctypes lint is correct or not. This depends on what the UCG-wg decides, but I wish this lint was actually perfect "according to the spec". Passing types through C FFI is already hard due to many factors, and guaranteed niche optimizations make reasoning about the correctness of C FFI code harder. It would be nice to be able to rely on this lint.

[newpavlov]

We have a similar situation in the wasi crate. We would like to rely on layouts being exactly the same for the following two structs:

#[repr(C)]
pub struct __wasi_event_t {
    pub userdata: u64,
    pub error: u16,
    pub type_: u8,
    pub u: T,
}

#[repr(C)]
pub struct Event {
    pub userdata: u64,
    pub res: Result<(), NonZeroU16>,
    pub type_: u8,
    pub u: T,
}

const _ASSERT1: [(); 32] = [(); core::mem::size_of::<__wasi_event_t>()];
const _ASSERT2: [(); 32] = [(); core::mem::size_of::<Event>()];
const _ASSERT3: [(); 0] =  [(); __WASI_ESUCCESS as usize];

Note the asserts. Since we check at compile time that both structs have exactly the same size, it means that niche optimization is applied, meaning that Result<(), NonZeroU16> and u16 have exactly the same memory representation. So will it be correct to cast *mut Event to *mut __wasi_event_t and then pass the resulting pointer to an FFI function?

[gnzlbg]

@eddyb I think this could be a small RFC proposing to guarantee this, and including the couple of motivating examples shown here. We don't have to wait for the UCGs to come up with an RFC.

I don't know if this might also be FCP material for a PR to the reference. I would be more comfortable with it being a proper RFC.