[FFmpeg-devel] [PATCH 2/3] cafdec: prevent overreading the info chunk

[Ronald S. Bultje]

Hi,

On Thu, Mar 3, 2011 at 8:04 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Thu, Mar 03, 2011 at 01:51:56PM +0100, Anton Khirnov wrote:
>> ---
>> ?libavformat/cafdec.c | ? ?5 +++--
>> ?1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
>> index d98c4bf..715dfdf 100644
>> --- a/libavformat/cafdec.c
>> +++ b/libavformat/cafdec.c
>> @@ -182,11 +182,12 @@ static void read_info_chunk(AVFormatContext *s, int64_t size)
>> ? ? ?AVIOContext *pb = s->pb;
>> ? ? ?unsigned int i;
>> ? ? ?unsigned int nb_entries = avio_rb32(pb);
>> + ? ?size -= 4;
>> ? ? ?for (i = 0; i < nb_entries; i++) {
>> ? ? ? ? ?char key[32];
>> ? ? ? ? ?char value[1024];
>> - ? ? ? ?get_strz(pb, key, sizeof(key));
>> - ? ? ? ?get_strz(pb, value, sizeof(value));
>> + ? ? ? ?size -= avio_get_str(pb, size, key, ? sizeof(key));
>> + ? ? ? ?size -= avio_get_str(pb, size, value, sizeof(value));
>> ? ? ? ? ?av_metadata_set2(&s->metadata, key, value, 0);
>> ? ? ?}
>> ?}
>
> I really dont belive that for the case where this makes a difference to the
> current code that this would be the correct way to handle it
>
> 2 strings of 100 bytes in a 102 bytes chunk leads to the second string to be
> 2 bytes long and silently used without error message

That's a broken file, nothing more.

The question is whether this patch increases the chance of parsing the
rest of the file correctly. There isn't really a "correct" answer to
that question. It might in some cases, it might not in others,
depending on how the file is/was damaged. Most likely it will fail
before and will still fail after this patch, just differently.

Ronald