Improve xsession execution

remoting/tools/me2me_virtual_host.py

 #!/usr/bin/python
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 # Virtual Me2Me implementation.  This script runs and manages the processes
 # required for a Virtual Me2Me desktop, which are: X server, X desktop
 # session, and Host process.
 # This script is intended to run continuously as a background daemon
 # process, running under an ordinary (non-root) user account.
@@ skipping 331 matching lines @@
     # list.
     label = "%dx%d" % self.sizes[0]
     args = ["xrandr", "-s", label]
     proc = subprocess.Popen(args, env=self.child_env, stdout=devnull,
                             stderr=devnull)
     proc.wait()
 
     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")
 
   def launch_session(self, x_args):
     self._init_child_env()
     self._setup_pulseaudio()
     self._launch_x_server(x_args)
     self._launch_x_session()
 
   def launch_host(self, host_config):
@@ skipping 100 matching lines @@
 
   If XSESSION_COMMAND is already set, its value is returned directly.
   Otherwise, a session is chosen for this system.
 
   Returns:
     A string containing the command to run, or a list of strings containing
     the executable program and its arguments, which is suitable for passing as
     the first parameter of subprocess.Popen().  If a suitable session cannot
     be found, returns None.
   """
   if XSESSION_COMMAND is not None:

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

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?

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

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.

     return XSESSION_COMMAND
 
   # If the session wrapper script (see below) is given a specific session as an
   # argument (such as ubuntu-2d on Ubuntu 12.04), the wrapper will run that
   # session instead of looking for custom .xsession files in the home directory.
   # So it's necessary to test for these files here.
   XSESSION_FILES = [
     "~/.chrome-remote-desktop-session",
     "~/.xsession",
     "~/.Xsession" ]
   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
   # alternative wrapper that is more likely to match the script that the
   # system actually uses for console desktop sessions.
   SESSION_WRAPPERS = [
     "/usr/sbin/lightdm-session",
     "/etc/gdm/Xsession",
     "/etc/X11/Xsession" ]
   for session_wrapper in SESSION_WRAPPERS:
@@ skipping 535 matching lines @@
         host_config.clear_auth()
         host_config.clear_host_info()
         host_config.save()
         return 0
 
 
 if __name__ == "__main__":
   logging.basicConfig(level=logging.DEBUG,
                       format="%(asctime)s:%(levelname)s:%(message)s")
   sys.exit(main())