diff options
author | mark@chromium.org <mark@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-09-01 21:44:24 +0000 |
---|---|---|
committer | mark@chromium.org <mark@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-09-01 21:44:24 +0000 |
commit | 84b2d1d81b8c1c39c3cadaf9771e2c78cc0c51c4 (patch) | |
tree | e4086b02a50d7cd8b4a149fc54f962f8d6d3f30c /base | |
parent | 5efd22803abd20ef4230f5e95443e9fabe06c5d4 (diff) | |
download | chromium_src-84b2d1d81b8c1c39c3cadaf9771e2c78cc0c51c4.zip chromium_src-84b2d1d81b8c1c39c3cadaf9771e2c78cc0c51c4.tar.gz chromium_src-84b2d1d81b8c1c39c3cadaf9771e2c78cc0c51c4.tar.bz2 |
Synchronize the parent and child processes during launch, ensuring that the
child doesn't try to check in with the Mach broker before the parent is ready.
This resolves a race between the browser and its child processes.
base::LaunchOptions gets a new member, synchronize, currently Mac-only
although there's nothing Mac-specific about the implementation. When set,
it allows base::LaunchProcess clients to request that the child process block
until released by the new base::LaunchSynchronize call. When creating child
processes, the child process launcher now requests this new synchronization
behavior to give it a chance to set up the Mach broker to expect the child
before the child tries to check in with it.
BUG=94295
TEST=Chrome should work. You see fewer of this logged:
Unknown process (pid) is sending Mach IPC messages!
But you still might see some of these due to bug 94543.
Review URL: http://codereview.chromium.org/7740036
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@99245 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'base')
-rw-r--r-- | base/process_util.h | 58 | ||||
-rw-r--r-- | base/process_util_posix.cc | 125 | ||||
-rw-r--r-- | base/process_util_unittest.cc | 4 |
3 files changed, 169 insertions, 18 deletions
diff --git a/base/process_util.h b/base/process_util.h index 66c63dd..34e9e91 100644 --- a/base/process_util.h +++ b/base/process_util.h @@ -200,6 +200,13 @@ BASE_EXPORT void CloseSuperfluousFds(const InjectiveMultimap& saved_map); typedef std::vector<std::pair<std::string, std::string> > environment_vector; typedef std::vector<std::pair<int, int> > file_handle_mapping_vector; +#if defined(OS_MACOSX) +// Used with LaunchOptions::synchronize and LaunchSynchronize, a +// LaunchSynchronizationHandle is an opaque value that LaunchProcess will +// create and set, and that LaunchSynchronize will consume and destroy. +typedef int* LaunchSynchronizationHandle; +#endif // defined(OS_MACOSX) + // Options for launching a subprocess that are passed to LaunchProcess(). // The default constructor constructs the object with default options. struct LaunchOptions { @@ -208,8 +215,13 @@ struct LaunchOptions { start_hidden(false), inherit_handles(false), as_user(NULL), empty_desktop_name(false) #else - environ(NULL), fds_to_remap(NULL), new_process_group(false), - clone_flags(0) + environ(NULL), fds_to_remap(NULL), new_process_group(false) +#if defined(OS_LINUX) + , clone_flags(0) +#endif // OS_LINUX +#if defined(OS_MACOSX) + , synchronize(NULL) +#endif // defined(OS_MACOSX) #endif // !defined(OS_WIN) {} @@ -251,9 +263,33 @@ struct LaunchOptions { // will be the same as its pid. bool new_process_group; +#if defined(OS_LINUX) // If non-zero, start the process using clone(), using flags as provided. int clone_flags; -#endif // !defined(OS_WIN) +#endif // defined(OS_LINUX) + +#if defined(OS_MACOSX) + // When non-NULL, a new LaunchSynchronizationHandle will be created and + // stored in *synchronize whenever LaunchProcess returns true in the parent + // process. The child process will have been created (with fork) but will + // be waiting (before exec) for the parent to call LaunchSynchronize with + // this handle. Only when LaunchSynchronize is called will the child be + // permitted to continue execution and call exec. LaunchSynchronize + // destroys the handle created by LaunchProcess. + // + // When synchronize is non-NULL, the parent must call LaunchSynchronize + // whenever LaunchProcess returns true. No exceptions. + // + // Synchronization is useful when the parent process needs to guarantee that + // it can take some action (such as recording the newly-forked child's + // process ID) before the child does something (such as using its process ID + // to communicate with its parent). + // + // |synchronize| and |wait| must not both be set simultaneously. + LaunchSynchronizationHandle* synchronize; +#endif // defined(OS_MACOSX) + +#endif // !defined(OS_WIN) }; // Launch a process via the command line |cmdline|. @@ -265,8 +301,11 @@ struct LaunchOptions { // Otherwise, the process handle will be implicitly closed. // // Unix-specific notes: -// - Before launching, all FDs open in the parent process will be marked as -// close-on-exec. +// - All file descriptors open in the parent process will be closed in the +// child process except for any preserved by options::fds_to_remap, and +// stdin, stdout, and stderr. If not remapped by options::fds_to_remap, +// stdin is reopened as /dev/null, and the child is allowed to inherit its +// parent's stdout and stderr. // - If the first argument on the command line does not contain a slash, // PATH will be searched. (See man execvp.) BASE_EXPORT bool LaunchProcess(const CommandLine& cmdline, @@ -319,6 +358,15 @@ BASE_EXPORT bool LaunchProcess(const std::vector<std::string>& argv, // The returned array is allocated using new[] and must be freed by the caller. BASE_EXPORT char** AlterEnvironment(const environment_vector& changes, const char* const* const env); + +#if defined(OS_MACOSX) + +// After a successful call to LaunchProcess with LaunchOptions::synchronize +// set, the parent process must call LaunchSynchronize to allow the child +// process to proceed, and to destroy the LaunchSynchronizationHandle. +BASE_EXPORT void LaunchSynchronize(LaunchSynchronizationHandle handle); + +#endif // defined(OS_MACOSX) #endif // defined(OS_POSIX) // Executes the application specified by |cl| and wait for it to exit. Stores diff --git a/base/process_util_posix.cc b/base/process_util_posix.cc index 059b7d7..06fa779 100644 --- a/base/process_util_posix.cc +++ b/base/process_util_posix.cc @@ -531,32 +531,72 @@ char** AlterEnvironment(const environment_vector& changes, bool LaunchProcess(const std::vector<std::string>& argv, const LaunchOptions& options, ProcessHandle* process_handle) { - pid_t pid; - InjectiveMultimap fd_shuffle1, fd_shuffle2; + size_t fd_shuffle_size = 0; if (options.fds_to_remap) { - fd_shuffle1.reserve(options.fds_to_remap->size()); - fd_shuffle2.reserve(options.fds_to_remap->size()); + fd_shuffle_size = options.fds_to_remap->size(); } + +#if defined(OS_MACOSX) + if (options.synchronize) { + // When synchronizing, the "read" end of the synchronization pipe needs + // to make it to the child process. This is handled by mapping it back to + // itself. + ++fd_shuffle_size; + } +#endif // defined(OS_MACOSX) + + InjectiveMultimap fd_shuffle1; + InjectiveMultimap fd_shuffle2; + fd_shuffle1.reserve(fd_shuffle_size); + fd_shuffle2.reserve(fd_shuffle_size); + scoped_array<char*> argv_cstr(new char*[argv.size() + 1]); scoped_array<char*> new_environ; if (options.environ) new_environ.reset(AlterEnvironment(*options.environ, GetEnvironment())); - if (options.clone_flags) { +#if defined(OS_MACOSX) + int synchronization_pipe_fds[2]; + file_util::ScopedFD synchronization_read_fd; + file_util::ScopedFD synchronization_write_fd; + + if (options.synchronize) { + // wait means "don't return from LaunchProcess until the child exits", and + // synchronize means "return from LaunchProcess but don't let the child + // run until LaunchSynchronize is called". These two options are highly + // incompatible. + CHECK(!options.wait); + + // Create the pipe used for synchronization. + if (HANDLE_EINTR(pipe(synchronization_pipe_fds)) != 0) { + PLOG(ERROR) << "pipe"; + return false; + } + + // The parent process will only use synchronization_write_fd as the write + // side of the pipe. It can close the read side as soon as the child + // process has forked off. The child process will only use + // synchronization_read_fd as the read side of the pipe. In that process, + // the write side can be closed as soon as it has forked. + synchronization_read_fd.reset(&synchronization_pipe_fds[0]); + synchronization_write_fd.reset(&synchronization_pipe_fds[1]); + } +#endif // OS_MACOSX + + pid_t pid; #if defined(OS_LINUX) + if (options.clone_flags) { pid = syscall(__NR_clone, options.clone_flags, 0, 0, 0); -#else - pid = -1; // hygiene; prevent clang warnings - NOTREACHED() << "Tried to use clone() on non-Linux system"; + } else #endif - } else { + { pid = fork(); } + if (pid < 0) { PLOG(ERROR) << "fork"; return false; - } - if (pid == 0) { + } else if (pid == 0) { // Child process // DANGER: fork() rule: in the child, if you don't end up doing exec*(), @@ -589,12 +629,20 @@ bool LaunchProcess(const std::vector<std::string>& argv, _exit(127); } } + #if defined(OS_MACOSX) RestoreDefaultExceptionHandler(); -#endif +#endif // defined(OS_MACOSX) ResetChildSignalHandlersToDefaults(); +#if defined(OS_MACOSX) + if (options.synchronize) { + // The "write" side of the synchronization pipe belongs to the parent. + synchronization_write_fd.reset(); // closes synchronization_pipe_fds[1] + } +#endif // defined(OS_MACOSX) + #if 0 // When debugging it can be helpful to check that we really aren't making // any hidden calls to malloc. @@ -602,7 +650,7 @@ bool LaunchProcess(const std::vector<std::string>& argv, reinterpret_cast<void*>(reinterpret_cast<intptr_t>(malloc) & ~4095); mprotect(malloc_thunk, 4096, PROT_READ | PROT_WRITE | PROT_EXEC); memset(reinterpret_cast<void*>(malloc), 0xff, 8); -#endif +#endif // 0 // DANGER: no calls to malloc are allowed from now on: // http://crbug.com/36678 @@ -616,6 +664,16 @@ bool LaunchProcess(const std::vector<std::string>& argv, } } +#if defined(OS_MACOSX) + if (options.synchronize) { + // Remap the read side of the synchronization pipe back onto itself, + // ensuring that it won't be closed by CloseSuperfluousFds. + int keep_fd = *synchronization_read_fd.get(); + fd_shuffle1.push_back(InjectionArc(keep_fd, keep_fd, false)); + fd_shuffle2.push_back(InjectionArc(keep_fd, keep_fd, false)); + } +#endif // defined(OS_MACOSX) + if (options.environ) SetEnvironment(new_environ.get()); @@ -625,10 +683,29 @@ bool LaunchProcess(const std::vector<std::string>& argv, CloseSuperfluousFds(fd_shuffle2); +#if defined(OS_MACOSX) + if (options.synchronize) { + // Do a blocking read to wait until the parent says it's OK to proceed. + // The byte that's read here is written by LaunchSynchronize. + char read_char; + int read_result = + HANDLE_EINTR(read(*synchronization_read_fd.get(), &read_char, 1)); + if (read_result != 1) { + RAW_LOG(ERROR, "LaunchProcess: synchronization read: error"); + _exit(127); + } + + // The pipe is no longer useful. Don't let it live on in the new process + // after exec. + synchronization_read_fd.reset(); // closes synchronization_pipe_fds[0] + } +#endif // defined(OS_MACOSX) + for (size_t i = 0; i < argv.size(); i++) argv_cstr[i] = const_cast<char*>(argv[i].c_str()); argv_cstr[argv.size()] = NULL; execvp(argv_cstr[0], argv_cstr.get()); + RAW_LOG(ERROR, "LaunchProcess: failed to execvp:"); RAW_LOG(ERROR, argv_cstr[0]); _exit(127); @@ -644,17 +721,39 @@ bool LaunchProcess(const std::vector<std::string>& argv, if (process_handle) *process_handle = pid; + +#if defined(OS_MACOSX) + if (options.synchronize) { + // The "read" side of the synchronization pipe belongs to the child. + synchronization_read_fd.reset(); // closes synchronization_pipe_fds[0] + *options.synchronize = new int(*synchronization_write_fd.release()); + } +#endif // defined(OS_MACOSX) } return true; } + bool LaunchProcess(const CommandLine& cmdline, const LaunchOptions& options, ProcessHandle* process_handle) { return LaunchProcess(cmdline.argv(), options, process_handle); } +#if defined(OS_MACOSX) +void LaunchSynchronize(LaunchSynchronizationHandle handle) { + int synchronization_fd = *handle; + file_util::ScopedFD synchronization_fd_closer(&synchronization_fd); + delete handle; + + // Write a '\0' character to the pipe. + if (HANDLE_EINTR(write(synchronization_fd, "", 1)) != 1) { + PLOG(ERROR) << "write"; + } +} +#endif // defined(OS_MACOSX) + ProcessMetrics::~ProcessMetrics() { } bool EnableInProcessStackDumping() { diff --git a/base/process_util_unittest.cc b/base/process_util_unittest.cc index bcc1308..0403d7a 100644 --- a/base/process_util_unittest.cc +++ b/base/process_util_unittest.cc @@ -538,7 +538,11 @@ std::string TestLaunchProcess(const base::environment_vector& env_changes, options.wait = true; options.environ = &env_changes; options.fds_to_remap = &fds_to_remap; +#if defined(OS_LINUX) options.clone_flags = clone_flags; +#else + CHECK_EQ(0, clone_flags); +#endif // OS_LINUX EXPECT_TRUE(base::LaunchProcess(args, options, NULL)); PCHECK(HANDLE_EINTR(close(fds[1])) == 0); |