After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 526320 - should not list mounts that the user doesn't have permission to use
should not list mounts that the user doesn't have permission to use
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
1.0.x
Other Linux
: Normal normal
: ---
Assigned To: Alexander Larsson
gtkdev
: 530379 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2008-04-05 13:38 UTC by Sebastien Bacher
Modified: 2008-12-08 04:33 UTC
See Also:
GNOME target: ---
GNOME version: 2.21/2.22


Attachments
Patch (1.21 KB, patch)
2008-04-28 20:53 UTC, David Zeuthen (not reading bugmail)
committed Details | Review
lbmount with mkdir_safe change ownership (881 bytes, text/plain)
2008-05-15 22:27 UTC, Jakob Perry
  Details
stat mount point only for local block devices (1.47 KB, patch)
2008-11-28 12:38 UTC, Martin Pitt
none Details | Review
stat mount point only for local block devices (1.33 KB, patch)
2008-12-03 23:19 UTC, Martin Pitt
committed Details | Review

Description Sebastien Bacher 2008-04-05 13:38:02 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)"
Comment 1 Sebastien Bacher 2008-04-28 16:15:42 UTC
*** Bug 530379 has been marked as a duplicate of this bug. ***
Comment 2 David Zeuthen (not reading bugmail) 2008-04-28 16:20:57 UTC
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.
Comment 3 Warren Togami 2008-04-28 16:41:14 UTC
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?
Comment 4 David Zeuthen (not reading bugmail) 2008-04-28 16:51:00 UTC
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.
Comment 5 David Zeuthen (not reading bugmail) 2008-04-28 16:58:06 UTC
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.
Comment 6 Sebastien Bacher 2008-04-28 17:31:23 UTC
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?
Comment 7 Warren Togami 2008-04-28 17:48:04 UTC
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.
Comment 8 David Zeuthen (not reading bugmail) 2008-04-28 17:59:15 UTC
(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?
Comment 9 Sebastien Bacher 2008-04-28 18:57:18 UTC
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?
Comment 10 David Zeuthen (not reading bugmail) 2008-04-28 20:01:11 UTC
(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?
Comment 11 David Zeuthen (not reading bugmail) 2008-04-28 20:02:17 UTC
And of course s/tokens[num_tokens - 1];/tokens[num_tokens - 1] = NULL;/. Gah, I wish Bugzilla had an edit button ;-)
Comment 12 David Zeuthen (not reading bugmail) 2008-04-28 20:53:34 UTC
Created attachment 110066 [details] [review]
Patch

Here's a patch. Please review and commit to trunk and stable.
Comment 13 Matthias Clasen 2008-04-28 22:33:54 UTC
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.
Comment 14 Warren Togami 2008-04-28 22:41:26 UTC
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 15 David Zeuthen (not reading bugmail) 2008-04-28 22:42:32 UTC
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).
Comment 16 David Zeuthen (not reading bugmail) 2008-04-28 22:45:09 UTC
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.
Comment 17 Sebastien Bacher 2008-04-29 12:39:32 UTC
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
Comment 18 Sebastien Bacher 2008-04-29 12:59:49 UTC
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?
Comment 19 Andy Stallwood 2008-05-01 16:59:30 UTC
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.
> 

Comment 20 Martin Pitt 2008-05-09 14:52:22 UTC
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)
Comment 21 Andy Stallwood 2008-05-09 17:01:14 UTC
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.
Comment 22 Martin Pitt 2008-05-09 17:19:07 UTC
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).
Comment 23 Jakob Perry 2008-05-15 22:27:21 UTC
Created attachment 110977 [details]
lbmount with mkdir_safe change ownership
Comment 24 Jakob Perry 2008-05-15 22:27:44 UTC
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.
Comment 25 Warren Togami 2008-05-16 03:05:59 UTC
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.)
Comment 26 Jakob Perry 2008-05-16 04:52:42 UTC
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.
Comment 27 Warren Togami 2008-05-16 14:09:33 UTC
> 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.
Comment 28 Warren Togami 2008-05-16 14:15:22 UTC
(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?)
Comment 29 Andy Stallwood 2008-05-18 16:37:48 UTC
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
Comment 30 Matthias Clasen 2008-06-10 18:00:34 UTC
Anything left to do here ?

If yes, please ropen.
Comment 31 Sebastien Bacher 2008-06-10 20:03:11 UTC
the issue described is still not fixed so reopening
Comment 32 Andy Stallwood 2008-06-11 17:04:14 UTC
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.
Comment 33 Martin Pitt 2008-06-11 17:36:56 UTC
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.
Comment 34 Matthias Clasen 2008-07-02 03:56:59 UTC
Martin, did you ever write the patch that you outlined in comment #22 ?
Comment 35 Martin Pitt 2008-07-02 08:22:54 UTC
Not yet, sorry. Vacation and some high-urgency tasks getting in between. It's still on my radar, though.
Comment 36 Martin Pitt 2008-11-28 12:38:22 UTC
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).
Comment 37 Matthias Clasen 2008-11-28 19:07:13 UTC
Don't we want to keep the subdir-of-/media approach working too ?
Comment 38 Martin Pitt 2008-11-29 12:48:47 UTC
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.
Comment 39 Martin Pitt 2008-12-03 23:19:18 UTC
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).
Comment 40 Matthias Clasen 2008-12-04 04:19:29 UTC
Thanks, please commit.
Comment 41 Martin Pitt 2008-12-04 17:39:49 UTC
> Thanks, please commit.

Great! I think/hope the "please commit" is not meant for me? (I'm not a GNOME committer).
Comment 42 Matthias Clasen 2008-12-04 18:04:16 UTC
I'll take care of it before the next release.
Comment 43 Matthias Clasen 2008-12-08 04:33:56 UTC
       * gunixmounts.c: Use g_access() to check accessibility of local devices.
        Patch by Martin Pitt