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

[Andreas Rheinhardt]

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 at 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