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

[Michael Niedermayer]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- 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/20120915/2abf707a/attachment.asc>