On 20/12/13 04:47, Sean McGovern wrote:
Ok, let's have a look at this thing.
> ---
> libavformat/oggdec.c | 75
> ++++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 61 insertions(+), 14 deletions(-)
>
> diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> index 46e4b9a..c6df0d2 100644
> --- 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)
This function is called just once and its fail path goes
ogg_save -> ogg_get_length -> ogg_read_header
ogg_read_header ends up freeing everything using ogg_read_close.
if save fails we should restore the ogg->streams otherwise we'd leak the
os->buf we overwrote.
Something like
while (--i) {
av_free(ogg->streams[i]->buf);
ogg->streams[i]->buf = ost->streams[i]->buf;
}
av_free(ost);
> static int ogg_restore(AVFormatContext *s, int discard)
> @@ -173,10 +190,16 @@ 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)
> + return AVERROR(ENOMEM);
> +
> if (new_avstream) {
> st = avformat_new_stream(s, NULL);
> - if (!st)
> + if (!st) {
> + av_freep(&os->buf);
> + av_freep(&os);
that would not work os is a pointer in the middle of an allocated block
if I read correctly.
> return AVERROR(ENOMEM);
> + }
>
> st->id = idx;
> avpriv_set_pts_info(st, 64, 1, 1000000);
> @@ -191,6 +214,9 @@ static int ogg_new_buf(struct ogg *ogg, int idx)
> uint8_t *nb = av_malloc(os->bufsize + FF_INPUT_BUFFER_PADDING_SIZE);
> int size = os->bufpos - os->pstart;
>
> + if (!nb)
> + return AVERROR(ENOMEM);
> +
> if (os->buf) {
> memcpy(nb, os->buf + os->pstart, size);
> av_free(os->buf);
> @@ -276,8 +302,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;
> + }
>
> ret = avio_read(bc, os->segments, nsegs);
> if (ret < nsegs)
> @@ -497,7 +526,7 @@ static int ogg_get_headers(AVFormatContext *s)
> static int ogg_get_length(AVFormatContext *s)
> {
> struct ogg *ogg = s->priv_data;
> - int i;
> + int i, ret = 0;
> int64_t size, end;
>
> if (!s->pb->seekable)
> @@ -512,7 +541,10 @@ static int ogg_get_length(AVFormatContext *s)
> return 0;
> end = size > MAX_PAGE_SIZE ? size - MAX_PAGE_SIZE : 0;
>
> - ogg_save(s);
> + ret = ogg_save(s);
> + if (ret < 0)
> + goto fail;
> +
> avio_seek(s->pb, end, SEEK_SET);
>
> while (!ogg_read_page(s, &i)) {
> @@ -525,9 +557,20 @@ static int ogg_get_length(AVFormatContext *s)
> }
> }
>
> - ogg_restore(s, 0);
> + ret = ogg_restore(s, 0);
> + if (ret < 0)
> + goto fail;
> +
> + return ret;
>
> - return 0;
> +fail:
> + for (i = 0; i < ogg->nstreams; i++) {
> + struct ogg_stream *os = ogg->streams + i;
> + av_freep(&os->buf);
> + }
> +
> + av_freep(&ogg->state);
we call the read_close function on failure so we could spare it.
> + return ret;
> }
>
> static int ogg_read_close(AVFormatContext *s)
> @@ -554,20 +597,24 @@ static int ogg_read_header(AVFormatContext *s)
> ogg->curidx = -1;
> //linear headers seek from start
> ret = ogg_get_headers(s);
> - if (ret < 0) {
> - ogg_read_close(s);
> - return ret;
> - }
> + if (ret < 0)
> + goto fail;
>
> for (i = 0; i < ogg->nstreams; i++)
> if (ogg->streams[i].header < 0)
> ogg->streams[i].codec = NULL;
>
> //linear granulepos seek from end
> - ogg_get_length(s);
> + ret = ogg_get_length(s);
> + if (ret < 0)
> + goto fail;
>
> //fill the extradata in the per codec callbacks
> return 0;
> +
> +fail:
> + ogg_read_close(s);
> + return ret;
This part is fine, I can play with the code a bit since you already di a
lot and this code is one of my original sins.
lu
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
