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

[Ronald S. Bultje]

Hi,

On Thu, Mar 3, 2011 at 1:31 PM, Anton Khirnov <anton at khirnov.net> wrote:
> On Thu, Mar 03, 2011 at 11:18:38AM -0500, Ronald S. Bultje wrote:
>> 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.
>>
>
> Ok, whatever, I don't care about this demuxer and don't want to waste
> more time bikeshedding over it. New code should behave the same as the
> old in the attached patch.

Thanks, patch is OK. Will queue.

Ronald