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

[Stefano Sabatini]

On date Tuesday 2011-05-24 04:04:00 +0200, Michael Niedermayer encoded:
> 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

Anyway, back to the original problem, please see attached
patchset.

Your patch can be applied on top of that, the issue reporter tells
that the patch fixes the issue, then we noted there are other memory
issues but I want to leave them for now, and possibly wait for a reply
from David Conrad (listed as file maintainer).
-- 
FFmpeg = Faithful and Fostering Murdering Perennial Elfic Glue
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-oggdec-add-integer-overflow-and-allocation-check-in-.patch
Type: text/x-diff
Size: 1094 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110524/ce066d85/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-oggdec-add-various-malloc-and-NULL-checks.patch
Type: text/x-diff
Size: 2684 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110524/ce066d85/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-oggdec-in-ogg_save-use-av_mallocz-for-resetting-allo.patch
Type: text/x-diff
Size: 1005 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110524/ce066d85/attachment-0002.bin>