I'll set up an image to try to reproduce this.  I've also cc'ed Fred.

However, I was unable to reproduce it.  I'm running Debian Lenny instead of etch, but other than that, I set up the system identically with the same permissions and sharing the music folder over Samba.

I used playlists 0 10 to get the IDs of my playlists, then playlists delete playlist_id:12 to delete my test playlist.

playlists 0 10 showed me the playlist was now gone, and refreshing the web interface also showed that.

Looking in the directory, the music was still there but the playlist wasn't, as expected.

Was there anything else going on there on your system that might affect the reproducibility of this bug, Daniel?  Needless to say it caught our attention promptly.

My worst fear is that somehow a stray ' mark is somehow feeding the CONTENTS of the playlist into a delete function.

The code at work here is Slim::Control::Commands::playlistsDeleteCommand.  using the given id, the code grabs a playlist object from the DB.  That object is then passed to Slim::Player::Playlist::removePlaylistFromDisk. There are two possible results here:

1) If the playlist object has a ->path member and the path exists, it's deleted.

2) if there path doesn't exist, then we delete  a file called <playlistdir>/<playlisttitle>.m3u

So, if the worry is about strange characters at play...Daniel, do you recall the title and/or path of the playlist that caused this problem?

I've since "fixed" my access rights of my Samba share to prevent user "slimserver" from deleting files but I can put it back for the purposes of this investigation.

I can recreate the EXACT sequence of events that led to this and report back. Any debugs in particular I should set?

I do remember however that the playlist id returned was something similar to 18167 (5 digit number in the 10,000s)
I only have about 15 playlists defined and don't muck around with them _that_much_.

One thing to note is that I rarely create a playlist from within slimserver. I use MusicIP on my WinXP PC (accessing the music through the Samba share). I then have a perl script running on my Linkstation which finds the playlist and converts/copies the playlist into the slimserver's playlist directory and then issues a "playlist play" CLI command to load up and start playing the playlist within seconds of its creation. I used to do a "rescan playlists" but found this was taking too much time. The reason I started experimenting with the new "playlists delete" command is to have my perl script find a changed playlist (looks at the DOS "archive" aka u:x flag and resets it) and "update" the slimserver playlist by deleting the old and playing the new so that it gets replaced right away.

Therefore my playlist.m3u file looks similar to this:
/mnt/music/artist - album/01-song.mp3
/mnt/music/otherartist - otheralbum/05-othersong.mp3

It doesn't have any of the other lines (the comments) that are usually found in slimserver generated playlists. I am able to do this (by selecting "save as extended playlist" option in MusicIP) but found it didn't make any difference either way. (until now that is... ;)

I'm just thinking now that there might be something to try here:

Say you have a 1 song playlist named "test.m3u".

-Do a CLI:playlist play "/path/test.m3u" (this adds the PL and starts playing it).
-Change /path/test.m3u to use a different song.
-Stop the playback of the song if still playing (as mine was not playing at the time)
-Do a CLI:playlists delete playlist_id:(id of test)
-Do a CLI:playlist play "/path/test.m3u" (this should now add the "new" playlist and start playing it).

Ok, here are some details:

Playlist name: /opt/slimserver/playlists/test.m3u

Contents of playlist:
\xef\xbb\xbf#CURTRACK 0
/mnt/music/4 Non Blondes - What's Up/01-What's Up (Edit).mp3

I will try to reproduce tonight after work.

Well, I can't reproduce it now...
I'm reasonably sure I did everything the same as before.
I'm doing a full database rescan right now so I'll try again tomorrow (~18000 tracks on a linkstation takes a long time).
Hopefully I'll be able to reproduce it tomorrow... :-/

Created attachment 1926 [details]
Slimserver log of when the .mp3 file gets deleted

Created attachment 1927 [details]
playlist that caused the problem

Got it! :)

Ok, after the wipe cache and rescan of last night, I tried it once more time.
Interestingly, I had a stray "test.m3u" file that I forgot to erase before I did the rescan, which contained another song. When I created a new playlist with the "/mnt/music/4 Non Blondes - What's Up/01-What's Up (Edit).mp3" line in it, it did not erase the file listed in the old test.m3u (although the permissions wouldn't have allowed it) but it erased the file listed in the *new* playlist! (see attachment). My song count in the Slimserver main http page is down by 1 and an inspection of the "4 Non Blondes - What's Up" directory reveals that song 01 is not longer there.

Also, the new playlist did not start playing, a message showed briefly on the Squeezebox, something similar to "could not find track 01-What's Up..." and the mode remained as "stop".

I hope I set enough debugs (probably too many!).
Let me know if I can do anything else to help.

Daniel

I'm curious about one bit of the log.  I'm not used to the CLI so maybe I'm reading this wrong:

2007-05-02 20:17:26.8606    Result: [@playlists] is loop with 1 elements:
2007-05-02 20:17:26.8629    Result:   0. [id] = [17916]
2007-05-02 20:17:26.8649    Result:   0. [playlist] = [Test]

Doesn't this mean the playlist ID is 17916, and not 1?

Indeed, playlist id is 17916.

Here is the output of command "playlists 0 100" from the CLI (with line splits for legibility):

playlists 0 100 
count%3A21 
id%3A17898 playlist%3AAmore 
id%3A17899 playlist%3AAmour 
id%3A17900 playlist%3AArgentina 
id%3A17901 playlist%3ABlue 
id%3A17902 playlist%3ABlueEyes 
id%3A17903 playlist%3ABrazilian 
id%3A17904 playlist%3ADonnaMobile 
id%3A17905 playlist%3AErnie 
id%3A17906 playlist%3AJazzEasy 
id%3A17907 playlist%3AMellow 
id%3A17908 playlist%3AMere 
id%3A17909 playlist%3AOrphee 
id%3A17910 playlist%3APiano 
id%3A17911 playlist%3AQuebec 
id%3A17912 playlist%3ARose 
id%3A17913 playlist%3ASunshine 
id%3A17914 playlist%3ASurvive 
id%3A17915 playlist%3ATango 
id%3A17916 playlist%3ATest <--------------
id%3A17917 playlist%3AUneChance 
id%3A17918 playlist%3AYou

It seems that playlist IDs follow song IDs (I have 17897 MP3 songs but only 17896 recognized by SS, haven't found out which one yet) so the first playlist ID is 17898...

Right.

(a) the test case is funky: it searches for playlist tests, which is found with id 17xxx, but the code then deletes playlist id 1 (i.e. not the returned value). So no surprises, playlist 17xxx file is not removed...

(b) indeed, for some reason the SS database stores playlists as tracks AND scans them after tracks so they get "high" IDs (after songs).

(c) now the bug seems to be the CLI does not check the playlist ID it receives is indeed a playlist and not a track. It goes ahead and calls Slim::Player::Playlist::removePlaylistFromDisk (as described by kdf) which apparently does not perform the check either. Tracks do have a path component and it exists (duh!) and voila, track removed!

Note that I think the same code is available and used with the web interface and therefore a similar "attack" (sending the wrong playlist id) is possible through the web as well.

Given I am not sure we want to propose this capability (remove song files), I guess this qualifies as a bug. The code in Commands.pm should check this is really a playlist and not a track.

Ahem, were it not for my dodgy perl programming, this vulnerability might not have been uncovered! ;)
I didn't consider this possibility (of passing a track's id) since it just so happened that my test playlist contained exactly and only that very song! (used the first song of the first album which happened to have track id 1).

It seems to me that it would be much safer if playlists had their own independent set of ids instead of sharing it all from the same lot! Especially when you add new tracks after a full rescan (or delete, whatever) because then you end up with an entertwined bunch of tracks & playlists with no easy visual way of differentiating them...

While I have your attentions, I'd like to ask for a small feature: the ability to load a playlist from an .m3u file into the "current" unnamed playlist without actually adding anything to the library. If a song is not found in the database, then there is no attempt at adding it, it would be simply not added to the current playlist.

This would make my life much easier in the sense that my MusicIP generated playlist could be played quite quickly on the slimserver without having to add or replace (remove/add) named playlists or (rescan playlists). I believe this would fit well within the set of available playlist manipulation CLI commands. If you think this has some merit, let me know if I should fill out an enhancement request (or other).

Thanks,
Daniel

cc'ing Dean on this since he expressed an interest.

Daniel, see bug 4428 to vote for your request.

Some sanity checks on things like the path of the 'about to be delete' file and the content type will help avoid a music file being deleted.  I'm not even going to try and claim that I understand the DBI interface, but the find is requesting type 'playlist' so I'm not sure why we get an audio file  from that.

Bug 4428 is not exactly the same, but I voted for it and added my little description in the comments.

Fixed in r11896. May I leave it to you to manage 6.5.x? Not sure we can commit in there anymore.

6.5.2 commit is still available, but it's a matter of targetting.  Chris, what's the call?  I'll gladly take care of merging if we need it.

The change looks very very safe to me.  If you have a moment, KDF, would you also put it in 6.5.2?  

Thanks Fred and KDF for looking at this one so quickly!  There's almost no bug I can think of worse for our users than one that might result in deleting their music, even though it turned out it was unlikely.

Well, as unlikely as using this line:
my $plid = ($str =~ / id%3A(\d+) /);

instead of:
my ($plid) = ($str =~ / id%3A(\d+) /);

Many thanks to all who took care of this rapidly! I certainly appreciate it.
Daniel

merged into 6.5.2 at change 11919, and for the link: trunk was fixed at change 11896
updated trunk changelogs at change 11920 to reflect the new target.

marking as fixed.  If there are any problems, please reopen.
confirmations also welcome, to make sure this problem is truly gone.

Fixed in 6.5.2, which is now released and available for download at http://www.slimdevices.com/su_downloads.html

If you're still experiencing this bug, please re-open it!