[Alsaplayer-devel] PATCH: Fixes partially invalid logic on shuffle

[Kevin Day]

Every time I clicked shuffle, I would always get a segfault.
I later discovered that if I select and play some item, shuffle will
not segfault.

Looking at the code I noticed a logic flaw:

With the file: alsaplayer-0.99.80.orig/app/Playlist.cpp
Around Lines:  697 and 707

Code: (*(queue.begin() + curritem - 1)).marked_to_keep_curritem = 1;
and this code: (*(queue.begin() + curritem - 1)).marked_to_keep_curritem = 0;

The logic is partially wrong, if the current item does not exist, then
it may be equal to -1.
This happens when alsaplayer is first loaded with items but none are
selected for the first time.
In which case curritem + queue.begin() is 0, thus the equation:
"queue.begin() + curritem - 1" results in -1 which is an invalid array
location.
The end result is a segfault and core dump.

Below I have a patch that fixes in two spots.

This is a quick patch i whipped togethor so I could listen to music
before I go to sleep without fearing a segfault.
And so as I am looking at this and typing this e-mail I am suspecting
that I should actually be checking for "queue.begin() + curritem > 0"
and not just "curritem > 0", but I am too tired to recompile and test.

diff -r -u alsaplayer-0.99.80.orig/app/Playlist.cpp
alsaplayer-0.99.80/app/Playlist.cpp
--- alsaplayer-0.99.80.orig/app/Playlist.cpp    2008-08-05 22:37:57 -0500
+++ alsaplayer-0.99.80/app/Playlist.cpp 2008-08-05 22:40:24 -0500
@@ -697,7 +697,10 @@
       Lock();

       // Mark curritem
-       (*(queue.begin() + curritem - 1)).marked_to_keep_curritem = 1;
+       if (curritem > 0)
+               (*(queue.begin() + curritem - 1)).marked_to_keep_curritem = 1;
+        else
+               (*(queue.begin())).marked_to_keep_curritem = 1;

       // Shuffle
       random_shuffle(queue.begin(), queue.end());
@@ -707,7 +710,10 @@
           if ((*p).marked_to_keep_curritem == 1)
               break;

-       (*(queue.begin() + curritem - 1)).marked_to_keep_curritem = 0;
+       if (curritem > 0)
+               (*(queue.begin() + curritem - 1)).marked_to_keep_curritem = 0;
+        else
+               (*(queue.begin())).marked_to_keep_curritem = 0;

       // Tell the subscribing interfaces about the change
       if(interfaces.size() > 0) {

-- 
Kevin Day