[MPlayer-dev-eng] [PATCH] libmpdemux/demux_audio: Skip ID3v2.4 tags.

[Reimar Döffinger]

On Mon, Jan 16, 2012 at 10:21:28PM +0100, Benoît Thébaudeau wrote:
> +static unsigned int id3v2_tag_size(stream_t *s) {
> +  uint8_t tag[4];
> +  unsigned int header_footer_size;
> +  unsigned int size;
> +  int i;
> +
> +  stream_read(s,tag,2);
> +  if(tag[0] == 0xff)
> +    return 0;
> +  header_footer_size = tag[1] & 0x10 ? 20 : 10;
> +
> +  stream_read(s,tag,4);
> +  size = 0;
> +  for(i = 0; i < 4; i++) {
> +    if (tag[i] & 0x80)
> +      return 0;
> +    size = size << 7 | tag[i];
> +  }

"tag" doesn't seem like really good naming to me.
Personally I'd also rather go with either:
1) Read all 6 bytes at once or
2) Use stream_read_char to read one-by-one

> +    } else if( hdr[0] == 'I' && hdr[1] == 'D' && hdr[2] == '3' && hdr[3] != 0xff &&
> +               (len = id3v2_tag_size(s)) > 0) {
> +      stream_skip(s,len-10);
>        step = 4;

Why accept hdr[3] values of 0 or 1 now? Aren't they invalid, too?
Also when failing id3v2_tag_size has read data and thus the data in
hdr is no longer in sync with the file position, so you must at least
set step to 4 in that case.
So I'm afraid it looks to me that squeezing that into the if made it
both uglier and wrong.
Just
> } else if( hdr[0] == 'I' && hdr[1] == 'D' && hdr[2] == '3' && hdr[3] != 0xff) {
>   unsigned len = id3v2_tag_size(s);
>   if (len > 0)
>     stream_skip(s,len-10);
>  step = 4;

Should be fine I think.


> +      if( tag[0] == '3' && tag[1] == 'D' && tag[2] == 'I' && (uint8_t)tag[3] != 0xff &&
> +          (len = id3v2_tag_size(s)) > 0) {
> +        if(len > demuxer->movi_end) {
> +          mp_msg(MSGT_DEMUX,MSGL_ERR,"[demux_audio] Bad ID3v2 tag size: larger than stream (%u)!!!\n",len);
> +          len = demuxer->movi_end;

First, you should check against len > demuxer->movi_end - demuxer->movi_start,
because "end" before "start" is not that much better than negative end
:-)
Next, I doubt there is much point in setting it such that the file is
detected as empty, I think most users would prefer if we did our best
to try to play a broken file instead of pretend it is empty.
So IMO if you detect an error set len to 0 - or maybe better 10.