Add real_path option to LaunchProcess

Add real_path option to LaunchProcess

When specified, real_path will be used as the location of the executable
to launch instead of argv[0]. This is useful when a custom argv[0] must
be specified.

This is needed by Chrome Remote desktop to start a login shell, which
requires invoking the shell with an argv[0] starting with a '-'
character (e.g., "-bash").

Committed: https://crrev.com/732f03d294cead0635f02d24a3fa656416f58c25
Cr-Commit-Position: refs/heads/master@{#422457}

[rkjnsn, 2016-09-14 19:58:48]

The CQ bit was checked by rkjnsn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 19:59:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 20:34:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 20:34:52]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[rkjnsn, 2016-09-14 21:01:52]

Patchset #1 (id:1) has been deleted

[rkjnsn, 2016-09-14 21:02:07]

The CQ bit was checked by rkjnsn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 21:02:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 22:00:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 22:00:49]

Dry run: This issue passed the CQ dry run.

[rkjnsn, 2016-09-14 22:28:49]

rkjnsn@chromium.org changed reviewers:
+ jamiewalch@chromium.org, lambroslambrou@chromium.org, mark@chromium.org,
rickyz@chromium.org, rsesek@chromium.org, sergeyu@chromium.org,
thakis@chromium.org

[rkjnsn, 2016-09-14 22:28:50]

I am making a change to Chrome Remote Desktop that involves invoking a user
log-in shell. The standard way to do this is to invoke the shell with an argv[0]
starting with a '-' character (e.g., "-bash"). This is not currently possible to
do with LaunchProcess, as LaunchProcess unconditionally uses argv[0] to locate
the target executable.

This patch adds a real_path field to LaunchOptions, which, when specified, is
used to locate the target executable instead of argv[0], allowing argv[0] to be
modified as needed.

The changes to launch_win.cc could use a review by someone more familiar with
Windows process creation, though I believe they do the correct thing based on
the documentation.

If this functionality is not desired in LaunchProcess, I can alternatively
manually handle setting up the environment and calling exec in our code.

Thanks.

[rkjnsn, 2016-09-15 02:48:00]

rkjnsn@chromium.org changed reviewers:
- jamiewalch@chromium.org, lambroslambrou@chromium.org, mark@chromium.org,
rickyz@chromium.org, rsesek@chromium.org, sergeyu@chromium.org

[Nico, 2016-09-15 19:03:46]

What's the advantage of using LaunchProcess in your code?

[rkjnsn, 2016-09-15 20:21:16]

On 2016/09/15 19:03:46, Nico wrote:
> What's the advantage of using LaunchProcess in your code?

LaunchProcess handles clearing the environment and setting the desired
environment variables, performing the fork and exec dance, and handling any
errors that might occur along the way, all of which would otherwise have to be
duplicated in our code. Additionally, LaunchProcess also ensures that extraneous
file descriptors are closed and signal handlers are reset.

[Nico, 2016-09-20 22:02:46]

On 2016/09/15 20:21:16, rkjnsn wrote:
> On 2016/09/15 19:03:46, Nico wrote:
> > What's the advantage of using LaunchProcess in your code?
> 
> LaunchProcess handles clearing the environment and setting the desired
> environment variables, performing the fork and exec dance, and handling any
> errors that might occur along the way, all of which would otherwise have to be
> duplicated in our code. Additionally, LaunchProcess also ensures that
extraneous
> file descriptors are closed and signal handlers are reset.

That sounds like a good reason. The CL description is a bit sparse. You say this
is to launch a login shell elsewhere, which requires passing "-bash"; move that
from the initial email to the commit message.

Why can't you pass "-bash" as argv[0]? I guess something else doesn't expect
that?

(Sorry, I don't have this code swapped in and I haven't found time to swap it in
this week.)

[rkjnsn, 2016-09-20 22:39:33]

Description was changed from

==========
Add real_path option to LaunchProcess

When specified, real_path will be used as the location of the executable
instead of argv[0]. This is useful when a custom argv[0] must be
specified.
==========

to

==========
Add real_path option to LaunchProcess

When specified, real_path will be used as the location of the executable
to launch instead of argv[0]. This is useful when a custom argv[0] must
be specified.

This is needed by Chrome Remote desktop to start a login shell, which
requires invoking the shell with an argv[0] starting with a '-'
character (e.g., "-bash").
==========

[rkjnsn, 2016-09-20 22:44:34]

On 2016/09/20 22:02:46, Nico wrote:
> On 2016/09/15 20:21:16, rkjnsn wrote:
> > On 2016/09/15 19:03:46, Nico wrote:
> > > What's the advantage of using LaunchProcess in your code?
> > 
> > LaunchProcess handles clearing the environment and setting the desired
> > environment variables, performing the fork and exec dance, and handling any
> > errors that might occur along the way, all of which would otherwise have to
be
> > duplicated in our code. Additionally, LaunchProcess also ensures that
> extraneous
> > file descriptors are closed and signal handlers are reset.
> 
> That sounds like a good reason. The CL description is a bit sparse. You say
this
> is to launch a login shell elsewhere, which requires passing "-bash"; move
that
> from the initial email to the commit message.
> 
> Why can't you pass "-bash" as argv[0]? I guess something else doesn't expect
> that?
> 
> (Sorry, I don't have this code swapped in and I haven't found time to swap it
in
> this week.)

Added the motivation for this change to the commit message.

The problem with passing "-bash" as argv[0] today is that LaunchProcess will
then try to execute a binary called "-bash", which doesn't exist. This patch
provides an alternative way to specify the binary to execute, allowing "-bash"
to be passed as argv[0] and the actual path to the shell to be provided through
"real_path".

[Nico, 2016-09-22 17:03:04]

Thanks. One more comment below.

https://codereview.chromium.org/2338133005/diff/20001/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/2338133005/diff/20001/base/process/launch.h#n...
base/process/launch.h:71: // If not empty, launch the specified executable
instead of
Does this ever make sense on non-posix? Do you know of use cases other than
login shells? If not, I'd make this #if defined(OS_POSIX).

[rkjnsn, 2016-09-22 20:27:26]

On 2016/09/22 17:03:04, Nico wrote:
> Thanks. One more comment below.
> 
> https://codereview.chromium.org/2338133005/diff/20001/base/process/launch.h
> File base/process/launch.h (right):
> 
>
https://codereview.chromium.org/2338133005/diff/20001/base/process/launch.h#n...
> base/process/launch.h:71: // If not empty, launch the specified executable
> instead of
> Does this ever make sense on non-posix? Do you know of use cases other than
> login shells? If not, I'd make this #if defined(OS_POSIX).

I can think of cases where it might make sense to do this on Windows, thought
admittedly none of them seem terribly likely to be encountered by Chrome. If you
prefer, I'm happy to make this change POSIX only unless/until it is needed on
Windows.

[Nico, 2016-09-22 20:42:19]

Yes, let's do that then.

On Thu, Sep 22, 2016 at 4:27 PM, <rkjnsn@chromium.org> wrote:

> On 2016/09/22 17:03:04, Nico wrote:
> > Thanks. One more comment below.
> >
> > https://codereview.chromium.org/2338133005/diff/20001/
> base/process/launch.h
> > File base/process/launch.h (right):
> >
> >
> https://codereview.chromium.org/2338133005/diff/20001/
> base/process/launch.h#newcode71
> > base/process/launch.h:71: // If not empty, launch the specified
> executable
> > instead of
> > Does this ever make sense on non-posix? Do you know of use cases other
> than
> > login shells? If not, I'd make this #if defined(OS_POSIX).
>
> I can think of cases where it might make sense to do this on Windows,
> thought
> admittedly none of them seem terribly likely to be encountered by Chrome.
> If you
> prefer, I'm happy to make this change POSIX only unless/until it is needed
> on
> Windows.
>
> https://codereview.chromium.org/2338133005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rkjnsn, 2016-09-22 20:53:11]

The CQ bit was checked by rkjnsn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 20:54:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2016-09-22 20:55:53]

lgtm

[commit-bot: I haz the power, 2016-09-22 22:58:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 22:58:37]

Dry run: This issue passed the CQ dry run.

[rkjnsn, 2016-10-03 16:48:34]

The CQ bit was checked by rkjnsn@chromium.org

[commit-bot: I haz the power, 2016-10-03 16:49:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 18:00:14]

Description was changed from

==========
Add real_path option to LaunchProcess

When specified, real_path will be used as the location of the executable
to launch instead of argv[0]. This is useful when a custom argv[0] must
be specified.

This is needed by Chrome Remote desktop to start a login shell, which
requires invoking the shell with an argv[0] starting with a '-'
character (e.g., "-bash").
==========

to

==========
Add real_path option to LaunchProcess

When specified, real_path will be used as the location of the executable
to launch instead of argv[0]. This is useful when a custom argv[0] must
be specified.

This is needed by Chrome Remote desktop to start a login shell, which
requires invoking the shell with an argv[0] starting with a '-'
character (e.g., "-bash").
==========

[commit-bot: I haz the power, 2016-10-03 18:00:15]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-10-03 18:03:17]

Description was changed from

==========
Add real_path option to LaunchProcess

When specified, real_path will be used as the location of the executable
to launch instead of argv[0]. This is useful when a custom argv[0] must
be specified.

This is needed by Chrome Remote desktop to start a login shell, which
requires invoking the shell with an argv[0] starting with a '-'
character (e.g., "-bash").
==========

to

==========
Add real_path option to LaunchProcess

When specified, real_path will be used as the location of the executable
to launch instead of argv[0]. This is useful when a custom argv[0] must
be specified.

This is needed by Chrome Remote desktop to start a login shell, which
requires invoking the shell with an argv[0] starting with a '-'
character (e.g., "-bash").

Committed: https://crrev.com/732f03d294cead0635f02d24a3fa656416f58c25
Cr-Commit-Position: refs/heads/master@{#422457}
==========

[commit-bot: I haz the power, 2016-10-03 18:03:18]

Patchset 2 (id:??) landed as
https://crrev.com/732f03d294cead0635f02d24a3fa656416f58c25
Cr-Commit-Position: refs/heads/master@{#422457}