xpra 4.3-r0-1 fails to launch when /etc/profile prints text

[allo-]

Describe the bug
When /etc/profile produces output (like printing information about the host), xpra start fails.

To Reproduce
Steps to reproduce the behavior:

• Add echo test to /etc/profile
• Try to run xpra start :10

System Information (please complete the following information):

• Server OS: Debian bullseye
• Xpra Server Version 4.3-r0-1

Additional context
The server tries to run:

source /etc/profile && python3.9 -c "import os, json;print(json.dumps(dict(os.environ)))"

This fails because the output of /etc/profile is invalid json. This may even be a security issue, when someone injects valid json into the output of /etc/profile.

A workaround is to remove sourcing /etc/profile. I guess it is sourced because it may set the python path, so the better solution is probably to redirect the output of sourcing the file or to better separate sourcing the profile script and running the command that dumps the environment.

#cmd = ['/bin/bash', '-c', '%s && %s' % (source, dump)]
cmd = ['/bin/bash', '-c', '%s' % (dump)]

[totaam]

sourcing /etc/profile by default was added in ddd9195, see #3083, it has nothing to do with PYTHONPATH.
The source feature was added in 4.0: #2688

Redirecting the output should be easy. I don't understand the second solution you are proposing.
Finding out what environment variables have changes is hard and the current solution does work.

BTW, this causes an ugly backtrace in the server log, but the server does run fine:

2021-12-24 23:45:52,303 Error decoding json output from sourcing script '/etc/profile'
Traceback (most recent call last):
  File "/usr/lib64/python3.9/site-packages/xpra/server/server_util.py", line 81, in decode
    env = json.loads(out)
  File "/usr/lib64/python3.9/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
  File "/usr/lib64/python3.9/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib64/python3.9/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

[allo-]

I do not really know what you did there and why but it fails on my machine that prints text on login with the message you posted. Redirecting the output may be a good solution, when it works together with "source". I am not sure if people may have scripts in their profile file, that should only on login, though.

[totaam]

I've just tried on bullseye and the problem with /etc/profile is definitely not fatal.

So, whatever is causing your server to fail to start, it isn't this feature.
You could try running with full debug (-d all) to see if there is a more telling message there.

[totaam]

The fixes above will be included in 4.3.1

Feel free to re-open if I've missed something.

[allo-]

I've just tried on bullseye and the problem with /etc/profile is definitely not fatal.

I am not sure if it was fatal. I saw the backtrace and then built the workaround without too much debugging.

Feel free to re-open if I've missed something.

I'll do when I encounter the problem in the next Debian packages, but I assume it will work just fine now.

[allo-]

I am still having the problem in the new release and see two other issues.

• I found when debugging the issue, that /bin/sh seems not support source but requires .. source seems to be a bashism.
• Much of the errors I am seeing are from a broken profile.d script, even when xpra uses bash. What causes the script to break is, that $COLUMNS is not set. I believe that /etc/profile should only be sourced in interactive shells (in contrast to for example /etc/bash.bashrc). Running bash -c 'source /etc/profile' may cause depending on the scripts in /etc/profile.d unexpected problems.

In my case the problems are only that a few calculations for pretty printing system stats are broken, but I could imagine more complicated profile scripts that may cause chaos when sources in a non-interactive shell.

It looks like I cannot re-open the bug myself.

[totaam]

source seems to be a bashism

Well, we try to find bash before using sh as fallback, so that's not a major issue.
That's a relief since the new release just came out.
The commit above switches to ..

I believe that /etc/profile should only be sourced in interactive shells
(..)
.. may cause depending on the scripts in /etc/profile.d unexpected problems

May vs we know that not sourcing /etc/profile does cause problems. (see #3083 and others)
Until someone figures out which (better) shell script(s) we have to run to get a fully operational desktop environment, /etc/profile will have to do.

It looks like I cannot re-open the bug myself.

Feel free to open a new one with suggestions for a better script to run, perhaps a distribution specific one?

[allo-]

I see now what changed for the original bug and I am not sure how to handle the rest.
The redirection 1>&2 fixes the json problem correctly, the rest of output are only wrong invocations of commands like expr and seq in my script, which create a lot of noisy errors on stderr and the few working outputs, which are redirected to stderr.

Now I wonder, if either stderr should be hidden as well (allowing for a clean output) of if it is important to have in case a profile script breaks and then it's unclear why xpra behaves different from an interactive session.
Of course this will not catch all errors that only occur in non-interactive sessions, but at least in my case, I see things breaking and would be wondering what's happening if xpra would silence all output.

For my use-case the most interesting way would be if xpra could provide the environment of the interactive shell it is launched in to the subshell, so the output during the xpra login would look like an actual login.

Another question is, if xpra cannot copy its own environment (I got the error when starting xpra from an interactive shell) when running the commands in the pipe. This would probably contain the environment variables needed for #3083 and similar use-cases, as /etc/profile was sourced. I am not sure if it works when xpra is invoked indirectly via ssh, but I think ssh has an option to force a login shell, that may be worth considering.

[totaam]

Now I wonder, if either stderr should be hidden as well..

I would rather show it somewhere so users will see if something breaks or is missing.

For my use-case the most interesting way would be if xpra could provide the environment of the interactive shell it is launched in to the subshell, so the output during the xpra login would look like an actual login.

I don't understand this bit. For experimenting, you can just try a custom source script and dump the environment from there.
For anything else, I'm not sure what the aim is or what you would want xpra to do.

..if xpra cannot copy its own environment..

I'm not sure what this means either. Can you clarify?

but I think ssh has an option to force a login shell

I'm not aware of such an option in the ssh protocol.
Don't you mean a pseudo terminal allocation? (openssh -t / -T)

[allo-]

First: My use-case is mostly fixed. I'll probably ignore the breaking things until I've the time to add some checks if it's an interactive shell to the profile files. But I think in general the idea is that a profile script can assume it's an interactive shell.

I would rather show it somewhere so users will see if something breaks or is missing.

Right. And in my case, where I show some system stats like free memory, it will be useful for xpra users.

For my use-case the most interesting way would be if xpra could provide the environment of the interactive shell it is launched in to the subshell, so the output during the xpra login would look like an actual login.

I don't understand this bit.

That's what I said above. It can be useful in many cases to have the output of the profile script like it's run on an interactive login (here: system info), but it should probably come from an shell that behaves like a login-shell so scripts don't break. But this probably cannot work as, for example, a shell with piped output cannot calculate the terminal columns.

..if xpra cannot copy its own environment..

I'm not sure what this means either. Can you clarify?

When I run xpra start :10 via ssh, then my current shell has the login environment, i.e., /etc/profile was already sourced and xpra should be able to read it from os.environ. This contain, for example, $COLUMNS, updated $PATH and many other more or less important variables like $XDG_ variables, which decide which menu entries are loaded. It also contains variables that were set by the user before running xpra, which are probably missing in your approach.

Maybe subprocess.Popen(cmd, stdout=subprocess.PIPE, env=os.environ) would be a clean solution to copy the environment in which xpra is running.

I'm not aware of such an option in the ssh protocol.

I guess I meant -t or -T and it doesn't work that way. running ssh 'sh -lc echo' also doesn't use a login shell even when -l is set. So maybe it is not possible and as said above there are probably some things a login shell can only provide when it is directly connected to a terminal.

[totaam]

It also contains variables that were set by the user before running xpra, which are probably missing in your approach.

Why would they be missing? Is /etc/profile wiping them?
If not then they should still be there.

Maybe subprocess.Popen(cmd, stdout=subprocess.PIPE, env=os.environ)

Do you mean that those environment variables are not exposed to the sourced script?
My reading of the source for Popen says that they should be:

            if env is None:
                env = os.environ

[allo-]

Hmm, interesting. I guess what I don't understand here is, why this isn't enough for the linked bug. You say:

This [XDG_DATA_DIRS] comes from /etc/profile.d/apps-bin-path.sh

And when it comes from this script, an interactive ssh session should have the variable set and subprocess.Popen should copy it from the shell that is running, for example, xpra start :10.

I suppose the problem in the linked bug comes from running ssh xpra start :10 (or what xpra does internally when you run xpra start with an ssh-url), what again has the problem not to be an interactive shell. ssh copies some variables from the client and allows the user to set own (if not disabled), but that's not what you're looking for when you need to set the data directories which are valid on the server and not on the client.

I wonder if it's something that Debian/Ubuntu should have set in /etc/environment instead of using a /etc/profile script, when it may be useful in non-interactive shells.

When ssh xpra start isn't using any interactive shells, I think I see no better solution to get the things set in profile scripts than what you're currently doing.

[totaam]

You say:

This XDG_DATA_DIRS comes from /etc/profile.d/apps-bin-path.sh

It wasn't me! I don't know.

ssh copies some variables from the client

I don't think it does. At least not the ones that matter here.

but that's not what you're looking for when you need to set the data directories which are valid on the server and not on the client

The problem with ssh logins is that they're not "proper" logins on some distributions (ie: Debian and derivatives) and therefore XDG_RUNTIME_DIR is not set and some other environment variables are missing.
You get those by doing a proper logind / pam session dance, which xpra can also do by starting sessions through the xpra system proxy service - that's one of its main purposes, but adds its own set of complications.

[allo-]

I don't think it does. At least not the ones that matter here.

Not the ones that matter. Only some things like $USER and $TZ needed for the bare minimum. But the client doesn't know the right values anyway.

The problem with ssh logins is that they're not "proper" logins on some distributions (ie: Debian and derivatives) and therefore XDG_RUNTIME_DIR is not set and some other environment variables are missing.

I would need to test with a fresh installation. I think an interactive sessions gets these variables, but a command run via sh -c command or ssh command does not. So your solution is a workaround and can break some things, but probably works for most systems. I have no idea how to improve it so it works for all use-cases, except for using pam and I didn't test the system procxy, yet.

The JSON output doesn't break anymore, so I would think leaving it as is will be enough.
Most thinks that break will probably be only output specific things using variables like $TERM, $COLUMNS and so on. I don't know a script myself that would break in dangerous ways and the default configuration of most distributions are probably not a problem at all.