GNOME Bugzilla – Bug 787731
g_file_query_filesystem_info() wrongly reports "filesystem::readonly" when previous mount on same device was readonly
Last modified: 2017-09-22 15:52:48 UTC
This bug was revealed when investigating Nautilus bug 703179. gio/glocalfile.c maintains a cache of mounts per device to provide info for g_file_query_filesystem_info(), here: https://git.gnome.org/browse/glib/tree/gio/glocalfile.c#n785 This cache uses stat() mtime as condition for when to drop the cache, this works fine for regular files, but these days they are checked against /proc/self/mountinfo on linux typically, /proc/self/mountinfo returns *always* the same mtime per process, so resulting in never-changing mounts, and so the cache is not dropped and so accumulates outdated entries from previous mounts on same device (mounts are cached per stat() st_dev). commit bd9e266e116cd39bb5c674eeb74eb55449793e7f already introduced correct /proc/ file monitoring (via POLLERR), now we make get_mounts_timestamp() to use it, and in case there's no monitor currently being run return a new timestamp, which means defaulting to mounts-always-change instead of current mounts-never-change, as the former is safer with respect refreshing cache's, as described for the GIO case. I will attach a patch that fixes the bug this way. I will be attaching a testcase which reproduces the bug. Testcase needs bindfs[1] command (to be able to mount a dir without root privileges as 'mount --bind' requires root). In Fedora this is installed with 'sudo dnf install bindfs'. I will also attach a patch with the fix described previously, which passes the testcase. [1] http://bindfs.org/
Created attachment 359851 [details] [review] gio/tests: Add testcase for bug 787731 Add testcase for function g_file_query_filesystem_info() reporting outdated info for "filesystem::readonly" attribute when said attribute was different in a previous mounted partition in the same device (as GIO maintains a mounts cache per 'st_dev' stat() member). To trigger a mount operation, testcase uses program 'bindfs' instead of 'mount --bind' as bindfs does not require root privileges. And 'fusermount -u' command is used to unmount said bindfs mount. As a reference in Fedora, 'bindfs' is installed from 'bindfs' package and 'fusermount' from 'fuse' package (this one is installed by default as being part of 'System Tools' group). The test creates a directory with a file in it, then mounts it readonly over another directory (the mountpoint), it then checks that g_file_query_filesystem_info() for the file in it indeed reports "filesystem::readonly" as TRUE. Then unmounts and mounts again this time rw (not readonly), it then checks again if g_file_query_filesystem_info() is reporting "filesystem::readonly" as TRUE, if that's the case, it confirms the bug by opening said file in write mode. Testcase is only added for Unix builds.
Created attachment 359852 [details] [review] gio/gunixmounts.c: Don't use mtime to monitor mounts on /proc/ Fix get_mounts_timestamp() to not use a stat'ed mtime for /proc/ files. Instead, use mount_poller_time if /proc/ watch is running, or otherwise return a new generated timestamp to always assume mounts-changed, which is safer than previous behaviour of always assuming mounts-not-changed (as mtime never changes for /proc/ files when queried from the same process). We say it's safer because allows caches depending on: g_unix_mounts_get(&time_read) g_unix_mounts_changed_since() to drop possibly outdated/duplicated values, as that was the case for the GIO mounts cache used in gio/glocalfile.c which provides mount info for g_file_query_filesystem_info() call, as described in below referenced bug. This fix complements related commit bd9e266e116cd39bb5c674eeb74eb55449793e7f
Review of attachment 359852 [details] [review]: This looks reasonable to me apart from one nitpick. I’d like a review from Ondrej as well though, since GIO is his area. ::: gio/gunixmounts.c @@ +1376,3 @@ return (guint64)buf.st_mtime; } + else if (proc_mounts_watch_is_running()) Nitpick: Need a space before `(` in the function call.
(In reply to Philip Withnall from comment #3) > Review of attachment 359852 [details] [review] [review]: > > This looks reasonable to me apart from one nitpick. I’d like a review from > Ondrej as well though, since GIO is his area. > > ::: gio/gunixmounts.c > @@ +1376,3 @@ > return (guint64)buf.st_mtime; > } > + else if (proc_mounts_watch_is_running()) > > Nitpick: Need a space before `(` in the function call. Thank you for super-fast response, I take note and will wait for the further review.
Review of attachment 359852 [details] [review]: Looks ok to me. I've already proposed a similar fix for it, see Bug 755052. I wonder why I did not use mount_poller_time, but this patch looks better than the mine :-) ::: gio/gunixmounts.c @@ +1583,3 @@ +{ + return proc_mounts_watch_source != NULL && + !g_source_is_destroyed (proc_mounts_watch_source); I am not really sure that this is safe after mount_monitor_stop() resp. after g_source_destroy(). I think that proc_mounts_watch_source = NULL should be added in mount_monitor_stop... Philip?
*** Bug 755052 has been marked as a duplicate of this bug. ***
Review of attachment 359852 [details] [review]: ::: gio/gunixmounts.c @@ +1583,3 @@ +{ + return proc_mounts_watch_source != NULL && + !g_source_is_destroyed (proc_mounts_watch_source); Good catch. It isn’t safe unless we hold a ref to the proc_mounts_watch_source after calling g_source_destroy() on it; or unless we clear it to NULL when destroying it (as you suggest). I think clearing it to NULL is a better approach. g_source_is_destroyed() is normally safe to call on a GSource which has been destroyed, but only if you hold a ref to the GSource. In this case, the final ref to the GSource is dropped when we call g_source_destroy().
Created attachment 359984 [details] [review] gio/gunixmounts.c: Don't use mtime to monitor mounts on /proc/ Fix get_mounts_timestamp() to not use a stat'ed mtime for /proc/ files. Instead, use mount_poller_time if /proc/ watch is running, or otherwise return a new generated timestamp to always assume mounts-changed, which is safer than previous behaviour of always assuming mounts-not-changed (as mtime never changes for /proc/ files when queried from the same process). We say it's safer because allows caches depending on: g_unix_mounts_get(&time_read) g_unix_mounts_changed_since() to drop possibly outdated/duplicated values, as that was the case for the GIO mounts cache used in gio/glocalfile.c which provides mount info for g_file_query_filesystem_info() call, as described in below referenced bug. This fix complements related commit bd9e266e116cd39bb5c674eeb74eb55449793e7f -------------------- Updated patch with review issues fixed.
Review of attachment 359984 [details] [review]: ++
(In reply to Philip Withnall from comment #9) > Review of attachment 359984 [details] [review] [review]: > > ++ Committed as: commit 32a9b88b206a42d524fc207e7c60a24ef2deaa56 commit c1a31c3aaa16d5335468a0d488e95feb905f7e73 Thank you!