[FFmpeg-devel] [PATCH 5/9] lavf/oggdec: rework allocations in ogg_new_streams().

[Clément Bœsch]

On Sat, Sep 15, 2012 at 02:18:22AM +0200, Michael Niedermayer wrote:
> On Sat, Sep 15, 2012 at 02:10:38AM +0200, Clément Bœsch wrote:
> > On Sat, Sep 15, 2012 at 01:30:09AM +0200, Reimar Döffinger wrote:
> > > On Sat, Sep 15, 2012 at 01:20:44AM +0200, Clément Bœsch wrote:
> > > > ---
> > > >  libavformat/oggdec.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> > > > index 05aeddd..451e392 100644
> > > > --- a/libavformat/oggdec.c
> > > > +++ b/libavformat/oggdec.c
> > > > @@ -169,14 +169,18 @@ static int ogg_new_stream(AVFormatContext *s, uint32_t serial, int new_avstream)
> > > >      AVStream *st;
> > > >      struct ogg_stream *os;
> > > >  
> > > > -    ogg->streams = av_realloc (ogg->streams,
> > > > -                               ogg->nstreams * sizeof (*ogg->streams));
> > > > +    ogg->streams = av_realloc_f(ogg->streams, ogg->nstreams,
> > > > +                                sizeof(*ogg->streams));
> > > > +    if (!ogg->streams)
> > > > +        return AVERROR(ENOMEM);
> > > >      memset (ogg->streams + idx, 0, sizeof (*ogg->streams));
> > > >      os = ogg->streams + idx;
> > > >      os->serial = serial;
> > > >      os->bufsize = DECODER_BUFFER_SIZE;
> > > >      os->buf = av_malloc(os->bufsize + FF_INPUT_BUFFER_PADDING_SIZE);
> > > >      os->header = -1;
> > > > +    if (!os->buf)
> > > > +        return AVERROR(ENOMEM);
> > > 
> > > I don't think nstreams should be incremented in the failure case?
> > > Also the realloc is not fully correct, it will still leak memory on
> > > failure (sorry if some later patch addresses this, just going through in
> > > order)
> > 
> > You're totally right, and no worry, no later patch addresses this problem.
> > The new attached patch should be better.
> > 
> > -- 
> > Clément B.
> 
> >  oggdec.c |   12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 17869455761809bd556c7153d4c2ee5b4f003eac  0005-lavf-oggdec-rework-allocations-in-ogg_new_streams.patch
> > From 3208fa20e4674f1c383aa62eaefa388d8a10d26b Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux at gmail.com>
> > Date: Fri, 14 Sep 2012 23:51:30 +0200
> > Subject: [PATCH 5/9] lavf/oggdec: rework allocations in ogg_new_streams().
> > 
> > ---
> >  libavformat/oggdec.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> > index 05aeddd..fbc35dd 100644
> > --- a/libavformat/oggdec.c
> > +++ b/libavformat/oggdec.c
> > @@ -165,18 +165,24 @@ static int ogg_new_stream(AVFormatContext *s, uint32_t serial, int new_avstream)
> >  {
> >  
> >      struct ogg *ogg = s->priv_data;
> > -    int idx = ogg->nstreams++;
> > +    int idx = ogg->nstreams;
> >      AVStream *st;
> >      struct ogg_stream *os;
> > +    size_t size;
> >  
> > -    ogg->streams = av_realloc (ogg->streams,
> > -                               ogg->nstreams * sizeof (*ogg->streams));
> > +    if (av_size_mult(ogg->nstreams + 1, sizeof(*ogg->streams), &size) < 0 ||
> > +        !(os = av_realloc(ogg->streams, size)))
> > +        return AVERROR(ENOMEM);
> > +    ogg->streams = os;
> > +    ogg->nstreams++;
> 
> without applying all patches and crosschecking i suspect that
> in the 1 stream replace case the stream is removed and then a new added
> but when this fails we end with the old 1 stream array and streams=0
> here while its 1 in AVFormatContext.
> That would leave the context inconsistent

If the replace case fails prior to this patch, no error will be raised but
AFAICT it will likely crash very quickly because of the NULL pointers and
the memset. One of the following patch make sure to split the replace code
from this function so it should be safe.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120915/f98f01ae/attachment.asc>