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@xxxxxxxxxx>
> ---
> 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.)
> + if (virFileIsDir(runtime_dir) &&
> + (stat(runtime_dir, &sb) == 0) && (sb.st_mode & S_IWUSR)) {
> + return g_build_filename(runtime_dir, "libvirt", NULL);
> + }
> + }
> +
> + /* either XDG_RUNTIME_DIR was set, or /run/usr/$UID wasn't writable */
> return g_build_filename(g_get_user_runtime_dir(), "libvirt", NULL);
> #endif
> }
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
