Command: Support posix_spawn() on FreeBSD/OSX/GNU Linux

[bdrewery]

No description provided.

[bdrewery]

FYI @alexcrichton, didn't assign you since you wrote the majority of the changes here.

[alexcrichton]

Nice! For posterity I originally didn't land this because it broke our test suite, notably the behavior of Command::new("nonexistent").status() changed. Today that returns an error whereas with posix_spawn (on Linux at least) it return success and then wait() returns an error.

@bdrewery you were mentioning that a try_wait may be necessary here? Or does FreeBSD's implementation of posix_spawn already handle that case differently?

[bdrewery]

FreeBSD's posix_spawn simply returns any error from exec already without any wait needed. The tests for this are passing:

test [run-pass] run-pass/command-before-exec.rs ... ok
test [run-pass] run-pass/command-exec.rs ... ok

It's able to do this because of vfork suspending the parent and sharing memory, so it is able to share the errno:

static int
do_posix_spawn(pid_t *pid, const char *path,
const posix_spawn_file_actions_t *fa,
const posix_spawnattr_t *sa,
char * const argv[], char * const envp[], int use_env_path)
{
pid_t p;
volatile int error = 0;
p = vfork();
switch (p) {
case -1:
return (errno);
case 0:
if (sa != NULL) {
error = process_spawnattr(*sa);
if (error)
_exit(127);
}
if (fa != NULL) {
error = process_file_actions(*fa);
if (error)
_exit(127);
}
if (use_env_path)
_execvpe(path, argv, envp != NULL ? envp : environ);
else
_execve(path, argv, envp != NULL ? envp : environ);
error = errno;
_exit(127);
default:
if (error != 0)
_waitpid(p, NULL, WNOHANG);
else if (pid != NULL)
*pid = p;
return (error);
}
}

This is intended and documented behavior.

 1. If posix_spawn() and posix_spawnp() fail for any of the reasons that would cause vfork() or one of the exec to fail, an error value is returned as described by vfork() and exec, respectively (or, if the error occurs after the calling process successfully returns, the
child process exits with exit status 127).

[alexcrichton]

Ah ok! I think I must have misunderstood then, that sounds perfect!

@bors: r+

[bors]

📌 Commit 85b82f2 has been approved by alexcrichton

[bors]

🌲 The tree is currently closed for pull requests below priority 200, this pull request will be tested once the tree is reopened

[cuviper]

For Linux we will need to detect a "new enough" glibc that contains the changes in https://sourceware.org/bugzilla/show_bug.cgi?id=10354#c12 specifically for the change noted in (2).

Just for reference, those commits are contained in glibc-2.24.

[bdrewery]

@cuviper is there a glibc function to call that returns the version?

[cuviper]

Yes, gnu_get_libc_version(). Or the POSIX confstr(_CS_GNU_LIBC_VERSION, buf, len) might be easier for portability, returning EINVAL for that name elsewhere.

[alexcrichton]

Oh nice catch @cuviper! I was testing locally with 2.23 when I initially wrote this, and I found that this program:

use std::process::Command;fnmain(){println!("{:?}",Command::new("nonexistent").spawn());}

where on today's standard library it prints an error but that's with running glibc 2.23 locally. Taking the same binary and running it with glibc 2.24 it prints an error (as today's rustc does).

I haven't tested on OSX yet but sounds like we can certainly do this for Linux too! We'll just need to check at runtime that the version is >= 2.24.

I also just tested with musl and it looks like they've implemented it in such a way (presumably the same way?) that prints an error as well, so on musl we can unconditionally use posix_spawn.

[alexcrichton]

@bdrewery would you be up for putting some of these checks in this PR? Or should I file a follow-up issue?

[bdrewery]

@alexcrichton I had thought the command-exec.rs cases covered this but I see now they don't use spawn but plenty of others do but lack a nonexistent test. I'll add some more here. I think adding other platforms should be separate. I was considering looking at OSX and Linux but need to get some dev environments setup still.

[alexcrichton]

@bdrewery oh I'm not too worried about test coverage here, when I tested this on Linux I originally got a lot of failures. If this gets past @bors I'd be pretty confident that it's working correctly!

(FWIW I tested OSX and I needed this diff to get errors out but it otherwise worked)

[bdrewery]

It looks like posix_spawn always returns errno directly, even on FreeBSD. Will retest there with from_raw_os_error.

I was just looking over OSX's opensource code and their posix_spawn manpage and thought it would work fine given they document these possible errors:

 [ENOENT] The new process file does not exist.
[ENOEXEC] The new process file has the appropriate access permission, but has an unrecognized format (e.g., an invalid magic number in its header).

It seems like they based their implementation on NetBSD's.
I don't expect this to be workable on OpenBSD since they use fork rather than vfork.
DragonFlyBSD appears to have the same implementation of FreeBSD so should work fine but still need to copy over the bindings.

[alexcrichton]

@bdrewery also to be clear I don't want to drag you into a bunch of portability nightmare scenarios, if you'd like to just do FreeBSD and be done with it that's perfectly ok!

[bdrewery]

@alexcrichton yup understood, I don't mind looking at some of these right now.

[bdrewery]

Tests pass on OSX for me so I've added support for it in, and fixed the error handling. Working on Linux support still.

[kennytm]

@bors r-

The env_saw_path function is unused in musl environment, causing an unused item warning.

[00:40:43] error: method is never used: `env_saw_path`
[00:40:43] --> libstd/sys/unix/process/process_common.rs:187:5
[00:40:43] |
[00:40:43] 187 | pub fn env_saw_path(&self) -> bool {
[00:40:43] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[00:40:43] |
[00:40:43] note: lint level defined here
[00:40:43] --> libstd/lib.rs:232:31
[00:40:43] |
[00:40:43] 232 | #![cfg_attr(not(stage0), deny(warnings))]
[00:40:43] | ^^^^^^^^
[00:40:43] = note: #[deny(dead_code)] implied by #[deny(warnings)]

[alexcrichton]

@bors: r+

[bors]

📌 Commit 70559c5 has been approved by alexcrichton