[FFmpeg-devel] [PATCH] MP3: ID3V2 corrupted => bad offset

[Michael Niedermayer]

On Fri, Dec 05, 2008 at 01:41:32PM +0100, Yvan Labadie wrote:
> Hi,
>
> According to the ID3v2 standard, frame sizes are stored using 32bit 
> SynchSafe Integer format 
> (http://www.id3.org/id3v2.4.0-structure?highlight=%28SynchSafe%29).
> That's why libavformat/mp3.c uses id3v2_get_size() function.
> But I discovered that a lot of mp3 (about a fourth of my mp3s!) use classic 
> 32bit unsigned integer!!!
> So with these mp3s ID3v2 parse fails and then AVFormatContext->data_offset 
> is invalid!
>
> and having a bad data_offset can be very critical for some applications 
> (like mine 0:-) ).
>
> For my patch, I use the ID3v2 size extracted from its header to deduce the 
> offset.
> Then after parsing ID3 I seek to this offset and I made another check by 
> verifying that it corresponds to a mp3 frame sync, and if it doesn't I look 
> for the first frame sync and made it the offset.
>
> if id3 tag is corrupted, result is :
> data_offset is good (so mp3_parse_vbr_tags success)
> tag parse still fail but if file doesn't respect standards, we cannot do a 
> lot...
>
>

> diff -urN ./libavformat/mp3.c ../ffmpeg-export-2008-12-05_patched/libavformat/mp3.c
> --- ./libavformat/mp3.c	2008-10-03 12:16:29.000000000 +0200
> +++ ../ffmpeg-export-2008-12-05_patched/libavformat/mp3.c	2008-12-05 13:03:05.000000000 +0100
> @@ -453,6 +453,7 @@
>      uint8_t buf[ID3v1_TAG_SIZE];
>      int len, ret, filesize;
>      int64_t off;
> +    uint16_t sync=0;
>  
>      st = av_new_stream(s, 0);
>      if (!st)

> @@ -488,14 +489,19 @@
>              ((buf[8] & 0x7f) << 7) |
>              (buf[9] & 0x7f);
>          id3v2_parse(s, len, buf[3], buf[5]);
> +        url_fseek(s->pb, len + ID3v2_HEADER_SIZE, SEEK_SET); //go at the end of the ID3v2 tag (avoid error on some corrupted tag)

this change is incorrect, its id3v2_parse() job to end at that point

>      } else {
>          url_fseek(s->pb, 0, SEEK_SET);
>      }
> -
> -    off = url_ftell(s->pb);
> -    mp3_parse_vbr_tags(s, st, off);
> -    url_fseek(s->pb, off, SEEK_SET);
> -

> +    

trailing whitespace is forbidden in ffmpeg svn


> +    do{ //we have to be sure we're at the begining of a frame otherwise offset will be incorrect and parse_vbr_tags will fail
> +        sync = sync << 8;
> +        sync |= (0x00ff & get_byte(s->pb));
> +    }while((sync&0xffe0) != 0xFFe0);//look for frame sync
> +    off = url_ftell(s->pb)-2;       //so this is the correct offset

this is fragile
what you should do is, if mp3_parse_vbr_tags() fails try the non sync safe
interpretation of the len _if and only if_ this is reliable in practice
otherwise there are existing functions that check the validity of mp3 frame
headers.

Either way, we need a sample mp3 that fails and this patch is postponed
until previously approved patches to mp3.c have been applied by someone

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081205/cfbd34c0/attachment.pgp>