On Thu, Jul 30, 2020 at 09:59:37AM +0100, Daniel P. Berrangé wrote: > On Wed, Jul 29, 2020 at 06:14:07PM -0400, Vivek Goyal wrote: > > Right now we create lock/pid file in /usr/local/var/... and unprivliged > > user does not have access to create files there. > > > > So create this file in per user cache dir as queried as specified > > by environment variable XDG_RUNTIME_DIR. > > > > Note: "su $USER" does not update XDG_RUNTIME_DIR and it still points to > > root user's director. So for now I create a directory /tmp/$UID to save > > lock/pid file. Dan pointed out that it can be a problem if a malicious > > app already has /tmp/$UID created. So we probably need to get rid of this. > > IMHO use of "su $USER" is simply user error and we don't need to > care about workarounds. They will see the startup fail due to > EPERM on /run/user/0 directory, and then they'll have to fix > their command to use "su - $USER" to setup a clean environment.
I tried "su - $USER". That clears the old XDG_RUNTIME_DIR but does not set new one. So now we have an empty XDG_RUNTIME_DIR env variable. But good thing is that now g_get_user_runtime_dir() returns "/home/$USER/.cache" and we can store user specific temp files there. So I agree that I will get rid of all the logic to create /tmp/$USER. "su $USER" will not be a supported path. > > > > + /* > > + * Unpriviliged users don't have access to /usr/local/var. Hence > > + * store lock/pid file in per user directory. Use environment > > + * variable XDG_RUNTIME_DIR. > > + * If one logs into the system as root and then does "su" then > > + * XDG_RUNTIME_DIR still points to root user directory. In that > > + * case create a directory for user in /tmp/$UID > > + */ > > + if (unprivileged) { > > + gchar *user_dir = NULL; > > + gboolean create_dir = false; > > + user_dir = g_strdup(g_get_user_runtime_dir()); > > + if (!user_dir || g_str_has_suffix(user_dir, "/0")) { > > + user_dir = g_strdup_printf("/tmp/%d", geteuid()); > > + create_dir = true; > > + } > > As above, I don't think we need to have this fallback code to deal > with something that is just user error. > > Also, g_get_user_runtime_dir() is guaranteed to return non-NULL. Thanks. I will get rid of (!user_dir) case. > > > + > > + if (create_dir && g_mkdir_with_parents(user_dir, S_IRWXU) < 0) { > > + fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s", > > + __func__, user_dir, strerror(errno)); > > + g_free(user_dir); > > + return false; > > + } > > + dir = g_strdup(user_dir); > > Don't we also want to be appending "virtiofsd" to this directory path > like we do in the privileged case ? Yes. I forgot to append "virtiofsd" dir. Will do. Thanks Vivek