[libc-0.2] posix_spawn_file_actions_t cannot be used on Linux after #3602

[japaric]

We found an issue when updating ferrocene's libc submodule (ferrocene/ferrocene#356) to revision 947a185 . the only change included in our libc update was PR #3602 .

when building stage 2 of libstd to x86_64-linux the build failed to execute the build script of the quote crate with the following error:

thread 'main' panicked at library/std/src/sys_common/process.rs:155:17:
called Result::unwrap() on an Err value: Os { code: 9, kind: Uncategorized, message: "Bad file descriptor" } 

where the mentioned line corresponds to this revision.

Tracking down the error led us to this usage of the posix_spawn API:

let mut file_actions = MaybeUninit::uninit();
cvt_nz(libc::posix_spawn_file_actions_init(file_actions.as_mut_ptr()))?;

This code allocates a posix_spawn_file_actions_init type on the stack. before #3602, the file_actions stack variable had the correct size and alignment but after #3602, it now has a size and alignment of *mut c_void (8 bytes on x86_64) and that results in UB when posix_spawn_file_actions_init is called.

GLIBC implements posix_spawn_file_actions_init as a memset operation that zeroes the struct. In Rust syntax, that would be more or less this code:

#[repr(C)]
struct posix_spawn_file_actions_t {
    // fields and padding here
}

unsafe fn posix_spawn_file_actions_init(actions: *mut posix_spawn_file_actions_t) -> c_int {
    ptr::write_bytes(actions, 0, 1);
    0
}

Bionic implements posix_spawn_file_actions_init differently. It uses a heap allocation as indirection. The Rust version of the bionic implementation looks roughly like this:

#[repr(C)]
struct __actual_posix_spawn_file_actions_t {
    // fields and padding here
}

type posix_spawn_file_actions_t = *mut c_void;

unsafe fn posix_spawn_file_actions_init(actions: *mut posix_spawn_file_actions_t) -> c_int {
    let mut alloc = Box::new(MaybeUninit::<__actual_posix_spawn_file_actions_t>::uninit());
    ptr::write_bytes(alloc.as_mut_ptr(), 0, 1);
    *actions = Box::into_raw(alloc).cast();
    0
}

This usage of posix_spawn_file_actions_t in libstd:

let mut file_actions = MaybeUninit::uninit();
cvt_nz(libc::posix_spawn_file_actions_init(file_actions.as_mut_ptr()))?;

is compatible with both GLIBC and Bionic but the libc crate needs to provide the correct size and alignment on Linux.

So, the conditional code should produce this on Linux

pub struct posix_spawn_file_actions_t {
    __allocated: ::c_int,
    __used: ::c_int,
    __actions: *mut ::c_int,
    __pad: [::c_int; 16],
}

but this on Android

pub type posix_spawn_file_actions_t = *mut ::c_void;

---

All this probably also applies to the posix_spawnattr_t type but I have yet to look into the details of the GLIBC code around that type.

I'll send up a fix PR after I have done some more testing.

[madsmtm]

@JohnTitor: This is blocking updating libc in the standard library, specifically because Cargo build scripts seem to break.

[madsmtm]

The problematic PR has been reverted in #3678.

[JohnTitor]

Note that #3608 exists since 0.2.153, #3678 just fixes #3677. We still have to merge #3609.

[silence-coding]

After wasting a day, I also found out that posix_spawnattr_init is fine in 0.2.153, but 0.2.154 causes Segmentation fault......

[madsmtm]

Note that #3608 exists since 0.2.153, #3678 just fixes #3677. We still have to merge #3609.

Hmm, I'm confused? The problem described in this issue is not present in v0.2.153, and the issue is resolved by #3678?

And #3677 is just a duplicate of this?

Or do you mean that we need to merge #3683? (EDIT: #3690)

[coolbluewater]

@madsmtm it looks like #3683 supersedes #3609. Additionally all checks have passed for #3683, so that needs to be merged asap.

[ngg]

#3690 supersedes #3683 now. What's the blocker here?

[cuviper]

I have yanked 0.2.154 with my T-libs crate access, but I don't think I have any permission in the repo to help get a new version out.

[newpavlov]

It would be nice to get v0.2.155 published as soon as possible. Yanking of v0.2.154 breaks the latest version of getrandom, see rust-random/getrandom#423.

[madsmtm]

#3690 has been merged, and 0.2.155 has been released, so I believe this can be closed.