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

[Michael Niedermayer]

On Thu, Mar 03, 2011 at 11:18:38AM -0500, Ronald S. Bultje wrote:
> 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.

It should print a warning if inconsistent data is detected

with my API:

if(   avio_get_str(pb, key, ? sizeof(key))   < 0
   || avio_get_str(pb, value, sizeof(value)) < 0)
    av_log( warning blah blag here)

with antons API:


if(   avio_get_str(pb, size, key, ? sizeof(key)) > FFMIN(size, sizeof(key)))
   || avio_get_str(pb, size, value, ? sizeof(value)) > FFMIN(size, sizeof(value)))
    av_log( warning blah blag here)



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

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110303/5baab730/attachment.pgp>