POSIX: Add code for shuffling file descriptors.

When forking a child process, one often wants to move existing file
descriptors to well known locations (stdout, stderr etc). However,
this is a process bedeviled with corner cases. Consider the case where
you open two file descriptors, get assigned fds 1 and 0 for them and
wish to shuffle them so that they end up in slots 0 and 1. Our current
code fails in this case.

We also have a problem where we're currently trying to mark file
descriptors as close-on-exec rather than closing them in the child
process. This is inherently broken in a multithreaded process where
another thread can open a file descriptor and race the loop which is
trying to mark them.

Thus, on Linux we switch to close-after-fork where we known that no
other threads exist in the process. On Mac, the code is sufficiently
different that this simple fix isn't applicable and one of the Mac
folks will need to take a look.

http://codereview.chromium.org/100127
BUG=11174


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

base/base.gyp

@@ -95,6 +95,8 @@
         'event_recorder_stubs.cc',
         'field_trial.cc',
         'field_trial.h',
+        'file_descriptor_shuffle.cc',
+        'file_descriptor_shuffle.h',
         'file_path.cc',
         'file_path.h',
         'file_util.cc',
@@ -446,6 +448,7 @@
             'sources!': [
               'data_pack.cc',
               'event_recorder_stubs.cc',
+              'file_descriptor_shuffle.cc',
               'message_pump_libevent.cc',
               'string16.cc',
             ],
@@ -558,6 +561,7 @@
         'debug_util_unittest.cc',
         'directory_watcher_unittest.cc',
         'field_trial_unittest.cc',
+        'file_descriptor_shuffle_unittest.cc',
         'file_path_unittest.cc',
         'file_util_unittest.cc',
         'file_version_info_unittest.cc',
@@ -655,6 +659,7 @@
           ],
           'sources!': [
             'data_pack_unittest.cc',
+            'file_descriptor_shuffle_unittest.cc',
           ],
         }, {  # OS != "win"
           'sources!': [

base/file_descriptor_shuffle.cc

+// Copyright (c) 2009 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/file_descriptor_shuffle.h"
+
+#include <errno.h>
+#include <unistd.h>
+
+#include "base/logging.h"
+
+namespace base {
+
+bool PerformInjectiveMultimap(const InjectiveMultimap& m_in,
+                              InjectionDelegate* delegate) {
+  InjectiveMultimap m(m_in);
+  std::vector<int> extra_fds;
+
+  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);
+
+    const bool is_identity = i->source == i->dest;
+
+    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);
+        }
+
+        j->source = temp_fd;
+        j->close = false;
+      }
+
+      if (i->close && i->source == j->dest)
+        i->close = false;
+
+      if (i->close && i->source == j->source) {
+        i->close = false;
+        j->close = true;
+      }
+    }
+
+    if (!is_identity) {
+      if (!delegate->Move(i->source, i->dest))
+        return false;
+    }
+
+    if (!is_identity && i->close)
+      delegate->Close(i->source);
+  }
+
+  for (std::vector<int>::const_iterator
+       i = extra_fds.begin(); i != extra_fds.end(); ++i) {
+    delegate->Close(*i);
+  }
+
+  return true;
+}
+
+bool FileDescriptorTableInjection::Duplicate(int* result, int fd) {
+  do {
+    *result = dup(fd);
+  } while(*result == -1 && errno == EINTR);
+
+  return *result >= 0;
+}
+
+bool FileDescriptorTableInjection::Move(int src, int dest) {
+  int result;
+
+  do {
+    result = dup2(src, dest);
+  } while (result == -1 && errno == EINTR);
+
+  return result != -1;
+}
+
+void FileDescriptorTableInjection::Close(int fd) {
+  int result;
+
+  do {
+    result = close(fd);
+  } while (result == -1 && errno == EINTR);
+}
+
+}  // namespace base

base/file_descriptor_shuffle.h

+// Copyright (c) 2009 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_FILE_DESCRIPTOR_SHUFFLE_H_
+#define BASE_FILE_DESCRIPTOR_SHUFFLE_H_
+
+// This code exists to perform the shuffling of file descriptors which is
+// commonly needed when forking subprocesses. The naive approve is very simple,
+// just call dup2 to setup the desired descriptors, but wrong. It's tough to
+// handle the edge cases (like mapping 0 -> 1, 1 -> 0) correctly.
+//
+// In order to unittest this code, it's broken into the abstract action (an
+// injective multimap) and the concrete code for dealing with file descriptors.
+// Users should use the code like this:
+//   base::InjectiveMultimap file_descriptor_map;
+//   file_descriptor_map.push_back(base::InjectionArc(devnull, 0, true));
+//   file_descriptor_map.push_back(base::InjectionArc(devnull, 2, true));
+//   file_descriptor_map.push_back(base::InjectionArc(pipe[1], 1, true));
+//   base::ShuffleFileDescriptors(file_descriptor_map);
+//
+// and trust the the Right Thing will get done.
+
+#include <vector>
+
+namespace base {
+
+// A Delegate which performs the actions required to perform an injective
+// multimapping in place.
+class InjectionDelegate {
+ public:
+  // Duplicate |fd|, an element of the domain, and write a fresh element of the
+  // domain into |result|. Returns true iff successful.
+  virtual bool Duplicate(int* result, int fd) = 0;
+  // Destructively move |src| to |dest|, overwriting |dest|. Returns true iff
+  // successful.
+  virtual bool Move(int src, int dest) = 0;
+  // Delete an element of the domain.
+  virtual void Close(int fd) = 0;
+};
+
+// An implementation of the InjectionDelegate interface using the file
+// descriptor table of the current process as the domain.
+class FileDescriptorTableInjection : public InjectionDelegate {
+  bool Duplicate(int* result, int fd);
+  bool Move(int src, int dest);
+  void Close(int fd);
+};
+
+// A single arc of the directed graph which describes an injective multimapping.
+struct InjectionArc {
+  InjectionArc(int in_source, int in_dest, bool in_close)
+      : source(in_source),
+        dest(in_dest),
+        close(in_close) {
+  }
+
+  int source;
+  int dest;
+  bool close;  // if true, delete the source element after performing the
+               // mapping.
+};
+
+typedef std::vector<InjectionArc> InjectiveMultimap;
+
+bool PerformInjectiveMultimap(const InjectiveMultimap& map,
+                              InjectionDelegate* delegate);
+
+static inline bool ShuffleFileDescriptors(const InjectiveMultimap& map) {
+  FileDescriptorTableInjection delegate;
+  return PerformInjectiveMultimap(map, &delegate);
+}
+
+}  // namespace base
+
+#endif  // !BASE_FILE_DESCRIPTOR_SHUFFLE_H_

base/file_descriptor_shuffle_unittest.cc

+// Copyright (c) 2009 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/file_descriptor_shuffle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using base::InjectiveMultimap;
+using base::InjectionArc;
+using base::PerformInjectiveMultimap;
+using base::InjectionDelegate;
+
+namespace {
+typedef testing::Test FileDescriptorShuffleTest;
+}
+
+// 'Duplicated' file descriptors start at this number
+static const int kDuplicateBase = 1000;
+
+struct Action {
+  enum Type {
+    CLOSE,
+    MOVE,
+    DUPLICATE,
+  };
+
+  Action(Type in_type, int in_fd1, int in_fd2 = -1)
+      : type(in_type),
+        fd1(in_fd1),
+        fd2(in_fd2) {
+  }
+
+  bool operator==(const Action& other) const {
+    return other.type == type &&
+           other.fd1 == fd1 &&
+           other.fd2 == fd2;
+  }
+
+  Type type;
+  int fd1;
+  int fd2;
+};
+
+class InjectionTracer : public InjectionDelegate {
+ public:
+  InjectionTracer()
+      : next_duplicate_(kDuplicateBase) {
+  }
+
+  bool Duplicate(int* result, int fd) {
+    *result = next_duplicate_++;
+    actions_.push_back(Action(Action::DUPLICATE, *result, fd));
+    return true;
+  }
+
+  bool Move(int src, int dest) {
+    actions_.push_back(Action(Action::MOVE, src, dest));
+    return true;
+  }
+
+  void Close(int fd) {
+    actions_.push_back(Action(Action::CLOSE, fd));
+  }
+
+  const std::vector<Action>& actions() const { return actions_; }
+
+ private:
+  int next_duplicate_;
+  std::vector<Action> actions_;
+};
+
+TEST(FileDescriptorShuffleTest, Empty) {
+  InjectiveMultimap map;
+  InjectionTracer tracer;
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &tracer));
+  EXPECT_EQ(0u, tracer.actions().size());
+}
+
+TEST(FileDescriptorShuffleTest, Noop) {
+  InjectiveMultimap map;
+  InjectionTracer tracer;
+  map.push_back(InjectionArc(0, 0, false));
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &tracer));
+  EXPECT_EQ(0u, tracer.actions().size());
+}
+
+TEST(FileDescriptorShuffleTest, NoopAndClose) {
+  InjectiveMultimap map;
+  InjectionTracer tracer;
+  map.push_back(InjectionArc(0, 0, true));
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &tracer));
+  EXPECT_EQ(0u, tracer.actions().size());
+}
+
+TEST(FileDescriptorShuffleTest, Simple1) {
+  InjectiveMultimap map;
+  InjectionTracer tracer;
+  map.push_back(InjectionArc(0, 1, false));
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &tracer));
+  EXPECT_EQ(1u, tracer.actions().size());
+  EXPECT_TRUE(tracer.actions()[0] == Action(Action::MOVE, 0, 1));
+}
+
+TEST(FileDescriptorShuffleTest, Simple2) {
+  InjectiveMultimap map;
+  InjectionTracer tracer;
+  map.push_back(InjectionArc(0, 1, false));
+  map.push_back(InjectionArc(2, 3, false));
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &tracer));
+  EXPECT_EQ(2u, tracer.actions().size());
+  EXPECT_TRUE(tracer.actions()[0] == Action(Action::MOVE, 0, 1));
+  EXPECT_TRUE(tracer.actions()[1] == Action(Action::MOVE, 2, 3));
+}
+
+TEST(FileDescriptorShuffleTest, Simple3) {
+  InjectiveMultimap map;
+  InjectionTracer tracer;
+  map.push_back(InjectionArc(0, 1, true));
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &tracer));
+  EXPECT_EQ(2u, tracer.actions().size());
+  EXPECT_TRUE(tracer.actions()[0] == Action(Action::MOVE, 0, 1));
+  EXPECT_TRUE(tracer.actions()[1] == Action(Action::CLOSE, 0));
+}
+
+TEST(FileDescriptorShuffleTest, Simple4) {
+  InjectiveMultimap map;
+  InjectionTracer tracer;
+  map.push_back(InjectionArc(10, 0, true));
+  map.push_back(InjectionArc(1, 1, true));
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &tracer));
+  EXPECT_EQ(2u, tracer.actions().size());
+  EXPECT_TRUE(tracer.actions()[0] == Action(Action::MOVE, 10, 0));
+  EXPECT_TRUE(tracer.actions()[1] == Action(Action::CLOSE, 10));
+}
+
+TEST(FileDescriptorShuffleTest, Cycle) {
+  InjectiveMultimap map;
+  InjectionTracer tracer;
+  map.push_back(InjectionArc(0, 1, false));
+  map.push_back(InjectionArc(1, 0, false));
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &tracer));
+  EXPECT_EQ(4u, tracer.actions().size());
+  EXPECT_TRUE(tracer.actions()[0] ==
+              Action(Action::DUPLICATE, kDuplicateBase, 1));
+  EXPECT_TRUE(tracer.actions()[1] == Action(Action::MOVE, 0, 1));
+  EXPECT_TRUE(tracer.actions()[2] == Action(Action::MOVE, kDuplicateBase, 0));
+  EXPECT_TRUE(tracer.actions()[3] == Action(Action::CLOSE, kDuplicateBase));
+}
+
+TEST(FileDescriptorShuffleTest, CycleAndClose1) {
+  InjectiveMultimap map;
+  InjectionTracer tracer;
+  map.push_back(InjectionArc(0, 1, true));
+  map.push_back(InjectionArc(1, 0, false));
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &tracer));
+  EXPECT_EQ(4u, tracer.actions().size());
+  EXPECT_TRUE(tracer.actions()[0] ==
+              Action(Action::DUPLICATE, kDuplicateBase, 1));
+  EXPECT_TRUE(tracer.actions()[1] == Action(Action::MOVE, 0, 1));
+  EXPECT_TRUE(tracer.actions()[2] == Action(Action::MOVE, kDuplicateBase, 0));
+  EXPECT_TRUE(tracer.actions()[3] == Action(Action::CLOSE, kDuplicateBase));
+}
+
+TEST(FileDescriptorShuffleTest, CycleAndClose2) {
+  InjectiveMultimap map;
+  InjectionTracer tracer;
+  map.push_back(InjectionArc(0, 1, false));
+  map.push_back(InjectionArc(1, 0, true));
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &tracer));
+  EXPECT_EQ(4u, tracer.actions().size());
+  EXPECT_TRUE(tracer.actions()[0] ==
+              Action(Action::DUPLICATE, kDuplicateBase, 1));
+  EXPECT_TRUE(tracer.actions()[1] == Action(Action::MOVE, 0, 1));
+  EXPECT_TRUE(tracer.actions()[2] == Action(Action::MOVE, kDuplicateBase, 0));
+  EXPECT_TRUE(tracer.actions()[3] == Action(Action::CLOSE, kDuplicateBase));
+}
+
+TEST(FileDescriptorShuffleTest, CycleAndClose3) {
+  InjectiveMultimap map;
+  InjectionTracer tracer;
+  map.push_back(InjectionArc(0, 1, true));
+  map.push_back(InjectionArc(1, 0, true));
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &tracer));
+  EXPECT_EQ(4u, tracer.actions().size());
+  EXPECT_TRUE(tracer.actions()[0] ==
+              Action(Action::DUPLICATE, kDuplicateBase, 1));
+  EXPECT_TRUE(tracer.actions()[1] == Action(Action::MOVE, 0, 1));
+  EXPECT_TRUE(tracer.actions()[2] == Action(Action::MOVE, kDuplicateBase, 0));
+  EXPECT_TRUE(tracer.actions()[3] == Action(Action::CLOSE, kDuplicateBase));
+}
+
+TEST(FileDescriptorShuffleTest, Fanout) {
+  InjectiveMultimap map;
+  InjectionTracer tracer;
+  map.push_back(InjectionArc(0, 1, false));
+  map.push_back(InjectionArc(0, 2, false));
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &tracer));
+  EXPECT_EQ(2u, tracer.actions().size());
+  EXPECT_TRUE(tracer.actions()[0] == Action(Action::MOVE, 0, 1));
+  EXPECT_TRUE(tracer.actions()[1] == Action(Action::MOVE, 0, 2));
+}
+
+TEST(FileDescriptorShuffleTest, FanoutAndClose1) {
+  InjectiveMultimap map;
+  InjectionTracer tracer;
+  map.push_back(InjectionArc(0, 1, true));
+  map.push_back(InjectionArc(0, 2, false));
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &tracer));
+  EXPECT_EQ(3u, tracer.actions().size());
+  EXPECT_TRUE(tracer.actions()[0] == Action(Action::MOVE, 0, 1));
+  EXPECT_TRUE(tracer.actions()[1] == Action(Action::MOVE, 0, 2));
+  EXPECT_TRUE(tracer.actions()[2] == Action(Action::CLOSE, 0));
+}
+
+TEST(FileDescriptorShuffleTest, FanoutAndClose2) {
+  InjectiveMultimap map;
+  InjectionTracer tracer;
+  map.push_back(InjectionArc(0, 1, false));
+  map.push_back(InjectionArc(0, 2, true));
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &tracer));
+  EXPECT_EQ(3u, tracer.actions().size());
+  EXPECT_TRUE(tracer.actions()[0] == Action(Action::MOVE, 0, 1));
+  EXPECT_TRUE(tracer.actions()[1] == Action(Action::MOVE, 0, 2));
+  EXPECT_TRUE(tracer.actions()[2] == Action(Action::CLOSE, 0));
+}
+
+TEST(FileDescriptorShuffleTest, FanoutAndClose3) {
+  InjectiveMultimap map;
+  InjectionTracer tracer;
+  map.push_back(InjectionArc(0, 1, true));
+  map.push_back(InjectionArc(0, 2, true));
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &tracer));
+  EXPECT_EQ(3u, tracer.actions().size());
+  EXPECT_TRUE(tracer.actions()[0] == Action(Action::MOVE, 0, 1));
+  EXPECT_TRUE(tracer.actions()[1] == Action(Action::MOVE, 0, 2));
+  EXPECT_TRUE(tracer.actions()[2] == Action(Action::CLOSE, 0));
+}
+
+class FailingDelegate : public InjectionDelegate {
+ public:
+  bool Duplicate(int* result, int fd) {
+    return false;
+  }
+
+  bool Move(int src, int dest) {
+    return false;
+  }
+
+  void Close(int fd) {
+  }
+};
+
+TEST(FileDescriptorShuffleTest, EmptyWithFailure) {
+  InjectiveMultimap map;
+  FailingDelegate failing;
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &failing));
+}
+
+TEST(FileDescriptorShuffleTest, NoopWithFailure) {
+  InjectiveMultimap map;
+  FailingDelegate failing;
+  map.push_back(InjectionArc(0, 0, false));
+
+  EXPECT_TRUE(PerformInjectiveMultimap(map, &failing));
+}
+
+TEST(FileDescriptorShuffleTest, Simple1WithFailure) {
+  InjectiveMultimap map;
+  FailingDelegate failing;
+  map.push_back(InjectionArc(0, 1, false));
+
+  EXPECT_FALSE(PerformInjectiveMultimap(map, &failing));
+}

base/process_util.h

@@ -44,6 +44,8 @@ struct IoCounters {
   unsigned long long WriteTransferCount;
   unsigned long long OtherTransferCount;
 };
+
+#include "base/file_descriptor_shuffle.h"
 #endif
 
 #if defined(OS_MACOSX)
@@ -88,7 +90,14 @@ ProcessId GetProcId(ProcessHandle process);
 #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.
+void CloseSuperfluousFds(const base::InjectiveMultimap& saved_map);
 #endif
 
 #if defined(OS_WIN)

base/process_util_linux.cc

@@ -31,30 +31,22 @@ namespace base {
 bool LaunchApp(const std::vector<std::string>& argv,
                const file_handle_mapping_vector& fds_to_remap,
                bool wait, ProcessHandle* process_handle) {
-  // Make sure we don't leak any FDs to the child process by marking all FDs
-  // as close-on-exec.
-  SetAllFDsToCloseOnExec();
-
   pid_t pid = fork();
   if (pid < 0)
     return false;
 
   if (pid == 0) {
-    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 {
-        dup2(src_fd, dest_fd);
-      }
+    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));
     }
 
+    if (!ShuffleFileDescriptors(fd_shuffle))
+      exit(127);
+
+    CloseSuperfluousFds(fd_shuffle);
+
     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());

base/process_util_posix.cc

@@ -14,6 +14,7 @@
 #include <unistd.h>
 
 #include <limits>
+#include <set>
 
 #include "base/basictypes.h"
 #include "base/logging.h"
@@ -95,8 +96,82 @@ class ScopedDIRClose {
 };
 typedef scoped_ptr_malloc<DIR, ScopedDIRClose> ScopedDIR;
 
+void CloseSuperfluousFds(const base::InjectiveMultimap& saved_mapping) {
+  std::set<int> saved_fds;
+
+  // 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);
+  }
+
+#if defined(OS_LINUX)
+  static const char fd_dir[] = "/proc/self/fd";
+#elif defined(OS_MACOSX)
+  static 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;
+
+    // Fallback case
+    struct rlimit nofile;
+    rlim_t num_fds;
+    if (getrlimit(RLIMIT_NOFILE, &nofile)) {
+      // getrlimit failed. Take a best guess.
+      num_fds = 8192;
+      DLOG(ERROR) << "getrlimit(RLIMIT_NOFILE) failed: " << errno;
+    } else {
+      num_fds = nofile.rlim_cur;
+    }
+
+    if (num_fds > INT_MAX)
+      num_fds = INT_MAX;
+
+    for (rlim_t i = 0; i < num_fds; ++i) {
+      const int fd = static_cast<int>(i);
+      if (saved_fds.find(fd) != saved_fds.end())
+        continue;
+
+      int result;
+      do {
+        result = close(fd);
+      } while (result == -1 && errno == EINTR);
+    }
+    return;
+  }
+
+  struct dirent *ent;
+  while ((ent = readdir(dir))) {
+    // Skip . and .. entries.
+    if (ent->d_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 || fd >= INT_MAX || errno)
+      continue;
+    if (saved_fds.find(fd) != saved_fds.end())
+      continue;
+
+    int result;
+    do {
+      result = close(fd);
+    } while (result == -1 && errno == EINTR);
+  }
+}
+
 // 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";
@@ -354,19 +429,15 @@ bool GetAppOutput(const CommandLine& cl, std::string* output) {
         if (dev_null < 0)
           exit(127);
 
-        int rv;
-        do {
-          rv = dup2(pipe_fd[1], STDOUT_FILENO);
-        } while (rv == -1 && errno == EINTR);
-        do {
-          rv = dup2(dev_null, STDERR_FILENO);
-        } while (rv == -1 && errno == EINTR);
-        if (pipe_fd[0] != STDOUT_FILENO && pipe_fd[0] != STDERR_FILENO)
-          close(pipe_fd[0]);
-        if (pipe_fd[1] != STDOUT_FILENO && pipe_fd[1] != STDERR_FILENO)
-          close(pipe_fd[1]);
-        if (dev_null != STDOUT_FILENO && dev_null != STDERR_FILENO)
-          close(dev_null);
+        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));
+
+        if (!ShuffleFileDescriptors(fd_shuffle))
+          exit(127);
+
+        CloseSuperfluousFds(fd_shuffle);
 
         const std::vector<std::string> argv = cl.argv();
         scoped_array<char*> argv_cstr(new char*[argv.size() + 1]);