Crash caused by lyric feature

Submitted by gunblade on Fri, 2009-08-14 22:49

Forums:

Noticed a bug in the code today which kept causing moc to crash in strange ways. (SVN revision 2161)

Turns out it was becaused of the strncpy in lyrics.c. The code has a const FILENAME_SIZE (set at 200), and uses this to allocate the lyrics_filename buffer, but then in the lyrics_remove_prefix function, the strncpy uses the strlen of the filename. This means if the filename is > 200 characters, then the strncpy will overwrite the memory after the buffer and cause a nasty crash.

I would suggest simply changing the xmalloc at line 64 from:

lyrics_filename = xmalloc (sizeof(char) * FILENAME_SIZE);

to:

lyrics_filename = xmalloc (sizeof(char) * (strlen(filename) + 1));

Its either that, or add some strlen(filename) > FILENAME_SIZE check in the lyrics_remove_prefix function, but the first
method should be more flexible.

I've also never used lyrics, so I havent come accross any issues - but there seems to be a similar issue where the lyri
cs array at the top of the file is pre-allocated using a fixed constant, but then on line 71 when the lyrics file is re
ad in, a while(fgets()) is used, with no bounds checking for the array. Again, this means a lyrics file with more than
128 lines would cause moc to crash.

gunblade

Mon, 2009-08-31 20:42

Something I forgot to mention - this bug will cause a crash with any files which have a path which are too long whether you use the lyrics feature or not - since the path is passed to the lyrics.c for it to check whether a lyrics file exists.

Here's a quick patch against r2169 that fixes the problem with the filename being too long.
The bug with the crash because of a lyrics file existing with more than 128 lines is still there, but I didnt fix it because that bug would only cause problems if you actually use the feature - which I dont. However it would be an easy fix by just counting the number of lines and mallocing the array after that, and only THEN reading in the actual contents - although this means 2 passes of the file.

Anyway, maybe I'm the only one with filepaths big enough to cause a crash, but I thought it would be a pretty important fix to apply before releasing the code as an actual release.

Index: lyrics.c
===================================================================
--- lyrics.c (revision 2169)
+++ lyrics.c (working copy)
@@ -21,11 +21,14 @@
static char *lyrics[LYRICS_LINE_NUMBER];

const unsigned short LINE_SIZE = 128;
-const unsigned short FILENAME_SIZE = 200;

-void lyrics_remove_prefix (const char *filename, char *new_name)
+void lyrics_remove_prefix (const char *filename, char **new_name)
{
- strncpy(new_name, filename, strlen(filename)-strlen(strrchr(filename, '.')));
+ unsigned int filelen;
+
+ filelen = strlen(filename)-strlen(strrchr(filename, '.'));
+ *new_name = xmalloc(sizeof(char) * (filelen + 1));
+ strncpy(*new_name, filename, filelen);
}

void lyrics_cleanup (const unsigned int n)
@@ -61,9 +64,7 @@
return lyrics;
}

- lyrics_filename = xmalloc (sizeof(char) * FILENAME_SIZE);
- memset (lyrics_filename, '\0', FILENAME_SIZE);
- lyrics_remove_prefix (filename, lyrics_filename);
+ lyrics_remove_prefix (filename, &lyrics_filename);

lyrics_file = fopen (lyrics_filename, "r");
if (lyrics_file != NULL) {

Edit: The code tags appear to be broken, or at least, they seem to stop on a blank line - so just removed them.

daper

Wed, 2009-10-07 11:00

I've forgot about this patch before releasing alpha4, but it's included now. One change I've made was to check if strrchr returned NULL.