[remoting] Reopen stdin/stdout after creating PipeMessagingChannel.

PipeMessagingChannel takes ownership of the passed File objects,
closing them. This means that stdin/stdout are closed, to prevent
accidental corruption of the native-messaging streams.

This CL reopens stdin/stdout against /dev/null, which protects them,
preventing any future calls to POSIX open() returning these descriptors.
For example, LaunchProcess (by default) treats FDs 0, 1, 2 specially
(it closes all descriptors except them), so having stdin/stdout directed
to some random file could cause unintended effects.

This CL also fixes PipeMessagingChannel ctor to not interleave the dup()
and close() calls:
dup, dup, close, close
instead of
dup, close, dup, close
so that the second dup() call now returns a high-numbered descriptor
instead of 0 (freed up by first close() call).

Bug: 1029858
Change-Id: I209f5734a06216323c54a680852b381fb6f2f96b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1947397
Commit-Queue: Lambros Lambrou <lambroslambrou@chromium.org>
Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Auto-Submit: Lambros Lambrou <lambroslambrou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#720768}

remoting/host/it2me/it2me_native_messaging_host_main.cc

@@ -238,6 +238,10 @@ int It2MeNativeMessagingHostMain(int argc, char** argv) {
   std::unique_ptr<extensions::NativeMessagingChannel> channel(
       new PipeMessagingChannel(std::move(read_file), std::move(write_file)));
 
+#if defined(OS_POSIX)
+  PipeMessagingChannel::ReopenStdinStdout();
+#endif  // defined(OS_POSIX)
+
   std::unique_ptr<ChromotingHostContext> context =
       ChromotingHostContext::Create(new remoting::AutoThreadTaskRunner(
           main_task_executor.task_runner(), run_loop.QuitClosure()));

remoting/host/native_messaging/pipe_messaging_channel.cc

@@ -9,53 +9,17 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
+#include "base/files/file.h"
 #include "base/location.h"
 #include "base/process/process_info.h"
 #include "base/values.h"
 #include "build/build_config.h"
 
-#if defined(OS_POSIX)
-#include <unistd.h>
-
-#include "base/posix/eintr_wrapper.h"
-#endif
-
-#if defined(OS_WIN)
-#include <windows.h>
-#endif
-
-namespace {
-
-base::File DuplicatePlatformFile(base::File file) {
-  base::PlatformFile result;
-#if defined(OS_WIN)
-  if (!DuplicateHandle(GetCurrentProcess(),
-                       file.TakePlatformFile(),
-                       GetCurrentProcess(),
-                       &result,
-                       0,
-                       FALSE,
-                       DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS)) {
-    PLOG(ERROR) << "Failed to duplicate handle " << file.GetPlatformFile();
-    return base::File();
-  }
-  return base::File(result);
-#elif defined(OS_POSIX)
-  result = HANDLE_EINTR(dup(file.GetPlatformFile()));
-  return base::File(result);
-#else
-#error Not implemented.
-#endif
-}
-
-}  // namespace
-
 namespace remoting {
 
 PipeMessagingChannel::PipeMessagingChannel(base::File input, base::File output)
-    : native_messaging_reader_(DuplicatePlatformFile(std::move(input))),
-      native_messaging_writer_(
-          new NativeMessagingWriter(DuplicatePlatformFile(std::move(output)))),
+    : native_messaging_reader_(input.Duplicate()),
+      native_messaging_writer_(new NativeMessagingWriter(output.Duplicate())),
       event_handler_(nullptr) {
   weak_ptr_ = weak_factory_.GetWeakPtr();
 }
@@ -64,6 +28,21 @@ PipeMessagingChannel::~PipeMessagingChannel() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
+// static
+void PipeMessagingChannel::ReopenStdinStdout() {
+#if defined(OS_POSIX)
+  base::FilePath dev_null("/dev/null");
+  int new_stdin =
+      base::File(dev_null, base::File::FLAG_OPEN | base::File::FLAG_READ)
+          .TakePlatformFile();
+  DCHECK_EQ(new_stdin, STDIN_FILENO);
+  int new_stdout =
+      base::File(dev_null, base::File::FLAG_OPEN | base::File::FLAG_WRITE)
+          .TakePlatformFile();
+  DCHECK_EQ(new_stdout, STDOUT_FILENO);
+#endif  // defined(OS_POSIX)
+}
+
 void PipeMessagingChannel::Start(EventHandler* event_handler) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(!event_handler_);

remoting/host/native_messaging/pipe_messaging_channel.h

@@ -37,6 +37,16 @@ class PipeMessagingChannel : public extensions::NativeMessagingChannel {
   PipeMessagingChannel(base::File input, base::File output);
   ~PipeMessagingChannel() override;
 
+  // If the ctor is called with |input| and |output| set to stdin/stdout,
+  // it will close those file-descriptors. In that case, this helper function
+  // should be used to recreate stdin/stdout as open files. This is needed on
+  // POSIX because a later call to open() will return the lowest available
+  // descriptors, and stdin or stdout could end up pointing at some random file,
+  // which could cause an issue when, say, launching a child process.
+  // This is POSIX-only (a no-op on other platforms) and is thread-unsafe, as it
+  // calls open() twice, expecting it to return 0 then 1.
+  static void ReopenStdinStdout();
+
   // extensions::NativeMessagingChannel implementation.
   void Start(EventHandler* event_handler) override;
   void SendMessage(std::unique_ptr<base::Value> message) override;

remoting/host/setup/me2me_native_messaging_host_main.cc

@@ -271,6 +271,10 @@ int Me2MeNativeMessagingHostMain(int argc, char** argv) {
   std::unique_ptr<extensions::NativeMessagingChannel> channel(
       new PipeMessagingChannel(std::move(read_file), std::move(write_file)));
 
+#if defined(OS_POSIX)
+  PipeMessagingChannel::ReopenStdinStdout();
+#endif  // defined(OS_POSIX)
+
   std::unique_ptr<ChromotingHostContext> context =
       ChromotingHostContext::Create(new remoting::AutoThreadTaskRunner(
           main_task_executor.task_runner(), run_loop.QuitClosure()));