[MPlayer-dev-eng] [PATCH] Overhauled the flac comment parsing in libmpdemux/demux_audio.c

[Reimar Döffinger]

On Mon, Aug 31, 2009 at 11:36:54PM -0600, selfacclaimedgenius at gmail.com wrote:
> I overhauled the flac vorbis-comment parsing function in
> 'libmpdemux/demux_audio.c'. I did this simply because I desire knowing what
> a song is by its tags, rather than relying on its file path alone.

Most of the code is unchanged except for variable renamings and
reindentation.
Which means that
a) the patch itself is unreadable
b) I or whoever reviews it will have to check it all instead of
just checking if all the known and huge security holes have been fixed.

And a quick look tells me the code is still full of issues anyway

> +						// Get current key=>value pair length
> +						comment_len = AV_RL32(ptr);
> +						ptr += 4;
> +						// Do some error checking
> +						comment = ptr;
> +						if (&comment[comment_len] < comments || &comment[comment_len] >= &comments[block_len]) return;

comment[comment_len] may overflow, which is undefined behaviour and thus
the compiler might decide to throw away the whole check in a critical
case.
Simple rule: the unvalidated data must stand alone, do not do any
operations with it and make sure there are no unexpected (esp. signed
<-> unsigned) conversions.


> +						char *c;
> +						c = strndup(comment, comment_len); // Get the whole sub-string to further process

strndup is a GNU extension, you can't use it.

> +							for(int j = 0; j < strlen(key); j++)
> +							{
> +								if(j == 0) key[j] = toupper(key[j]);
> +								else key[j] = tolower(key[j]);

Since key is case-insensitive within MPlayer, normalization would be
only for display. If that is desired, it should be done in some central
code, not in the demuxer.

> +								// If there are any invalid chars, according to spec, skip field.
> +								if((int*)key[j] < 0x20 || (int*)key[j] > 0x7D) goto flac_comment_cleanup;

WTF are those casts?

> +							if(strlen(table[j].flac) && strcasecmp(key, table[j].flac) == 0)

strlen(table[j].flac) here is the same as table[j].flac[0] just with
excessive waste of CPU time and memory bandwidth.
I hope you can understand that this very quick and partial review has
not convinced me a bit that the new proposed code isn't a security
nightmare.
If you don't have the experience to write code (near) 100% iron-clad the
first time, I'd suggest to start from the existing code and comb through
it to find all the issues (I think there should be a good handful)
instead of rewriting it and possible end up with even more than before -
after all I think I had started fixing the old code, I just considered
it too much).
Or I think you can just use -demuxer lavf to get the metadata...