Re: [PATCH RFC] util: pick a better runtime directory when XDG_RUNTIME_DIR isn't set

On Mon, Feb 17, 2025 at 02:14:49AM -0500, Laine Stump wrote: > ====== > > I'm sending this as an RFC just because what it's doing feels kind of > dirty - directly examining XDG_RUNTIME_DIR seems like an "end run" > around the Glib API. If anyone has a better idea of what to do, please > give details :-) > > ====== > > When running unprivileged (i.e. not as root, but as a regular user), > libvirt calls g_get_user_runtime_dir() (from Glib) to get the name of > a directory where status files can be saved. This is a directory that > is 1) writeable by the current user, and 2) will remain there until > the host reboots, but then 3) be erased after the reboot. This is used > for pidfiles, sockets created to communicate between processes, status > XML of active domains, etc. > > Normally g_get_user_runtime_dir() returns the setting of > XDG_RUNTIME_DIR in the user's environment; usually this is set to > /run/user/${UID} (e.g. /run/user/1000) - that directory is created > when a user first logs in and is owned by the user, but is cleared out > when the system reboots (more specifically, this directory usually > resides in a tmpfs, and so disappears when that tmpfs is unmounted). > > But sometimes XDG_RUNTIME_DIR isn't set in the user's environment. In > that case, g_get_user_runtime_dir() returns ${HOME}/.config > (e.g. /home/laine/.config). This directory fulfills the first 2 > criteria above, but fails the 3rd. This isn't just some pedantic > complaint - libvirt actually depends on the directory being cleared > out during a reboot - otherwise it might think that stale status files > are indicating active guests when in fact the guests were shutdown > during the reboot). > > In my opinion this behavior is a bug in Glib - see the requirements > for XDG_RUNTIME in the FreeDesktop documentation here: > > https://specifications.freedesktop.org/basedir-spec/latest/#variables > > but they've documented the behavior as proper in the Glib docs for > g_get_user_runtime_dir(), and so likely will consider it not a bug. > > Beyond that, aside from failing the "must be cleared out during a reboot" > requirement, use of $HOME/.cache in this way also disturbs SELinux, > which gives an AVC denial when libvirt (or passt) tries to create a > file or socket in that directory (the SELinux policy permits use of > /run/user/$UID, but not of $HOME/.config). We *could* add that to the > SELinux policy, but since the glib behavior doesn't > > All of the above is a very long leadup to the functionality in this > patch: rather than blindly accepting the path returned from > g_get_user_runtime_dir(), we first check if XDG_RUNTIME_DIR is set; if > it isn't set then we look to see if /run/user/$UID exists and is > writable by this user, if so we use *that* as the directory for our > status files. Otherwise (both when XDG_RUNTIME_DIR is set, and when > /run/user/$UID isn't usable) we fallback to just using the path > returned by g_get_user_runtime_dir() - that isn't perfect, but it's > what we were doing before, so at least it's not any worse. > > Resolves: https://issues.redhat.com/browse/RHEL-70222 > Signed-off-by: Laine Stump <laine(a)redhat.com&gt; > --- > src/util/virutil.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/src/util/virutil.c b/src/util/virutil.c > index 2abcb282fe..4c7f4b62bc 100644 > --- a/src/util/virutil.c > +++ b/src/util/virutil.c > @@ -538,6 +538,28 @@ char *virGetUserRuntimeDirectory(void) > #ifdef WIN32 > return g_strdup(g_get_user_runtime_dir()); > #else > + /* tl;dr - if XDG_RUNTIME_DIR is set, use g_get_user_runtime_dir(). > + * if not set, then see if /run/user/$UID works > + * if so, use that, else fallback to g_get_user_runtime_dir() > + * > + * this is done because the directory returned by > + * g_get_user_runtime_dir() when XDG_RUNTIME_DIR isn't set is > + * "suboptimal" (it's a location that is owned by the user, but > + * isn't erased when the user completely logs out) > + */ > + > + if (!getenv("XDG_RUNTIME_DIR")) { > + g_autofree char *runtime_dir = NULL; > + struct stat sb; > + > + runtime_dir = g_strdup_printf("/run/user/%d", getuid()); I'm wondering if this should be geteuid. (I'm not completely clear myself, I guess it depends on whether this code could be a called in a setuid / dropped privileges case, and if so what directory should be used.)