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 684093 - Gnome-shell-hotplug-sniffer block access to mounted sshfs filesystem
Gnome-shell-hotplug-sniffer block access to mounted sshfs filesystem
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
3.4.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2012-09-15 14:45 UTC by Giovanni Mascellani
Modified: 2012-10-03 17:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Autorun: don't scan the filesystem if autorun is disabled (3.17 KB, patch)
2012-09-15 15:15 UTC, Giovanni Campagna
needs-work Details | Review
autorunManager: Don't scan the filesystem if autorun is disabled (3.34 KB, patch)
2012-09-16 17:56 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
autorunManager: Don't try to scan remote filesystems (1.51 KB, patch)
2012-09-16 17:56 UTC, Jasper St. Pierre (not reading bugmail)
rejected Details | Review
autorunManager: Clean up mount operations (2.37 KB, patch)
2012-09-19 07:09 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
autorunManager: Don't scan the filesystem if autorun is disabled (1.48 KB, patch)
2012-09-19 07:09 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
autorunManager: Don't try to scan remote filesystems (1.96 KB, patch)
2012-09-19 07:10 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
autorunManager: Don't scan the filesystem if autorun is disabled (1.62 KB, patch)
2012-09-26 15:43 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
autorunManager: Don't try to scan remote filesystems (1.97 KB, patch)
2012-10-03 17:35 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
autorunManager: Don't try to scan remote filesystems (1.97 KB, patch)
2012-10-03 17:36 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Giovanni Mascellani 2012-09-15 14:45:36 UTC
Hi.

I'm using Gnome shell 3.4.2 on Debian unstable. I'm experiencing the following bug: I need to mount a filesystem with sshfs (via fuse) in a subdirectory of my home. When I do so, gnome-shell-hotplug-sniffer starts searching through this filesystem to find interesting things. Unfortunately this file is a whole filesystem of another computer, and it includes things like /proc/kmem and similars.

What happens is that I can't access the mounted filesystem (every read access blocks forever). I think that gnome-shell-hotplug-sniffer uses so aggressively the filesystem (and sshfs has probably problems with such intense and parallel usage), that fuse isn't able to handle the situation and starts blocking everything.

I think that Gnome shell should be somewhat more considerate when scanning a big filesystem, or at least that there should be some way to disable it.

This bug was already triaged with Giovanni Campagna.

Thanks, Giovanni (yes, that's my name too!).
Comment 1 Giovanni Mascellani 2012-09-15 14:49:16 UTC
BTW, the workaround suggested by Giovanni Campagna is to create a directory named /DCIM/100ABCDE in the mounted filesystem. It worked!
Comment 2 Giovanni Campagna 2012-09-15 15:00:36 UTC
My simple solution (doable for 3.6, I think) is to check for autorun-never GSettings key before spawning the hotplug-sniffer or accessing the filesystem in any way.
Comment 3 Giovanni Campagna 2012-09-15 15:15:18 UTC
Created attachment 224413 [details] [review]
Autorun: don't scan the filesystem if autorun is disabled

Content-type scanning can be expensive, but in some situations it's
difficult to know so in advance, in particular for remote filesystems
that are exposed as native, such as nfs or fuse-sshfs. Allowing the user to
disable autorun entirely in this situation is an acceptable workaround.

The only behavioural change with this patch, other than not accessing
mounted filesystems, is that the resident notification brings you to Files,
rather than the application configured to handle the specific device.
I don't think this is a problem, as a user disabling autorun entirely
is likely to consider our choice of applications inappropriate anyway.
Comment 4 Milan Bouchet-Valat 2012-09-16 09:38:30 UTC
I thought there was a time limit to these attempts to find a content type? It probably does not make sense to crawl a filesystem for more than a few seconds: after that, the user has opened the filesystem by his own devices anyway.
Comment 5 Giovanni Campagna 2012-09-16 16:54:14 UTC
The limit is 1500 milliseconds. On a decent system, with 10 parallel threads and non-blocking IO, this means a lot of requests, and even if the requestor process id dead, the fuse fs has to answer them.
Comment 6 Jasper St. Pierre (not reading bugmail) 2012-09-16 17:03:14 UTC
Can't we exclude sshfs/other types of remote FS from autorun?
Comment 7 Jasper St. Pierre (not reading bugmail) 2012-09-16 17:09:31 UTC
Review of attachment 224413 [details] [review]:

::: js/ui/components/autorunManager.js
@@ +184,3 @@
+
+    _processMount: function(mount, hotplug) {
+        if (!this._settings.get_boolean(SETTING_DISABLE_AUTORUN)) {

I hate double negatives.

    let autorunEnabled = !this._settings.get_boolean(SETTING_DISABLE_AUTORUN);
    if (autorunEnabled) {
        ...

@@ +198,3 @@
+            this._residentSource.addMount(mount, apps);
+            if (hotplug)
+                this._transDispatcher.addMount(mount, apps, contentTypes);

... this wasn't tested, was it?

(and this code should probably be split out between the two paths)
Comment 8 Giovanni Campagna 2012-09-16 17:15:11 UTC
(In reply to comment #7)
> Review of attachment 224413 [details] [review]:
> 
> [...]
> 
> @@ +198,3 @@
> +            this._residentSource.addMount(mount, apps);
> +            if (hotplug)
> +                this._transDispatcher.addMount(mount, apps, contentTypes);
> 
> ... this wasn't tested, was it?

It was tested. I noticed contentTypes was not there (because the other path caused an error), thought it would be irrelevant as addMount() returns if autorun is disable, but obviously forgot the ReferenceError.

> (and this code should probably be split out between the two paths)

Which two paths? Hotplug vs not hotplug, or autorun vs no autorun?
(It used to be split, but I like it more if it's consolidated)
Comment 9 Giovanni Campagna 2012-09-16 17:19:03 UTC
(In reply to comment #6)
> Can't we exclude sshfs/other types of remote FS from autorun?

I don't know. I could check filesystem::type, but who decides the content of the white/blacklist? Is it only known local fs, like ext2/3/4, btrfs, xfs, hfs... or only known network fs, like nfs, sshfs, anything -fuse...
Do we simply wait for users to file another bug if they have an exotic FS that can't handle the hotplug sniffer?
Comment 10 Jasper St. Pierre (not reading bugmail) 2012-09-16 17:56:13 UTC
Created attachment 224451 [details] [review]
autorunManager: Don't scan the filesystem if autorun is disabled

Content-Type scanning can be expensive, and the user disabled autorun, so...
Comment 11 Jasper St. Pierre (not reading bugmail) 2012-09-16 17:56:25 UTC
Created attachment 224452 [details] [review]
autorunManager: Don't try to scan remote filesystems

Content-Type scanning can be super expensive. The autorun manager is meant
for local filesystems that are plugged into a USB port or similar, not
remote NFS or sshfs mounts.



What about something like this?
Comment 12 Giovanni Campagna 2012-09-16 18:22:51 UTC
Review of attachment 224451 [details] [review]:

::: js/ui/components/autorunManager.js
@@ +183,3 @@
     },
 
+    _contentTypeDiscovered: function(mount, apps, contentTypes, hotplug) {

Ehm, where is this hotplug coming from?
Comment 13 Giovanni Campagna 2012-09-16 18:26:14 UTC
Review of attachment 224452 [details] [review]:

Your patch is wrong (or at least, it's a behavior change I don't agree with): it hides all network mounts from the transient dispatcher and the resident source, meaning that you can't launch or stop network locations from the shell.
Comment 14 Jasper St. Pierre (not reading bugmail) 2012-09-19 07:09:52 UTC
Created attachment 224700 [details] [review]
autorunManager: Clean up mount operations

Add a processMount function that's shared between mount scanning
and hotplug.
Comment 15 Jasper St. Pierre (not reading bugmail) 2012-09-19 07:09:57 UTC
Created attachment 224701 [details] [review]
autorunManager: Don't scan the filesystem if autorun is disabled

Content-Type scanning can be expensive, and the user disabled autorun, so...
Comment 16 Jasper St. Pierre (not reading bugmail) 2012-09-19 07:10:00 UTC
Created attachment 224702 [details] [review]
autorunManager: Don't try to scan remote filesystems

Content-Type scanning can be super expensive. The autorun manager is meant
for local filesystems that are plugged into a USB port or similar, not
remote NFS or sshfs mounts.
Comment 17 Jasper St. Pierre (not reading bugmail) 2012-09-19 07:11:05 UTC
These patches should keep the same invariants as last time. I still don't have a test case to try out automounting, so these are "blind", per se.
Comment 18 Giovanni Campagna 2012-09-19 11:14:57 UTC
They all look good. I'll test them and report.
I don't think they're blocking 3.6.0 anyway, it's fine if we get them in 3.6.1
Comment 19 Jasper St. Pierre (not reading bugmail) 2012-09-20 22:41:54 UTC
Did you test them?
Comment 20 Giovanni Campagna 2012-09-21 07:28:26 UTC
Just did:
    JS ERROR: !!!   Exception was: TypeError: this._settings is undefined
    JS ERROR: !!!     message = '"this._settings is undefined"'
    JS ERROR: !!!     fileName = '"/opt/gnome/share/gnome-shell/js/ui/components/autorunManager.js"'
    JS ERROR: !!!     lineNumber = '108'
    JS ERROR: !!!     stack = '"([object _private_unknown_GProxyMount])@/opt/gnome/share/gnome-shell/js/ui/components/autorunManager.js:108
wrapper([object _private_unknown_GProxyMount])@/opt/gnome/share/gjs-1.0/lang.js:204
([object _private_unknown_GProxyMount],true)@/opt/gnome/share/gnome-shell/js/ui/components/autorunManager.js:212
wrapper([object _private_unknown_GProxyMount],true)@/opt/gnome/share/gjs-1.0/lang.js:204
([object _private_unknown_GUnionVolumeMonitor],[object _private_unknown_GProxyMount])@/opt/gnome/share/gnome-shell/js/ui/components/autorunManager.js:228
wrapper([object _private_unknown_GUnionVolumeMonitor],[object _private_unknown_GProxyMount])@/opt/gnome/share/gjs-1.0/lang.js:204

Anyway, multiple workarounds exists, so this is not a blocker for 3.6.0. There'll plenty of time to test it more thoroughly.
Comment 21 Jasper St. Pierre (not reading bugmail) 2012-09-26 15:43:38 UTC
Created attachment 225223 [details] [review]
autorunManager: Don't scan the filesystem if autorun is disabled

Content-Type scanning can be expensive, and the user disabled autorun, so...
Comment 22 Jasper St. Pierre (not reading bugmail) 2012-10-02 19:13:24 UTC
Can I get a review on this for 3.6.1?
Comment 23 Giovanni Campagna 2012-10-03 17:19:17 UTC
Review of attachment 224700 [details] [review]:

Yes
Comment 24 Giovanni Campagna 2012-10-03 17:19:52 UTC
Review of attachment 225223 [details] [review]:

Looks good.
Comment 25 Giovanni Campagna 2012-10-03 17:24:01 UTC
Review of attachment 224702 [details] [review]:

Tested and works fine with a real USB key, but one thing that comes to my mind is: do AFP or MPT (GDaemonMounts) include a GVolume and a GDrive? I guess not, looking at gvfs sources, so this code regresses opening music players and cameras with the right app.
Comment 26 Jasper St. Pierre (not reading bugmail) 2012-10-03 17:30:40 UTC
(In reply to comment #25)
> Review of attachment 224702 [details] [review]:
> 
> Tested and works fine with a real USB key, but one thing that comes to my mind
> is: do AFP or MPT (GDaemonMounts) include a GVolume and a GDrive? I guess not,
> looking at gvfs sources, so this code regresses opening music players and
> cameras with the right app.

AFP is for over the network, and it even has a volume. You're right for MPT, though, so I'll change the patch to err on the side of caution in this case.
Comment 27 Jasper St. Pierre (not reading bugmail) 2012-10-03 17:35:12 UTC
Attachment 224700 [details] pushed as 7e496b1 - autorunManager: Clean up mount operations
Attachment 225223 [details] pushed as ff9509b - autorunManager: Don't scan the filesystem if autorun is disabled
Comment 28 Jasper St. Pierre (not reading bugmail) 2012-10-03 17:35:40 UTC
Created attachment 225696 [details] [review]
autorunManager: Don't try to scan remote filesystems

Content-Type scanning can be super expensive. The autorun manager is meant
for local filesystems that are plugged into a USB port or similar, not
remote NFS or sshfs mounts.
Comment 29 Jasper St. Pierre (not reading bugmail) 2012-10-03 17:36:53 UTC
Created attachment 225697 [details] [review]
autorunManager: Don't try to scan remote filesystems

Content-Type scanning can be super expensive. The autorun manager is meant
for local filesystems that are plugged into a USB port or similar, not
remote NFS or sshfs mounts.
Comment 30 Giovanni Campagna 2012-10-03 17:45:16 UTC
Review of attachment 225697 [details] [review]:

OK

(I though AFP was Apple Filing Protocol, and used to access iPhones...)
Comment 31 Jasper St. Pierre (not reading bugmail) 2012-10-03 17:54:15 UTC
(In reply to comment #30)
> Review of attachment 225697 [details] [review]:
> 
> OK
> 
> (I though AFP was Apple Filing Protocol, and used to access iPhones...)

It is Apple Filing Protocol. It's not used to access iPhones, though.
Comment 32 Jasper St. Pierre (not reading bugmail) 2012-10-03 17:56:53 UTC
# Bug 684093 - Gnome-shell-hotplug-sniffer block access to mounted sshfs filesystem - UNCONFIRMED

Attachment 225697 [details] pushed as 0ad739e - autorunManager: Don't try to scan remote filesystems