[FFmpeg-devel] [PATCH] oggdec: add integer overflow and allocation check in ogg_read_page()

[Stefano Sabatini]

On date Monday 2011-05-23 19:15:45 +0200, Michael Niedermayer encoded:
> On Mon, May 23, 2011 at 06:44:11PM +0200, Stefano Sabatini wrote:
> > On date Monday 2011-05-23 05:15:27 +0200, Michael Niedermayer encoded:
> > > On Mon, May 23, 2011 at 12:04:29AM +0200, Stefano Sabatini wrote:
> > > > ---
> > > >  libavformat/oggdec.c |    8 +++++++-
> > > >  1 files changed, 7 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> > > > index 7f65365..f137b97 100644
> > > > --- a/libavformat/oggdec.c
> > > > +++ b/libavformat/oggdec.c
> > > > @@ -288,7 +288,13 @@ static int ogg_read_page(AVFormatContext *s, int *str)
> > > >      }
> > > >  
> > > >      if (os->bufsize - os->bufpos < size){
> > > > -        uint8_t *nb = av_malloc (os->bufsize *= 2);
> > > > +        uint8_t *nb;
> > > > +        if (os->bufsize > SIZE_MAX/2) {
> > > > +            av_log(s, AV_LOG_ERROR, "Ogg page with size %u is too big\n", os->bufsize);
> > > > +            return AVERROR_INVALIDDATA;
> > > > +        }
> > > > +        if (!(nb = av_malloc(os->bufsize *= 2)))
> > > > +            return AVERROR(ENOMEM);
> > > 
> > > i hope there is a better solution than allocating several gigabyte
> > 
> > Yes, but this at least is fixing a crash.
> 
> please review attached patch
> note this is a RFC, i have not checked if this has sideeffects and
> i do not know why the if() was there.

commit 40c5e1fa2e5fd668ed69528d91521b46ec64f96a
Author: Måns Rullgård <mans at mansr.com>
Date:   Sun Jun 25 12:23:54 2006 +0000

    10l: don't allocate a new buffer quite so often
    
    Originally committed as revision 5523 to svn://svn.ffmpeg.org/ffmpeg/trunk

diff --git a/libavformat/ogg2.c b/libavformat/ogg2.c
index 9cb3d8c..b29bfe9 100644
--- a/libavformat/ogg2.c
+++ b/libavformat/ogg2.c
@@ -193,6 +193,7 @@ ogg_new_stream (AVFormatContext * s, uint32_t serial)
     os = ogg->streams + idx;
     os->serial = serial;
     os->bufsize = DECODER_BUFFER_SIZE;
+    os->buf = av_malloc(os->bufsize);
     os->header = -1;
 
     st = av_new_stream (s, idx);
@@ -279,7 +280,7 @@ ogg_read_page (AVFormatContext * s, int *str)
 
     os = ogg->streams + idx;
 
-    if(os->segp == os->nsegs)
+    if(os->psize > 0)
         ogg_new_buf(ogg, idx);
 
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Asymptotically faster algorithms should always be preferred if you have
> asymptotical amounts of data

> From ea6e633d62b7ab7deac746e49e41f1754ae18a0f Mon Sep 17 00:00:00 2001
> From: Michael Niedermayer <michaelni at gmx.at>
> Date: Mon, 23 May 2011 19:10:15 +0200
> Subject: [PATCH] ioggdec: fix runaway allocation
> 
> fixes ticket185
> 
> Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> ---
>  libavformat/oggdec.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> index 7f65365..c90e222 100644
> --- a/libavformat/oggdec.c
> +++ b/libavformat/oggdec.c
> @@ -258,8 +258,7 @@ static int ogg_read_page(AVFormatContext *s, int *str)
>      os = ogg->streams + idx;
>      os->page_pos = avio_tell(bc) - 27;
>  
> -    if(os->psize > 0)
> -        ogg_new_buf(ogg, idx);
> +    ogg_new_buf(ogg, idx);

>From what I see ogg_new_buf() moves the data at the begin of the
buffer, creating a new buffer when the data is memcpyied (BTW
ogg_new_buf() is not a particularly helpful name).

I suppose a better solution would be to use a fifo. Maybe the check
was added for avoiding continuos reallocation of the buffer, so
removing the check may have a performance penalty, but right at the
moment I still can't understand what this psize really is for. Also I
see many missing NULL checks and av_realloc() may be used in many
places.

David are you reading?
-- 
FFmpeg = Formidable and Frenzy Magnificient Portable Elitarian Gadget