Improve xsession execution

Improve xsession execution

When running a custom xsession (such as ~/.xsession), execute it with
/bin/sh -c, which copes better with scripts that have the executable
permission set, but don't have a she-bang line, for example.

BUG=167409
TEST=Verify with ~/.xsession executable but no she-bang. Also verify with default unity-2d session on Precise.
NOTRY=true


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

[Sergey Ulanov, 2013-01-02 18:42:42]

https://codereview.chromium.org/11715003/diff/1/remoting/tools/me2me_virtual_...
File remoting/tools/me2me_virtual_host.py (right):

https://codereview.chromium.org/11715003/diff/1/remoting/tools/me2me_virtual_...
remoting/tools/me2me_virtual_host.py:368: shell=isinstance(
It looks wrong to use shell=True in some cases, but not the others. Why can't we
always use shell here?
Popen() expects a single command when using shell, so I think choose_x_session()
needs to be modified to always return the command as a string instead of a list.

[Wez, 2013-01-02 19:46:20]

https://codereview.chromium.org/11715003/diff/1/remoting/tools/me2me_virtual_...
File remoting/tools/me2me_virtual_host.py (right):

https://codereview.chromium.org/11715003/diff/1/remoting/tools/me2me_virtual_...
remoting/tools/me2me_virtual_host.py:368: shell=isinstance(
On 2013/01/02 18:42:42, sergeyu wrote:
> It looks wrong to use shell=True in some cases, but not the others. Why can't
we
> always use shell here?

Two reasons for preferring "shell=False":
1.  Reduced risk of something user-specified making it into a command-line
supplied to the shell and doing something nasty.
2.  Reduced overhead in the common case that the shell is not necessary.

> Popen() expects a single command when using shell, so I think
choose_x_session()
> needs to be modified to always return the command as a string instead of a
list.

[Sergey Ulanov, 2013-01-02 22:56:54]

https://codereview.chromium.org/11715003/diff/1/remoting/tools/me2me_virtual_...
File remoting/tools/me2me_virtual_host.py (left):

https://codereview.chromium.org/11715003/diff/1/remoting/tools/me2me_virtual_...
remoting/tools/me2me_virtual_host.py:501: return startup_file
An alternative fix is to change this line to return ["/bin/sh", "-c",
startup_file], and keep rest of the code as is.

https://codereview.chromium.org/11715003/diff/1/remoting/tools/me2me_virtual_...
File remoting/tools/me2me_virtual_host.py (right):

https://codereview.chromium.org/11715003/diff/1/remoting/tools/me2me_virtual_...
remoting/tools/me2me_virtual_host.py:368: shell=isinstance(
On 2013/01/02 19:46:20, Wez wrote:
> On 2013/01/02 18:42:42, sergeyu wrote:
> > It looks wrong to use shell=True in some cases, but not the others. Why
can't
> we
> > always use shell here?
> 
> Two reasons for preferring "shell=False":
> 1.  Reduced risk of something user-specified making it into a command-line
> supplied to the shell and doing something nasty.

Not sure how this makes any difference. The command is generated in this script
and doesn't depend on user settings.

> 2.  Reduced overhead in the common case that the shell is not necessary.

This makes sense, but I'm not sure the overhead is big enough to justify making
this code more complicated. 
In either case, using type of XSESSION_COMMAND to determine if shell should be
used isn't the right approach. Its confusing and it doesn't work correctly here:
choose_x_session() may return string or list depending on existence of
/usr/bin/unity-2d-panel even when shell is not needed.

If we do want to avoid running shell when it is not necessary, then a better
approach might be to fix 
choose_x_session() to add  /bin/sh -c in front of the command when it is
necessary.

> 
> > Popen() expects a single command when using shell, so I think
> choose_x_session()
> > needs to be modified to always return the command as a string instead of a
> list.
>

https://codereview.chromium.org/11715003/diff/1/remoting/tools/me2me_virtual_...
remoting/tools/me2me_virtual_host.py:490: if XSESSION_COMMAND is not None:
It looks wrong that choose_x_session() is used to initialize  XSESSION_COMMAND
and then later it looks in that variable itself. Remove these two lines from
here?

https://codereview.chromium.org/11715003/diff/1/remoting/tools/me2me_virtual_...
remoting/tools/me2me_virtual_host.py:504: return startup_file
Does this still work correctly when .xsession or .chrome-remote-desktop-session
isn't marked as executable?

[Lambros, 2013-01-03 01:50:26]

https://codereview.chromium.org/11715003/diff/1/remoting/tools/me2me_virtual_...
File remoting/tools/me2me_virtual_host.py (left):

https://codereview.chromium.org/11715003/diff/1/remoting/tools/me2me_virtual_...
remoting/tools/me2me_virtual_host.py:501: return startup_file
On 2013/01/02 22:56:54, sergeyu wrote:
> An alternative fix is to change this line to return ["/bin/sh", "-c",
> startup_file], and keep rest of the code as is.

Done.

https://codereview.chromium.org/11715003/diff/1/remoting/tools/me2me_virtual_...
File remoting/tools/me2me_virtual_host.py (right):

https://codereview.chromium.org/11715003/diff/1/remoting/tools/me2me_virtual_...
remoting/tools/me2me_virtual_host.py:490: if XSESSION_COMMAND is not None:
On 2013/01/02 22:56:54, sergeyu wrote:
> It looks wrong that choose_x_session() is used to initialize  XSESSION_COMMAND
> and then later it looks in that variable itself. Remove these two lines from
> here?
I'm going to remove XSESSION_COMMAND altogether, in another CL. It was there
simply so that people could edit this script near the top to specify a session.
We don't need it anymore.

[Sergey Ulanov, 2013-01-04 21:33:09]

lgtm

[commit-bot: I haz the power, 2013-01-04 22:23:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/11715003/7001

[commit-bot: I haz the power, 2013-01-04 22:23:41]

Change committed as 175212