X broken after update to qubes-gui-vm3-0.13-1

[stefan....@gmail.com]

Hi all,

I installed the latest updates (xen stuff, and other updates including qubes-gui-vm, qubes-core-vm). Since the last reboot, I can't start any X apps.

When trying to open a shell using "qrexec-client -d sys-firewall user:bash", I get the error "no such file: /tmp/qubes-session-env" (doing the same as root, I get the same error, but at least I'm able to issue commands afterwards).

In .xsession-errors, I find the message: "abrt-applet: Failed to open connection to session manager: 'SESSION_MANAGER environment variable not defined'"

The X server seems to have failed to start.

After downgrading to qubes-gui-vm-3.0.11 (yum downgrade ...), everything went back to normal.

Anyone else having the same problems?

Stefan.

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Thu, Oct 29, 2015 at 02:59:10PM -0700, stefan....@gmail.com wrote:
> Hi all,
>
> I installed the latest updates (xen stuff, and other updates including qubes-gui-vm, qubes-core-vm). Since the last reboot, I can't start any X apps.
>
> When trying to open a shell using "qrexec-client -d sys-firewall user:bash", I get the error "no such file: /tmp/qubes-session-env" (doing the same as root, I get the same error, but at least I'm able to issue commands afterwards).
>
> In .xsession-errors, I find the message: "abrt-applet: Failed to open connection to session manager: 'SESSION_MANAGER environment variable not defined'"

Do you have any other errors there?

>
> The X server seems to have failed to start.
>
>
> After downgrading to qubes-gui-vm-3.0.11 (yum downgrade ...), everything went back to normal.
>
>
> Anyone else having the same problems?
>
>
> Stefan.
>

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBCAAGBQJWMqIPAAoJENuP0xzK19csclMH/2RKYEenyeHK8+QGlx4Yr82Y
/3/6Uguuid4IGZyFC7Zl/D/gQdLbwCfUQzpdL/V9a3ll91MB/d2rYV39MV3bkxVP
zIWbs0zEzqCpGTu5kGl56xRSGLGlsMZRUeUnelGZofBZkliUUtgfcAtO/WjeDrvb
1KqD9e0MpoCulYxQEn9b3XIWFns8OYI8k8V7c0h9jCtYiawULYEMuBCYwcv8ujzz
Rtk6E7lU7rFpfVjVTYulUIDA2P+n9G9BAMVLinZmTB1xQczlUp7ellwHGvuPN30a
bTU3JUjQrUFLP+JSMiDcK5JruRY1LowPs1SLBkdbAg6exOj2MfLST0ejDcoWc/Q=
=Q09j
-----END PGP SIGNATURE-----

[Stefan Schlott]

Hi,

>> In .xsession-errors, I find the message: "abrt-applet: Failed to open connection to session manager: 'SESSION_MANAGER environment variable not defined'"
>
> Do you have any other errors there?

No, nothing else (though it's possible I missed something, working with
"qrexec-client ... bash" is not very comfortable).


Stefan.

[Matt McCutchen]

I ran into two problems when I upgraded from qubes-gui-vm-3.0.11 to qubes-gui-vm-3.0.13 (Fedora 21 TemplateVM), and I finally managed to track both of them down.  Stefan, maybe one of them is the same as yours.  If not, the two versions differ only in the handling of environment variables, so maybe you'll be able to track down which environment variable is causing your problem.

1. The TemplateVM had the debugmode package installed, which includes a script /etc/profile.d/debug.sh that sets MALLOC_PERTURB_.  Starting in qubes-gui-vm-3.0.12, /usr/bin/qubes-run-xorg.sh uses "su -l" on the path that starts the X server, so this script is loaded and the X server receives MALLOC_PERTURB_ in its environment.  This appears to be causing a bunch of errors related to X atoms in .xsession-errors:

(abrt:1232): Gdk-CRITICAL **: gdk_atom_intern: assertion 'atom_name != NULL' failed
(abrt:1232): Gdk-CRITICAL **: gdk_atom_intern: assertion 'atom_name != NULL' failed
(gnome-settings-daemon:1216): Gdk-CRITICAL **: gdk_atom_intern: assertion 'atom_name != NULL' failed
(gnome-settings-daemon:1216): Gdk-CRITICAL **: gdk_atom_intern: assertion 'atom_name != NULL' failed
(nm-applet:1241): Gdk-CRITICAL **: gdk_atom_intern: assertion 'atom_name != NULL' failed
(nm-applet:1241): Gdk-CRITICAL **: gdk_atom_intern: assertion 'atom_name != NULL' failed
(notification-daemon:1250): Gdk-CRITICAL **: gdk_atom_intern: assertion 'atom_name != NULL' failed
(nm-applet:1241): Gdk-WARNING **: gdkproperty-x11.c:311 invalid X atom: 623191333
(nm-applet:1241): Gdk-WARNING **: gdkproperty-x11.c:311 invalid X atom: 623191333
(gnome-settings-daemon:1216): Gdk-WARNING **: gdkproperty-x11.c:311 invalid X atom: 623191333
(gnome-settings-daemon:1216): Gdk-WARNING **: gdkproperty-x11.c:311 invalid X atom: 623191333
(abrt:1232): Gdk-WARNING **: gdkproperty-x11.c:311 invalid X atom: 623191333
(abrt:1232): Gdk-WARNING **: gdkproperty-x11.c:311 invalid X atom: 623191333
(gnome-abrt:1387): Gdk-CRITICAL **: gdk_atom_intern: assertion 'atom_name != NULL' failed
(gnome-abrt:1387): Gdk-CRITICAL **: gdk_atom_intern: assertion 'atom_name != NULL' failed
(gnome-abrt:1387): Gdk-CRITICAL **: gdk_atom_intern: assertion 'atom_name != NULL' failed
(gnome-abrt:1387): Gdk-CRITICAL **: gdk_atom_intern: assertion 'atom_name != NULL' failed

One application (notification-daemon) treats the error as fatal and crashes, causing an abrt notification; this is how I noticed the problem.  Otherwise, the system appears to be usable.  (To diagnose the problem, first I observed that it went away when I replaced the "su -l" by "su", then I reviewed the list of environment variables present under "su -l" but not "su" and guessed that MALLOC_PERTURB_ was the only one likely to cause a problem like this.)  I replaced the X server with a wrapper script that unsets MALLOC_PERTURB_ and the problem went away.  This probably indicates a memory error in some code in the X server process; I don't know if it's Qubes-specific.  I wasn't able to reproduce any memory errors under valgrind.

Unless and until this problem is confirmed to reflect a memory safety bug in Qubes code, I'm not sure there's any change to make to Qubes here; we should just try to get the word out that anyone seeing this problem should try disabling MALLOC_PERTURB_.

2. Before /usr/bin/qubes-run-xorg.sh was changed to go through a login shell, I had achieved a similar result by creating a ~/.profile file (which is loaded by /etc/X11/xinit/xinitrc) that calls /etc/profile.  /etc/X11/xinit/xinitrc loads ~/.profile long before it calls qubes-session, so /tmp/qubes-session-env does not yet exist.  In qubes-gui-vm-3.0.13, /etc/profile.d/qubes-session.sh no longer checks "-O /tmp/qubes-session-env" (i.e., /tmp/qubes-session-env exists and is owned by the current user) before running ". /tmp/qubes-session-env" (https://github.com/marmarek/qubes-gui-agent-linux/commit/51dc5deaed6ffddaf68e1f12b0c22eb235e57ada).  And /etc/X11/xinit/xinitrc specifies "#!/bin/sh", and when bash is called as sh, it treats "." of a nonexistent file as fatal.  So xinitrc exits, which apparently causes the X server to exit.  (To diagnose this, I observed the problem went away when I restored the "-O /tmp/qubes-session-env".  Then I added debug output to /etc/profile.d/qubes-session.sh to see all the processes on the system that called it, and eventually I noticed that xinitrc seemed to be exiting after it failed to load /tmp/qubes-session-env.  I was stumped at how that could happen since it wasn't mentioned in the man page for the "." command.  Eventually I guessed this might be a "sh" behavior.)

Obviously, I should remove the call from ~/.profile to /etc/profile now that that is done by /usr/bin/qubes-run-xorg.sh, and that indeed fixes the problem.  But I think it might be prudent for Qubes to keep the "-O /tmp/qubes-session-env" anyway.  Marek, what was the rationale for removing it?

BTW, Marek, it looks like you may have forgotten to push the qubes-gui-vm 3.0.13 changes to https://github.com/QubesOS/qubes-gui-agent-linux before releasing the package.

Matt

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Sat, Oct 31, 2015 at 06:51:19PM -0700, Matt McCutchen wrote:
> I ran into two problems when I upgraded from qubes-gui-vm-3.0.11 to
> qubes-gui-vm-3.0.13 (Fedora 21 TemplateVM), and I finally managed to track
> both of them down. Stefan, maybe one of them is the same as yours. If
> not, the two versions differ only in the handling of environment variables,
> so maybe you'll be able to track down which environment variable is causing
> your problem.

Thanks for debugging this!

Yes, may be so. Or some library (GDK?) used by those processes.

"." on non-existent file isn't fatal normally, so it looked like
unneeded check (and the file should be created always, anyway). And
having that error message somewhere would help debugging why some
environment variables wasn't set. But if you add "set -e" in some other
profile.d (or ~/.profile) script, "." would fail and terminate that
shell. So I'll restore that check...

> BTW, Marek, it looks like you may have forgotten to push the qubes-gui-vm
> 3.0.13 changes to https://github.com/QubesOS/qubes-gui-agent-linux before
> releasing the package.

Indeed, done.

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBCAAGBQJWNX0dAAoJENuP0xzK19csgSMH/jOLaVzHS59TTW0Mmy67VCXq
I2YUQztu9noCAv5s7usk9PqmHSJUMeoFpubHtQaeWkjpoPjy+ulcjJ0izalhvBjP
kD0P5zDwnQL/HIqfRVCPaSAQBiEzdSgYrjuA//+Wao8qIiyLEHOHE9cX9iRFhYRc
LXuN5zlUkpNTIqTocFT4e5UCt2uHOc7zxIc7WnhtSCcXZL4ZiuQLHlV94Bd/6s+H
LNN3XzvocdT6++KuMyEw9sD+3Xr+b+NgkijtjGz+T7MSXnxZOZDy0D8epG1XNPS+
VCo5YmCpa9BaJ/JR7F0mdbCHpA6XENxAHvOV+C+yhL4aR6YpaZsbBsB/uEPTdXQ=
=xo2X
-----END PGP SIGNATURE-----

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Sun, Nov 01, 2015 at 03:46:53AM +0100, Marek Marczykowski-Górecki wrote:
> On Sat, Oct 31, 2015 at 06:51:19PM -0700, Matt McCutchen wrote:
> > Obviously, I should remove the call from ~/.profile to /etc/profile now
> > that that is done by /usr/bin/qubes-run-xorg.sh, and that indeed fixes the
> > problem. But I think it might be prudent for Qubes to keep the "-O
> > /tmp/qubes-session-env" anyway. Marek, what was the rationale for removing
> > it?
>
> "." on non-existent file isn't fatal normally, so it looked like
> unneeded check (and the file should be created always, anyway). And
> having that error message somewhere would help debugging why some
> environment variables wasn't set. But if you add "set -e" in some other
> profile.d (or ~/.profile) script, "." would fail and terminate that
> shell. So I'll restore that check...

Hmm, maybe the whole /etc/qubes-session-env is not needed anymore, since
all the user processes are started through qrexec-fork-server (child of
qubes-session)? That qrexec-fork-server is used only for processes of
the same user (others are called directly from qrexec-agent using "su
- -"), but with "-O /tmp/qubes-session-env" test restored, the file will
not be loaded in any of those cases.
The only situation where /tmp/qubes-session-env would still be useful,
is that where qrexec-fork-server died - qrexec-agent will fallback to "su
- -" then. But that would be a failure anyway and not loading
/tmp/qubes-session-env isn't that fatal.

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBCAAGBQJWNX+iAAoJENuP0xzK19cscRMH/AlZPDmJ0/YPP4zA6xYh3q9K
K4i2lvWpTricOnqZqqZMX+gKuSuZb7d/6gh+xwpzcR5qiSszEtLtm57C5TEsNVG+
MkiG9vB+aQn+DmIcBI/QLEtcEHJ4IgAd+7Nmk5aI78boegF8/yCg8OOzC3yf8elk
BFHC0nQ86FgiiOM9WnSlTvc2akDxC3tPim+DT38XDDxydcuJek6Bs38X2W62Jh6S
Ni61oiWvTH5HV94ydWeUl8+KXSjxx0ZaVeE5/m4dD1L6eYVHopUtJ5Qwn/I8BKmO
HTdLLrdK1f35K+jkpxojbJUR+AbBw4JokaGqzILhZ0jX1jXStuxXObil11bUY+M=
=APfj
-----END PGP SIGNATURE-----

[yaqu]

On Sun, 1 Nov 2015 03:57:39 +0100, Marek Marczykowski-Górecki
<marm...@invisiblethingslab.com> wrote:

> Hmm, maybe the whole /etc/qubes-session-env is not needed anymore,
> since all the user processes are started through qrexec-fork-server
> (child of qubes-session)? That qrexec-fork-server is used only for
> processes of the same user (others are called directly from
> qrexec-agent using "su
> - -"), but with "-O /tmp/qubes-session-env" test restored, the file
> will not be loaded in any of those cases.
> The only situation where /tmp/qubes-session-env would still be useful,
> is that where qrexec-fork-server died - qrexec-agent will fallback to
> "su
> - -" then. But that would be a failure anyway and not loading
> /tmp/qubes-session-env isn't that fatal.

From default user's point of view, qubes-session-env is not needed, as
long as qrexec-fork-server is running.

As for other users's processes (excluding root), qubes-session-env is
useless, because of file permissions set intentionally to 0600 by
/usr/bin/qubes-session.

Root processes are able to import qubes-session-env, but they didn't,
until last changes (removing test for ownership in qubes-session.sh).

The question is, do we care about processes of users other than default
one?

If not, qubes-session-env is not needed. If yes, qubes-session-env
needs to be fixed.

Regards,

--
yaqu

[Matt McCutchen]

On Sun, 2015-11-01 at 03:46 +0100, Marek Marczykowski-Górecki wrote:
> On Sat, Oct 31, 2015 at 06:51:19PM -0700, Matt McCutchen wrote:
> > This probably indicates
> > a memory error in some code in the X server process; I don't know if it's
> > Qubes-specific. I wasn't able to reproduce any memory errors under
> > valgrind.
>
> Yes, may be so. Or some library (GDK?) used by those processes.

Note, while the errors are reported by the X clients, they occur when
MALLOC_PERTURB_ is used on the X server. The X server does not load GDK
and does load some Qubes-specific code, so the bug may be in Qubes.

> "." on non-existent file isn't fatal normally, so it looked like
> unneeded check (and the file should be created always, anyway).

It might be worth mentioning (though the point is moot now) that I saw
four processes try to read /tmp/qubes-session-env before it was created:

- Login shell started by /usr/bin/qubes-run-xorg.sh (which will later
create /tmp/qubes-session-env)
- Login shell started by qrexec-agent for WaitForSession
- Login shell started by /etc/qubes-rpc/qubes.WaitForSession
- Login shell started by qrexec-agent for SetMonitorLayout

(This doesn't include xinitrc, which only read it because of my
~/.profile customization.)

> Hmm, maybe the whole /etc/qubes-session-env is not needed anymore, since
> all the user processes are started through qrexec-fork-server (child of
> qubes-session)? That qrexec-fork-server is used only for processes of
> the same user (others are called directly from qrexec-agent using "su
> -"), but with "-O /tmp/qubes-session-env" test restored, the file will
> not be loaded in any of those cases.

User processes started with "qvm-run --nogui" don't use
qrexec-fork-server. Is there a reason for that? If you change it, then
I agree /tmp/qubes-session-env can be removed.

As an aside, I didn't know about qrexec-fork-server and I think it is a
big improvement! In addition to the reasons you listed on
https://github.com/QubesOS/qubes-core-agent-linux/commit/700c240d370eadadc64c8252c7c3650127da4fc3, it removes the need for all login scripts that run after /etc/profile.d/qubes-session.sh to be able to recover from a state in which all environment variables are already set but other process attributes (umask, ulimits, etc.) are not.

Matt

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Sun, Nov 01, 2015 at 04:51:54PM -0500, Matt McCutchen wrote:
> On Sun, 2015-11-01 at 03:46 +0100, Marek Marczykowski-Górecki wrote:
> > On Sat, Oct 31, 2015 at 06:51:19PM -0700, Matt McCutchen wrote:
> > > This probably indicates
> > > a memory error in some code in the X server process; I don't know if it's
> > > Qubes-specific. I wasn't able to reproduce any memory errors under
> > > valgrind.
> >
> > Yes, may be so. Or some library (GDK?) used by those processes.
>
> Note, while the errors are reported by the X clients, they occur when
> MALLOC_PERTURB_ is used on the X server. The X server does not load GDK
> and does load some Qubes-specific code, so the bug may be in Qubes.

Yes, theoretically it may be.

> > "." on non-existent file isn't fatal normally, so it looked like
> > unneeded check (and the file should be created always, anyway).
>
> It might be worth mentioning (though the point is moot now) that I saw
> four processes try to read /tmp/qubes-session-env before it was created:
>
> - Login shell started by /usr/bin/qubes-run-xorg.sh (which will later
> create /tmp/qubes-session-env)
> - Login shell started by qrexec-agent for WaitForSession
> - Login shell started by /etc/qubes-rpc/qubes.WaitForSession
> - Login shell started by qrexec-agent for SetMonitorLayout
>
> (This doesn't include xinitrc, which only read it because of my
> ~/.profile customization.)
>
> > Hmm, maybe the whole /etc/qubes-session-env is not needed anymore, since
> > all the user processes are started through qrexec-fork-server (child of
> > qubes-session)? That qrexec-fork-server is used only for processes of
> > the same user (others are called directly from qrexec-agent using "su
> > -"), but with "-O /tmp/qubes-session-env" test restored, the file will
> > not be loaded in any of those cases.
>
> User processes started with "qvm-run --nogui" don't use
> qrexec-fork-server. Is there a reason for that?

The point of --nogui is to launch a process within minimalistic
environment (which loads fast). For example some simple service calls
(time set etc).
The major difference is on Windows, where it really doesn't attach such
process to the whole GUI session. But in Linux VM case the difference is
in two places:
- GUI daemon isn't started on demand (when the VM was started with
qvm-start --no-guid), so no qubes.WaitForSession call etc
- process is started in simple, minimal environment with just "su -"

In the above spirit, it should not load /tmp/qubes-session-env in such
case.

> If you change it, then
> I agree /tmp/qubes-session-env can be removed.

Ok, so to not introduce to much change, I think for R3.0 the "-O" check
should be restored and for R3.1 /tmp/qubes-session-env removed.

> As an aside, I didn't know about qrexec-fork-server and I think it is a
> big improvement! In addition to the reasons you listed on
> https://github.com/QubesOS/qubes-core-agent-linux/commit/700c240d370eadadc64c8252c7c3650127da4fc3, it removes the need for all login scripts that run after /etc/profile.d/qubes-session.sh to be able to recover from a state in which all environment variables are already set but other process attributes (umask, ulimits, etc.) are not.

The only thing left to do, to not constantly fight with all that logind
etc tools, is to properly link user session with X session. Currently
it is workarounded by setting XDG_SEAT=seat0, which apparently works in
most cases.

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBCAAGBQJWNqEAAAoJENuP0xzK19csGoAH/1E7mnaLjRLyQrCpSnHrDG+i
ezdMRnObJ8HtDVJ9nEIye854MUy8fApZDsYwvlXUkErASXBcoj1uCAKDBpDWKXNU
KfltQoJ/rXfJ8K0n3gH+5Pjfd2UTUWl54rikdAkN/f61hiZAxdSJXS8J/RqbTTMv
Avf3/pgEQqhqrqn84PjsspymL9rHDn4HmH+XOP2IdG1naWCo+dXTtNScfGLUY7mG
xl8nPPd3xUhPgPAHhjTqh0Fl0/pA85xQ5WDELZRBNTAa0/UvnTpak5QOUvKY8N4k
E5HOlZrhJRCKtBiAOMbKjDBu0AMbBGG+q3W+vu8njM+FWpcUEkcM5xunHmO3WI4=
=5acT
-----END PGP SIGNATURE-----

[yaqu]

On Mon, 2 Nov 2015 00:32:16 +0100, Marek Marczykowski-Górecki
<marm...@invisiblethingslab.com> wrote:

> > If you change it, then
> > I agree /tmp/qubes-session-env can be removed.
>
> Ok, so to not introduce to much change, I think for R3.0 the "-O"
> check should be restored and for R3.1 /tmp/qubes-session-env removed.

If /tmp/qubes-session-env stays in R3.0, could you consider applying
some changes in /usr/bin/qubes-session (patch attached)?

`export -p` replaced `env` to fix issues with multiline variables, but
filtering output of `export -p` by grep is still risky. If any of
filtered out variables happens to be multiline, result will be broken.
Unsetting variables in a subshell is safer:

(
umask 0077
unset PWD TERM SHELL RUNLEVEL PATH SHLVL LOGNAME USER MEM
export -p > /tmp/qubes-session-env.tmp
)

There is no need to unset $_, as `export -p` does not print it,
and no need to restore umask, as it is changed only in subshell.

Currently qubes-session changes umask from 0002 to 0022. I do not know
if it is intentional (maybe author wanted to restore previous value?),
so I am not touching it.

Variables set by gnome-keyring-daemon are appended to qubes-session-env
as declaration, so they will not be exported to environment when
qubes-session-env is sourced by qubes-session.sh. It can be fixed by
dumping environment not at the beginning of qubes-session, but just
before starting qrexec-fork-server.

Additionally qubes-session-env will get more variables (e.g.
QUBES_KEYMAP), but I do not think it is a problem, since they are set in
qrexec-fork-server's environment and passed to user's processes anyway.

BTW is there a reason why xsetroot is called twice in qubes-session?

--
yaqu

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Mon, Nov 02, 2015 at 09:36:55AM +0100, yaqu wrote:
> On Mon, 2 Nov 2015 00:32:16 +0100, Marek Marczykowski-Górecki
> <marm...@invisiblethingslab.com> wrote:
>
> > > If you change it, then
> > > I agree /tmp/qubes-session-env can be removed.
> >
> > Ok, so to not introduce to much change, I think for R3.0 the "-O"
> > check should be restored and for R3.1 /tmp/qubes-session-env removed.
>
> If /tmp/qubes-session-env stays in R3.0, could you consider applying
> some changes in /usr/bin/qubes-session (patch attached)?

Sure, thanks!

> `export -p` replaced `env` to fix issues with multiline variables, but
> filtering output of `export -p` by grep is still risky. If any of
> filtered out variables happens to be multiline, result will be broken.
> Unsetting variables in a subshell is safer:
>
> (
> umask 0077
> unset PWD TERM SHELL RUNLEVEL PATH SHLVL LOGNAME USER MEM
> export -p > /tmp/qubes-session-env.tmp
> )

+1

> There is no need to unset $_, as `export -p` does not print it,
> and no need to restore umask, as it is changed only in subshell.
>
> Currently qubes-session changes umask from 0002 to 0022. I do not know
> if it is intentional (maybe author wanted to restore previous value?),
> so I am not touching it.

I think it should be safe to remove that also. It was for restoring
"old" value.

> Variables set by gnome-keyring-daemon are appended to qubes-session-env
> as declaration, so they will not be exported to environment when
> qubes-session-env is sourced by qubes-session.sh. It can be fixed by
> dumping environment not at the beginning of qubes-session, but just
> before starting qrexec-fork-server.

Additional problem here is that those variables are not exported.
$ /usr/bin/gnome-keyring-daemon --start
SSH_AUTH_SOCK=/run/user/1000/keyring/ssh
GPG_AGENT_INFO=/run/user/1000/keyring/gpg:0:1

So actually commit changing `env` to `export -p` broke it (by no longer
prefixing `export`). This doesn't looks like any standard API for
setting environment variables, but I guess the real thing used by
gnome-keyring is gnome session manager dbus API, which we don't have.

Maybe we should have it in fork-server?

Anyway for now, export needs to be prepended there (or simply variables
needs to be exported after that eval).

> Additionally qubes-session-env will get more variables (e.g.
> QUBES_KEYMAP), but I do not think it is a problem, since they are set in
> qrexec-fork-server's environment and passed to user's processes anyway.
>
> BTW is there a reason why xsetroot is called twice in qubes-session?

I don't think so, most likely copy&paste mistake.

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBCAAGBQJWN0d+AAoJENuP0xzK19csaTUH/juVrw/8ECyP2ejoHmvvMsb8
p0Uk3L47UO8otOc3VknTXNdGwjK9bKLBoPD6veC9jUrFihlLO5y6a5UI4/XJvI2c
thaettwsWf/ZLKs5yJAzFNvcePc00Rtloxx3l/UhwPigind48hLdSbP4FRpdmM4v
fn4VLZUoirkLjUtafAGpCLJkaWukI9FJnwyUzlz2Q374UX5+cq++tGsImvRgxlA2
X0WJGzhpq3p09Wrk8GvcxfcR8otDZHdBjuYF3HbnZ77wJ+1m0WjdcupihZb69TpL
jjh4Ub1KvBED93R41uW7P8O89zQNL8xZyztPFX2SsO3nVAWaXXQk1TzpZrVSjTY=
=/flQ
-----END PGP SIGNATURE-----

[yaqu]

On Mon, 2 Nov 2015 12:22:38 +0100, Marek Marczykowski-Górecki
<marm...@invisiblethingslab.com> wrote:

> > Variables set by gnome-keyring-daemon are appended to
> > qubes-session-env as declaration, so they will not be exported to
> > environment when qubes-session-env is sourced by qubes-session.sh.
> > It can be fixed by dumping environment not at the beginning of
> > qubes-session, but just before starting qrexec-fork-server.
>
> Additional problem here is that those variables are not exported.
> $ /usr/bin/gnome-keyring-daemon --start
> SSH_AUTH_SOCK=/run/user/1000/keyring/ssh
> GPG_AGENT_INFO=/run/user/1000/keyring/gpg:0:1

That's what I said :-) And that's why I moved creating
qubes-session-env to just before starting qrexec-fork-server. That way
in qubes-session-env is a full copy of fork-server's environment, with
gnome-keyring-daemon's variables. In case of death of fork-server,
user's processes will get from qubes-session-env all needed vars.

> So actually commit changing `env` to `export -p` broke it (by no
> longer prefixing `export`). This doesn't looks like any standard API
> for setting environment variables, but I guess the real thing used by
> gnome-keyring is gnome session manager dbus API, which we don't have.
>
> Maybe we should have it in fork-server?
>
> Anyway for now, export needs to be prepended there (or simply
> variables needs to be exported after that eval).

Now I get it :-) No, inside qubes-session variables from
gnome-keyring-daemon *are* exported after eval, because of `set -a`. All
variables created or modified after this `set` are automatically
exported to environment (including VMTYPE, UPDTYPE and those from
gnome-keyring-daemon).

The only problem here was with qubes-session-env syntax (simple
declaration instead of exporting).

Regards,

--
yaqu

[yaqu]

On Mon, 2 Nov 2015 12:22:38 +0100, Marek Marczykowski-Górecki
<marm...@invisiblethingslab.com> wrote:

> > Currently qubes-session changes umask from 0002 to 0022. I do not
> > know if it is intentional (maybe author wanted to restore previous
> > value?), so I am not touching it.
>
> I think it should be safe to remove that also. It was for restoring
> "old" value.
>

[...]

> > BTW is there a reason why xsetroot is called twice in qubes-session?
>
> I don't think so, most likely copy&paste mistake.

I'm attaching a simple patch which removes unneeded umask and
redundant xsetroot from qubes-session.

Also, qubes-session "hangs" itself in a sleep loop. While "sleep 365d"
looks nice and funny, it can be replaced with a simple endless sleep:

sleep infinity

Or shortly "sleep inf". It works, because coreutils' sleep accepts any
floating-point number as an argument, which may be 'infinity',
according to `man strtod`.

I have considered replacing the loop with `exec sleep inf` to release
bash and save some memory, but qubes-session may spawns some children
(e.g. ssh-agent). If one of them gets terminated, bash will read its
exit code, but sleep (as parent) will not, turning terminated child
into zombie.

`sleep 365d` is also used in other scripts:
qubes-core-agent-linux/qubes-rpc/qubes.WaitForSession
qubes-desktop-linux-xfce4/xfce4-settings-qubes/inhibit-systemd-power-handling.desktop

Is it OK to use `sleep inf` in all these scripts?

--
yaqu

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Mon, Nov 02, 2015 at 07:06:01PM +0100, yaqu wrote:
> On Mon, 2 Nov 2015 12:22:38 +0100, Marek Marczykowski-Górecki
> <marm...@invisiblethingslab.com> wrote:
>
> > > Currently qubes-session changes umask from 0002 to 0022. I do not
> > > know if it is intentional (maybe author wanted to restore previous
> > > value?), so I am not touching it.
> >
> > I think it should be safe to remove that also. It was for restoring
> > "old" value.
> >
> [...]
> > > BTW is there a reason why xsetroot is called twice in qubes-session?
> >
> > I don't think so, most likely copy&paste mistake.
>
> I'm attaching a simple patch which removes unneeded umask and
> redundant xsetroot from qubes-session.
>
> Also, qubes-session "hangs" itself in a sleep loop. While "sleep 365d"
> looks nice and funny, it can be replaced with a simple endless sleep:
>
> sleep infinity
>
> Or shortly "sleep inf". It works, because coreutils' sleep accepts any
> floating-point number as an argument, which may be 'infinity',
> according to `man strtod`.

Nice, there is no info about that in sleep's man page.

> I have considered replacing the loop with `exec sleep inf` to release
> bash and save some memory, but qubes-session may spawns some children
> (e.g. ssh-agent). If one of them gets terminated, bash will read its
> exit code, but sleep (as parent) will not, turning terminated child
> into zombie.

Previously there were kill -STOP $$ (that ASCII art was about that line),
but was changed to sleep exactly for that reason.

> `sleep 365d` is also used in other scripts:
> qubes-core-agent-linux/qubes-rpc/qubes.WaitForSession
> qubes-desktop-linux-xfce4/xfce4-settings-qubes/inhibit-systemd-power-handling.desktop
>
> Is it OK to use `sleep inf` in all these scripts?

Yes :)
Documentation suggests using 'env sleep' to prevent clashes with shell
built-in sleep implementation.

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBCAAGBQJWN69RAAoJENuP0xzK19csrKQH/2+K7r7cr6OXpnFmcmIWQp4g
SfcMjuywkZEwZY8VI27md4/HPqpi0chIZURPPkFiKHPpBC/Alx94ZXJAUDA6FKyu
cxEVZnEE6IX43O3uAm3GLEk2z/1McLACV0ozfO1O+Ic0AZrfGIemKrWpJ8awzDmN
4bOvVg6RpyCG7MG9XRZwbAWF0mLZz+t+YoKBKhdw1vsnlhZ1YZCPDh9wLHeDbPBZ
+sFGETWsEmR1PUWBBOzFxVcTAPsLHlkJ5Zcg83FeHv/ZUgbdSu85vP9dWD6PrqwS
vvvsdNsUM1nmwusdsy9qR/xfy26HeoJZ1uaxPXvhye8NS9M4f17z9BGlUGV7Z3g=
=vlfy
-----END PGP SIGNATURE-----

[yaqu]

On Mon, 2 Nov 2015 19:45:37 +0100, Marek Marczykowski-Górecki
<marm...@invisiblethingslab.com> wrote:

> > `sleep 365d` is also used in other scripts:
> > qubes-core-agent-linux/qubes-rpc/qubes.WaitForSession
> > qubes-desktop-linux-xfce4/xfce4-settings-qubes/inhibit-systemd-power-handling.desktop
> >
> > Is it OK to use `sleep inf` in all these scripts?
>
> Yes :)

Patches attached:
0001 for qubes-gui-agent-linux
0002 for qubes-core-agent-linux
0003 for qubes-desktop-linux-xfce4

> Documentation suggests using 'env sleep' to prevent clashes with shell
> built-in sleep implementation.

Good point, just in case.

BTW I have found builtin sleep only in ksh (with supported infinity)
and mksh (no infinity). Busybox doesn't count (no infinty, of course).

--
yaqu

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Tue, Nov 03, 2015 at 11:41:28AM +0100, yaqu wrote:
> On Mon, 2 Nov 2015 19:45:37 +0100, Marek Marczykowski-Górecki
> <marm...@invisiblethingslab.com> wrote:
>
> > > `sleep 365d` is also used in other scripts:
> > > qubes-core-agent-linux/qubes-rpc/qubes.WaitForSession
> > > qubes-desktop-linux-xfce4/xfce4-settings-qubes/inhibit-systemd-power-handling.desktop
> > >
> > > Is it OK to use `sleep inf` in all these scripts?
> >
> > Yes :)
>
> Patches attached:
> 0001 for qubes-gui-agent-linux
> 0002 for qubes-core-agent-linux
> 0003 for qubes-desktop-linux-xfce4

Applied, thanks!

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBCAAGBQJWOk0PAAoJENuP0xzK19cswywIAIJ2PMvSTt4EMPqTVFeAg2Q3
FTjZgepOPV5lNgUmP3cOUMUuqGmkt2UqxIeMKhvYrqogjIJqHAqH8qz6Ykxnbub/
FCmyiWr9c3uqABPpneuRC/fY+q+LBqgxgToME8ZjfZKaJPSkB/cHcEzPwpCLyR1x
ScelOesprebQI48I+bKhtlXp8iQeuPy2nMVOcPd9emF3zrR0AUT86prkrCJL/7mU
ZBV7uk1t/GxlzcI4XYg6lMTJLc/7Lf3oS8RpJCrOtwwpbB8nYlD+vDMK6TJULdEh
4ZhebMALoH7Ko0LoPR1oePekQn7rU6l0EGKlJfunUlJGrLlN9ydi11mR5+2Ofpk=
=rYdB
-----END PGP SIGNATURE-----