On Mon, Dec 9, 2013 at 6:48 AM, Diego Biurrun <[email protected]> wrote:
> On Mon, Dec 09, 2013 at 12:22:20AM -0500, Sean McGovern wrote:
> > --- a/libavformat/oggdec.c
> > +++ b/libavformat/oggdec.c
> > @@ -60,9 +60,13 @@ static const struct ogg_codec * const ogg_codecs[] = {
> > static int ogg_save(AVFormatContext *s)
> > {
> > struct ogg *ogg = s->priv_data;
> > + int i, ret = 0;
> > struct ogg_state *ost =
> > av_malloc(sizeof(*ost) + (ogg->nstreams - 1) *
> sizeof(*ogg->streams));
> > - int i;
> > +
> > + if (!ost)
> > + return AVERROR(ENOMEM);
> > +
> > ost->pos = avio_tell(s->pb);
> > ost->curidx = ogg->curidx;
> > ost->next = ogg->state;
> > @@ -72,12 +76,20 @@ static int ogg_save(AVFormatContext *s)
> > for (i = 0; i < ogg->nstreams; i++) {
> > struct ogg_stream *os = ogg->streams + i;
> > os->buf = av_mallocz(os->bufsize +
> FF_INPUT_BUFFER_PADDING_SIZE);
> > + if (!os->buf) {
> > + ret = AVERROR(ENOMEM);
> > + goto fail;
> > + }
> > memcpy(os->buf, ost->streams[i].buf, os->bufpos);
> > }
> >
> > ogg->state = ost;
> >
> > return 0;
> > +
> > +fail:
> > + av_freep(&ost);
>
> ost is a pointer, why are you taking its address?
>
> If a goto label is only used once and only contains one cleanup call
> I suggest avoiding the goto and cleaning up in place.
>
Agreed and fixed.
>
> > @@ -173,6 +186,11 @@ static int ogg_new_stream(AVFormatContext *s,
> uint32_t serial, int new_avstream)
> > os->header = -1;
> > os->start_granule = OGG_NOGRANULE_VALUE;
> >
> > + if (!os->buf) {
> > + ret = AVERROR(ENOMEM);
> > + goto fail;
> > + }
> > +
> > if (new_avstream) {
> > st = avformat_new_stream(s, NULL);
> > if (!st)
> > @@ -183,6 +201,10 @@ static int ogg_new_stream(AVFormatContext *s,
> uint32_t serial, int new_avstream)
> > }
> >
> > return idx;
> > +
> > +fail:
> > + av_freep(&os);
> > + return ret;
> > }
>
> Same; you also need to clean up os if os-buf allocation fails.
>
Fixed.
>
> > @@ -276,8 +301,11 @@ 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);
> > + if (os->psize > 0) {
> > + ret = ogg_new_buf(ogg, idx);
> > + if (ret < 0)
> > + return ret;
> > + }
>
> The calls to ogg_new_stream() also need to be checked.
> There's a call to av_malloc afterwards that will leak the memory
> allocated here I believe.
>
They are already checked to return if the index value is less than 0.
-- Sean McG.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
