This is a follow up to bug 5099. With the current solution VSM starts the session with: args = [tlsession, "/bin/bash", "-c", "exec -l \"$SHELL\" -c \"%s\"" % (xstartupfile), This means that we are: * Hardcoding /bin/bash in vsm/sessionstart.py. In practice perhaps not a big problem, but means that ThinLinc will not run on systems without Bash. It also means that you cannot avoid Bash for policy/security reasons etc. * Executing two shells. The purpose of the first shell is just to change the first character of argv[0] to "-", but starting a new shell (including reading all startup scripts etc) just for that is is a little bit too much, I think, when this can be done in existing C code instead (tl-xinit). This change also means that if the Bash startup scripts are running slowly, this change will cut that time in half. My suggestion is instead: args = [tlsession, self.session_env["SHELL"], "-c", xstartupfile, Plus the tweaks in tl-xinit.
(In reply to comment #0) > * Hardcoding /bin/bash in vsm/sessionstart.py. In practice perhaps not a big > problem, but means that ThinLinc will not run on systems without Bash. It also > means that you cannot avoid Bash for policy/security reasons etc. This argument is mostly moot, since our startup scripts (xsession etc) also requires Bash.
(In reply to comment #0) > * Executing two shells. The purpose of the first shell is just to change the > first character of argv[0] to "-", but starting a new shell (including reading > all startup scripts etc) just for that is is a little bit too much, I think, > when this can be done in existing C code instead (tl-xinit). > > This change also means that if the Bash startup scripts are running slowly, > this change will cut that time in half. It turns out that Bash does not read any startup scripts when just using "-c", unless BASH_ENV is defined. (I had it, thus why I saw .bashrc during strace.) This means that the suggested change is mostly cosmetic.
The current code doesn't seem to handle if $SHELL or xstartupfile includes quotes.
(In reply to comment #4) > The current code doesn't seem to handle if $SHELL or xstartupfile includes > quotes. One case where this actually fails is if you install ThinLinc in a directory which includes whitespace: # mv /opt/thinlinc "/opt/thinlinc 4.9.0" # ln -s "/opt/thinlinc 4.9.0" /opt/thinlinc When trying to login, you'll get: -/bin/bash: /opt/thinlinc: är en katalog tl-xinit: client terminated and returned 126 We could argue that installing ThinLinc this way is not supported, but IMHO the code should be more robust than this.
Created attachment 892 [details] Suggested patch Updated patch for latest trunk: * Correctly handle xstartupfile with whitespace * Shorter: Avoid changing subprocess prototype; provide extension
