Process: allow closed target fd. am: 1193286ebe

am: e6218de3c6

Change-Id: Ibdf909e9a32b844a6169775ffcd1b5c9463c1fa4

2 files changed, 28 insertions, 39 deletions

diff --git a/brillo/process.cc b/brillo/process.cc
index 8773244..ead6f20 100644
--- a/brillo/process.cc
+++ b/brillo/process.cc
@@ -20,6 +20,7 @@
 #include <base/logging.h>
 #include <base/memory/ptr_util.h>
 #include <base/posix/eintr_wrapper.h>
+#include <base/posix/file_descriptor_shuffle.h>
 #include <base/process/process_metrics.h>
 #include <base/strings/string_number_conversions.h>
 #include <base/strings/string_util.h>
@@ -141,20 +142,6 @@ int ProcessImpl::GetPipe(int child_fd) {
 }
 
 bool ProcessImpl::PopulatePipeMap() {
-  // Verify all target fds are already open.  With this assumption we
-  // can be sure that the pipe fds created below do not overlap with
-  // any of the target fds which simplifies how we dup2 to them.  Note
-  // that multi-threaded code could close i->first between this loop
-  // and the next.
-  for (PipeMap::iterator i = pipe_map_.begin(); i != pipe_map_.end(); ++i) {
-    struct stat stat_buffer;
-    if (fstat(i->first, &stat_buffer) < 0) {
-      int saved_errno = errno;
-      LOG(ERROR) << "Unable to fstat fd " << i->first << ": " << saved_errno;
-      return false;
-    }
-  }
-
   for (PipeMap::iterator i = pipe_map_.begin(); i != pipe_map_.end(); ++i) {
     if (i->second.is_bound_) {
       // already have a parent fd, and the child fd gets dup()ed later.
@@ -243,24 +230,19 @@ bool ProcessImpl::Start() {
     if (close_unused_file_descriptors_) {
       CloseUnusedFileDescriptors();
     }
-    // Close parent's side of the child pipes. dup2 ours into place and
-    // then close our ends.
-    for (PipeMap::iterator i = pipe_map_.begin(); i != pipe_map_.end(); ++i) {
-      if (i->second.parent_fd_ != -1)
-        IGNORE_EINTR(close(i->second.parent_fd_));
-      // If we want to bind a fd to the same fd in the child, we don't need to
-      // close and dup2 it.
-      if (i->second.child_fd_ == i->first)
-        continue;
-      HANDLE_EINTR(dup2(i->second.child_fd_, i->first));
+
+    base::InjectiveMultimap fd_shuffle;
+    for (const auto& it : pipe_map_) {
+      // Close parent's side of the child pipes.
+      if (it.second.parent_fd_ != -1)
+        IGNORE_EINTR(close(it.second.parent_fd_));
+
+      fd_shuffle.emplace_back(it.second.child_fd_, it.first, true);
     }
-    // Defer the actual close() of the child fd until afterward; this lets the
-    // same child fd be bound to multiple fds using BindFd. Don't close the fd
-    // if it was bound to itself.
-    for (PipeMap::iterator i = pipe_map_.begin(); i != pipe_map_.end(); ++i) {
-      if (i->second.child_fd_ == i->first)
-        continue;
-      IGNORE_EINTR(close(i->second.child_fd_));
+
+    if (!base::ShuffleFileDescriptors(&fd_shuffle)) {
+      PLOG(ERROR) << "Could not shuffle file descriptors";
+      _exit(kErrorExitStatus);
     }
 
     if (!input_file_.empty()) {
diff --git a/brillo/process_unittest.cc b/brillo/process_unittest.cc
index d980e2b..f65cf34 100644
--- a/brillo/process_unittest.cc
+++ b/brillo/process_unittest.cc
@@ -28,7 +28,6 @@ static const char kBinStat[] = "/usr/bin/stat";
 
 static const char kBinSh[] = SYSTEM_PREFIX "/bin/sh";
 static const char kBinCat[] = SYSTEM_PREFIX "/bin/cat";
-static const char kBinCp[] = SYSTEM_PREFIX "/bin/cp";
 static const char kBinEcho[] = SYSTEM_PREFIX "/bin/echo";
 static const char kBinFalse[] = SYSTEM_PREFIX "/bin/false";
 static const char kBinSleep[] = SYSTEM_PREFIX "/bin/sleep";
@@ -181,11 +180,11 @@ TEST_F(ProcessTest, BadExecutable) {
 }
 
 void ProcessTest::CheckStderrCaptured() {
-  std::string contents;
   process_.AddArg(kBinSh);
   process_.AddArg("-c");
   process_.AddArg("echo errormessage 1>&2 && exit 1");
   EXPECT_EQ(1, process_.Run());
+  std::string contents;
   EXPECT_TRUE(base::ReadFileToString(FilePath(output_file_), &contents));
   EXPECT_NE(std::string::npos, contents.find("errormessage"));
   EXPECT_EQ("", GetLog());
@@ -207,7 +206,6 @@ FilePath ProcessTest::GetFdPath(int fd) {
 }
 
 TEST_F(ProcessTest, RedirectStderrUsingPipe) {
-  std::string contents;
   process_.RedirectOutput("");
   process_.AddArg(kBinSh);
   process_.AddArg("-c");
@@ -219,6 +217,7 @@ TEST_F(ProcessTest, RedirectStderrUsingPipe) {
   EXPECT_GE(pipe_fd, 0);
   EXPECT_EQ(-1, process_.GetPipe(STDOUT_FILENO));
   EXPECT_EQ(-1, process_.GetPipe(STDIN_FILENO));
+  std::string contents;
   EXPECT_TRUE(base::ReadFileToString(GetFdPath(pipe_fd), &contents));
   EXPECT_NE(std::string::npos, contents.find("errormessage"));
   EXPECT_EQ("", GetLog());
@@ -228,15 +227,23 @@ TEST_F(ProcessTest, RedirectStderrUsingPipeWhenPreviouslyClosed) {
   int saved_stderr = dup(STDERR_FILENO);
   close(STDERR_FILENO);
   process_.RedirectOutput("");
-  process_.AddArg(kBinCp);
+  process_.AddArg(kBinSh);
+  process_.AddArg("-c");
+  process_.AddArg("echo errormessage >&2 && exit 1");
   process_.RedirectUsingPipe(STDERR_FILENO, false);
-  EXPECT_FALSE(process_.Start());
-  EXPECT_TRUE(FindLog("Unable to fstat fd 2:"));
+  EXPECT_EQ(1, process_.Run());
+  int pipe_fd = process_.GetPipe(STDERR_FILENO);
+  EXPECT_GE(pipe_fd, 0);
+  EXPECT_EQ(-1, process_.GetPipe(STDOUT_FILENO));
+  EXPECT_EQ(-1, process_.GetPipe(STDIN_FILENO));
+  std::string contents;
+  EXPECT_TRUE(base::ReadFileToString(GetFdPath(pipe_fd), &contents));
+  EXPECT_NE(std::string::npos, contents.find("errormessage"));
+  EXPECT_EQ("", GetLog());
   dup2(saved_stderr, STDERR_FILENO);
 }
 
 TEST_F(ProcessTest, RedirectStdoutUsingPipe) {
-  std::string contents;
   process_.RedirectOutput("");
   process_.AddArg(kBinEcho);
   process_.AddArg("hello world\n");
@@ -247,13 +254,13 @@ TEST_F(ProcessTest, RedirectStdoutUsingPipe) {
   EXPECT_GE(pipe_fd, 0);
   EXPECT_EQ(-1, process_.GetPipe(STDERR_FILENO));
   EXPECT_EQ(-1, process_.GetPipe(STDIN_FILENO));
+  std::string contents;
   EXPECT_TRUE(base::ReadFileToString(GetFdPath(pipe_fd), &contents));
   EXPECT_NE(std::string::npos, contents.find("hello world\n"));
   EXPECT_EQ("", GetLog());
 }
 
 TEST_F(ProcessTest, RedirectStdinUsingPipe) {
-  std::string contents;
   const char kMessage[] = "made it!\n";
   process_.AddArg(kBinCat);
   process_.RedirectUsingPipe(STDIN_FILENO, true);