[FFmpeg-devel,v2,5/8] avformat/utils: Fix memleaks II

Submitted by Andreas Rheinhardt on Aug. 19, 2019, 9:56 p.m.

Details

Message ID 20190819215624.49795-5-andreas.rheinhardt@gmail.com
State New
Headers show

Commit Message

Andreas Rheinhardt Aug. 19, 2019, 9:56 p.m.
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(-)

Comments

mypopy@gmail.com Aug. 20, 2019, 2:40 a.m.
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.
>  }
>
Andreas Rheinhardt Aug. 20, 2019, 2:46 a.m.
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
Andriy Gelman Aug. 25, 2019, 3:43 a.m.
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
Andreas Rheinhardt Sept. 9, 2019, 10:03 p.m.
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

Patch hide | download patch | download mbox

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)