Add stdin_handle and stdout_handle in base::LaunchOptions.

Add stdin_handle and stdout_handle in base::LaunchOptions.

New members in LaunchOptions will allow to redirect stdin and stdout on
Windows.

This change was originally implemented by eaugusti@chromium.com in
crrev.com/10918255 .

BUG=142915
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175268

[Sergey Ulanov, 2012-12-01 01:14:51]

+rvargas
Ricardo, I've addressed your comments from
https://codereview.chromium.org/10918255 in this CL.

[rvargas (doing something else), 2012-12-01 01:40:45]

https://codereview.chromium.org/11419267/diff/3001/base/process_util_win.cc
File base/process_util_win.cc (right):

https://codereview.chromium.org/11419267/diff/3001/base/process_util_win.cc#n...
base/process_util_win.cc:299: options.stdin_handle :
GetStdHandle(STD_INPUT_HANDLE);
What happens when the current process has a non-default handle and the caller
does not specify all handles? We'll attempt to pass that value to the child
process :(

How is this intended to work with chromium code in general? Do we expect the
current process to allow handle inheritance? We'd have to document that
requirement.

[Sergey Ulanov, 2012-12-05 00:23:14]

https://codereview.chromium.org/11419267/diff/3001/base/process_util_win.cc
File base/process_util_win.cc (right):

https://codereview.chromium.org/11419267/diff/3001/base/process_util_win.cc#n...
base/process_util_win.cc:299: options.stdin_handle :
GetStdHandle(STD_INPUT_HANDLE);
On 2012/12/01 01:40:45, rvargas wrote:
> What happens when the current process has a non-default handle and the caller
> does not specify all handles? We'll attempt to pass that value to the child
> process :(

Isn't this the default behavior, i.e. aren't stdin and stdout inherited in the
child processes by default? In either case this is consistent with
base::GetAppOutput()

> 
> How is this intended to work with chromium code in general? Do we expect the
> current process to allow handle inheritance? We'd have to document that
> requirement.

Good point. Added comments and DCHECK().

[rvargas (doing something else), 2012-12-05 21:51:35]

https://codereview.chromium.org/11419267/diff/3001/base/process_util_win.cc
File base/process_util_win.cc (right):

https://codereview.chromium.org/11419267/diff/3001/base/process_util_win.cc#n...
base/process_util_win.cc:299: options.stdin_handle :
GetStdHandle(STD_INPUT_HANDLE);
On 2012/12/05 00:23:15, sergeyu wrote:
> On 2012/12/01 01:40:45, rvargas wrote:
> > What happens when the current process has a non-default handle and the
caller
> > does not specify all handles? We'll attempt to pass that value to the child
> > process :(
> 
> Isn't this the default behavior, i.e. aren't stdin and stdout inherited in the
> child processes by default? In either case this is consistent with
> base::GetAppOutput()

Sorry, I don't really follow this. What do you mean by default behavior?. stdxx
should not be inherited by default... a process is wired to the console it
belongs to. Of more exactly, the handles used for STD_XX_HANDLE should not be
inherited by default.

As for base::GetAppOutput, I don't see who uses that on Windows other than tests
(and some Upgrade code), so most likely the scenario is quite restricted. In any
case, that should not be the case for base code (or at least it should be
documented).

Just to be clear, if I have redirected IO and use this function to redirect
stdout to a specific file, the child process will get my stdin as a bonus, and
that's not good.

> 
> > 
> > How is this intended to work with chromium code in general? Do we expect the
> > current process to allow handle inheritance? We'd have to document that
> > requirement.
> 
> Good point. Added comments and DCHECK().

[Sergey Ulanov, 2012-12-05 22:38:53]

https://codereview.chromium.org/11419267/diff/3001/base/process_util_win.cc
File base/process_util_win.cc (right):

https://codereview.chromium.org/11419267/diff/3001/base/process_util_win.cc#n...
base/process_util_win.cc:299: options.stdin_handle :
GetStdHandle(STD_INPUT_HANDLE);
On 2012/12/05 21:51:36, rvargas wrote:
> On 2012/12/05 00:23:15, sergeyu wrote:
> > On 2012/12/01 01:40:45, rvargas wrote:
> > > What happens when the current process has a non-default handle and the
> caller
> > > does not specify all handles? We'll attempt to pass that value to the
child
> > > process :(
> > 
> > Isn't this the default behavior, i.e. aren't stdin and stdout inherited in
the
> > child processes by default? In either case this is consistent with
> > base::GetAppOutput()
> 
> Sorry, I don't really follow this. What do you mean by default behavior?.
stdxx
> should not be inherited by default... a process is wired to the console it
> belongs to. Of more exactly, the handles used for STD_XX_HANDLE should not be
> inherited by default.

I didn't know about it. Is it documented anywhere in MSDN? I couldn't find much
information about default behavior and assumed that it's the same as with POSIX
systems (where stdio streams are inherited). What's the right way to get the
default behavior (redirect stdio to console)?

> 
> As for base::GetAppOutput, I don't see who uses that on Windows other than
tests
> (and some Upgrade code), so most likely the scenario is quite restricted. In
any
> case, that should not be the case for base code (or at least it should be
> documented).
> 
> Just to be clear, if I have redirected IO and use this function to redirect
> stdout to a specific file, the child process will get my stdin as a bonus, and
> that's not good.
> 

Yes, I see the issue you are concerned about. I've added DCHECK to verify that
stdin_handle is always set here.
Note that if we ever use Chrome's stdin for anything important then there is
still problem on POSIX systems where stdin is inherited by default when another
process is started from chrome and there are no safeguards to unsure it's always
redirected properly.


> > 
> > > 
> > > How is this intended to work with chromium code in general? Do we expect
the
> > > current process to allow handle inheritance? We'd have to document that
> > > requirement.
> > 
> > Good point. Added comments and DCHECK().
>

[rvargas (doing something else), 2012-12-06 00:52:16]

lgtm after addressing the nit.

https://codereview.chromium.org/11419267/diff/3001/base/process_util_win.cc
File base/process_util_win.cc (right):

https://codereview.chromium.org/11419267/diff/3001/base/process_util_win.cc#n...
base/process_util_win.cc:299: options.stdin_handle :
GetStdHandle(STD_INPUT_HANDLE);
On 2012/12/05 22:38:53, sergeyu wrote:
> On 2012/12/05 21:51:36, rvargas wrote:
> > On 2012/12/05 00:23:15, sergeyu wrote:
> > > On 2012/12/01 01:40:45, rvargas wrote:
> > > > What happens when the current process has a non-default handle and the
> > caller
> > > > does not specify all handles? We'll attempt to pass that value to the
> child
> > > > process :(
> > > 
> > > Isn't this the default behavior, i.e. aren't stdin and stdout inherited in
> the
> > > child processes by default? In either case this is consistent with
> > > base::GetAppOutput()
> > 
> > Sorry, I don't really follow this. What do you mean by default behavior?.
> stdxx
> > should not be inherited by default... a process is wired to the console it
> > belongs to. Of more exactly, the handles used for STD_XX_HANDLE should not
be
> > inherited by default.
> 
> I didn't know about it. Is it documented anywhere in MSDN? I couldn't find
much
> information about default behavior and assumed that it's the same as with
POSIX
> systems (where stdio streams are inherited). What's the right way to get the
> default behavior (redirect stdio to console)?

If the default behavior were to inherit handles there wouldn't be a need to set
STARTF_USESTDHANDLES here, right?

As long as there is a console, stdout should be directed to that console (note
that not all Windows apps have consoles). There are specific functions to
manipulate consoles, and a console is always available even if the standard IO
handles are redirected to something else.

http://msdn.microsoft.com/en-us/library/windows/desktop/ms682055(v=vs.85).aspx

> 
> > 
> > As for base::GetAppOutput, I don't see who uses that on Windows other than
> tests
> > (and some Upgrade code), so most likely the scenario is quite restricted. In
> any
> > case, that should not be the case for base code (or at least it should be
> > documented).
> > 
> > Just to be clear, if I have redirected IO and use this function to redirect
> > stdout to a specific file, the child process will get my stdin as a bonus,
and
> > that's not good.
> > 
> 
> Yes, I see the issue you are concerned about. I've added DCHECK to verify that
> stdin_handle is always set here.
> Note that if we ever use Chrome's stdin for anything important then there is
> still problem on POSIX systems where stdin is inherited by default when
another
> process is started from chrome and there are no safeguards to unsure it's
always
> redirected properly.
> 
> 
> > > 
> > > > 
> > > > How is this intended to work with chromium code in general? Do we expect
> the
> > > > current process to allow handle inheritance? We'd have to document that
> > > > requirement.
> > > 
> > > Good point. Added comments and DCHECK().
> > 
>

https://codereview.chromium.org/11419267/diff/12001/base/process_util.h
File base/process_util.h (right):

https://codereview.chromium.org/11419267/diff/12001/base/process_util.h#newco...
base/process_util.h:290: // |stdin_handle| must be set too (i.e. stdin must
always be redirected if any
Why not just say that if any handle must be inherited all three handles must be
set?. Messing stderr (or stdout) sounds almost as bad as reading my input :)

[brettw, 2012-12-07 19:12:50]

owners lgtm based on ricardo's review.

[Sergey Ulanov, 2012-12-07 23:11:56]

https://codereview.chromium.org/11419267/diff/3001/base/process_util_win.cc
File base/process_util_win.cc (right):

https://codereview.chromium.org/11419267/diff/3001/base/process_util_win.cc#n...
base/process_util_win.cc:299: options.stdin_handle :
GetStdHandle(STD_INPUT_HANDLE);
On 2012/12/06 00:52:16, rvargas wrote:
> On 2012/12/05 22:38:53, sergeyu wrote:
> > On 2012/12/05 21:51:36, rvargas wrote:
> > > On 2012/12/05 00:23:15, sergeyu wrote:
> > > > On 2012/12/01 01:40:45, rvargas wrote:
> > > > > What happens when the current process has a non-default handle and the
> > > caller
> > > > > does not specify all handles? We'll attempt to pass that value to the
> > child
> > > > > process :(
> > > > 
> > > > Isn't this the default behavior, i.e. aren't stdin and stdout inherited
in
> > the
> > > > child processes by default? In either case this is consistent with
> > > > base::GetAppOutput()
> > > 
> > > Sorry, I don't really follow this. What do you mean by default behavior?.
> > stdxx
> > > should not be inherited by default... a process is wired to the console it
> > > belongs to. Of more exactly, the handles used for STD_XX_HANDLE should not
> be
> > > inherited by default.
> > 
> > I didn't know about it. Is it documented anywhere in MSDN? I couldn't find
> much
> > information about default behavior and assumed that it's the same as with
> POSIX
> > systems (where stdio streams are inherited). What's the right way to get the
> > default behavior (redirect stdio to console)?
> 
> If the default behavior were to inherit handles there wouldn't be a need to
set
> STARTF_USESTDHANDLES here, right?
> 

What I was asking about is if there is a way to redirect one of the streams and
keep default behavior for others.

https://codereview.chromium.org/11419267/diff/12001/base/process_util.h
File base/process_util.h (right):

https://codereview.chromium.org/11419267/diff/12001/base/process_util.h#newco...
base/process_util.h:290: // |stdin_handle| must be set too (i.e. stdin must
always be redirected if any
On 2012/12/06 00:52:16, rvargas wrote:
> Why not just say that if any handle must be inherited all three handles must
be
> set?. Messing stderr (or stdout) sounds almost as bad as reading my input :)

Done.

[commit-bot: I haz the power, 2012-12-07 23:30:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11419267/18001

[rvargas (doing something else), 2012-12-07 23:46:52]

https://codereview.chromium.org/11419267/diff/3001/base/process_util_win.cc
File base/process_util_win.cc (right):

https://codereview.chromium.org/11419267/diff/3001/base/process_util_win.cc#n...
base/process_util_win.cc:299: options.stdin_handle :
GetStdHandle(STD_INPUT_HANDLE);
> What I was asking about is if there is a way to redirect one of the streams
and
> keep default behavior for others.

I suspect passing INVALID_HANDLE_VALUE has that effect (aka not passing a
handle), but I have never tried that.

[commit-bot: I haz the power, 2012-12-08 00:04:57]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[jschuh, 2012-12-09 00:38:51]

I hate to drive-by after the fact, but this should really be using
PROC_THREAD_ATTRIBUTE_HANDLE_LIST to avoid handle pollution on Vista and above.
The easiest way to do that is to use base::win::StartupInformation to manage the
additional attribute.

[rvargas (doing something else), 2012-12-10 19:05:29]

On 2012/12/09 00:38:51, Justin Schuh wrote:
> I hate to drive-by after the fact, but this should really be using
> PROC_THREAD_ATTRIBUTE_HANDLE_LIST to avoid handle pollution on Vista and
above.
> The easiest way to do that is to use base::win::StartupInformation to manage
the
> additional attribute.

I thought about that, but given that
- A large percentage of users of this API are tools/tests
- We still have to support XP
- This CL is not directly usable in production code anyway (as in to launch a
random process from the browser)

I thought the extra work (multiple implementations) was not worth the trouble.

Maybe we need to spell out the limitations on the header?

[commit-bot: I haz the power, 2013-01-03 00:16:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11419267/18001

[jschuh, 2013-01-03 00:22:01]

This patch launches the process in an unsafe manner. If you're going to land it,
then at a minimum you need comments clearly explaining that the handle
redirection mechanism is for test code only, and is not safe in production code.

[commit-bot: I haz the power, 2013-01-03 00:42:37]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-03 00:42:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11419267/32003

[jschuh, 2013-01-03 01:19:27]

On 2013/01/03 00:42:44, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/sergeyu%40chromium.org/11419267/32003

Sorry, but as I stated above, I can't let you check this in without at least a
comment explaining why this cannot be used in production code.

[Sergey Ulanov, 2013-01-03 18:57:14]

On 2013/01/03 01:19:27, Justin Schuh wrote:
> On 2013/01/03 00:42:44, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >
https://chromium-status.appspot.com/cq/sergeyu%2540chromium.org/11419267/32003
> 
> Sorry, but as I stated above, I can't let you check this in without at least a
> comment explaining why this cannot be used in production code.

Sorry I didn't notice your comment the first time before clicking commit bit
again. Will not try to land it again without your approval.
What would be the right way to redirect IO in production code? Would it be
acceptable to use PROC_THREAD_ATTRIBUTE_HANDLE_LIST on Vista and above and
bInheritHandle flag on XP?

[jschuh, 2013-01-03 23:48:38]

On 2013/01/03 18:57:14, sergeyu wrote:
> On 2013/01/03 01:19:27, Justin Schuh wrote:
> > On 2013/01/03 00:42:44, I haz the power (commit-bot) wrote:
> > > CQ is trying da patch. Follow status at
> > >
>
https://chromium-status.appspot.com/cq/sergeyu%252540chromium.org/11419267/32003
> > 
> > Sorry, but as I stated above, I can't let you check this in without at least
a
> > comment explaining why this cannot be used in production code.
> 
> Sorry I didn't notice your comment the first time before clicking commit bit
> again. Will not try to land it again without your approval.
> What would be the right way to redirect IO in production code? Would it be
> acceptable to use PROC_THREAD_ATTRIBUTE_HANDLE_LIST on Vista and above and
> bInheritHandle flag on XP?

That will certainly work for Vista+, but the only safe way to pass handles on XP
and below is to use an intermediate process with a clean handle space.

[Sergey Ulanov, 2013-01-04 21:48:38]

Ok, so then we will need to use a separate launcher process to launch native
messaging hosts, and we will still need this CL to implement it. I've added
comment in process_util.h next to inherit_handles flag to make it clear that it
should be used carefully. PTAL.

On 2013/01/03 23:48:38, Justin Schuh wrote:
> On 2013/01/03 18:57:14, sergeyu wrote:
> > On 2013/01/03 01:19:27, Justin Schuh wrote:
> > > On 2013/01/03 00:42:44, I haz the power (commit-bot) wrote:
> > > > CQ is trying da patch. Follow status at
> > > >
> >
>
https://chromium-status.appspot.com/cq/sergeyu%25252540chromium.org/11419267/...
> > > 
> > > Sorry, but as I stated above, I can't let you check this in without at
least
> a
> > > comment explaining why this cannot be used in production code.
> > 
> > Sorry I didn't notice your comment the first time before clicking commit bit
> > again. Will not try to land it again without your approval.
> > What would be the right way to redirect IO in production code? Would it be
> > acceptable to use PROC_THREAD_ATTRIBUTE_HANDLE_LIST on Vista and above and
> > bInheritHandle flag on XP?
> 
> That will certainly work for Vista+, but the only safe way to pass handles on
XP
> and below is to use an intermediate process with a clean handle space.

[jschuh, 2013-01-04 21:57:06]

lgtm, accepting my nits on the wording.

https://codereview.chromium.org/11419267/diff/46001/base/process_util.h
File base/process_util.h (right):

https://codereview.chromium.org/11419267/diff/46001/base/process_util.h#newco...
base/process_util.h:270: // it may cause some handles to be leaked to the
process being launched.
More accurately, it should be something like:

... this flag should be used only when running short-lived, trusted binaries,
because open handles from other libraries and subsystems will leak to the child
process, causing errors such as open socket hangs.

[Sergey Ulanov, 2013-01-05 00:33:05]

https://codereview.chromium.org/11419267/diff/46001/base/process_util.h
File base/process_util.h (right):

https://codereview.chromium.org/11419267/diff/46001/base/process_util.h#newco...
base/process_util.h:270: // it may cause some handles to be leaked to the
process being launched.
On 2013/01/04 21:57:06, Justin Schuh wrote:
> More accurately, it should be something like:
> 
> ... this flag should be used only when running short-lived, trusted binaries,
> because open handles from other libraries and subsystems will leak to the
child
> process, causing errors such as open socket hangs.

Done.

[commit-bot: I haz the power, 2013-01-05 00:35:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11419267/52001

[commit-bot: I haz the power, 2013-01-05 02:30:15]

Change committed as 175268