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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Jul 16, 2024 at 07:52:00PM +0900, Akihiko Odaki wrote:
> On 2024/07/16 18:53, Daniel P. Berrangé wrote:
> > 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.
> 
> It also checks LocalAppData, which should be usually available.
> 
> g_get_user_runtime_dir() is not a proper fallback in case neither of
> XDG_RUNTIME_DIR and LocalAppData are available. g_get_user_cache_dir(),
> which gets called by g_get_user_runtime_dir(), internally uses:
> - XDG_CACHE_HOME or
> - FOLDERID_InternetCache
> 
> g_get_user_cache_dir() just returns NULL if neither of them is available.
> 
> We can't expect XDG_CACHE_HOME is present when XDG_RUNTIME_DIR is missing.
> FOLDERID_InternetCache points to %LOCALAPPDATA%\Microsoft\Windows\Temporary
> Internet Files, according to:
> https://learn.microsoft.com/en-us/windows/win32/shell/knownfolderid
> 
> So we can't expect FOLDERID_InternetCache is available when LocalAppData is
> missing.

XDG_CACHE_HOME isn't required to be present. Glib will use a fallback
location if XDG_CACHE_HOME isn't set, and it will mkdir() the location
if it doesn't exist.


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 :|