On Wed, Dec 11, 2013 at 5:49 PM, Diego Biurrun <[email protected]> wrote:
> On Tue, Dec 10, 2013 at 08:17:40PM -0500, Sean McGovern wrote:
> > --- a/libavformat/oggdec.c
> > +++ b/libavformat/oggdec.c
> > @@ -72,12 +76,25 @@ 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) {
> > + int n;
> > +
> > + for (n = 0; n < i; n++) {
> > + struct ogg_stream *os = ogg->streams + n;
> > + av_freep(os->buf);
>
> I think you can reuse os from above.
>
You can't. It's not loop invariant, and you have to clean up all the
previous allocations from this loop if it runs out of memory.
>
> See what Anton wrote about av_freep().
>
Noted, and fixed.
>
> > + }
> > +
> > + av_freep(ost);
> > + ret = AVERROR(ENOMEM);
> > + break;
> > + }
> > memcpy(os->buf, ost->streams[i].buf, os->bufpos);
> > }
> >
> > - ogg->state = ost;
> > + if (ost)
> > + ogg->state = ost;
>
> This looks a bit odd since the function checks ost when it gets allocated,
> so in this case goto may be the cleaner solution.
>
Ironic since that's what I had a few revisions back -- changed again to a
goto.
>
> > @@ -276,8 +304,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;
> > + }
>
> You need to free the memory allocated by ogg_new_stream() above.
> Also in the remaining error returns in the function.
>
I agree, but it's not clear where I should be free()'ing this.
>
> > @@ -525,9 +559,9 @@ static int ogg_get_length(AVFormatContext *s)
> > }
> > }
> >
> > - ogg_restore(s, 0);
> > + ret = ogg_restore(s, 0);
> >
> > - return 0;
> > + return ret;
> > }
>
> If ogg_restore() fails, you leak memory from ogg_save().
>
FIXED. I definitely missed that -- I think this patch is starting to break
my brain...
-- Sean McG.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
