POSIX: don't allocate memory after forking.

Previously we would allocate memory in the child process. However, the
allocation might have happened while the malloc lock was held,
resulting in a deadlock.

This patch removes allocation from the child but probably makes Mac's
startup time slower until a Mac person can implement
dir_reader_posix.h.

TEST=Unittest for new code
BUG=36678

http://codereview.chromium.org/672003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@41181 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 536 insertions, 118 deletions

diff --git a/base/base.gyp b/base/base.gyp
index 2eec8e1..30d746d 100644
--- a/base/base.gyp
+++ b/base/base.gyp
@@ -61,7 +61,7 @@
         'crypto/signature_verifier_unittest.cc',
         'data_pack_unittest.cc',
         'debug_util_unittest.cc',
-        'file_watcher_unittest.cc',
+        'dir_reader_posix_unittest.cc',
         'event_trace_consumer_win_unittest.cc',
         'event_trace_controller_win_unittest.cc',
         'event_trace_provider_win_unittest.cc',
@@ -70,6 +70,7 @@
         'file_path_unittest.cc',
         'file_util_unittest.cc',
         'file_version_info_unittest.cc',
+        'file_watcher_unittest.cc',
         'gfx/rect_unittest.cc',
         'gmock_unittest.cc',
         'histogram_unittest.cc',
@@ -181,6 +182,7 @@
             '../third_party/icu/icu.gyp:icudata',
           ],
           'sources!': [
+            'dir_reader_posix_unittest.cc',
             'file_descriptor_shuffle_unittest.cc',
           ],
         }, {  # OS != "win"
diff --git a/base/base.gypi b/base/base.gypi
index c9d2e08..1fbb537 100644
--- a/base/base.gypi
+++ b/base/base.gypi
@@ -61,6 +61,9 @@
           'debug_util_mac.cc',
           'debug_util_posix.cc',
           'debug_util_win.cc',
+          'dir_reader_fallback.h',
+          'dir_reader_linux.h',
+          'dir_reader_posix.h',
           'event_trace_consumer_win.h',
           'event_trace_controller_win.cc',
           'event_trace_controller_win.h',
diff --git a/base/dir_reader_fallback.h b/base/dir_reader_fallback.h
new file mode 100644
index 0000000..c8f02e6
--- /dev/null
+++ b/base/dir_reader_fallback.h
@@ -0,0 +1,30 @@
+// 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_
+
+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_
diff --git a/base/dir_reader_linux.h b/base/dir_reader_linux.h
new file mode 100644
index 0000000..7fd534f
--- /dev/null
+++ b/base/dir_reader_linux.h
@@ -0,0 +1,97 @@
+// 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_
+
+#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) {
+  }
+
+  ~DirReaderLinux() {
+    if (fd_ >= 0) {
+      if (HANDLE_EINTR(close(fd_)))
+        RAW_LOG(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) {
+      DPLOG(FATAL) << "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_
diff --git a/base/dir_reader_posix.h b/base/dir_reader_posix.h
new file mode 100644
index 0000000..e9ad11e
--- /dev/null
+++ b/base/dir_reader_posix.h
@@ -0,0 +1,30 @@
+// 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_
+
+#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.
+
+#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_
diff --git a/base/dir_reader_posix_unittest.cc b/base/dir_reader_posix_unittest.cc
new file mode 100644
index 0000000..04f7243
--- /dev/null
+++ b/base/dir_reader_posix_unittest.cc
@@ -0,0 +1,91 @@
+// 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.
+
+#include "base/dir_reader_posix.h"
+
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include "base/logging.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using base::DirReaderPosix;
+
+namespace {
+typedef testing::Test DirReaderPosixUnittest;
+
+TEST(DirReaderPosixUnittest, Read) {
+  static const unsigned kNumFiles = 100;
+
+  if (DirReaderPosix::IsFallback())
+    return;
+
+  char kDirTemplate[] = "/tmp/org.chromium.dir-reader-posix-XXXXXX";
+  const char* dir = mkdtemp(kDirTemplate);
+  CHECK(dir);
+
+  const int prev_wd = open(".", O_RDONLY | O_DIRECTORY);
+  CHECK_GE(prev_wd, 0);
+
+  PCHECK(chdir(dir) == 0);
+
+  for (unsigned i = 0; i < kNumFiles; i++) {
+    char buf[16];
+    snprintf(buf, sizeof(buf), "%d", i);
+    const int fd = open(buf, O_CREAT | O_RDONLY | O_EXCL, 0600);
+    PCHECK(fd >= 0);
+    PCHECK(close(fd) == 0);
+  }
+
+  std::set<unsigned> seen;
+
+  DirReaderPosix reader(dir);
+  EXPECT_TRUE(reader.IsValid());
+
+  if (!reader.IsValid())
+    return;
+
+  bool seen_dot = false, seen_dotdot = false;
+
+  for (; reader.Next(); ) {
+    if (strcmp(reader.name(), ".") == 0) {
+      seen_dot = true;
+      continue;
+    }
+    if (strcmp(reader.name(), "..") == 0) {
+      seen_dotdot = true;
+      continue;
+    }
+
+    SCOPED_TRACE(testing::Message() << "reader.name(): " << reader.name());
+
+    char *endptr;
+    const unsigned long value = strtoul(reader.name(), &endptr, 10);
+
+    EXPECT_FALSE(*endptr);
+    EXPECT_LT(value, kNumFiles);
+    EXPECT_EQ(0u, seen.count(value));
+    seen.insert(value);
+  }
+
+  for (unsigned i = 0; i < kNumFiles; i++) {
+    char buf[16];
+    snprintf(buf, sizeof(buf), "%d", i);
+    PCHECK(unlink(buf) == 0);
+  }
+
+  PCHECK(rmdir(dir) == 0);
+
+  PCHECK(fchdir(prev_wd) == 0);
+  PCHECK(close(prev_wd) == 0);
+
+  EXPECT_TRUE(seen_dot);
+  EXPECT_TRUE(seen_dotdot);
+  EXPECT_EQ(kNumFiles, seen.size());
+}
+
+}  // anonymous namespace
diff --git a/base/file_descriptor_shuffle.cc b/base/file_descriptor_shuffle.cc
index e722a29..2bb156b 100644
--- a/base/file_descriptor_shuffle.cc
+++ b/base/file_descriptor_shuffle.cc
@@ -12,28 +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;
 
-  for (InjectiveMultimap::iterator i = m.begin(); i != m.end(); ++i) {
+  // DANGER: this function may not allocate.
+
+  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) {
+    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 {
+            RAW_LOG(ERROR, "PerformInjectiveMultimapDestructive overflowed "
+                           "extra_fds. Leaking file descriptors!");
+          }
         }
 
         j->source = temp_fd;
@@ -58,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;
diff --git a/base/file_descriptor_shuffle.h b/base/file_descriptor_shuffle.h
index e0cb88b..e1c93cd 100644
--- a/base/file_descriptor_shuffle.h
+++ b/base/file_descriptor_shuffle.h
@@ -69,9 +69,13 @@ typedef std::vector<InjectionArc> InjectiveMultimap;
 bool PerformInjectiveMultimap(const InjectiveMultimap& map,
                               InjectionDelegate* delegate);
 
-static inline bool ShuffleFileDescriptors(const InjectiveMultimap& map) {
+bool PerformInjectiveMultimapDestructive(InjectiveMultimap* map,
+                                         InjectionDelegate* delegate);
+
+// 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
diff --git a/base/process_util.h b/base/process_util.h
index cf4bd01..0c8c474 100644
--- a/base/process_util.h
+++ b/base/process_util.h
@@ -111,12 +111,6 @@ bool AdjustOOMScore(ProcessId process, int score);
 #endif
 
 #if defined(OS_POSIX)
-// Sets all file descriptors to close on exec except for stdin, stdout
-// and stderr.
-// TODO(agl): remove this function
-// WARNING: do not use. It's inherently race-prone in the face of
-// multi-threading.
-void SetAllFDsToCloseOnExec();
 // Close all file descriptors, expect those which are a destination in the
 // given multimap. Only call this function in a child process where you know
 // that there aren't any other threads.
diff --git a/base/process_util_posix.cc b/base/process_util_posix.cc
index fdac7a2..f9f0f29 100644
--- a/base/process_util_posix.cc
+++ b/base/process_util_posix.cc
@@ -18,6 +18,7 @@
 
 #include "base/compiler_specific.h"
 #include "base/debug_util.h"
+#include "base/dir_reader_posix.h"
 #include "base/eintr_wrapper.h"
 #include "base/logging.h"
 #include "base/platform_thread.h"
@@ -28,8 +29,13 @@
 #include "base/time.h"
 #include "base/waitable_event.h"
 
+
 #if defined(OS_MACOSX)
+#include <crt_externs.h>
+#define environ (*_NSGetEnviron())
 #include "base/mach_ipc_mac.h"
+#else
+extern char** environ;
 #endif
 
 const int kMicrosecondsPerSecond = 1000000;
@@ -173,24 +179,26 @@ class ScopedDIRClose {
 };
 typedef scoped_ptr_malloc<DIR, ScopedDIRClose> ScopedDIR;
 
-void CloseSuperfluousFds(const base::InjectiveMultimap& saved_mapping) {
 #if 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";
 #elif defined(OS_SOLARIS)
   static const rlim_t kSystemDefaultMaxFds = 8192;
-  static const char fd_dir[] = "/dev/fd";
+  static const char kFDDir[] = "/dev/fd";
 #elif defined(OS_FREEBSD)
   static const rlim_t kSystemDefaultMaxFds = 8192;
-  static const char fd_dir[] = "/dev/fd";
+  static const char kFDDir[] = "/dev/fd";
 #elif defined(OS_OPENBSD)
   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;
+
+void CloseSuperfluousFds(const base::InjectiveMultimap& saved_mapping) {
+  // DANGER: no calls to malloc are allowed from now on:
+  // http://crbug.com/36678
 
   // Get the maximum number of FDs possible.
   struct rlimit nofile;
@@ -206,25 +214,20 @@ 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 i;
+      for (i = saved_mapping.begin(); i != saved_mapping.end(); i++) {
+        if (fd == i->dest)
+          break;
+      }
+      if (i != saved_mapping.end())
         continue;
 
       // Since we're just trying to close anything we can find,
@@ -233,20 +236,27 @@ void CloseSuperfluousFds(const base::InjectiveMultimap& saved_mapping) {
     }
     return;
   }
-  int dir_fd = dirfd(dir);
 
-  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 (fd == STDIN_FILENO || fd == STDOUT_FILENO || fd == STDERR_FILENO)
       continue;
-    if (saved_fds.find(fd) != saved_fds.end())
+    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;
@@ -262,41 +272,6 @@ void CloseSuperfluousFds(const base::InjectiveMultimap& saved_mapping) {
   }
 }
 
-// Sets all file descriptors to close on exec except for stdin, stdout
-// and stderr.
-// TODO(agl): Remove this function. It's fundamentally broken for multithreaded
-// apps.
-void SetAllFDsToCloseOnExec() {
-#if defined(OS_LINUX)
-  const char fd_dir[] = "/proc/self/fd";
-#elif defined(OS_MACOSX) || defined(OS_FREEBSD) || defined(OS_OPENBSD) || \
-    defined(OS_SOLARIS)
-  const char fd_dir[] = "/dev/fd";
-#endif
-  ScopedDIR dir_closer(opendir(fd_dir));
-  DIR *dir = dir_closer.get();
-  if (NULL == dir) {
-    DLOG(ERROR) << "Unable to open " << fd_dir;
-    return;
-  }
-
-  struct dirent *ent;
-  while ((ent = readdir(dir))) {
-    // Skip . and .. entries.
-    if (ent->d_name[0] == '.')
-      continue;
-    int i = atoi(ent->d_name);
-    // We don't close stdin, stdout or stderr.
-    if (i <= STDERR_FILENO)
-      continue;
-
-    int flags = fcntl(i, F_GETFD);
-    if ((flags == -1) || (fcntl(i, F_SETFD, flags | FD_CLOEXEC) == -1)) {
-      DLOG(ERROR) << "fcntl failure.";
-    }
-  }
-}
-
 #if defined(OS_MACOSX)
 static std::string MachErrorCode(kern_return_t err) {
   return StringPrintf("0x%x %s", err, mach_error_string(err));
@@ -358,21 +333,144 @@ static pid_t fork_and_get_task(task_t* child_task) {
 }
 
 bool LaunchApp(const std::vector<std::string>& argv,
-               const environment_vector& environ,
+               const environment_vector& env_changes,
                const file_handle_mapping_vector& fds_to_remap,
                bool wait, ProcessHandle* process_handle) {
   return LaunchAppAndGetTask(
-      argv, environ, fds_to_remap, wait, NULL, process_handle);
+      argv, env_changes, fds_to_remap, wait, NULL, process_handle);
 }
 #endif  // defined(OS_MACOSX)
 
+// AlterEnvironment returns a modified environment vector, constructed from the
+// current environment and the list of changes given in |changes|. Each key in
+// the environment is matched against the first element of the pairs. In the
+// event of a match, the value is replaced by the second of the pair, unless
+// the second is empty, in which case the key-value is removed.
+//
+// The returned array is allocated using new[] and must be freed by the caller.
+static char** AlterEnvironment(const environment_vector& changes) {
+  unsigned count = 0;
+  unsigned size = 0;
+
+  // First assume that all of the current environment will be included.
+  for (unsigned i = 0; environ[i]; i++) {
+    const char *const pair = environ[i];
+    count++;
+    size += strlen(pair) + 1 /* terminating NUL */;
+  }
+
+  for (environment_vector::const_iterator
+       j = changes.begin(); j != changes.end(); j++) {
+    bool found = false;
+    const char *pair;
+
+    for (unsigned i = 0; environ[i]; i++) {
+      pair = environ[i];
+      const char *const equals = strchr(pair, '=');
+      if (!equals)
+        continue;
+      const unsigned keylen = equals - pair;
+      if (keylen == j->first.size() &&
+          memcmp(pair, j->first.data(), keylen) == 0) {
+        found = true;
+        break;
+      }
+    }
+
+    // if found, we'll either be deleting or replacing this element.
+    if (found) {
+      count--;
+      size -= strlen(pair) + 1;
+      if (j->second.size())
+        found = false;
+    }
+
+    // if !found, then we have a new element to add.
+    if (!found) {
+      count++;
+      size += j->first.size() + 1 /* '=' */ + j->second.size() + 1 /* NUL */;
+    }
+  }
+
+  uint8_t *buffer = new uint8_t[sizeof(char*) * count + size];
+  char **const ret = reinterpret_cast<char**>(buffer);
+  unsigned k = 0;
+  char *scratch = reinterpret_cast<char*>(buffer + sizeof(char*) * count);
+
+  for (unsigned i = 0; environ[i]; i++) {
+    const char *const pair = environ[i];
+    const char *const equals = strchr(pair, '=');
+    if (!equals) {
+      const unsigned len = strlen(pair);
+      ret[k++] = scratch;
+      memcpy(scratch, pair, len + 1);
+      scratch += len + 1;
+      continue;
+    }
+    const unsigned keylen = equals - pair;
+    bool handled = false;
+    for (environment_vector::const_iterator
+         j = changes.begin(); j != changes.end(); j++) {
+      if (j->first.size() == keylen &&
+          memcmp(j->first.data(), pair, keylen) == 0) {
+        if (!j->second.empty()) {
+          ret[k++] = scratch;
+          memcpy(scratch, pair, keylen + 1);
+          scratch += keylen + 1;
+          memcpy(scratch, j->second.c_str(), j->second.size() + 1);
+          scratch += j->second.size() + 1;
+        }
+        handled = true;
+        break;
+      }
+    }
+
+    if (!handled) {
+      const unsigned len = strlen(pair);
+      ret[k++] = scratch;
+      memcpy(scratch, pair, len + 1);
+      scratch += len + 1;
+    }
+  }
+
+  // Now handle new elements
+  for (environment_vector::const_iterator
+       j = changes.begin(); j != changes.end(); j++) {
+    bool found = false;
+    for (unsigned i = 0; environ[i]; i++) {
+      const char *const pair = environ[i];
+      const char *const equals = strchr(pair, '=');
+      if (!equals)
+        continue;
+      const unsigned keylen = equals - pair;
+      if (keylen == j->first.size() &&
+          memcmp(pair, j->first.data(), keylen) == 0) {
+        found = true;
+        break;
+      }
+    }
+
+    if (!found) {
+      ret[k++] = scratch;
+      memcpy(scratch, j->first.data(), j->first.size());
+      scratch += j->first.size();
+      *scratch++ = '=';
+      memcpy(scratch, j->second.c_str(), j->second.size() + 1);
+      scratch += j->second.size() + 1;
+     }
+  }
+
+  ret[k] = NULL;
+  return ret;
+}
+
 #if defined(OS_MACOSX)
 bool LaunchAppAndGetTask(
 #else
 bool LaunchApp(
 #endif
     const std::vector<std::string>& argv,
-    const environment_vector& environ,
+    const environment_vector& env_changes,
     const file_handle_mapping_vector& fds_to_remap,
     bool wait,
 #if defined(OS_MACOSX)
@@ -380,6 +478,12 @@ bool LaunchApp(
 #endif
     ProcessHandle* process_handle) {
   pid_t pid;
+  InjectiveMultimap fd_shuffle1, fd_shuffle2;
+  fd_shuffle1.reserve(fds_to_remap.size());
+  fd_shuffle2.reserve(fds_to_remap.size());
+  scoped_array<char*> argv_cstr(new char*[argv.size() + 1]);
+  scoped_array<char*> new_environ(AlterEnvironment(env_changes));
+
 #if defined(OS_MACOSX)
   if (task_handle == NULL) {
     pid = fork();
@@ -403,45 +507,44 @@ bool LaunchApp(
     RestoreDefaultExceptionHandler();
 #endif
 
-    InjectiveMultimap fd_shuffle;
+#if 0
+    // When debugging it can be helpful to check that we really aren't making
+    // any hidden calls to malloc.
+    void *malloc_thunk =
+        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
+
+    // DANGER: no calls to malloc are allowed from now on:
+    // http://crbug.com/36678
+
     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));
     }
 
-    for (environment_vector::const_iterator it = environ.begin();
-         it != environ.end(); ++it) {
-      if (it->first.empty())
-        continue;
-
-      if (it->second.empty()) {
-        unsetenv(it->first.c_str());
-      } else {
-        setenv(it->first.c_str(), it->second.c_str(), 1);
-      }
-    }
+    environ = new_environ.get();
 
     // 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.
-    if (!ShuffleFileDescriptors(fd_shuffle))
-      _exit(127);
 
-    // If we are using the SUID sandbox, it sets a magic environment variable
-    // ("SBX_D"), so we remove that variable from the environment here on the
-    // off chance that it's already set.
-    unsetenv("SBX_D");
+    // fd_shuffle1 is mutated by this call because it cannot malloc.
+    if (!ShuffleFileDescriptors(&fd_shuffle1))
+      _exit(127);
 
-    CloseSuperfluousFds(fd_shuffle);
+    CloseSuperfluousFds(fd_shuffle2);
 
-    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());
-    PLOG(ERROR) << "LaunchApp: execvp(" << argv_cstr[0] << ") failed";
+    RAW_LOG(ERROR, "LaunchApp: failed to execvp:");
+    RAW_LOG(ERROR, argv_cstr[0]);
     _exit(127);
   } else {
     // Parent process
@@ -634,6 +737,12 @@ static bool GetAppOutputInternal(const CommandLine& cl, char* const envp[],
                                  bool do_search_path) {
   int pipe_fd[2];
   pid_t pid;
+  InjectiveMultimap fd_shuffle1, fd_shuffle2;
+  const std::vector<std::string>& argv = cl.argv();
+  scoped_array<char*> argv_cstr(new char*[argv.size() + 1]);
+
+  fd_shuffle1.reserve(3);
+  fd_shuffle2.reserve(3);
 
   // Either |do_search_path| should be false or |envp| should be null, but not
   // both.
@@ -652,6 +761,8 @@ static bool GetAppOutputInternal(const CommandLine& cl, char* const envp[],
 #if defined(OS_MACOSX)
         RestoreDefaultExceptionHandler();
 #endif
+        // DANGER: no calls to malloc are allowed from now on:
+        // http://crbug.com/36678
 
         // 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
@@ -662,18 +773,20 @@ static bool GetAppOutputInternal(const CommandLine& cl, char* const envp[],
         if (dev_null < 0)
           _exit(127);
 
-        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));
+        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.
+
+        std::copy(fd_shuffle1.begin(), fd_shuffle1.end(),
+                  std::back_inserter(fd_shuffle2));
 
-        if (!ShuffleFileDescriptors(fd_shuffle))
+        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;
diff --git a/base/process_util_unittest.cc b/base/process_util_unittest.cc
index 3170b6a..d22a60a 100644
--- a/base/process_util_unittest.cc
+++ b/base/process_util_unittest.cc
@@ -294,6 +294,48 @@ TEST_F(ProcessUtilTest, FDRemapping) {
   DPCHECK(ret == 0);
 }
 
+static std::string TestLaunchApp(const base::environment_vector& env_changes) {
+  std::vector<std::string> args;
+  base::file_handle_mapping_vector fds_to_remap;
+  ProcessHandle handle;
+
+  args.push_back("bash");
+  args.push_back("-c");
+  args.push_back("echo $BASE_TEST");
+
+  int fds[2];
+  PCHECK(pipe(fds) == 0);
+
+  fds_to_remap.push_back(std::make_pair(fds[1], 1));
+  EXPECT_TRUE(LaunchApp(args, env_changes, fds_to_remap,
+                        true /* wait for exit */, &handle));
+  PCHECK(close(fds[1]) == 0);
+
+  char buf[32];
+  const ssize_t n = HANDLE_EINTR(read(fds[0], buf, sizeof(buf)));
+  PCHECK(n > 0);
+  return std::string(buf, n);
+}
+
+TEST_F(ProcessUtilTest, LaunchApp) {
+  base::environment_vector env_changes;
+
+  env_changes.push_back(std::make_pair(std::string("BASE_TEST"),
+                                       std::string("bar")));
+  EXPECT_EQ("bar\n", TestLaunchApp(env_changes));
+  env_changes.clear();
+
+  setenv("BASE_TEST", "testing", 1 /* override */);
+  EXPECT_EQ("testing\n", TestLaunchApp(env_changes));
+
+  env_changes.push_back(std::make_pair(std::string("BASE_TEST"),
+                                       std::string("")));
+  EXPECT_EQ("\n", TestLaunchApp(env_changes));
+
+  env_changes[0].second = "foo";
+  EXPECT_EQ("foo\n", TestLaunchApp(env_changes));
+}
+
 TEST_F(ProcessUtilTest, GetAppOutput) {
   std::string output;
   EXPECT_TRUE(GetAppOutput(CommandLine(FilePath("true")), &output));