Bug 1400061 - Stop using SetAllFDsToCloseOnExec when launching processes on OS X. r?billm
As its original comments indicate, SetAllFDsToCloseOnExec has an
unavoidable race condition if another thread creates file descriptors
during launch. Instead, use POSIX_SPAWN_CLOEXEC_DEFAULT, which is an
Apple-specific extension to posix_spawn that accomplished the desired
effect atomically.
This patch also introduces some RAII to simplify cleanup in error cases.
MozReview-Commit-ID: 6oHggs77AiY
--- a/ipc/chromium/src/base/process_util_mac.mm
+++ b/ipc/chromium/src/base/process_util_mac.mm
@@ -8,16 +8,17 @@
#include <fcntl.h>
#include <spawn.h>
#include <sys/wait.h>
#include <string>
#include "base/eintr_wrapper.h"
#include "base/logging.h"
+#include "mozilla/ScopeExit.h"
namespace {
static mozilla::EnvironmentLog gProcessLog("MOZ_PROCESS_LOG");
} // namespace
namespace base {
@@ -37,42 +38,40 @@ bool LaunchApp(const std::vector<std::st
bool retval = true;
char* argv_copy[argv.size() + 1];
for (size_t i = 0; i < argv.size(); i++) {
argv_copy[i] = const_cast<char*>(argv[i].c_str());
}
argv_copy[argv.size()] = NULL;
- // Make sure we don't leak any FDs to the child process by marking all FDs
- // as close-on-exec.
- SetAllFDsToCloseOnExec();
-
EnvironmentArray vars = BuildEnvironmentArray(env_vars_to_set);
posix_spawn_file_actions_t file_actions;
if (posix_spawn_file_actions_init(&file_actions) != 0) {
return false;
}
+ auto file_actions_guard = mozilla::MakeScopeExit([&file_actions] {
+ posix_spawn_file_actions_destroy(&file_actions);
+ });
// Turn fds_to_remap array into a set of dup2 calls.
for (file_handle_mapping_vector::const_iterator it = fds_to_remap.begin();
it != fds_to_remap.end();
++it) {
int src_fd = it->first;
int dest_fd = it->second;
if (src_fd == dest_fd) {
int flags = fcntl(src_fd, F_GETFD);
if (flags != -1) {
fcntl(src_fd, F_SETFD, flags & ~FD_CLOEXEC);
}
} else {
if (posix_spawn_file_actions_adddup2(&file_actions, src_fd, dest_fd) != 0) {
- posix_spawn_file_actions_destroy(&file_actions);
return false;
}
}
}
// Set up the CPU preference array.
cpu_type_t cpu_types[1];
switch (arch) {
@@ -90,38 +89,50 @@ bool LaunchApp(const std::vector<std::st
break;
}
// Initialize spawn attributes.
posix_spawnattr_t spawnattr;
if (posix_spawnattr_init(&spawnattr) != 0) {
return false;
}
+ auto spawnattr_guard = mozilla::MakeScopeExit([&spawnattr] {
+ posix_spawnattr_destroy(&spawnattr);
+ });
// Set spawn attributes.
size_t attr_count = 1;
size_t attr_ocount = 0;
if (posix_spawnattr_setbinpref_np(&spawnattr, attr_count, cpu_types, &attr_ocount) != 0 ||
attr_ocount != attr_count) {
- posix_spawnattr_destroy(&spawnattr);
+ return false;
+ }
+
+ // Prevent the child process from inheriting any file descriptors
+ // that aren't named in `file_actions`. (This is an Apple-specific
+ // extension to posix_spawn.)
+ if (posix_spawnattr_setflags(&spawnattr, POSIX_SPAWN_CLOEXEC_DEFAULT) != 0) {
return false;
}
+ // Exempt std{in,out,err} from being closed by POSIX_SPAWN_CLOEXEC_DEFAULT.
+ for (int fd = 0; fd <= STDERR_FILENO; ++fd) {
+ if (posix_spawn_file_actions_addinherit_np(&file_actions, fd) != 0) {
+ return false;
+ }
+ }
+
int pid = 0;
int spawn_succeeded = (posix_spawnp(&pid,
argv_copy[0],
&file_actions,
&spawnattr,
argv_copy,
vars.get()) == 0);
- posix_spawn_file_actions_destroy(&file_actions);
-
- posix_spawnattr_destroy(&spawnattr);
-
bool process_handle_valid = pid > 0;
if (!spawn_succeeded || !process_handle_valid) {
retval = false;
} else {
gProcessLog.print("==> process %d launched child process %d\n",
GetCurrentProcId(), pid);
if (wait)
HANDLE_EINTR(waitpid(pid, 0, 0));