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

[Michael Niedermayer]

On Fri, Dec 05, 2008 at 04:08:11PM +0100, Yvan Labadie wrote:
>
> M?ns Rullg?rd a ?crit :
>> Michael Niedermayer wrote:
>>   
>>> On Fri, Dec 05, 2008 at 02:18:03PM -0000, M?ns Rullg?rd wrote:
>>>     
>>>> [...]
>>>> ID3v2.3 and earlier used a plain integer.  The "sync safe" encoding was
>>>> introduced in v2.4.
>>>>       
>>> that explains it, assuming all the failing files are < 2.4
>>> a proper solution would then be to fix <2.4 support
>>>     
> Ooh! that kind of explains everything... :D
>
> So I join a patch that uses the conversion corresponding to the version 
> with a simple switch/case...

review below


>> That said, I wouldn't be very surprised if there are files out there
>> with any kind of errors.  We're talking about mp3 files afters all...
>>   
> Sure, so I should still do a check of mp3_parse_vbr_tags() and correct the 
> offset if it fails... like we discuss in previous mail from Michael.

only if it is needed for an actual file


> 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 15:55:38.000000000 +0100
> @@ -259,13 +259,20 @@
>          url_fskip(s->pb, id3v2_get_size(s->pb, 4));
>  
>      while(len >= taghdrlen) {
> -        if(isv34) {
> +        switch(version) {
> +        case 3:
> +            tag  = get_be32(s->pb);
> +            tlen  = get_be32(s->pb);
> +            get_be16(s->pb); /* flags */
> +            break;
> +        case 4:
>              tag  = get_be32(s->pb);
>              tlen = id3v2_get_size(s->pb, 4);
>              get_be16(s->pb); /* flags */
> -        } else {
> +            break;
> +        default: //so version 2
>              tag  = get_be24(s->pb);
> -            tlen = id3v2_get_size(s->pb, 3);
> +            tlen  = get_be24(s->pb);
>          }

id write:

if     (version < 3) tag  = get_be24(s->pb);
else                 tag  = get_be32(s->pb);

if     (version < 3) tlen = get_be24(s->pb);
else if(version ==3) tlen = get_be32(s->pb);
else                 tlen = id3v2_get_size(s->pb, 4);

if     (version >=3) get_be16(s->pb); /* flags */

and this is ok if someone tests it and confirms it works with all the
respective variants of id3 (checking it against the various specs is also
a good idea)

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

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/dd41a7a5/attachment.pgp>