Skip to content

Commit

Permalink
Use posix_spawn instead of vfork + exec
Browse files Browse the repository at this point in the history
Bare vfork + exec are tricky to use, since it's easy to accidentally
corrupt the parent process's state from the child process. It's also
unsupported in Rust (rust-lang/libc#1596).

I think we could work around this in Rust by using inline assembly or a
C helper function to wrap the fork and exec, but `posix_spawn` basically
does that for us already.
  • Loading branch information
sporksmith committed Mar 22, 2023
1 parent 297ab3f commit 9a71c14
Showing 1 changed file with 38 additions and 46 deletions.
84 changes: 38 additions & 46 deletions src/main/host/managed_thread.c
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <inttypes.h>
#include <sched.h>
#include <search.h>
#include <spawn.h>
#include <string.h>
#include <sys/prctl.h>
#include <sys/syscall.h>
Expand Down Expand Up @@ -133,8 +134,10 @@ static pid_t _managedthread_fork_exec(ManagedThread* thread, const char* file,
const char* const* argv_in, const char* const* envp_in,
const char* workingDir, int straceFd, int shimlogFd) {
utility_debugAssert(file != NULL);
// We actually use `posix_spawn` here, which is a safer abstraction around
// vfork + exec.

// execve technically takes arrays of pointers to *mutable* char.
// posix_spawn technically takes arrays of pointers to *mutable* char.
// conservatively dup here.
gchar** argv = g_strdupv((gchar**)argv_in);
gchar** envp = g_strdupv((gchar**)envp_in);
Expand All @@ -146,55 +149,44 @@ static pid_t _managedthread_fork_exec(ManagedThread* thread, const char* file,
utility_panic("pipe2: %s", g_strerror(errno));
}

// vfork has superior performance to fork with large workloads.
pid_t pid = vfork();

// Beware! Unless you really know what you're doing, don't add any code
// between here and the execvpe below. The forked child process is sharing
// memory and control structures with the parent at this point. See
// `man 2 vfork`.

switch (pid) {
case -1:
utility_panic("fork failed");
return -1;
break;
case 0: {
// child

// *Don't* close the write end of the pipe on exec.
if (fcntl(pipefd[1], F_SETFD, 0)) {
die_after_vfork();
}
posix_spawn_file_actions_t file_actions;
if (posix_spawn_file_actions_init(&file_actions) != 0) {
utility_panic("posix_spawn_file_actions_init: %s", g_strerror(errno));
}

// clear the FD_CLOEXEC flag for the strace fd so that it's available in the shim
if (straceFd >= 0 && fcntl(straceFd, F_SETFD, 0)) {
die_after_vfork();
}
// Dup the write end of the pipe; the dup'd descriptor won't have O_CLOEXEC set.
if (posix_spawn_file_actions_adddup2(&file_actions, pipefd[1], pipefd[1]) != 0) {
utility_panic("posix_spawn_file_actions_adddup2: %s", g_strerror(errno));
}

// set stdout/stderr as the shim log, and clear the FD_CLOEXEC flag so that it's
// available in the shim
if (dup2(shimlogFd, STDOUT_FILENO) < 0) {
die_after_vfork();
}
if (dup2(shimlogFd, STDERR_FILENO) < 0) {
die_after_vfork();
}
// Likewise for straceFd.
if (straceFd >= 0) {
if (posix_spawn_file_actions_adddup2(&file_actions, straceFd, straceFd) != 0) {
utility_panic("posix_spawn_file_actions_adddup2: %s", g_strerror(errno));
}
}

// Set the working directory
if (chdir(workingDir) < 0) {
die_after_vfork();
}
// set stdout/stderr as the shim log, and clear the FD_CLOEXEC flag so that it's
// available in the shim
if (posix_spawn_file_actions_adddup2(&file_actions, shimlogFd, STDOUT_FILENO) != 0) {
utility_panic("posix_spawn_file_actions_adddup2: %s", g_strerror(errno));
}
if (posix_spawn_file_actions_adddup2(&file_actions, shimlogFd, STDERR_FILENO) != 0) {
utility_panic("posix_spawn_file_actions_adddup2: %s", g_strerror(errno));
}

int rc = execvpe(file, argv, envp);
if (rc == -1) {
die_after_vfork();
}
// Unreachable
die_after_vfork();
}
default: // parent
break;
// Set the working directory
if (posix_spawn_file_actions_addchdir_np(&file_actions, workingDir) != 0) {
utility_panic("posix_spawn_file_actions_addchdir_np: %s", g_strerror(errno));
}

pid_t pid;
if (posix_spawn(&pid, file, &file_actions, NULL, argv, envp) != 0) {
utility_panic("posix_spawn: %s", g_strerror(errno));
}

if (posix_spawn_file_actions_destroy(&file_actions) != 0) {
utility_panic("posix_spawn_file_actions: %s", g_strerror(errno));
}

// *Must* close the write-end of the pipe, so that the child's copy is the
Expand Down

0 comments on commit 9a71c14

Please sign in to comment.