double free when using --randomize with unloadable image file

[hidroto]

Setup

feh version 3.10.2-2-g2d8f9e1
compile time switches: verscmp xinerama
create two files
normal.png a normal loadable image file
empty.file an empty file

Run

feh --randomize normal.png empty.file

then move through the filelist with arrow keys.
feh wont always crash depending on the order of the filelist and perhaps other factors.
but when it does crash it reports

free(): double free detected in tcache 2
and the shell reports the process is terminated by signal SIGABRT.

may be the same bug as #535

[doughdemon]

Was able to reproduce.

The following seems to fix it.

diff --git a/src/slideshow.c b/src/slideshow.c
index 3944a68..c6c6901 100644
--- a/src/slideshow.c
+++ b/src/slideshow.c
@@ -227,6 +227,12 @@ void slideshow_change_image(winwidget winwid, int change, int render)
        /* The for loop prevents us looping infinitely */
        for (i = 0; i < our_filelist_len; i++) {
                winwidget_free_image(winwid);
+
+               if (last) {
+                       filelist = feh_file_remove_from_list(filelist, last);
+                       last = NULL;
+               }
+
 #ifdef HAVE_LIBEXIF
                /*
                 * An EXIF data chunk requires up to 50 kB of space. For large and
@@ -324,11 +330,6 @@ void slideshow_change_image(winwidget winwid, int change, int render)
                        break;
                }

-               if (last) {
-                       filelist = feh_file_remove_from_list(filelist, last);
-                       last = NULL;
-               }
-
                if (opt.on_last_slide == ON_LAST_SLIDE_HOLD && previous_file &&
                        ((current_file == filelist && change == SLIDE_NEXT) ||
                        (previous_file == filelist && change == SLIDE_PREV))) {

What causes the bug is I believe as follows:
At i=0, the for loop tries to go to the invalid empty.file. It fails and sets the last variable. Then, at i=1, since we are at the last image of the list, it is reordered, and the next file is randomly chosen. Only then, the logic to remove the invalid file from the list executes. Now, it may happen that the randomly chosen file is exactly the file to be removed from the list. Then, feh tries to remove the invalid file twice, and it crashes.

[doughdemon]

Applying pull request #414 is probably a better way to fix this.

[nigels-com]

Still looks like a problem. Using valgrind:

==426573== Invalid read of size 8
==426573==    at 0x12B5EB: slideshow_change_image (slideshow.c:338)
==426573==    by 0x11FC9A: feh_event_handle_generic (keyevents.c:483)
==426573==    by 0x11F77D: feh_event_handle_keypress (keyevents.c:413)
==426573==    by 0x121C76: feh_main_iteration (main.c:181)
==426573==    by 0x1219F4: main (main.c:105)
==426573==  Address 0x6005680 is 0 bytes inside a block of size 24 free'd
==426573==    at 0x484988F: free (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==426573==    by 0x117460: gib_list_remove (gib_list.c:422)
==426573==    by 0x113AC6: feh_file_remove_from_list (filelist.c:125)
==426573==    by 0x12B579: slideshow_change_image (slideshow.c:328)
==426573==    by 0x11FC9A: feh_event_handle_generic (keyevents.c:483)
==426573==    by 0x11F77D: feh_event_handle_keypress (keyevents.c:413)
==426573==    by 0x121C76: feh_main_iteration (main.c:181)
==426573==    by 0x1219F4: main (main.c:105)
==426573==  Block was alloc'd at
==426573==    at 0x4846828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==426573==    by 0x1304C4: _emalloc (utils.c:85)
==426573==    by 0x116D81: gib_list_new (gib_list.c:36)
==426573==    by 0x116E60: gib_list_add_front (gib_list.c:137)
==426573==    by 0x11440C: add_file_to_filelist_recursively (filelist.c:288)
==426573==    by 0x12A1CD: feh_parse_option_array (options.c:875)
==426573==    by 0x128563: init_parse_options (options.c:90)
==426573==    by 0x1218F1: main (main.c:57)
==426573== 
==426573== Invalid read of size 8
==426573==    at 0x118457: feh_load_image (imlib.c:339)
==426573==    by 0x136D6F: winwidget_loadimage (winwidget.c:833)
==426573==    by 0x12B5FF: slideshow_change_image (slideshow.c:338)
==426573==    by 0x11FC9A: feh_event_handle_generic (keyevents.c:483)
==426573==    by 0x11F77D: feh_event_handle_keypress (keyevents.c:413)
==426573==    by 0x121C76: feh_main_iteration (main.c:181)
==426573==    by 0x1219F4: main (main.c:105)
==426573==  Address 0x6005570 is 0 bytes inside a block of size 48 free'd
==426573==    at 0x484988F: free (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==426573==    by 0x113914: feh_file_free (filelist.c:81)
==426573==    by 0x113A39: feh_file_remove_from_list (filelist.c:122)
==426573==    by 0x12B579: slideshow_change_image (slideshow.c:328)
==426573==    by 0x11FC9A: feh_event_handle_generic (keyevents.c:483)
==426573==    by 0x11F77D: feh_event_handle_keypress (keyevents.c:413)
==426573==    by 0x121C76: feh_main_iteration (main.c:181)
==426573==    by 0x1219F4: main (main.c:105)
==426573==  Block was alloc'd at
==426573==    at 0x4846828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==426573==    by 0x1304C4: _emalloc (utils.c:85)
==426573==    by 0x1137E5: feh_file_new (filelist.c:48)
==426573==    by 0x1143F7: add_file_to_filelist_recursively (filelist.c:288)
==426573==    by 0x12A1CD: feh_parse_option_array (options.c:875)
==426573==    by 0x128563: init_parse_options (options.c:90)
==426573==    by 0x1218F1: main (main.c:57)