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 633309 - Guard against setting the library folder to / or ~
Guard against setting the library folder to / or ~
Status: RESOLVED FIXED
Product: banshee
Classification: Other
Component: general
1.8.0
Other Linux
: Normal normal
: 1.x
Assigned To: Banshee Maintainers
Banshee Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-10-27 23:56 UTC by rmflagg
Modified: 2011-03-22 15:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Dump running under Ubuntu 10.04 (1.56 KB, text/plain)
2010-10-27 23:56 UTC, rmflagg
  Details
check questionable directories and prevent starting the library watcher for them (1.60 KB, patch)
2011-02-09 08:23 UTC, Frank Ziegler
needs-work Details | Review
check for root directory and prevent starting the library watcher for it (1.32 KB, patch)
2011-02-10 01:48 UTC, Frank Ziegler
needs-work Details | Review
check for root directory and prevent starting the library watcher for it (1.24 KB, patch)
2011-02-11 01:28 UTC, Frank Ziegler
committed Details | Review

Description rmflagg 2010-10-27 23:56:43 UTC
Created attachment 173367 [details]
Dump running under Ubuntu 10.04

It's pretty simple bug: I run Banshee, it opens and the crashes.
Comment 1 Bertrand Lorentz 2010-10-28 18:28:43 UTC
Thank you for your bug report.

This seems to be caused by the FileSystemWatcher extension, which apparently has trouble watching your Music or Videos folder.

Could you run the following two commands in a terminal and post the result here ?

gconftool --get /apps/banshee-1/sources/_music_library_source_-_library/library-location

gconftool --get /apps/banshee-1/sources/_video_library_source_-_video_library/library-location


These should print out the current values configured for your Music and Videos folders.
Comment 2 rmflagg 2010-11-01 19:19:08 UTC
Here are the results of the commands:

myaccount@myaccount-laptop:~$ gconftool --get /apps/banshee-1/sources/_music_library_source_-_library/library-location
/
ouaejk@ouaejk-laptop:~$ gconftool --get /apps/banshee-1/sources/_video_library_source_-_video_library/library-location
/home/myaccount/Videos
Comment 3 rmflagg 2010-11-01 19:20:40 UTC
Please disregard the above comment...here are the correct results:

myaccount@myaccount-laptop:~$ gconftool --get
/apps/banshee-1/sources/_music_library_source_-_library/library-location
/
myaccount@myaccount-laptop:~$ gconftool --get
/apps/banshee-1/sources/_video_library_source_-_video_library/library-location
/home/myaccount/Videos
Comment 4 Bertrand Lorentz 2010-11-01 21:21:42 UTC
Hmm, the location of your music library is set to / ?
That's probably the cause of your problem.

Could edit the value of this parameter and set it to a "normal" value, like "/home/myaccount/Music" ?
You can use the "gconf-editor" tool to navigate to the parameter and edit it, or run the following command :

gconftool --type string --set apps/banshee-1/sources/_music_library_source_-_library/library-location "/home/myaccount/Music"
Comment 5 rmflagg 2010-11-01 21:40:19 UTC
That took care of it! Thanks!
Comment 6 Gabriel Burt 2010-11-02 00:22:14 UTC
We really need to guard against this.
Comment 7 Alexander Kojevnikov 2010-11-07 06:34:24 UTC
(In reply to comment #6)
> We really need to guard against this.

Changing the summary to reflect this. Also marking as gnome-love, should be a simple enough task for a novice contributor.
Comment 8 Frank Ziegler 2011-02-09 08:23:46 UTC
Created attachment 180445 [details] [review]
check questionable directories and prevent starting the library watcher for them

Proposed patch: check for questionable directories and for them, instead of starting the library watcher, throw a warning.

Fixes Banshee crash when library path is set to / or ~
Comment 9 Gabriel Burt 2011-02-09 20:30:51 UTC
Review of attachment 180445 [details] [review]:

Thanks, Frank.

::: src/Extensions/Banshee.LibraryWatcher/Banshee.LibraryWatcher/SourceWatcher.cs
@@ +80,1 @@
                 throw new Exception ("Will not create LibraryWatcher for the entire home directory");

isn't this check ^ already doing the home-directory check?

@@ +82,3 @@
 
+            if (path.Equals (Environment.SystemDirectory)
+             || path.Equals ("//")

This probably isn't valid on windows -- maybe use System.IO.Path.DirectorySeparator ?

@@ +88,3 @@
+                                         library.Name,
+                                         path,
+                                         Environment.GetFolderPath (Environment.SpecialFolder.MyMusic));

This warning is a little misleading w/ the SpecialFolder.MyMusic bit -- since the user can set whatever directory they want, plus we can watch the video folder etc.  I think just remove that part of the warning.
Comment 10 Frank Ziegler 2011-02-10 01:48:16 UTC
Created attachment 180545 [details] [review]
check for root directory and prevent starting the library watcher for it

Updated after Gabriel's review. Home directory was already checked, now determining the root path in a OS independent manner and throwing an exception the way the existing code is doing it.
Comment 11 Gabriel Burt 2011-02-10 23:05:11 UTC
Review of attachment 180545 [details] [review]:

::: src/Extensions/Banshee.LibraryWatcher/Banshee.LibraryWatcher/SourceWatcher.cs
@@ +81,3 @@
             }
 
+            if (path.Equals (Environment.SystemDirectory)

This is "" for me -- is this check necessary in your testing/on your setup?  Are you using Linux?

@@ +82,3 @@
 
+            if (path.Equals (Environment.SystemDirectory)
+             || path.Equals (Path.GetPathRoot (Environment.CurrentDirectory) + Path.DirectorySeparatorChar))

this value "//" for me -- I don't think you need the + Path.DSC part

@@ +83,3 @@
+            if (path.Equals (Environment.SystemDirectory)
+             || path.Equals (Path.GetPathRoot (Environment.CurrentDirectory) + Path.DirectorySeparatorChar))
+            {

put braces for if's on the same line as the condition, and put the condition all on one line if it's < 120 chars
Comment 12 Frank Ziegler 2011-02-11 01:28:19 UTC
Created attachment 180631 [details] [review]
check for root directory and prevent starting the library watcher for it

Removed the SystemDirectory check, I am on Linux and it was "" for me as well.

For some Reason the value of the root directory comes out at "//" and it does work. Without Path.DSC it does not, coming to the value of "/". I put my Music Library to "/" via gconf for testing. I assume, there is some point in the code where the pathes are assumed "naked" and the Path.DSC is added. Haven't verified that, but I had to put Path.DSC in order to get the patch working.

Also conformed to the coding guidelines now.

Thanks for the great support Gabriel, just getting started with this.
Comment 13 Gabriel Burt 2011-03-22 15:33:44 UTC
Comment on attachment 180631 [details] [review]
check for root directory and prevent starting the library watcher for it

Committed, thanks Frank.  I also pushed a patch checking for null/empty path, and for non-existent path.