Initialize IPC:ChannelHandle from existing HANDLE

Initialize IPC:ChannelHandle from existing HANDLE

 1] Add a ctor to IPC:ChannelHandle that takes a pipe HANDLE.
 2] Corresponding change in Channel::ChannelImpl::CreatePipe to
    attach to the given pipe instead of creating a new one.

  Being able to hand over a pipe handle to IPC::Channel in this way has
  other advantages such as using anonymous pipes and safer connections
  between process with different level of privileges.   Here's how it
  can be done:

  Server Process:
   - Create a server pipe, anonymous pipe is fine too.
   - Server creates a client handle of pipe using CreateFile.
   - Use DuplicateHandle to duplicate this handle for the client process.
   - pass over the handle value to the client process, say using comman	line.

  Client process:
   - Simply receive the handle from the server and hand it over to
     IPC:Channel using IPC::ChannelHandle.

Apart from being more flexible, this is more secure as it removes
the 'connection window' while using names.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=118786

[sanjeevr, 2012-01-19 15:36:32]

https://chromiumcodereview.appspot.com/9150030/diff/1/ipc/ipc_channel_handle.h
File ipc/ipc_channel_handle.h (right):

https://chromiumcodereview.appspot.com/9150030/diff/1/ipc/ipc_channel_handle....
ipc/ipc_channel_handle.h:42: ChannelHandle(HANDLE h) : pipe(h) {}
A channel handle that is created with only a handle cannot be sent over an IPC
channel (like the Posix one can be). This needs to be mentioned. Or we can use
bInherit as TRUE in the DuplicateHandle call below. This way just marshalling
the handle value over will work. But for that you need to change
ParamTraits<ChannelHandle>

[cpu_(ooo_6.6-7.5), 2012-01-19 20:56:04]

I support the idea, but have questions, see below.

https://chromiumcodereview.appspot.com/9150030/diff/6002/base/win/scoped_hand...
File base/win/scoped_handle.h (right):

https://chromiumcodereview.appspot.com/9150030/diff/6002/base/win/scoped_hand...
base/win/scoped_handle.h:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
2012

https://chromiumcodereview.appspot.com/9150030/diff/6002/base/win/scoped_hand...
base/win/scoped_handle.h:64: operator HANDLE() const { return handle_; }
can you make it so it looks like Get()? line-wise I mean.

https://chromiumcodereview.appspot.com/9150030/diff/6002/base/win/scoped_hand...
base/win/scoped_handle.h:81: 
can you add a method HANDLE Duplicate() const that does like:

*out_handle = NULL or INVALID_HANDLE_VALUE;
DuplicateHandle(GetCurrentProcess(),
                handle,
                GetCurrentProcess(),
                out_handle_,
                0,
                FALSE,
                DUPLICATE_SAME_ACCESS);

https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_hand...
File ipc/ipc_channel_handle.h (right):

https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_hand...
ipc/ipc_channel_handle.h:22: // On POSIX, we instead pass around handles to
channel endpoints via IPC.
With this change we change the behavior significantly, no longer you can pass a
ChannelHandle over ipc to another process and expect that it can open it.

https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_hand...
ipc/ipc_channel_handle.h:68: base::win::ScopedHandle pipe;
so base::FileDescriptor does not close the descriptor on destruction, in other
words is not a scoped handle, so I am thinking that here you should just have
the raw handle so the behavior is the same.

This will also eliminate the duplicatehandle part.

https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_win.cc
File ipc/ipc_channel_win.cc (right):

https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_win....
ipc/ipc_channel_win.cc:117: const std::wstring pipe_name =
PipeName(channel_handle.name);
pipe_name needs to move down. Another idea is to check that !name.empty() and
pipe.IsValid() cannot be true at the same time.

https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_win....
ipc/ipc_channel_win.cc:120: DuplicateHandle(GetCurrentProcess(),
On duplicate failure pipe_ what value does it contain?

Seems prudent to return false here. Or see my comment in the scopedhandle file.

https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_win....
ipc/ipc_channel_win.cc:129: PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED |
indentation seems weird

[amit, 2012-01-19 21:09:19]

I agree with the concern about ScopedHandle closing the ChannelHandle.  I
can remove it and also avoid corresponding issues with ScopedHandle and
revert those changes.

There's no obvious way to fix the limitation of not being able to marshal
ChannelHandle with a pipe. However, I can't imagine a scenario where it
will be needed. I can put in a DCHECK in the marshalling code to catch it?


On Thu, Jan 19, 2012 at 12:56 PM, <cpu@chromium.org> wrote:

> I support the idea, but have questions, see below.
>
>
> https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
>
base/win/scoped_handle.h<https://chromiumcodereview.appspot.com/9150030/diff/6002/base/win/scoped_handle.h>
> File base/win/scoped_handle.h (right):
>
> https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
>
base/win/scoped_handle.h#**newcode1<https://chromiumcodereview.appspot.com/9150030/diff/6002/base/win/scoped_handle.h#newcode1>
> base/win/scoped_handle.h:1: // Copyright (c) 2010 The Chromium Authors.
> All rights reserved.
> 2012
>
> https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
>
base/win/scoped_handle.h#**newcode64<https://chromiumcodereview.appspot.com/9150030/diff/6002/base/win/scoped_handle.h#newcode64>
> base/win/scoped_handle.h:64: operator HANDLE() const { return handle_; }
> can you make it so it looks like Get()? line-wise I mean.
>
> https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
>
base/win/scoped_handle.h#**newcode81<https://chromiumcodereview.appspot.com/9150030/diff/6002/base/win/scoped_handle.h#newcode81>
> base/win/scoped_handle.h:81:
> can you add a method HANDLE Duplicate() const that does like:
>
> *out_handle = NULL or INVALID_HANDLE_VALUE;
> DuplicateHandle(**GetCurrentProcess(),
>                handle,
>                GetCurrentProcess(),
>                out_handle_,
>                0,
>                FALSE,
>                DUPLICATE_SAME_ACCESS);
>
> https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
>
ipc/ipc_channel_handle.h<https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_handle.h>
> File ipc/ipc_channel_handle.h (right):
>
> https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
>
ipc/ipc_channel_handle.h#**newcode22<https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_handle.h#newcode22>
> ipc/ipc_channel_handle.h:22: // On POSIX, we instead pass around handles
> to channel endpoints via IPC.
> With this change we change the behavior significantly, no longer you can
> pass a ChannelHandle over ipc to another process and expect that it can
> open it.
>
> https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
>
ipc/ipc_channel_handle.h#**newcode68<https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_handle.h#newcode68>
> ipc/ipc_channel_handle.h:68: base::win::ScopedHandle pipe;
> so base::FileDescriptor does not close the descriptor on destruction, in
> other words is not a scoped handle, so I am thinking that here you
> should just have the raw handle so the behavior is the same.
>
> This will also eliminate the duplicatehandle part.
>
> https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
>
ipc/ipc_channel_win.cc<https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_win.cc>
> File ipc/ipc_channel_win.cc (right):
>
> https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
>
ipc/ipc_channel_win.cc#**newcode117<https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_win.cc#newcode117>
> ipc/ipc_channel_win.cc:117: const std::wstring pipe_name =
> PipeName(channel_handle.name);
> pipe_name needs to move down. Another idea is to check that
> !name.empty() and pipe.IsValid() cannot be true at the same time.
>
> https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
>
ipc/ipc_channel_win.cc#**newcode120<https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_win.cc#newcode120>
> ipc/ipc_channel_win.cc:120: DuplicateHandle(**GetCurrentProcess(),
> On duplicate failure pipe_ what value does it contain?
>
> Seems prudent to return false here. Or see my comment in the
> scopedhandle file.
>
> https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
>
ipc/ipc_channel_win.cc#**newcode129<https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_win.cc#newcode129>
> ipc/ipc_channel_win.cc:129: PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED |
> indentation seems weird
>
>
https://chromiumcodereview.**appspot.com/9150030/<https://chromiumcodereview....
>

[cpu_(ooo_6.6-7.5), 2012-01-20 04:35:47]

Yes, let's try that

On 2012/01/19 21:09:19, amit wrote:
> I agree with the concern about ScopedHandle closing the ChannelHandle.  I
> can remove it and also avoid corresponding issues with ScopedHandle and
> revert those changes.
> 
> There's no obvious way to fix the limitation of not being able to marshal
> ChannelHandle with a pipe. However, I can't imagine a scenario where it
> will be needed. I can put in a DCHECK in the marshalling code to catch it?
> 
> 
> On Thu, Jan 19, 2012 at 12:56 PM, <mailto:cpu@chromium.org> wrote:
> 
> > I support the idea, but have questions, see below.
> >
> >
> > https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
> >
>
base/win/scoped_handle.h<https://chromiumcodereview.appspot.com/9150030/diff/6002/base/win/scoped_handle.h>
> > File base/win/scoped_handle.h (right):
> >
> > https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
> >
>
base/win/scoped_handle.h#**newcode1<https://chromiumcodereview.appspot.com/9150030/diff/6002/base/win/scoped_handle.h#newcode1>
> > base/win/scoped_handle.h:1: // Copyright (c) 2010 The Chromium Authors.
> > All rights reserved.
> > 2012
> >
> > https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
> >
>
base/win/scoped_handle.h#**newcode64<https://chromiumcodereview.appspot.com/9150030/diff/6002/base/win/scoped_handle.h#newcode64>
> > base/win/scoped_handle.h:64: operator HANDLE() const { return handle_; }
> > can you make it so it looks like Get()? line-wise I mean.
> >
> > https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
> >
>
base/win/scoped_handle.h#**newcode81<https://chromiumcodereview.appspot.com/9150030/diff/6002/base/win/scoped_handle.h#newcode81>
> > base/win/scoped_handle.h:81:
> > can you add a method HANDLE Duplicate() const that does like:
> >
> > *out_handle = NULL or INVALID_HANDLE_VALUE;
> > DuplicateHandle(**GetCurrentProcess(),
> >                handle,
> >                GetCurrentProcess(),
> >                out_handle_,
> >                0,
> >                FALSE,
> >                DUPLICATE_SAME_ACCESS);
> >
> > https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
> >
>
ipc/ipc_channel_handle.h<https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_handle.h>
> > File ipc/ipc_channel_handle.h (right):
> >
> > https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
> >
>
ipc/ipc_channel_handle.h#**newcode22<https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_handle.h#newcode22>
> > ipc/ipc_channel_handle.h:22: // On POSIX, we instead pass around handles
> > to channel endpoints via IPC.
> > With this change we change the behavior significantly, no longer you can
> > pass a ChannelHandle over ipc to another process and expect that it can
> > open it.
> >
> > https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
> >
>
ipc/ipc_channel_handle.h#**newcode68<https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_handle.h#newcode68>
> > ipc/ipc_channel_handle.h:68: base::win::ScopedHandle pipe;
> > so base::FileDescriptor does not close the descriptor on destruction, in
> > other words is not a scoped handle, so I am thinking that here you
> > should just have the raw handle so the behavior is the same.
> >
> > This will also eliminate the duplicatehandle part.
> >
> > https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
> >
>
ipc/ipc_channel_win.cc<https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_win.cc>
> > File ipc/ipc_channel_win.cc (right):
> >
> > https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
> >
>
ipc/ipc_channel_win.cc#**newcode117<https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_win.cc#newcode117>
> > ipc/ipc_channel_win.cc:117: const std::wstring pipe_name =
> > PipeName(channel_handle.name);
> > pipe_name needs to move down. Another idea is to check that
> > !name.empty() and pipe.IsValid() cannot be true at the same time.
> >
> > https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
> >
>
ipc/ipc_channel_win.cc#**newcode120<https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_win.cc#newcode120>
> > ipc/ipc_channel_win.cc:120: DuplicateHandle(**GetCurrentProcess(),
> > On duplicate failure pipe_ what value does it contain?
> >
> > Seems prudent to return false here. Or see my comment in the
> > scopedhandle file.
> >
> > https://chromiumcodereview.**appspot.com/9150030/diff/6002/**
> >
>
ipc/ipc_channel_win.cc#**newcode129<https://chromiumcodereview.appspot.com/9150030/diff/6002/ipc/ipc_channel_win.cc#newcode129>
> > ipc/ipc_channel_win.cc:129: PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED |
> > indentation seems weird
> >
> >
>
https://chromiumcodereview.**appspot.com/9150030/%3Chttps://chromiumcoderevie...>
> >

[amit, 2012-01-20 18:06:52]

Changed ChannelHandle to copy handle value, reverted ScopedHandle and addressed
other comments.

PTAL.

[cpu_(ooo_6.6-7.5), 2012-01-20 22:22:24]

lgtm with comments and if you promise a simple test in a following CL.

http://codereview.chromium.org/9150030/diff/8006/ipc/ipc_channel_handle.h
File ipc/ipc_channel_handle.h (right):

http://codereview.chromium.org/9150030/diff/8006/ipc/ipc_channel_handle.h#new...
ipc/ipc_channel_handle.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
2012

http://codereview.chromium.org/9150030/diff/8006/ipc/ipc_channel_handle.h#new...
ipc/ipc_channel_handle.h:45: ChannelHandle(const char* n) : name(n) {}
I worry that now we have a const char* ctor and a void* ctor, should we add
explicit to one (or both of the ctors?)

http://codereview.chromium.org/9150030/diff/8006/ipc/ipc_channel_win.cc
File ipc/ipc_channel_win.cc (right):

http://codereview.chromium.org/9150030/diff/8006/ipc/ipc_channel_win.cc#newcode1
ipc/ipc_channel_win.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights
reserved.
2012

http://codereview.chromium.org/9150030/diff/8006/ipc/ipc_channel_win.cc#newco...
ipc/ipc_channel_win.cc:130: return false;
How do you feel about calling GetNamedPipeInfo  and checking we are getting what
we expect. In particular I would like to check if this is the server side or the
client side and it is a byte mode pipe.

[amit, 2012-01-20 23:20:47]

Added a test, made the HANDLE ctor explicit, verified mode using
GetNamedPipeInfo along other suggested changes.  

Let me know if you wish to take another look.

[jam, 2012-01-20 23:25:06]

http://codereview.chromium.org/9150030/diff/15002/ipc/ipc_channel_handle.h
File ipc/ipc_channel_handle.h (right):

http://codereview.chromium.org/9150030/diff/15002/ipc/ipc_channel_handle.h#ne...
ipc/ipc_channel_handle.h:16: #if defined(OS_WIN)
nit: in this file and ipc_message_utils.cc, if there's a OS_POSIX/OS_WIN follow
each other, it's slightly more readable to use #elif

[amit, 2012-01-23 07:20:06]

Changed in ipc_channel_handle.h but not in ipc_message_utils.cc as I didn't want
to change the order of params.

[cpu_(ooo_6.6-7.5), 2012-01-23 18:55:34]

lgtm on the changes, one important comment below:

http://codereview.chromium.org/9150030/diff/16001/ipc/ipc_channel_win.cc
File ipc/ipc_channel_win.cc (right):

http://codereview.chromium.org/9150030/diff/16001/ipc/ipc_channel_win.cc#newc...
ipc/ipc_channel_win.cc:140: DCHECK(flags & PIPE_SERVER_END);
This should be return false; here and in line 142.

[amit, 2012-01-23 19:58:40]

http://codereview.chromium.org/9150030/diff/16001/ipc/ipc_channel_win.cc
File ipc/ipc_channel_win.cc (right):

http://codereview.chromium.org/9150030/diff/16001/ipc/ipc_channel_win.cc#newc...
ipc/ipc_channel_win.cc:140: DCHECK(flags & PIPE_SERVER_END);
On 2012/01/23 18:55:34, cpu wrote:
> This should be return false; here and in line 142.

Done.

[commit-bot: I haz the power, 2012-01-23 22:38:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/amit@chromium.org/9150030/19002

[commit-bot: I haz the power, 2012-01-23 22:38:15]

Presubmit check for 9150030-19002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
New code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.
    ipc/ipc_channel_win.cc:117

Presubmit checks took 1.0s to calculate.

[commit-bot: I haz the power, 2012-01-24 00:57:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/amit@chromium.org/9150030/22002

[commit-bot: I haz the power, 2012-01-24 02:36:07]

Change committed as 118786