The Windows portion of Native Messagaing

chrome/browser/extensions/api/messaging/native_process_launcher_win.cc

 // Copyright (c) 2012 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 "chrome/browser/extensions/api/messaging/native_process_launcher.h"
 
 #include <windows.h>
 
+#include "base/command_line.h"
 #include "base/logging.h"
 #include "base/process_util.h"
 #include "base/stringprintf.h"
 #include "base/string_number_conversions.h"
+#include "base/utf_string_conversions.h"
+
+namespace {
+
+int pipe_counter = 0;
+
+}  // namespace
 
 namespace extensions {
 
 bool NativeProcessLauncher::LaunchNativeProcess(

[rvargas (doing something else), 2012/11/29 01:19:23]

We cannot do this on the IO thread. There's a bunch of code here that blocks.

[eaugusti, 2012/12/07 02:21:08]

Ok, the NativeProcessHost itself lives on the IO thread (see comment in
message_service.cc) but we already have to move to the FILE thread for a path
check. So we can launch there.

     const FilePath& path,
     base::ProcessHandle* native_process_handle,
     NativeMessageProcessHost::FileHandle* read_file,
     NativeMessageProcessHost::FileHandle* write_file) const {
-  NOTREACHED();
-  return false;
+  SECURITY_ATTRIBUTES sa_attr;
+  // Set the bInheritHandle flag so pipe handles are inherited.
+  sa_attr.nLength = sizeof(SECURITY_ATTRIBUTES);
+  sa_attr.bInheritHandle = TRUE;
+  sa_attr.lpSecurityDescriptor = NULL;
+
+  DWORD pipe_open_mode = PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED;
+  DWORD pipe_mode = PIPE_TYPE_BYTE | PIPE_READMODE_BYTE;
+  DWORD max_instances = 1;
+  DWORD buffer_size = 10 * 1024 * 1024;
+  // TODO(eriq): How long to make the timeout?
+  DWORD timeout = 5000;
+
+  string16 out_pipe_name = ASCIIToWide(base::StringPrintf(
+      "\\\\.\\pipe\\chrome.nativeMessage.out.%d", ++pipe_counter));
+  string16 in_pipe_name = ASCIIToWide(base::StringPrintf(
+      "\\\\.\\pipe\\chrome.nativeMessage.in.%d", ++pipe_counter));
+
+  HANDLE stdin_parent_pipe = NULL;
+  HANDLE stdout_parent_pipe = NULL;
+
+  HANDLE stdin_child_pipe = NULL;
+  HANDLE stdout_child_pipe = NULL;
+
+  // Create the pipes to read and write from.
+  stdout_parent_pipe = CreateNamedPipeW(out_pipe_name.c_str(), pipe_open_mode,
+                                        pipe_mode, max_instances, buffer_size,
+                                        buffer_size, timeout, &sa_attr);
+  if (stdout_parent_pipe == INVALID_HANDLE_VALUE) {
+    NOTREACHED() << "Failed to create pipe";

[rvargas (doing something else), 2012/11/29 01:19:23]

we should not really dcheck on the return value of a system call. It will fail
at some point.

[Sergey Ulanov, 2012/12/01 01:23:00]

I think NOTREACHED() is the right thing to do here. It fails in debug builds
allowing to detect if there is an issue, yet handles the problem properly in
Release builds. base code does it in some places as well.

[rvargas (doing something else), 2012/12/01 01:57:24]

This pattern makes the developer who writes the code confident that it doesn't
fail because it works fine on a local machine, release builds never report an
issue from the field, and random developers once in a while get DCHECKs without
any actual action item because it really requires some planetary alignment to
cause the OS to fail the operation.

NOTREACHED should check preconditions of the code itself (under direct control
of the programmer). System calls are not that.

If you are seriously worried about some related change causing this code to
fail, a DLOG somewhere sounds like a better solution (although DLOGs will bloat
the code more than NOTREACHED).

[eaugusti, 2012/12/07 02:21:08]

What about using a DPLOG?
It makes sense not to use NOTREACHED() because it is used to catch programmer
errors, and not system errors. My main concern is that something happens on some
trybot and someone is left without any indication of what failed in the this
sequence of system calls.

Is the bloat an issue here? (Forgive my ignorance, I though D*LOG was pretty
cheap).

[rvargas (doing something else), 2012/12/12 00:20:22]

You may be surprised by the amount of code inserted by a log... definitely looks
more innocuous than what it is.

D(P)LOGs are ok, but I would log once from the caller as opposed to adding 5 log
points from this code.

+    return false;
+  }
+
+  DWORD desired_access = GENERIC_READ | GENERIC_WRITE;
+  DWORD share_mode = FILE_SHARE_READ | FILE_SHARE_WRITE;
+
+  stdout_child_pipe = CreateFileW(out_pipe_name.c_str(), desired_access,
+                                  share_mode, &sa_attr, OPEN_EXISTING,
+                                  FILE_FLAG_OVERLAPPED, NULL);
+  if (stdout_child_pipe == INVALID_HANDLE_VALUE) {
+    NOTREACHED() << "Failed to create pipe";
+    return false;
+  }
+
+  stdin_parent_pipe = CreateNamedPipeW(in_pipe_name.c_str(), pipe_open_mode,
+                                       pipe_mode, max_instances, buffer_size,
+                                       buffer_size, timeout, &sa_attr);
+  if (stdin_parent_pipe == INVALID_HANDLE_VALUE) {
+    NOTREACHED() << "Failed to create pipe";
+    return false;
+  }
+
+  stdin_child_pipe = CreateFileW(in_pipe_name.c_str(), desired_access,
+                                 share_mode, &sa_attr, OPEN_EXISTING,
+                                 FILE_FLAG_OVERLAPPED, NULL);
+  if (stdin_child_pipe == INVALID_HANDLE_VALUE) {
+    NOTREACHED() << "Failed to create pipe";
+    return false;
+  }
+
+  // Don't inherit the parents'
+  if (!SetHandleInformation(stdin_parent_pipe, HANDLE_FLAG_INHERIT, 0) ||
+      !SetHandleInformation(stdout_parent_pipe, HANDLE_FLAG_INHERIT, 0)) {
+    NOTREACHED() << "Failed to enable pipe inheritance";
+    return false;
+  }
+
+  CommandLine line(path);
+  base::LaunchOptions options;
+  // Need to inherit the file handles.
+  options.inherit_handles = true;

[rvargas (doing something else), 2012/11/29 01:19:23]

am I reading this correctly? it looks like the new process will inherit the
browser handles. That's a no-no.

[Sergey Ulanov, 2012/12/01 01:23:00]

I'm not an expect, but according to MSDN (
http://msdn.microsoft.com/en-us/library/windows/desktop/ms724466(v=vs.85).aspx )
this flag doesn't really mean that the child process inherits all handles. Just
the ones that we explicitly allow to be inherited.

[rvargas (doing something else), 2012/12/01 01:57:24]

Correct, only handles marked for inheritance are inherited, but we still don't
use potentially dangerous patterns with the browser.

[Sergey Ulanov, 2012/12/04 23:50:43]

Do you have any suggestions on how we can launch an external process with
redirected stdin and stdout without using this flag?

In chromium code bInheritHandle is set to true for file descriptors only in a
few places and in most cases it's specifically to allow stdio redirection, so it
should be safe to use this flag here, but I agree that it's better to be careful
about it.

[rvargas (doing something else), 2012/12/05 20:59:05]

Yeah, that's a pain in the back. I only see using an intermediate process to
launch the target app: only inherit handles from that intermediate process,
which by definition has very little code to go over. The additional overhead
would be fine considering the use case. But it requires setting up a regular IPC
to pass the handles to the intermediate process :(

While it is true that the native process probably has access to do anything it
wants, so it may be able to extract whatever it wishes from the browser process,
I still think it is better to make sure we are not giving direct access to
something that we don't want to, and inspecting the code now looking for
inheritable handles doesn't mean we'll be fine in six months.

Now I'm not so sure the extra code is worth it.

[rvargas (doing something else), 2012/12/06 19:15:06]

Note that a better option is to use another IPC mechanism instead of redirection
of standard IO. For example, passing a named pipe in the command line is trivial
and very robust (both from the point of view of our code and the native app),
and AFAIK it is the mechanism selected for Pepper (uniformity being another
plus).

+  options.stdin_handle = stdin_child_pipe;
+  options.stdout_handle = stdout_child_pipe;
+  options.start_hidden = true;
+
+  if (!base::LaunchProcess(line.GetCommandLineString(), options,
+                           native_process_handle)) {
+    LOG(ERROR) << "Error launching process";
+    return false;
+  }
+
+  *read_file = stdout_parent_pipe;
+  *write_file = stdin_parent_pipe;
+
+  return true;
 }
 
 }  // namespace extensions