Message ID | HE1PR0301MB2154AD8B8D7789D78FDE98F08F759@HE1PR0301MB2154.eurprd03.prod.outlook.com |
---|---|
State | Accepted |
Commit | 9a471c5437d34cd1e63520b47f50a0fa605a5688 |
Headers | show |
Series | [FFmpeg-devel] avformat/rmdec: Fix memleaks upon read_header failure | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
Andreas Rheinhardt: > For both the RealMedia as well as the IVR demuxer (which share the same > context) each AVStream's priv_data contains an AVPacket that might > contain data (even when reading the header) and therefore needs to be > unreferenced. Up until now, this has not always been done: > > The RealMedia demuxer didn't do it when allocating a new stream's > priv_data failed although there might be other streams with packets to > unreference. (The reason for this was that until recently rm_read_close() > couldn't handle an AVStream without priv_data, so one had to choose > between a potential crash and a memleak.) > > The IVR demuxer meanwhile never ever called read_close so that the data > already contained in packets leaks upon error. > > This patch fixes both demuxers by adding the appropriate cleanup code. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavformat/rmdec.c | 38 ++++++++++++++++++++++---------------- > 1 file changed, 22 insertions(+), 16 deletions(-) > > diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c > index b6f42183e8..1dec70e95b 100644 > --- a/libavformat/rmdec.c > +++ b/libavformat/rmdec.c > @@ -614,8 +614,10 @@ static int rm_read_header(AVFormatContext *s) > get_str8(pb, mime, sizeof(mime)); /* mimetype */ > st->codecpar->codec_type = AVMEDIA_TYPE_DATA; > st->priv_data = ff_rm_alloc_rmstream(); > - if (!st->priv_data) > - return AVERROR(ENOMEM); > + if (!st->priv_data) { > + ret = AVERROR(ENOMEM); > + goto fail; > + } > > size = avio_rb32(pb); > codec_pos = avio_tell(pb); > @@ -1249,20 +1251,19 @@ static int ivr_read_header(AVFormatContext *s) > } > > for (n = 0; n < nb_streams; n++) { > - st = avformat_new_stream(s, NULL); > - if (!st) > - return AVERROR(ENOMEM); > - st->priv_data = ff_rm_alloc_rmstream(); > - if (!st->priv_data) > - return AVERROR(ENOMEM); > + if (!(st = avformat_new_stream(s, NULL)) || > + !(st->priv_data = ff_rm_alloc_rmstream())) { > + ret = AVERROR(ENOMEM); > + goto fail; > + } > > if (avio_r8(pb) != 1) > - return AVERROR_INVALIDDATA; > + goto invalid_data; > > count = avio_rb32(pb); > for (i = 0; i < count; i++) { > if (avio_feof(pb)) > - return AVERROR_INVALIDDATA; > + goto invalid_data; > > type = avio_r8(pb); > tlen = avio_rb32(pb); > @@ -1274,25 +1275,25 @@ static int ivr_read_header(AVFormatContext *s) > } else if (type == 4 && !strncmp(key, "OpaqueData", tlen)) { > ret = ffio_ensure_seekback(pb, 4); > if (ret < 0) > - return ret; > + goto fail; > if (avio_rb32(pb) == MKBETAG('M', 'L', 'T', 'I')) { > ret = rm_read_multi(s, pb, st, NULL); > } else { > if (avio_feof(pb)) > - return AVERROR_INVALIDDATA; > + goto invalid_data; > avio_seek(pb, -4, SEEK_CUR); > ret = ff_rm_read_mdpr_codecdata(s, pb, st, st->priv_data, len, NULL); > } > > if (ret < 0) > - return ret; > + goto fail; > } else if (type == 4) { > int j; > > av_log(s, AV_LOG_DEBUG, "%s = '0x", key); > for (j = 0; j < len; j++) { > if (avio_feof(pb)) > - return AVERROR_INVALIDDATA; > + goto invalid_data; > av_log(s, AV_LOG_DEBUG, "%X", avio_r8(pb)); > } > av_log(s, AV_LOG_DEBUG, "'\n"); > @@ -1309,14 +1310,19 @@ static int ivr_read_header(AVFormatContext *s) > } > > if (avio_r8(pb) != 6) > - return AVERROR_INVALIDDATA; > + goto invalid_data; > avio_skip(pb, 12); > avio_skip(pb, avio_rb64(pb) + pos - avio_tell(s->pb)); > if (avio_r8(pb) != 8) > - return AVERROR_INVALIDDATA; > + goto invalid_data; > avio_skip(pb, 8); > > return 0; > +invalid_data: > + ret = AVERROR_INVALIDDATA; > +fail: > + rm_read_close(s); > + return ret; > } > > static int ivr_read_packet(AVFormatContext *s, AVPacket *pkt) > Will apply this patchset. - Andreas
diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c index b6f42183e8..1dec70e95b 100644 --- a/libavformat/rmdec.c +++ b/libavformat/rmdec.c @@ -614,8 +614,10 @@ static int rm_read_header(AVFormatContext *s) get_str8(pb, mime, sizeof(mime)); /* mimetype */ st->codecpar->codec_type = AVMEDIA_TYPE_DATA; st->priv_data = ff_rm_alloc_rmstream(); - if (!st->priv_data) - return AVERROR(ENOMEM); + if (!st->priv_data) { + ret = AVERROR(ENOMEM); + goto fail; + } size = avio_rb32(pb); codec_pos = avio_tell(pb); @@ -1249,20 +1251,19 @@ static int ivr_read_header(AVFormatContext *s) } for (n = 0; n < nb_streams; n++) { - st = avformat_new_stream(s, NULL); - if (!st) - return AVERROR(ENOMEM); - st->priv_data = ff_rm_alloc_rmstream(); - if (!st->priv_data) - return AVERROR(ENOMEM); + if (!(st = avformat_new_stream(s, NULL)) || + !(st->priv_data = ff_rm_alloc_rmstream())) { + ret = AVERROR(ENOMEM); + goto fail; + } if (avio_r8(pb) != 1) - return AVERROR_INVALIDDATA; + goto invalid_data; count = avio_rb32(pb); for (i = 0; i < count; i++) { if (avio_feof(pb)) - return AVERROR_INVALIDDATA; + goto invalid_data; type = avio_r8(pb); tlen = avio_rb32(pb); @@ -1274,25 +1275,25 @@ static int ivr_read_header(AVFormatContext *s) } else if (type == 4 && !strncmp(key, "OpaqueData", tlen)) { ret = ffio_ensure_seekback(pb, 4); if (ret < 0) - return ret; + goto fail; if (avio_rb32(pb) == MKBETAG('M', 'L', 'T', 'I')) { ret = rm_read_multi(s, pb, st, NULL); } else { if (avio_feof(pb)) - return AVERROR_INVALIDDATA; + goto invalid_data; avio_seek(pb, -4, SEEK_CUR); ret = ff_rm_read_mdpr_codecdata(s, pb, st, st->priv_data, len, NULL); } if (ret < 0) - return ret; + goto fail; } else if (type == 4) { int j; av_log(s, AV_LOG_DEBUG, "%s = '0x", key); for (j = 0; j < len; j++) { if (avio_feof(pb)) - return AVERROR_INVALIDDATA; + goto invalid_data; av_log(s, AV_LOG_DEBUG, "%X", avio_r8(pb)); } av_log(s, AV_LOG_DEBUG, "'\n"); @@ -1309,14 +1310,19 @@ static int ivr_read_header(AVFormatContext *s) } if (avio_r8(pb) != 6) - return AVERROR_INVALIDDATA; + goto invalid_data; avio_skip(pb, 12); avio_skip(pb, avio_rb64(pb) + pos - avio_tell(s->pb)); if (avio_r8(pb) != 8) - return AVERROR_INVALIDDATA; + goto invalid_data; avio_skip(pb, 8); return 0; +invalid_data: + ret = AVERROR_INVALIDDATA; +fail: + rm_read_close(s); + return ret; } static int ivr_read_packet(AVFormatContext *s, AVPacket *pkt)
For both the RealMedia as well as the IVR demuxer (which share the same context) each AVStream's priv_data contains an AVPacket that might contain data (even when reading the header) and therefore needs to be unreferenced. Up until now, this has not always been done: The RealMedia demuxer didn't do it when allocating a new stream's priv_data failed although there might be other streams with packets to unreference. (The reason for this was that until recently rm_read_close() couldn't handle an AVStream without priv_data, so one had to choose between a potential crash and a memleak.) The IVR demuxer meanwhile never ever called read_close so that the data already contained in packets leaks upon error. This patch fixes both demuxers by adding the appropriate cleanup code. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavformat/rmdec.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-)