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

[Yvan Labadie]

Michael Niedermayer a ?crit :
> [...]
>> @@ -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
>   
it'is because it happens that id3v2_parse() job fail that I do that, but 
I could put this line at the end of id3v2_parse()
> [...]
> -
>   
>> +    
>>     
>
> trailing whitespace is forbidden in ffmpeg svn
>   
oops, bad check, sorry
>   
>> +    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.
>   
OK, I'll try to make it more reliable
> 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
>   
for example, this song 
http://labadiey.free.fr/divers/sample_fail_id3parse.mp3
[...]