Fix LaunchProcess to clone stdio descriptors even with fds_to_map.

Previously we would only clone the stdio FDs into the child if the
option.fds_to_map was empty. Callers instead expect that stdio FDs will
always be cloned into the child, but can also be overridden via the
fds_to_map option.

Bug: 706592
Change-Id: Ifd401b75ceda3b8fe1fb69604ff361ba50b2f40d
Reviewed-on: https://chromium-review.googlesource.com/598749
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491782}

base/process/launch_fuchsia.cc

@@ -115,13 +115,22 @@ Process LaunchProcess(const std::vector<std::string>& argv,
     launchpad_set_environ(lp, new_environ.get());
   else
     to_clone |= LP_CLONE_ENVIRON;
-
-  if (options.fds_to_remap.empty())
-    to_clone |= LP_CLONE_MXIO_STDIO;
   launchpad_clone(lp, to_clone);
 
-  for (const auto& src_target : options.fds_to_remap)
+  // Clone the mapped file-descriptors, plus any of the stdio descriptors
+  // which were not explicitly specified.
+  bool stdio_already_mapped[3] = {false};
+  for (const auto& src_target : options.fds_to_remap) {
+    if (static_cast<size_t>(src_target.second) <
+        arraysize(stdio_already_mapped))
+      stdio_already_mapped[src_target.second] = true;
     launchpad_clone_fd(lp, src_target.first, src_target.second);
+  }
+  for (size_t stdio_fd = 0; stdio_fd < arraysize(stdio_already_mapped);
+       ++stdio_fd) {
+    if (!stdio_already_mapped[stdio_fd])
+      launchpad_clone_fd(lp, stdio_fd, stdio_fd);
+  }
 
   mx_handle_t proc;
   const char* errmsg;

base/process/process_util_unittest.cc

@@ -680,6 +680,68 @@ TEST_F(ProcessUtilTest, MAYBE_FDRemapping) {
   DPCHECK(ret == 0);
 }
 
+const char kPipeValue = '\xcc';
+MULTIPROCESS_TEST_MAIN(ProcessUtilsVerifyStdio) {
+  // Write to stdio so the parent process can observe output.
+  CHECK_EQ(1, HANDLE_EINTR(write(STDOUT_FILENO, &kPipeValue, 1)));
+
+  // Close all of the handles, to verify they are valid.
+  CHECK_EQ(0, IGNORE_EINTR(close(STDIN_FILENO)));
+  CHECK_EQ(0, IGNORE_EINTR(close(STDOUT_FILENO)));
+  CHECK_EQ(0, IGNORE_EINTR(close(STDERR_FILENO)));
+  return 0;
+}
+
+TEST_F(ProcessUtilTest, FDRemappingIncludesStdio) {
+  int dev_null = open("/dev/null", O_RDONLY);
+  ASSERT_LT(2, dev_null);
+
+  // Backup stdio and replace it with the write end of a pipe, for our
+  // child process to inherit.
+  int pipe_fds[2];
+  int result = pipe(pipe_fds);
+  ASSERT_EQ(0, result);
+  int backup_stdio = dup(STDOUT_FILENO);
+  ASSERT_LE(0, backup_stdio);
+  result = dup2(pipe_fds[1], STDOUT_FILENO);
+  ASSERT_EQ(STDOUT_FILENO, result);
+
+  // Launch the test process, which should inherit our pipe stdio.
+  base::LaunchOptions options;
+  options.fds_to_remap.push_back(std::pair<int, int>(dev_null, dev_null));
+  base::SpawnChildResult spawn_child =
+      SpawnChildWithOptions("ProcessUtilsVerifyStdio", options);
+  ASSERT_TRUE(spawn_child.process.IsValid());
+
+  // Restore stdio, so we can output stuff.
+  result = dup2(backup_stdio, STDOUT_FILENO);
+  ASSERT_EQ(STDOUT_FILENO, result);
+
+  // Close our copy of the write end of the pipe, so that the read()
+  // from the other end will see EOF if it wasn't copied to the child.
+  result = IGNORE_EINTR(close(pipe_fds[1]));
+  ASSERT_EQ(0, result);
+
+  result = IGNORE_EINTR(close(backup_stdio));
+  ASSERT_EQ(0, result);
+  result = IGNORE_EINTR(close(dev_null));
+  ASSERT_EQ(0, result);
+
+  // Read from the pipe to verify that it is connected to the child
+  // process' stdio.
+  char buf[16] = {0};
+  EXPECT_EQ(1, HANDLE_EINTR(read(pipe_fds[0], buf, sizeof(buf))));
+  EXPECT_EQ(kPipeValue, buf[0]);
+
+  result = IGNORE_EINTR(close(pipe_fds[0]));
+  ASSERT_EQ(0, result);
+
+  int exit_code;
+  ASSERT_TRUE(spawn_child.process.WaitForExitWithTimeout(
+      base::TimeDelta::FromSeconds(5), &exit_code));
+  EXPECT_EQ(0, exit_code);
+}
+
 namespace {
 
 std::string TestLaunchProcess(const std::vector<std::string>& args,
@@ -922,8 +984,6 @@ MULTIPROCESS_TEST_MAIN(process_util_test_die_immediately) {
 }
 
 #if !defined(OS_ANDROID)
-const char kPipeValue = '\xcc';
-
 class ReadFromPipeDelegate : public base::LaunchOptions::PreExecDelegate {
  public:
   explicit ReadFromPipeDelegate(int fd) : fd_(fd) {}