Comment 9 for bug 95917

I was experiencing this in Hardy Heron, and it was very annoying, so I decided to do something about it and try my hand at debugging it. So I installed the source package, compiled it and ran it through GDB. Sure enough it segfaulted after changing a few wallpapers. Output was very similar to the above, but the line that caught my attention was:

#6 0x0804ee7a in f_set_rand_wallpaper (button=0x82b0768, user_data=0x0) at wp_tray_util.c:371

Looking at line 371 of wp_tray_util.c:

free(sz_randfile);

So it appears that freeing this memory is causing the segfault. That pointer contains the full path of the randomly selected wallpaper, and the memory gets allocated in a function (in the same source file) called f_get_dir_entry.
It took me a while to figure it out, because it seemed to be happening randomly, but eventually I discovered that the
bug is definitely in the f_get_dir_entry function. The problem resides in this block of code:

                // found the target yet?^M
                if(*p_dir_trgt == 0)^M
                {^M
                        // malloc a new string^M
                        *sz_dir_trgt = (gchar *)malloc(100);^M
                        ^M
                        getcwd(*sz_dir_trgt, 100);^M
^M
                        // copy the target string^M
                        strncat(*sz_dir_trgt, "/", 100);^M
                        strncat(*sz_dir_trgt, entry->d_name, 100);^M
                }// end if^M

There are 2 problems with this code that I see. The first is a buffer overflow, the cause of the segfault: only 100 characters are being allocated for the file name. That might be fine if it were just the file name, but this is the full path. So if the full path is longer than 100 characters, there is a problem. The directory I am using is almost 90 characters long by itself!!!Sure enough, it crashed every time the randomly selected filename was over 100 characters, but worked fine when the path is under 100 characters. Altering the code above and replacing 100 wherever it occurs with 1024 fixes the problem for me.

The second problem is the way the author is using strncat. Using strncat to try and prevent a buffer overflow was a good idea, but he is calling it multiple times. The 3rd parameter to strncat is the max bytes to copy, not the max size of the buffer. So he allocates 100 bytes, the fills some of those bytes with the current directory and a trailing slash, and then fills some more with the actual file name. However, if the path + trailing slash is 99 characters long (for example), strncat will still happily append up to 100 characters on the end of the string when he copies the filename. Basically, in the last strncat , the max bytes shouldn't be a static value, but should subtract the current length of the string from the number of allocated bytes.

I'd be interested to know if this fixes the problems for others. I can never tell if my problem is the exact same as the one other people are describing. To repeat my fix on your computer (assumes you have build-essentials installed):

apt-get build-dep wallpaper-tray
apt-get source wallpaper-tray

cd wallpaper-tray-0.4.6/src/

Edit the "wp_tray_util.c" file with your favorite text editor. Find the lines of code I mentioned above (right around line 125), and replace every "100" with "1024". Save and exit the editor.

Then change to the main source directory ( wallpaper-tray-0.4.6 ), compile:
./configure
make

This won't install the compiled files though. You can run the newly compiled version of the program from the command line to test it out. Its under the "src" directory, its called "wp_tray" .

If anyone knows how to to get this information upstream, please do. I would provide a diff, but I don't have time right now to do it properly ( I made a lot of changes to my source file to print out debugging messages, so I'd have to redo it all on my end to get a diff with just the relevant changes.)

I hope this helps somebody, enjoy :)