Skip to content

Commit e6425d3

Browse files
committed
Use posix_spawn instead of vfork + exec
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.
1 parent 297ab3f commit e6425d3

File tree

1 file changed

+42
-51
lines changed

1 file changed

+42
-51
lines changed

src/main/host/managed_thread.c

Lines changed: 42 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include <inttypes.h>
77
#include <sched.h>
88
#include <search.h>
9+
#include <spawn.h>
910
#include <string.h>
1011
#include <sys/prctl.h>
1112
#include <sys/syscall.h>
@@ -129,12 +130,13 @@ static gchar** _add_u64_to_env(gchar** envp, const char* var, uint64_t x) {
129130
return envp;
130131
}
131132

132-
static pid_t _managedthread_fork_exec(ManagedThread* thread, const char* file,
133-
const char* const* argv_in, const char* const* envp_in,
134-
const char* workingDir, int straceFd, int shimlogFd) {
135-
utility_debugAssert(file != NULL);
133+
static pid_t _managedthread_spawn(ManagedThread* thread, const char* file,
134+
const char* const* argv_in, const char* const* envp_in,
135+
const char* workingDir, int straceFd, int shimlogFd) {
136+
// We use `posix_spawn` to spawn the child process. This should be functionally
137+
// equivalent to vfork + execve, but wrapped in a safer and more portable API.
136138

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

149-
// vfork has superior performance to fork with large workloads.
150-
pid_t pid = vfork();
151-
152-
// Beware! Unless you really know what you're doing, don't add any code
153-
// between here and the execvpe below. The forked child process is sharing
154-
// memory and control structures with the parent at this point. See
155-
// `man 2 vfork`.
156-
157-
switch (pid) {
158-
case -1:
159-
utility_panic("fork failed");
160-
return -1;
161-
break;
162-
case 0: {
163-
// child
164-
165-
// *Don't* close the write end of the pipe on exec.
166-
if (fcntl(pipefd[1], F_SETFD, 0)) {
167-
die_after_vfork();
168-
}
151+
posix_spawn_file_actions_t file_actions;
152+
if (posix_spawn_file_actions_init(&file_actions) != 0) {
153+
utility_panic("posix_spawn_file_actions_init: %s", g_strerror(errno));
154+
}
169155

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

175-
// set stdout/stderr as the shim log, and clear the FD_CLOEXEC flag so that it's
176-
// available in the shim
177-
if (dup2(shimlogFd, STDOUT_FILENO) < 0) {
178-
die_after_vfork();
179-
}
180-
if (dup2(shimlogFd, STDERR_FILENO) < 0) {
181-
die_after_vfork();
182-
}
161+
// Likewise for straceFd.
162+
if (straceFd >= 0) {
163+
if (posix_spawn_file_actions_adddup2(&file_actions, straceFd, straceFd) != 0) {
164+
utility_panic("posix_spawn_file_actions_adddup2: %s", g_strerror(errno));
165+
}
166+
}
183167

184-
// Set the working directory
185-
if (chdir(workingDir) < 0) {
186-
die_after_vfork();
187-
}
168+
// set stdout/stderr as the shim log, and clear the FD_CLOEXEC flag so that it's
169+
// available in the shim
170+
if (posix_spawn_file_actions_adddup2(&file_actions, shimlogFd, STDOUT_FILENO) != 0) {
171+
utility_panic("posix_spawn_file_actions_adddup2: %s", g_strerror(errno));
172+
}
173+
if (posix_spawn_file_actions_adddup2(&file_actions, shimlogFd, STDERR_FILENO) != 0) {
174+
utility_panic("posix_spawn_file_actions_adddup2: %s", g_strerror(errno));
175+
}
188176

189-
int rc = execvpe(file, argv, envp);
190-
if (rc == -1) {
191-
die_after_vfork();
192-
}
193-
// Unreachable
194-
die_after_vfork();
195-
}
196-
default: // parent
197-
break;
177+
// Set the working directory
178+
if (posix_spawn_file_actions_addchdir_np(&file_actions, workingDir) != 0) {
179+
utility_panic("posix_spawn_file_actions_addchdir_np: %s", g_strerror(errno));
180+
}
181+
182+
pid_t pid;
183+
if (posix_spawn(&pid, file, &file_actions, NULL, argv, envp) != 0) {
184+
utility_panic("posix_spawn: %s", g_strerror(errno));
185+
}
186+
187+
if (posix_spawn_file_actions_destroy(&file_actions) != 0) {
188+
utility_panic("posix_spawn_file_actions: %s", g_strerror(errno));
198189
}
199190

200191
// *Must* close the write-end of the pipe, so that the child's copy is the
@@ -262,7 +253,7 @@ void managedthread_run(ManagedThread* mthread, const char* pluginPath, const cha
262253
logPath, O_WRONLY | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH | S_IWOTH);
263254
utility_alwaysAssert(shimlogFd >= 0);
264255

265-
mthread->nativePid = _managedthread_fork_exec(
256+
mthread->nativePid = _managedthread_spawn(
266257
mthread, pluginPath, argv, (const char* const*)myenvv, workingDir, straceFd, shimlogFd);
267258

268259
// should be opened in the shim, so no need for it anymore

0 commit comments

Comments
 (0)