Improve xsession execution

remoting/tools/me2me_virtual_host.py

Index: remoting/tools/me2me_virtual_host.py
diff --git a/remoting/tools/me2me_virtual_host.py b/remoting/tools/me2me_virtual_host.py
index eda56485241f35cd9d8ddc310a0f40da2423b34e..d13b98fa407e6a591d29428699d93e23396ca392 100755
--- a/remoting/tools/me2me_virtual_host.py
+++ b/remoting/tools/me2me_virtual_host.py
@@ -349,17 +349,24 @@ class Desktop:
     devnull.close()
 
   def _launch_x_session(self):
-    # Start desktop session
+    # Start desktop session.
     # The /dev/null input redirection is necessary to prevent the X session
     # reading from stdin.  If this code runs as a shell background job in a
     # terminal, any reading from stdin causes the job to be suspended.
     # Daemonization would solve this problem by separating the process from the
     # controlling terminal.
+    # If XSESSION_COMMAND is a single item, running with shell=True will
+    # run the file with "/bin/sh -c" which is intelligent about how to execute
+    # the file: It will try to exec() it directly and if that fails, it will
+    # parse any she-bang line and use that, otherwise it will run the file's
+    # contents as a shell-script.
     logging.info("Launching X session: %s" % XSESSION_COMMAND)
     self.session_proc = subprocess.Popen(XSESSION_COMMAND,
                                          stdin=open(os.devnull, "r"),
                                          cwd=HOME_DIR,
-                                         env=self.child_env)
+                                         env=self.child_env,
+                                         shell=isinstance(

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

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]

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.

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

Not sure how this makes any difference. The command is generated in this script
and doesn't depend on user settings.
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.

+                                             XSESSION_COMMAND, str))
     if not self.session_proc.pid:
       raise Exception("Could not start X session")
 
@@ -494,14 +501,7 @@ def choose_x_session():
   for startup_file in XSESSION_FILES:
     startup_file = os.path.expanduser(startup_file)
     if os.path.exists(startup_file):
-      # Use the same logic that a Debian system typically uses with ~/.xsession
-      # (see /etc/X11/Xsession.d/50x11-common_determine-startup), to determine
-      # exactly how to run this file.
-      if os.access(startup_file, os.X_OK):
-        return startup_file

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

An alternative fix is to change this line to return ["/bin/sh", "-c",
startup_file], and keep rest of the code as is.

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

Done.

-      else:
-        shell = os.environ.get("SHELL", "sh")
-        return [shell, startup_file]
+      return startup_file

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

Does this still work correctly when .xsession or .chrome-remote-desktop-session
isn't marked as executable?

 
   # Choose a session wrapper script to run the session. On some systems,
   # /etc/X11/Xsession fails to load the user's .profile, so look for an