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

[Michael Niedermayer]

On Tue, May 24, 2011 at 12:36:23AM +0200, Stefano Sabatini wrote:
> 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

memmove() could be used avoiding realloc.
and when size is 0 not even that is needed


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

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110524/8b22453f/attachment.asc>