diff options
| -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 | ||||
| -rw-r--r-- | content/browser/child_process_launcher.cc | 50 | ||||
| -rw-r--r-- | content/browser/mach_broker_mac.cc | 4 | ||||
| -rw-r--r-- | content/browser/mach_broker_mac.h | 8 |
6 files changed, 204 insertions, 45 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); diff --git a/content/browser/child_process_launcher.cc b/content/browser/child_process_launcher.cc index 6150965..0892b66 100644 --- a/content/browser/child_process_launcher.cc +++ b/content/browser/child_process_launcher.cc @@ -156,34 +156,38 @@ class ChildProcessLauncher::Context } #endif // defined(OS_POSIX) && !defined(OS_MACOSX) - bool launched = false; -#if defined(OS_MACOSX) - // It is possible for the child process to die immediately after - // launching. To prevent leaking MachBroker map entries in this case, - // lock around all of LaunchProcess(). If the child dies, the death - // notification will be processed by the MachBroker after the call to - // AddPlaceholderForPid(), enabling proper cleanup. - { // begin scope for AutoLock - MachBroker* broker = MachBroker::GetInstance(); - base::AutoLock lock(broker->GetLock()); + // Actually launch the app. + base::LaunchOptions options; + options.environ = &env; + options.fds_to_remap = &fds_to_map; - // This call to |PrepareForFork()| will start the MachBroker listener - // thread, if it is not already running. Therefore the browser process - // will be listening for Mach IPC before LaunchProcess() is called. - broker->PrepareForFork(); -#endif +#if defined(OS_MACOSX) + // Use synchronization to make sure that the MachBroker is ready to + // receive a check-in from the new process before the new process + // actually tries to check in. + base::LaunchSynchronizationHandle synchronization_handle; + options.synchronize = &synchronization_handle; +#endif // defined(OS_MACOSX) - // Actually launch the app. - base::LaunchOptions options; - options.environ = &env; - options.fds_to_remap = &fds_to_map; - launched = base::LaunchProcess(*cmd_line, options, &handle); + bool launched = base::LaunchProcess(*cmd_line, options, &handle); #if defined(OS_MACOSX) - if (launched) + if (launched) { + MachBroker* broker = MachBroker::GetInstance(); + { + base::AutoLock lock(broker->GetLock()); + + // Make sure the MachBroker is running, and inform it to expect a + // check-in from the new process. + broker->EnsureRunning(); broker->AddPlaceholderForPid(handle); - } // end scope for AutoLock -#endif + } + + // Now that the MachBroker is ready, the child may continue. + base::LaunchSynchronize(synchronization_handle); + } +#endif // defined(OS_MACOSX) + if (!launched) handle = base::kNullProcessHandle; } diff --git a/content/browser/mach_broker_mac.cc b/content/browser/mach_broker_mac.cc index 86d1fcf..ad92a5a 100644 --- a/content/browser/mach_broker_mac.cc +++ b/content/browser/mach_broker_mac.cc @@ -128,7 +128,9 @@ MachBroker::MachBroker() : listener_thread_started_(false) { MachBroker::~MachBroker() {} -void MachBroker::PrepareForFork() { +void MachBroker::EnsureRunning() { + lock_.AssertAcquired(); + if (!listener_thread_started_) { listener_thread_started_ = true; diff --git a/content/browser/mach_broker_mac.h b/content/browser/mach_broker_mac.h index 6efeab4..d78b0d7 100644 --- a/content/browser/mach_broker_mac.h +++ b/content/browser/mach_broker_mac.h @@ -39,8 +39,9 @@ class MachBroker : public base::ProcessMetrics::PortProvider, static MachBroker* GetInstance(); // Performs any necessary setup that cannot happen in the constructor. - // Clients MUST call this method before fork()ing any children. - void PrepareForFork(); + // Callers MUST acquire the lock given by GetLock() before calling this + // method (and release the lock afterwards). + void EnsureRunning(); struct MachInfo { MachInfo() : mach_task_(MACH_PORT_NULL) {} @@ -69,7 +70,8 @@ class MachBroker : public base::ProcessMetrics::PortProvider, void InvalidatePid(base::ProcessHandle pid); // The lock that protects this MachBroker object. Clients MUST acquire and - // release this lock around calls to PlaceholderForPid() and FinalizePid(). + // release this lock around calls to EnsureRunning(), PlaceholderForPid(), + // and FinalizePid(). base::Lock& GetLock(); // Returns the Mach port name to use when sending or receiving messages. |