GNOME Bugzilla – Bug 526320
should not list mounts that the user doesn't have permission to use
Last modified: 2008-12-08 04:33:56 UTC
The bug has been opened on https://bugs.launchpad.net/ubuntu/+source/gvfs/+bug/210379 "After upgrading to Hardy, all 3 of my network drive maps (mounted via /etc/fstab) are shown as icons on the desktop to both my users, when on Gutsy it only showed icons for the drives I (andy) had permissions to read, and the same (in reverse) for my second user (jo). The share I don't have permissions to won't actually open (which is exactly correct), but the icons are still shown on the desktop for all 3. http://launchpadlibrarian.net/13116477/Screenshot.png Screenshot of Desktop showing icons for mounted drives (525.4 KiB, image/png) This shows the permissions on the mounted drives. As you'll see I shouldn't get an icon on my desktop for "JoDocs", and jo shouldn't get icons for "AndyDocs" or "AndySharedDocs" andy@andy-laptop:/media$ ls -al total 20 drwxr-xr-x 8 root root 4096 2008-04-03 21:25 . drwxr-xr-x 21 root root 4096 2008-04-03 21:20 .. drwx--x--x 26 andy andy 0 2008-04-03 20:27 AndyDocs drwx------ 9 andy andy 0 2008-03-28 17:34 AndySharedDocs drwx------ 10 jo jo 0 2008-03-29 13:57 JoDocs (I've deleted cdrom0 etc. from the output to make it easier to read)"
*** Bug 530379 has been marked as a duplicate of this bug. ***
Hmm, actually this needs to be fixed in gio, not gvfs. So reassigning. And the same concerns voiced in bug 530379 comment 1 still applies here: > The only concern I have with doing something like this is the use of the > access() system call. It might be expensive to do (would need to do it for > every mount point) and, worse, it might hang the process on some file systems > (e.g. autofs and nfs). I haven't looked too closely into the code though.
David further clarified: * He agrees that it is reasonable to hide mountpoints if they are not accessible by the user. * However, he is concerned about access() because the kernel can hang the process in the case of stale NFS mountpoints. It must not hang during enumeration. * If we found a way to achieve the same goal as access() that works or fails without hanging then we should do it. Any ideas?
Btw, look at the workarounds in _gnome_vfs_unix_mount_get_unix_device() here http://svn.gnome.org/svn/gnome-vfs/trunk/libgnomevfs/gnome-vfs-unix-mounts.c for problems with trying to do IO on mount points. I'm honestly not sure how to do this in a clean way.
Maybe one way to do this is through mount options. Recent util-linux-ng supports the comment= option and IIRC, multiple invocations of it. So if you we make gio honor options like 'comment=gio-only-show-for-uid=500', 'comment=gio-only-show-for-uid=501' then LTSP et. al. can just use this option. Seems like a really ugly workaround though.
we have the g_access call in the ubuntu gnome-vfs since 2006 and didn't get issues reported due to it as far as I know, in which cases do you think that's an issue exactly?
Isn't gvfs doing a lot more than gnome-vfs? Was gnome-vfs handling NFS mounts? OTOH, I asked davidz if there is any legitimate case where NFS mounts would happen in media.
(In reply to comment #6) > we have the g_access call in the ubuntu gnome-vfs since 2006 and didn't get > issues reported due to it as far as I know, in which cases do you think that's > an issue exactly? It's surely an issue for networked mounts (nfs, autofs, some fuse based stuff) that times out. Then we hang calling access() and as a result GNOME logins etc. will fail when a mount times out because we hang in the volume monitor instance of every process (panel, nautilus, settings daemon all instantiate a volume monitor). I hope you agree this would be a problem. It's not only a problem if it hangs; enumerating mounts is supposed to be fast. Putting in code paths that does IO will break that guarantee and potentially make it very slow. As a result the whole desktop will feel sluggish. Also, just look at the code in comment 5 where someone at least *tries* to avoid doing IO in gnome-vfs's equivalent code. In fact, the patch in bug 352381 conveniently ignores the presence of this code and probably just hangs in calling access(). As to why you haven't seen gnome-vfs reports: can you test how well ubuntu works with this patch if networked mounts are timing out? Does gnome-vfs-daemon hang? Can you even login?
right, I agree that we should avoid slow or hanging calls, the mountpoint is usually a local directory though no? maybe we can do a such only a local directories?
(In reply to comment #9) > right, I agree that we should avoid slow or hanging calls, the mountpoint is > usually a local directory though no? maybe we can do a such only a local > directories? The problem is that this would require a stat() call and even this would hang / be slow. But maybe there is a way. For the LTSP case at least the /media/davidz part of /media/davidz/foo would have a high change of being local (but see below). So maybe if we do something like this (completely untested): can_access = TRUE; if (g_str_has_prefix (mount_point, "/media")) { tokens = g_strsplit (mount_point, G_DIR_SEPARATOR, 0); num_tokens = g_strv_length (tokens); if (num_tokens > 2) { char *path; g_free (tokens[num_tokens - 1]); tokens[num_tokens - 1]; path = g_build_filenamev (tokens); can_access = (g_access (path, R_OK|X_OK) == 0) g_free (path); } } g_strfreev (tokens); I think it would be fine. Of course if people do /media/nfs_mount /media/nfs_mount/other_nfs_mount we'd be screwing them over if the connection for the latter hangs. Which is probably fine as it would rarely happen. Thoughts?
And of course s/tokens[num_tokens - 1];/tokens[num_tokens - 1] = NULL;/. Gah, I wish Bugzilla had an edit button ;-)
Created attachment 110066 [details] [review] Patch Here's a patch. Please review and commit to trunk and stable.
Looks fine to me. If it turns out to be a problem in some fringe cases involving hung NFS mounts, we can add some way to turn it off, e.g. via an envvar.
I tested this patch against glib2-2.16.3-4.fc9. It works beautifully but it required the below bug fix to ltspfs. === modified file 'src/lbmount.c' --- src/lbmount.c 2008-01-07 23:33:22 +0000 +++ src/lbmount.c 2008-04-28 22:23:23 +0000 @@ -100,7 +100,7 @@ void mkdir_safe(char *dir) { - if (mkdir(dir, 0755)) { + if (mkdir(dir, 0750)) { if (errno == EEXIST) { /* OK, we've been told that this already exists. Make sure * nothing's mounted on top of it, so we aren't doing something This is committed to ltspfs-trunk and will be released as ltspfs-0.5.2.
Comment on attachment 110066 [details] [review] Patch Committed to trunk and the glib-2-16 branch 2008-04-28 David Zeuthen <davidz@redhat.com> * gunixmounts.c (g_unix_mount_guess_should_display): Avoid displaying mounts in a subdirectory not accessible to the user (#526320).
So even though this doesn't fix the original bug, there's now a mechanism in place for avoiding to display mounts that are not accessible to the user: place such mounts in a subdirectory in /media with appropriate access controls. Which, for reasons explained above, is the best we can do. So I'm closing this bug as FIXED.
are you sure that calling access() on the mountpoint of a nfs mount is doing any network access and not just stating the local directory? your change will fix the ltsp usecase but will not work for all the other local mounts, usb keys plugged, etc
do we have a way to know that a mount is a local one and maybe do the g_access call only for local partition and removable devices?
Hi, I've changed my set-up to mount to subdirectories in /media (for example "/media/andy/Andy Docs on Buffalo"), permissions for which shown below, but I still get all the mounts displayed on the desktop, even though I have no access to read them (for example /media/jo) andy@andy-laptop:/media$ ls -al total 24 drwxr-xr-x 6 root root 4096 2008-05-01 17:54 . drwxr-xr-x 21 root root 4096 2008-04-13 20:29 .. drwx------ 4 andy andy 4096 2008-04-29 19:35 andy drwx------ 4 jo jo 4096 2008-04-29 19:41 jo andy@andy-laptop:/media/andy$ ls -al total 8 drwx------ 4 andy andy 4096 2008-04-29 19:35 . drwxr-xr-x 6 root root 4096 2008-05-01 17:54 .. drwx--x--x 27 andy andy 0 2008-04-20 17:51 Andy Docs on Buffalo drwx------ 8 andy andy 0 2008-04-05 20:16 Shared Docs on Buffalo For what it's worth, if this worked I think it would be a very good fix for the problem. Not sure if this helps or not. Cheers Andy (In reply to comment #16) > So even though this doesn't fix the original bug, there's now a mechanism in > place for avoiding to display mounts that are not accessible to the user: place > such mounts in a subdirectory in /media with appropriate access controls. > > Which, for reasons explained above, is the best we can do. So I'm closing this > bug as FIXED. >
This was applied to a testing package for Ubuntu. IMHO this achieves nothing at all, since the default is to put mounts under /media/label, not /media/someotherdir/label (where somedir could be one per-user, and 0700). Thus the situation does not change at all: one user plugs in an USB key, and all other users get a nautilus window and a nautilus error message popped into their face. I see the caution of not trying to access() a mount point which belongs to a remote directory. Would it be acceptable for you to only do an access() check if the mounted volume is a local device? (by looking at /proc/mounts, or better, the device in hal)
I'd personally be happy with the /media/someotherdir/label approach, as it is mapped network drives that I have the problem with (which are mapped via /etc/fstab, so I can specify the mount directory), although I agree that fixing it for the mounts like /media/label would be best if it could be achieved.
On second look, glib gio/gunixmounts.c already does everything we need: it figures out _PATH_MOUNTED (/proc/mounts), _g_get_unix_mounts() iterates over it, so all that's left is to add a test whether mnt_fsname is a local device node, and if it is, access() it. That looks trivial and cheap in terms of runtime overhead? I'm willing to give that a shot, if David agrees with the general idea (bug is assigned to me in Ubuntu).
Created attachment 110977 [details] lbmount with mkdir_safe change ownership
The issues i've run into here, with openSuSE LTSP/KIWI is that when multiple users mount their drives, the drives show up on everyone's desktop. Using patches submitted by David in comment #12, and Warren in comment #14, I was able to get lbmount to fail nicely for the users. The problem here is that lbmount makes the directory owned by root, then changes the permissions (from comment #14) to not allow the user to access the folder! If you add the code below, replacing mkdir_safe function, it will cause the folder to be created, then grant the user owner permissions. When we added the attached code to the other patches, we have a fully functional lbmount, which fixes the original problem.
Jakob, please do not confuse the issue. GNOME's problem here is more general than LTSP and lbmount. You need to submit your suggested changes to LTSP upstream because it is an isolated problem. (At first glance I don't like your suggested change because it seems to be based on some other SuSE-unique error. Let's see what LTSP upstream says.)
Warren, you're right. The permissions issue is a lbmount issue only, something not related to gnome. The patch in comment #12 is the gnome fix. Your fix warren, is also unrelated to gnome, but lbmount. I'm not sure if this is SuSE specific, but lbmount as it exists now gives the wrong permissions (which Warren fixed) and wrong ownership (which my patch fixes). Its been passed onto the Kiwi-LTSP folks already and should make it into SuSE 11. On my SuSE dev box, I have to chmod +s lbmount for it to work. The reason I mention this patch here is for those who stumble upon this issue when working on LTSP. its been reported in other distros than SuSE.
> lbmount as it exists now gives the wrong permissions (which Warren fixed) > and wrong ownership (which my patch fixes). I checked upstream about this as I first thought the ownership was incorrect. It turns out that the ownership actually is intended to be this way by design. Let's bring this to the upstream LTSP list and not confuse this GNOME ticket.
(Permission correction in lbmount plus the patch in Comment #12 makes this work on Fedora without putting devices onto the desktops of other users. This doesn't work for you?)
I can confirm that my original bug is now fixed with the latest update. I only see network drives on my desktop (and in Places) which I have permissions for, and likewise for my other users. Well done, and thanks everybody. Andy
Anything left to do here ? If yes, please ropen.
the issue described is still not fixed so reopening
This bug is fixed for me (I originally reported it), and has been since the 18th May. As far as I'm concerned, there's no more work needed on this.
Hm, not for me. If I have two user sessions on the machine , and one plugs in a USB drive (so that it gets mounted for that user, with umask=077 by default), then the other user session gets a nasty "permission denied" error out of the blue, and an useless drive symbol on his desktop.
Martin, did you ever write the patch that you outlined in comment #22 ?
Not yet, sorry. Vacation and some high-urgency tasks getting in between. It's still on my radar, though.
Created attachment 123606 [details] [review] stat mount point only for local block devices Sorry for taking so long, this slipped my attention for a while. This patch works well for the case I described above (other user plugs in an umask 077 USB stick), and is very conservative IMHO: it only stat()s the mount point if the mounted device starts with '/' (mostly for efficiency reasons), and is an existant block device. I tested it with an accessible and an inaccessible mount, and also with a non-existing block device (mv /dev/... after mount; in that case the mount appears again).
Don't we want to keep the subdir-of-/media approach working too ?
I removed that extra check to avoid further stat()ing, since that subdir-of-media was a workaround to fix a particular subclass of this bug. However, I'm happy to leave it and just add the logic of my recent patch.
Created attachment 123908 [details] [review] stat mount point only for local block devices There we go, I kept the original parent dir check, and just added the new test. Tested again with several different cases, and it behaves well for me now (not showing drive icons and "can't access" error messages any more if a different user plugs in a device).
Thanks, please commit.
> Thanks, please commit. Great! I think/hope the "please commit" is not meant for me? (I'm not a GNOME committer).
I'll take care of it before the next release.
* gunixmounts.c: Use g_access() to check accessibility of local devices. Patch by Martin Pitt