| Message ID | 20190819215624.49795-5-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | bf79e4426a94d17ca49b01ab1624c5d59ae59bb2 |
| Headers | show |
On Tue, Aug 20, 2019 at 6:11 AM Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > > Up until now, avformat_find_stream_info had a potential for memleaks: > When everything is fine, it read packets and (depending upon whether > AVFMT_FLAG_NOBUFFER was set) put them in a packet list or unreferenced > them when they were no longer needed. But upon failure, said packets > would leak if they were not already on the packet list. This patch fixes > this. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/utils.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/libavformat/utils.c b/libavformat/utils.c > index bae2f9e0db..96c02bb7fc 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -3801,7 +3801,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > &ic->internal->packet_buffer_end, > &pkt1, 0); > if (ret < 0) > - goto find_stream_info_err; > + goto unref_then_goto_end; > > pkt = &ic->internal->packet_buffer_end->pkt; > } else { > @@ -3816,7 +3816,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > if (!st->internal->avctx_inited) { > ret = avcodec_parameters_to_context(avctx, st->codecpar); > if (ret < 0) > - goto find_stream_info_err; > + goto unref_then_goto_end; > st->internal->avctx_inited = 1; > } > > @@ -3904,7 +3904,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > if (!st->internal->avctx->extradata) { > ret = extract_extradata(st, pkt); > if (ret < 0) > - goto find_stream_info_err; > + goto unref_then_goto_end; > } > > /* If still no information, we try to open the codec and to > @@ -4180,6 +4180,10 @@ find_stream_info_err: > av_log(ic, AV_LOG_DEBUG, "After avformat_find_stream_info() pos: %"PRId64" bytes read:%"PRId64" seeks:%d frames:%d\n", > avio_tell(ic->pb), ic->pb->bytes_read, ic->pb->seek_count, count); > return ret; > + > +unref_then_goto_end: > + av_packet_unref(&pkt1); > + goto find_stream_info_err; goto follow a goto, you can just add av_packet_unref(&pkt1) before jump to the label find_stream_info_err. > } >
mypopy@gmail.com: > On Tue, Aug 20, 2019 at 6:11 AM Andreas Rheinhardt > <andreas.rheinhardt@gmail.com> wrote: >> >> Up until now, avformat_find_stream_info had a potential for memleaks: >> When everything is fine, it read packets and (depending upon whether >> AVFMT_FLAG_NOBUFFER was set) put them in a packet list or unreferenced >> them when they were no longer needed. But upon failure, said packets >> would leak if they were not already on the packet list. This patch fixes >> this. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavformat/utils.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/libavformat/utils.c b/libavformat/utils.c >> index bae2f9e0db..96c02bb7fc 100644 >> --- a/libavformat/utils.c >> +++ b/libavformat/utils.c >> @@ -3801,7 +3801,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >> &ic->internal->packet_buffer_end, >> &pkt1, 0); >> if (ret < 0) >> - goto find_stream_info_err; >> + goto unref_then_goto_end; >> >> pkt = &ic->internal->packet_buffer_end->pkt; >> } else { >> @@ -3816,7 +3816,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >> if (!st->internal->avctx_inited) { >> ret = avcodec_parameters_to_context(avctx, st->codecpar); >> if (ret < 0) >> - goto find_stream_info_err; >> + goto unref_then_goto_end; >> st->internal->avctx_inited = 1; >> } >> >> @@ -3904,7 +3904,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >> if (!st->internal->avctx->extradata) { >> ret = extract_extradata(st, pkt); >> if (ret < 0) >> - goto find_stream_info_err; >> + goto unref_then_goto_end; >> } >> >> /* If still no information, we try to open the codec and to >> @@ -4180,6 +4180,10 @@ find_stream_info_err: >> av_log(ic, AV_LOG_DEBUG, "After avformat_find_stream_info() pos: %"PRId64" bytes read:%"PRId64" seeks:%d frames:%d\n", >> avio_tell(ic->pb), ic->pb->bytes_read, ic->pb->seek_count, count); >> return ret; >> + >> +unref_then_goto_end: >> + av_packet_unref(&pkt1); >> + goto find_stream_info_err; > goto follow a goto, you can just add av_packet_unref(&pkt1) before > jump to the label find_stream_info_err. But then I'd have to add three av_packet_unref. - Andreas
Andreas, On Mon, 19. Aug 23:56, Andreas Rheinhardt wrote: > Up until now, avformat_find_stream_info had a potential for memleaks: > When everything is fine, it read packets and (depending upon whether > AVFMT_FLAG_NOBUFFER was set) put them in a packet list or unreferenced > them when they were no longer needed. But upon failure, said packets > would leak if they were not already on the packet list. This patch fixes > this. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/utils.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/libavformat/utils.c b/libavformat/utils.c > index bae2f9e0db..96c02bb7fc 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -3801,7 +3801,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > &ic->internal->packet_buffer_end, > &pkt1, 0); > if (ret < 0) > - goto find_stream_info_err; > + goto unref_then_goto_end; > > pkt = &ic->internal->packet_buffer_end->pkt; > } else { > @@ -3816,7 +3816,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > if (!st->internal->avctx_inited) { > ret = avcodec_parameters_to_context(avctx, st->codecpar); > if (ret < 0) > - goto find_stream_info_err; > + goto unref_then_goto_end; > st->internal->avctx_inited = 1; > } > > @@ -3904,7 +3904,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > if (!st->internal->avctx->extradata) { > ret = extract_extradata(st, pkt); > if (ret < 0) > - goto find_stream_info_err; > + goto unref_then_goto_end; > } > > /* If still no information, we try to open the codec and to > @@ -4180,6 +4180,10 @@ find_stream_info_err: > av_log(ic, AV_LOG_DEBUG, "After avformat_find_stream_info() pos: %"PRId64" bytes read:%"PRId64" seeks:%d frames:%d\n", > avio_tell(ic->pb), ic->pb->bytes_read, ic->pb->seek_count, count); > return ret; > + > +unref_then_goto_end: > + av_packet_unref(&pkt1); > + goto find_stream_info_err; I wonder if you can get rid of this extra label by adding the following in find_stream_info_err if (pkt1.data) av_packet_unref(&pkt1) > } > > AVProgram *av_find_program_from_stream(AVFormatContext *ic, AVProgram *last, int s) Andriy
Andriy Gelman: > Andreas, > > On Mon, 19. Aug 23:56, Andreas Rheinhardt wrote: >> Up until now, avformat_find_stream_info had a potential for memleaks: >> When everything is fine, it read packets and (depending upon whether >> AVFMT_FLAG_NOBUFFER was set) put them in a packet list or unreferenced >> them when they were no longer needed. But upon failure, said packets >> would leak if they were not already on the packet list. This patch fixes >> this. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavformat/utils.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/libavformat/utils.c b/libavformat/utils.c >> index bae2f9e0db..96c02bb7fc 100644 >> --- a/libavformat/utils.c >> +++ b/libavformat/utils.c >> @@ -3801,7 +3801,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >> &ic->internal->packet_buffer_end, >> &pkt1, 0); >> if (ret < 0) >> - goto find_stream_info_err; >> + goto unref_then_goto_end; >> >> pkt = &ic->internal->packet_buffer_end->pkt; >> } else { >> @@ -3816,7 +3816,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >> if (!st->internal->avctx_inited) { >> ret = avcodec_parameters_to_context(avctx, st->codecpar); >> if (ret < 0) >> - goto find_stream_info_err; >> + goto unref_then_goto_end; >> st->internal->avctx_inited = 1; >> } >> >> @@ -3904,7 +3904,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >> if (!st->internal->avctx->extradata) { >> ret = extract_extradata(st, pkt); >> if (ret < 0) >> - goto find_stream_info_err; >> + goto unref_then_goto_end; >> } >> >> /* If still no information, we try to open the codec and to >> @@ -4180,6 +4180,10 @@ find_stream_info_err: >> av_log(ic, AV_LOG_DEBUG, "After avformat_find_stream_info() pos: %"PRId64" bytes read:%"PRId64" seeks:%d frames:%d\n", >> avio_tell(ic->pb), ic->pb->bytes_read, ic->pb->seek_count, count); >> return ret; >> + >> +unref_then_goto_end: >> + av_packet_unref(&pkt1); >> + goto find_stream_info_err; > > I wonder if you can get rid of this extra label by adding > the following in find_stream_info_err > > if (pkt1.data) > av_packet_unref(&pkt1) > No. There is a goto find_stream_info_err in line 3661 and at this point pkt1 is still uninitialized. (Besides, a packet needs to be unreferenced if it has side data or if it is refcounted, so the check would be wrong. Did I already mention that av_init_packet doesn't even set the data and size fields?) - Andreas
diff --git a/libavformat/utils.c b/libavformat/utils.c index bae2f9e0db..96c02bb7fc 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -3801,7 +3801,7 @@ FF_ENABLE_DEPRECATION_WARNINGS &ic->internal->packet_buffer_end, &pkt1, 0); if (ret < 0) - goto find_stream_info_err; + goto unref_then_goto_end; pkt = &ic->internal->packet_buffer_end->pkt; } else { @@ -3816,7 +3816,7 @@ FF_ENABLE_DEPRECATION_WARNINGS if (!st->internal->avctx_inited) { ret = avcodec_parameters_to_context(avctx, st->codecpar); if (ret < 0) - goto find_stream_info_err; + goto unref_then_goto_end; st->internal->avctx_inited = 1; } @@ -3904,7 +3904,7 @@ FF_ENABLE_DEPRECATION_WARNINGS if (!st->internal->avctx->extradata) { ret = extract_extradata(st, pkt); if (ret < 0) - goto find_stream_info_err; + goto unref_then_goto_end; } /* If still no information, we try to open the codec and to @@ -4180,6 +4180,10 @@ find_stream_info_err: av_log(ic, AV_LOG_DEBUG, "After avformat_find_stream_info() pos: %"PRId64" bytes read:%"PRId64" seeks:%d frames:%d\n", avio_tell(ic->pb), ic->pb->bytes_read, ic->pb->seek_count, count); return ret; + +unref_then_goto_end: + av_packet_unref(&pkt1); + goto find_stream_info_err; } AVProgram *av_find_program_from_stream(AVFormatContext *ic, AVProgram *last, int s)
Up until now, avformat_find_stream_info had a potential for memleaks: When everything is fine, it read packets and (depending upon whether AVFMT_FLAG_NOBUFFER was set) put them in a packet list or unreferenced them when they were no longer needed. But upon failure, said packets would leak if they were not already on the packet list. This patch fixes this. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/utils.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)