Re: [PATCH v4 1/7] util: Introduce qemu_get_runtime_dir()

[Daniel P . Berrangé]

On Tue, Jul 16, 2024 at 04:27:31PM +0900, Akihiko Odaki wrote:
> qemu_get_runtime_dir() returns a dynamically allocated directory path
> that is appropriate for storing runtime files. It corresponds to "run"
> directory in Unix.
> 
> With a tree-wide search, it was found that there are several cases
> where such a functionality is implemented so let's have one as a common
> utlity function.
> 
> A notable feature of qemu_get_runtime_dir() is that it uses
> $XDG_RUNTIME_DIR if available. While the function is often called by
> executables which requires root privileges, it is still possible that
> they are called from a user without privilege to write the system
> runtime directory. In fact, I decided to write this patch when I ran
> virtiofsd in a Linux namespace created by a normal user and realized
> it tries to write the system runtime directory, not writable in this
> case. $XDG_RUNTIME_DIR should provide a writable directory in such
> cases.
> 
> This function does not use qemu_get_local_state_dir() or its logic
> for Windows. Actually the implementation of qemu_get_local_state_dir()
> for Windows seems not right as it calls g_get_system_data_dirs(),
> which refers to $XDG_DATA_DIRS. In Unix terminology, it is basically
> "/usr/share", not "/var", which qemu_get_local_state_dir() is intended
> to provide. Instead, this function try to use the following in order:
> - $XDG_RUNTIME_DIR
> - LocalAppData folder
> - get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
> 
> This function does not use g_get_user_runtime_dir() either as it
> falls back to g_get_user_cache_dir() when $XDG_DATA_DIRS is not
> available. In the case, we rather use:
> get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Message-Id: <20230921075425.16738-2-akihiko.odaki@daynix.com>
> ---
>  include/qemu/osdep.h | 12 ++++++++++++
>  util/oslib-posix.c   | 11 +++++++++++
>  util/oslib-win32.c   | 26 ++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 191916f38e6d..fe8609fc1375 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -670,6 +670,18 @@ void qemu_set_cloexec(int fd);
>   */
>  char *qemu_get_local_state_dir(void);
>  
> +/**
> + * qemu_get_runtime_dir:
> + *
> + * Return a dynamically allocated directory path that is appropriate for 
> storing
> + * runtime files. It corresponds to "run" directory in Unix, and uses
> + * $XDG_RUNTIME_DIR if available.
> + *
> + * The caller is responsible for releasing the value returned with g_free()
> + * after use.
> + */
> +char *qemu_get_runtime_dir(void);
> +
>  /**
>   * qemu_getauxval:
>   * @type: the auxiliary vector key to lookup
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index e76441695bdc..9599509a9aa7 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -278,6 +278,17 @@ qemu_get_local_state_dir(void)
>      return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR);
>  }
>  
> +char *
> +qemu_get_runtime_dir(void)
> +{
> +    char *env = getenv("XDG_RUNTIME_DIR");
> +    if (env) {
> +        return g_strdup(env);
> +    }
> +
> +    return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
> +}

I'm not convinced this is the correct logic to be following.

In the cover letter you mention not using g_get_user_runtime_dir()
because it falls back to XDG_CACHE_HOME, and we need to fallback
to LOCALSTATEDIR/run. This is not right for normal users though,
where falling back to LOCALSTATEDIR/run is always wrong, as it
won't be writable - the g_get_user_runtime_dir() fallback is
desirable for non-root users.

IMHO we should be doing something more like this

#ifndef WIN32
  if (geteuid() == 0) {
     return  get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
  } else {
#endif
     return g_get_user_runtime_dir();
#ifndef WIN32
  }
#endif

> +
>  void qemu_set_tty_echo(int fd, bool echo)
>  {
>      struct termios tty;
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index b623830d624f..8c5a02ee881d 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -27,6 +27,8 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include <shlobj.h>
> +#include <wchar.h>
>  #include <windows.h>
>  #include "qapi/error.h"
>  #include "qemu/main-loop.h"
> @@ -237,6 +239,30 @@ qemu_get_local_state_dir(void)
>      return g_strdup(data_dirs[0]);
>  }
>  
> +char *
> +qemu_get_runtime_dir(void)
> +{
> +    size_t size = GetEnvironmentVariableA("XDG_RUNTIME_DIR", NULL, 0);
> +    if (size) {
> +        char *env = g_malloc(size);
> +        GetEnvironmentVariableA("XDG_RUNTIME_DIR", env, size);
> +        return env;
> +    }
> +
> +    PWSTR wpath;
> +    const wchar_t *cwpath;
> +    if (!SHGetKnownFolderPath(&FOLDERID_LocalAppData, KF_FLAG_DEFAULT, NULL, 
> &wpath)) {
> +        cwpath = wpath;
> +        size = wcsrtombs(NULL, &cwpath, 0, &(mbstate_t){0}) + 1;
> +        char *path = g_malloc(size);
> +        wcsrtombs(path, &cwpath, size, &(mbstate_t){0});
> +        CoTaskMemFree(wpath);
> +        return path;
> +    }
> +
> +    return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
> +}
> +
>  void qemu_set_tty_echo(int fd, bool echo)
>  {
>      HANDLE handle = (HANDLE)_get_osfhandle(fd);
> 
> -- 
> 2.45.2
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|