Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Bug 622992 - Update Chrome IPC to not allocate memory after forking +…

… remove hack. r=cjones a=legneato
  • Loading branch information...
commit c9e44a989b2906971df6aaf23dc05e0f8620f8a8 1 parent c2c56db
@gcp gcp authored
View
31 ipc/chromium/src/base/dir_reader_fallback.h
@@ -0,0 +1,31 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_DIR_READER_FALLBACK_H_
+#define BASE_DIR_READER_FALLBACK_H_
+#pragma once
+
+namespace base {
+
+class DirReaderFallback {
+ public:
+ // Open a directory. If |IsValid| is true, then |Next| can be called to start
+ // the iteration at the beginning of the directory.
+ explicit DirReaderFallback(const char* directory_path) { }
+ // After construction, IsValid returns true iff the directory was
+ // successfully opened.
+ bool IsValid() const { return false; }
+ // Move to the next entry returning false if the iteration is complete.
+ bool Next() { return false; }
+ // Return the name of the current directory entry.
+ const char* name() { return 0;}
+ // Return the file descriptor which is being used.
+ int fd() const { return -1; }
+ // Returns true if this is a no-op fallback class (for testing).
+ static bool IsFallback() { return true; }
+};
+
+} // namespace base
+
+#endif // BASE_DIR_READER_FALLBACK_H_
View
99 ipc/chromium/src/base/dir_reader_linux.h
@@ -0,0 +1,99 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_DIR_READER_LINUX_H_
+#define BASE_DIR_READER_LINUX_H_
+#pragma once
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdint.h>
+#include <sys/syscall.h>
+#include <unistd.h>
+
+#include "base/logging.h"
+#include "base/eintr_wrapper.h"
+
+// See the comments in dir_reader_posix.h about this.
+
+namespace base {
+
+struct linux_dirent {
+ uint64_t d_ino;
+ int64_t d_off;
+ unsigned short d_reclen;
+ unsigned char d_type;
+ char d_name[0];
+};
+
+class DirReaderLinux {
+ public:
+ explicit DirReaderLinux(const char* directory_path)
+ : fd_(open(directory_path, O_RDONLY | O_DIRECTORY)),
+ offset_(0),
+ size_(0) {
+ memset(buf_, 0, sizeof(buf_));
+ }
+
+ ~DirReaderLinux() {
+ if (fd_ >= 0) {
+ if (HANDLE_EINTR(close(fd_)))
+ DLOG(ERROR) << "Failed to close directory handle";
+ }
+ }
+
+ bool IsValid() const {
+ return fd_ >= 0;
+ }
+
+ // Move to the next entry returning false if the iteration is complete.
+ bool Next() {
+ if (size_) {
+ linux_dirent* dirent = reinterpret_cast<linux_dirent*>(&buf_[offset_]);
+ offset_ += dirent->d_reclen;
+ }
+
+ if (offset_ != size_)
+ return true;
+
+ const int r = syscall(__NR_getdents64, fd_, buf_, sizeof(buf_));
+ if (r == 0)
+ return false;
+ if (r == -1) {
+ DLOG(ERROR) << "getdents64 returned an error: " << errno;
+ return false;
+ }
+ size_ = r;
+ offset_ = 0;
+ return true;
+ }
+
+ const char* name() const {
+ if (!size_)
+ return NULL;
+
+ const linux_dirent* dirent =
+ reinterpret_cast<const linux_dirent*>(&buf_[offset_]);
+ return dirent->d_name;
+ }
+
+ int fd() const {
+ return fd_;
+ }
+
+ static bool IsFallback() {
+ return false;
+ }
+
+ private:
+ const int fd_;
+ unsigned char buf_[512];
+ size_t offset_, size_;
+
+ DISALLOW_COPY_AND_ASSIGN(DirReaderLinux);
+};
+
+} // namespace base
+
+#endif // BASE_DIR_READER_LINUX_H_
View
37 ipc/chromium/src/base/dir_reader_posix.h
@@ -0,0 +1,37 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_DIR_READER_POSIX_H_
+#define BASE_DIR_READER_POSIX_H_
+#pragma once
+
+#include "build/build_config.h"
+
+// This header provides a class, DirReaderPosix, which allows one to open and
+// read from directories without allocating memory. For the interface, see
+// the generic fallback in dir_reader_fallback.h.
+
+// Mac note: OS X has getdirentries, but it only works if we restrict Chrome to
+// 32-bit inodes. There is a getdirentries64 syscall in 10.6, but it's not
+// wrapped and the direct syscall interface is unstable. Using an unstable API
+// seems worse than falling back to enumerating all file descriptors so we will
+// probably never implement this on the Mac.
+
+#if defined(OS_LINUX)
+#include "base/dir_reader_linux.h"
+#else
+#include "base/dir_reader_fallback.h"
+#endif
+
+namespace base {
+
+#if defined(OS_LINUX)
+typedef DirReaderLinux DirReaderPosix;
+#else
+typedef DirReaderFallback DirReaderPosix;
+#endif
+
+} // namespace base
+
+#endif // BASE_DIR_READER_POSIX_H_
View
40 ipc/chromium/src/base/file_descriptor_shuffle.cc
@@ -12,26 +12,36 @@
namespace base {
-bool PerformInjectiveMultimap(const InjectiveMultimap& m_in,
- InjectionDelegate* delegate) {
- InjectiveMultimap m(m_in);
- std::vector<int> extra_fds;
+bool PerformInjectiveMultimapDestructive(
+ InjectiveMultimap* m, InjectionDelegate* delegate) {
+ static const size_t kMaxExtraFDs = 16;
+ int extra_fds[kMaxExtraFDs];
+ unsigned next_extra_fd = 0;
+
+ // DANGER: this function may not allocate.
- for (InjectiveMultimap::iterator i = m.begin(); i != m.end(); ++i) {
+ for (InjectiveMultimap::iterator i = m->begin(); i != m->end(); ++i) {
int temp_fd = -1;
// We DCHECK the injectiveness of the mapping.
- for (InjectiveMultimap::iterator j = i + 1; j != m.end(); ++j)
- DCHECK(i->dest != j->dest);
+ for (InjectiveMultimap::iterator j = i + 1; j != m->end(); ++j) {
+ DCHECK(i->dest != j->dest) << "Both fd " << i->source
+ << " and " << j->source << " map to " << i->dest;
+ }
const bool is_identity = i->source == i->dest;
- for (InjectiveMultimap::iterator j = i + 1; j != m.end(); ++j) {
+ for (InjectiveMultimap::iterator j = i + 1; j != m->end(); ++j) {
if (!is_identity && i->dest == j->source) {
if (temp_fd == -1) {
if (!delegate->Duplicate(&temp_fd, i->dest))
return false;
- extra_fds.push_back(temp_fd);
+ if (next_extra_fd < kMaxExtraFDs) {
+ extra_fds[next_extra_fd++] = temp_fd;
+ } else {
+ DLOG(ERROR) << "PerformInjectiveMultimapDestructive overflowed "
+ << "extra_fds. Leaking file descriptors!";
+ }
}
j->source = temp_fd;
@@ -56,14 +66,18 @@ bool PerformInjectiveMultimap(const InjectiveMultimap& m_in,
delegate->Close(i->source);
}
- for (std::vector<int>::const_iterator
- i = extra_fds.begin(); i != extra_fds.end(); ++i) {
- delegate->Close(*i);
- }
+ for (unsigned i = 0; i < next_extra_fd; i++)
+ delegate->Close(extra_fds[i]);
return true;
}
+bool PerformInjectiveMultimap(const InjectiveMultimap& m_in,
+ InjectionDelegate* delegate) {
+ InjectiveMultimap m(m_in);
+ return PerformInjectiveMultimapDestructive(&m, delegate);
+}
+
bool FileDescriptorTableInjection::Duplicate(int* result, int fd) {
*result = HANDLE_EINTR(dup(fd));
return *result >= 0;
View
7 ipc/chromium/src/base/file_descriptor_shuffle.h
@@ -65,10 +65,13 @@ typedef std::vector<InjectionArc> InjectiveMultimap;
bool PerformInjectiveMultimap(const InjectiveMultimap& map,
InjectionDelegate* delegate);
+bool PerformInjectiveMultimapDestructive(InjectiveMultimap* map,
+ InjectionDelegate* delegate);
-static inline bool ShuffleFileDescriptors(const InjectiveMultimap& map) {
+// This function will not call malloc but will mutate |map|
+static inline bool ShuffleFileDescriptors(InjectiveMultimap *map) {
FileDescriptorTableInjection delegate;
- return PerformInjectiveMultimap(map, &delegate);
+ return PerformInjectiveMultimapDestructive(map, &delegate);
}
} // namespace base
View
29 ipc/chromium/src/base/process_util_linux.cc
@@ -51,39 +51,34 @@ bool LaunchApp(const std::vector<std::string>& argv,
const environment_map& env_vars_to_set,
bool wait, ProcessHandle* process_handle,
ProcessArchitecture arch) {
-#ifdef MOZ_MEMORY_ANDROID
- /* We specifically don't call pthread_atfork in jemalloc because it is not
- available in bionic until 2.3. However without it, jemalloc could
- potentially deadlock, when stl allocates memory through jemalloc, after
- fork and before execvp. Therefore, we must manually inform jemalloc here */
- ::_malloc_prefork();
-#endif
+ scoped_array<char*> argv_cstr(new char*[argv.size() + 1]);
+ // Illegal to allocate memory after fork and before execvp
+ InjectiveMultimap fd_shuffle1, fd_shuffle2;
+ fd_shuffle1.reserve(fds_to_remap.size());
+ fd_shuffle2.reserve(fds_to_remap.size());
+
pid_t pid = fork();
-#ifdef MOZ_MEMORY_ANDROID
- ::_malloc_postfork();
-#endif
if (pid < 0)
return false;
if (pid == 0) {
- InjectiveMultimap fd_shuffle;
for (file_handle_mapping_vector::const_iterator
it = fds_to_remap.begin(); it != fds_to_remap.end(); ++it) {
- fd_shuffle.push_back(InjectionArc(it->first, it->second, false));
+ fd_shuffle1.push_back(InjectionArc(it->first, it->second, false));
+ fd_shuffle2.push_back(InjectionArc(it->first, it->second, false));
}
- if (!ShuffleFileDescriptors(fd_shuffle))
- exit(127);
+ if (!ShuffleFileDescriptors(&fd_shuffle1))
+ _exit(127);
- CloseSuperfluousFds(fd_shuffle);
+ CloseSuperfluousFds(fd_shuffle2);
for (environment_map::const_iterator it = env_vars_to_set.begin();
it != env_vars_to_set.end(); ++it) {
if (setenv(it->first.c_str(), it->second.c_str(), 1/*overwrite*/))
- exit(127);
+ _exit(127);
}
- scoped_array<char*> argv_cstr(new char*[argv.size() + 1]);
for (size_t i = 0; i < argv.size(); i++)
argv_cstr[i] = const_cast<char*>(argv[i].c_str());
argv_cstr[argv.size()] = NULL;
View
115 ipc/chromium/src/base/process_util_posix.cc
@@ -1,3 +1,4 @@
+//* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -25,6 +26,7 @@
#include "base/sys_info.h"
#include "base/time.h"
#include "base/waitable_event.h"
+#include "base/dir_reader_posix.h"
const int kMicrosecondsPerSecond = 1000000;
@@ -86,6 +88,10 @@ bool KillProcess(ProcessHandle process_id, int exit_code, bool wait) {
return result;
}
+#ifdef ANDROID
+typedef unsigned long int rlim_t;
+#endif
+
// A class to handle auto-closing of DIR*'s.
class ScopedDIRClose {
public:
@@ -97,19 +103,20 @@ class ScopedDIRClose {
};
typedef scoped_ptr_malloc<DIR, ScopedDIRClose> ScopedDIR;
-#ifdef ANDROID
-typedef unsigned long int rlim_t;
-#endif
void CloseSuperfluousFds(const base::InjectiveMultimap& saved_mapping) {
-#if defined(OS_LINUX)
+ // DANGER: no calls to malloc are allowed from now on:
+ // http://crbug.com/36678
+#if defined(ANDROID)
+ static const rlim_t kSystemDefaultMaxFds = 1024;
+ static const char kFDDir[] = "/proc/self/fd";
+#elif defined(OS_LINUX)
static const rlim_t kSystemDefaultMaxFds = 8192;
- static const char fd_dir[] = "/proc/self/fd";
+ static const char kFDDir[] = "/proc/self/fd";
#elif defined(OS_MACOSX)
static const rlim_t kSystemDefaultMaxFds = 256;
- static const char fd_dir[] = "/dev/fd";
+ static const char kFDDir[] = "/dev/fd";
#endif
- std::set<int> saved_fds;
// Get the maximum number of FDs possible.
struct rlimit nofile;
@@ -125,52 +132,63 @@ void CloseSuperfluousFds(const base::InjectiveMultimap& saved_mapping) {
if (max_fds > INT_MAX)
max_fds = INT_MAX;
- // Don't close stdin, stdout and stderr
- saved_fds.insert(STDIN_FILENO);
- saved_fds.insert(STDOUT_FILENO);
- saved_fds.insert(STDERR_FILENO);
-
- for (base::InjectiveMultimap::const_iterator
- i = saved_mapping.begin(); i != saved_mapping.end(); ++i) {
- saved_fds.insert(i->dest);
- }
-
- ScopedDIR dir_closer(opendir(fd_dir));
- DIR *dir = dir_closer.get();
- if (NULL == dir) {
- DLOG(ERROR) << "Unable to open " << fd_dir;
+ DirReaderPosix fd_dir(kFDDir);
+ if (!fd_dir.IsValid()) {
// Fallback case: Try every possible fd.
for (rlim_t i = 0; i < max_fds; ++i) {
const int fd = static_cast<int>(i);
- if (saved_fds.find(fd) != saved_fds.end())
+ if (fd == STDIN_FILENO || fd == STDOUT_FILENO || fd == STDERR_FILENO)
+ continue;
+ InjectiveMultimap::const_iterator j;
+ for (j = saved_mapping.begin(); j != saved_mapping.end(); j++) {
+ if (fd == j->dest)
+ break;
+ }
+ if (j != saved_mapping.end())
continue;
+ // Since we're just trying to close anything we can find,
+ // ignore any error return values of close().
HANDLE_EINTR(close(fd));
}
return;
}
- struct dirent *ent;
- while ((ent = readdir(dir))) {
+ const int dir_fd = fd_dir.fd();
+
+ for ( ; fd_dir.Next(); ) {
// Skip . and .. entries.
- if (ent->d_name[0] == '.')
+ if (fd_dir.name()[0] == '.')
continue;
char *endptr;
errno = 0;
- const long int fd = strtol(ent->d_name, &endptr, 10);
- if (ent->d_name[0] == 0 || *endptr || fd < 0 || errno)
+ const long int fd = strtol(fd_dir.name(), &endptr, 10);
+ if (fd_dir.name()[0] == 0 || *endptr || fd < 0 || errno)
continue;
- if (saved_fds.find(fd) != saved_fds.end())
+ if (fd == STDIN_FILENO || fd == STDOUT_FILENO || fd == STDERR_FILENO)
+ continue;
+ InjectiveMultimap::const_iterator i;
+ for (i = saved_mapping.begin(); i != saved_mapping.end(); i++) {
+ if (fd == i->dest)
+ break;
+ }
+ if (i != saved_mapping.end())
+ continue;
+ if (fd == dir_fd)
continue;
// When running under Valgrind, Valgrind opens several FDs for its
// own use and will complain if we try to close them. All of
// these FDs are >= |max_fds|, so we can check against that here
// before closing. See https://bugs.kde.org/show_bug.cgi?id=191758
- if (fd < static_cast<int>(max_fds))
- HANDLE_EINTR(close(fd));
+ if (fd < static_cast<int>(max_fds)) {
+ int ret = HANDLE_EINTR(close(fd));
+ if (ret != 0) {
+ DLOG(ERROR) << "Problem closing fd";
+ }
+ }
}
}
@@ -419,6 +437,13 @@ bool GetAppOutput(const CommandLine& cl, std::string* output) {
int pipe_fd[2];
pid_t pid;
+ // Illegal to allocate memory after fork and before execvp
+ InjectiveMultimap fd_shuffle1, fd_shuffle2;
+ fd_shuffle1.reserve(3);
+ fd_shuffle2.reserve(3);
+ const std::vector<std::string>& argv = cl.argv();
+ scoped_array<char*> argv_cstr(new char*[argv.size() + 1]);
+
if (pipe(pipe_fd) < 0)
return false;
@@ -429,27 +454,35 @@ bool GetAppOutput(const CommandLine& cl, std::string* output) {
return false;
case 0: // child
{
+ // Obscure fork() rule: in the child, if you don't end up doing exec*(),
+ // you call _exit() instead of exit(). This is because _exit() does not
+ // call any previously-registered (in the parent) exit handlers, which
+ // might do things like block waiting for threads that don't even exist
+ // in the child.
int dev_null = open("/dev/null", O_WRONLY);
if (dev_null < 0)
- exit(127);
+ _exit(127);
+
+ fd_shuffle1.push_back(InjectionArc(pipe_fd[1], STDOUT_FILENO, true));
+ fd_shuffle1.push_back(InjectionArc(dev_null, STDERR_FILENO, true));
+ fd_shuffle1.push_back(InjectionArc(dev_null, STDIN_FILENO, true));
+ // Adding another element here? Remeber to increase the argument to
+ // reserve(), above.
- InjectiveMultimap fd_shuffle;
- fd_shuffle.push_back(InjectionArc(pipe_fd[1], STDOUT_FILENO, true));
- fd_shuffle.push_back(InjectionArc(dev_null, STDERR_FILENO, true));
- fd_shuffle.push_back(InjectionArc(dev_null, STDIN_FILENO, true));
+ std::copy(fd_shuffle1.begin(), fd_shuffle1.end(),
+ std::back_inserter(fd_shuffle2));
- if (!ShuffleFileDescriptors(fd_shuffle))
- exit(127);
+ // fd_shuffle1 is mutated by this call because it cannot malloc.
+ if (!ShuffleFileDescriptors(&fd_shuffle1))
+ _exit(127);
- CloseSuperfluousFds(fd_shuffle);
+ CloseSuperfluousFds(fd_shuffle2);
- const std::vector<std::string> argv = cl.argv();
- scoped_array<char*> argv_cstr(new char*[argv.size() + 1]);
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());
- exit(127);
+ _exit(127);
}
default: // parent
{
Please sign in to comment.
Something went wrong with that request. Please try again.