[FFmpeg-devel] [PATCH V2] vf_hwupload: Add missing return value check

Jun Zhao mypopydev at gmail.com
Fri Mar 10 02:43:31 EET 2017



On 2017/3/10 7:49, Mark Thompson wrote:
> On 09/03/17 00:33, Jun Zhao wrote:
>> On 2017/3/8 16:58, Mark Thompson wrote:
>>> On 08/03/17 01:25, Jun Zhao wrote:
>>>> ping ?
>>>>
>>>> On 2017/3/3 9:35, Jun Zhao wrote:
>>>>> V2: Fix the potential memory leak.2
>>>>>
>>>>> From eb283d277679b5dac9c43e8d3c98bcc9367b592f Mon Sep 17 00:00:00 2001
>>>>> From: Jun Zhao <jun.zhao at intel.com>
>>>>> Date: Fri, 3 Mar 2017 09:25:53 +0800
>>>>> Subject: [PATCH] vf_hwupload: Add missing return value check
>>>>>
>>>>> Add missing return value checks and fix the potential memory leak.
>>>>>
>>>>> Signed-off-by: Jun Zhao <jun.zhao at intel.com>
>>>>> ---
>>>>>  libavfilter/vf_hwupload.c | 18 ++++++++++++------
>>>>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/libavfilter/vf_hwupload.c b/libavfilter/vf_hwupload.c
>>>>> index 08af2dd..4b63a2a 100644
>>>>> --- a/libavfilter/vf_hwupload.c
>>>>> +++ b/libavfilter/vf_hwupload.c
>>>>> @@ -74,22 +74,28 @@ static int hwupload_query_formats(AVFilterContext *avctx)
>>>>>      if (input_pix_fmts) {
>>>>>          for (i = 0; input_pix_fmts[i] != AV_PIX_FMT_NONE; i++) {
>>>>>              err = ff_add_format(&input_formats, input_pix_fmts[i]);
>>>>> -            if (err < 0) {
>>>>> -                ff_formats_unref(&input_formats);
>>>>> +            if (err < 0)
>>>>>                  goto fail;
>>>>> -            }
>>>>>          }
>>>>>      }
>>>>>  
>>>>> -    ff_formats_ref(input_formats, &avctx->inputs[0]->out_formats);
>>>>> +    if ((err = ff_formats_ref(input_formats, &avctx->inputs[0]->out_formats)) < 0)
>>>>> +        goto fail;
>>>>>  
>>>>> -    ff_formats_ref(ff_make_format_list(output_pix_fmts),
>>>>> -                   &avctx->outputs[0]->in_formats);
>>>>> +    if ((err = ff_formats_ref(ff_make_format_list(output_pix_fmts),
>>>>> +                              &avctx->outputs[0]->in_formats)) < 0) {
>>>>> +        ff_formats_unref(&input_formats);
>>>>> +        goto fail;
>>>>> +    }
>>>>>  
>>>>>      av_hwframe_constraints_free(&constraints);
>>>>>      return 0;
>>>>>  
>>>>>  fail:
>>>>> +    if (input_formats) {
>>>>> +        av_free(input_formats->formats);
>>>>> +        av_free(input_formats);
>>>>> +    }
>>>>>      av_buffer_unref(&ctx->hwdevice_ref);
>>>>>      av_hwframe_constraints_free(&constraints);
>>>>>      return err;
>>>>> -- 
>>>>> 2.9.3
>>>>>
>>>
>>> Crashes if the second ff_formats_ref() fails:
>>>
>>> ==32719== Invalid read of size 8
>>> ==32719==    at 0x23F6E6: ff_formats_unref (formats.c:478)
>>> ==32719==    by 0x22A028: free_link (avfilter.c:783)
>>> ==32719==    by 0x22A103: avfilter_free (avfilter.c:805)
>>> ==32719==    by 0x22C2EF: avfilter_graph_free (avfiltergraph.c:123)
>>> ==32719==    by 0x1EF08C: cleanup_filtergraph (ffmpeg_filter.c:994)
>>> ==32719==    by 0x1EFA81: configure_filtergraph (ffmpeg_filter.c:1168)
>>> ==32719==    by 0x1F7B48: ifilter_send_frame (ffmpeg.c:2193)
>>> ==32719==    by 0x1F7DC8: send_frame_to_filters (ffmpeg.c:2278)
>>> ==32719==    by 0x1F8A89: decode_video (ffmpeg.c:2469)
>>> ==32719==    by 0x1F9375: process_input_packet (ffmpeg.c:2614)
>>> ==32719==    by 0x1FFC8A: process_input (ffmpeg.c:4350)
>>> ==32719==    by 0x200104: transcode_step (ffmpeg.c:4461)
>>> ==32719==  Address 0x12f78798 is 24 bytes inside a block of size 32 free'd
>>> ==32719==    at 0x4C2CDDB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>>> ==32719==    by 0x13BDC21: av_free (mem.c:239)
>>> ==32719==    by 0x23F7E8: ff_formats_unref (formats.c:478)
>>> ==32719==    by 0x2F8681: hwupload_query_formats (vf_hwupload.c:87)
>>> ==32719==    by 0x22CA9C: filter_query_formats (avfiltergraph.c:317)
>>> ==32719==    by 0x22D040: query_formats (avfiltergraph.c:445)
>>> ==32719==    by 0x22F368: graph_config_formats (avfiltergraph.c:1159)
>>> ==32719==    by 0x22F81A: avfilter_graph_config (avfiltergraph.c:1270)
>>> ==32719==    by 0x1EF688: configure_filtergraph (ffmpeg_filter.c:1096)
>>> ==32719==    by 0x1F7B48: ifilter_send_frame (ffmpeg.c:2193)
>>> ==32719==    by 0x1F7DC8: send_frame_to_filters (ffmpeg.c:2278)
>>> ==32719==    by 0x1F8A89: decode_video (ffmpeg.c:2469)
>>> ==32719==  Block was alloc'd at
>>> ==32719==    at 0x4C2DF16: memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>>> ==32719==    by 0x4C2E021: posix_memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>>> ==32719==    by 0x13BD9DD: av_malloc (mem.c:97)
>>> ==32719==    by 0x13BDC75: av_mallocz (mem.c:254)
>>> ==32719==    by 0x23EF33: ff_make_format_list (formats.c:285)
>>> ==32719==    by 0x2F85A2: hwupload_query_formats (vf_hwupload.c:69)
>>> ==32719==    by 0x22CA9C: filter_query_formats (avfiltergraph.c:317)
>>> ==32719==    by 0x22D040: query_formats (avfiltergraph.c:445)
>>> ==32719==    by 0x22F368: graph_config_formats (avfiltergraph.c:1159)
>>> ==32719==    by 0x22F81A: avfilter_graph_config (avfiltergraph.c:1270)
>>> ==32719==    by 0x1EF688: configure_filtergraph (ffmpeg_filter.c:1096)
>>> ==32719==    by 0x1F7B48: ifilter_send_frame (ffmpeg.c:2193)
>>>
>>> ... more invalid reads ...
>>>
>>> ==32719== Process terminating with default action of signal 11 (SIGSEGV)
>>> ==32719==  Access not within mapped region at address 0x0
>>> ==32719==    at 0x13BDC35: av_freep (mem.c:247)
>>> ==32719==    by 0x23FBF6: ff_set_common_channel_layouts (formats.c:552)
>>> ==32719==    by 0x240100: default_query_formats_common (formats.c:586)
>>> ==32719==    by 0x24015C: ff_default_query_formats (formats.c:599)
>>> ==32719==    by 0x22D051: query_formats (avfiltergraph.c:447)
>>> ==32719==    by 0x22F368: graph_config_formats (avfiltergraph.c:1159)
>>> ==32719==    by 0x22F81A: avfilter_graph_config (avfiltergraph.c:1270)
>>> ==32719==    by 0x1EF688: configure_filtergraph (ffmpeg_filter.c:1096)
>>> ==32719==    by 0x1F68C5: flush_encoders (ffmpeg.c:1876)
>>> ==32719==    by 0x20032E: transcode (ffmpeg.c:4538)
>>> ==32719==    by 0x20097D: main (ffmpeg.c:4720)
>>
>> Mark, can you give the test command for this crash? It's will help me quick reproduce/debug this issue.Tks.
> 
> Instrument ff_formats_ref() to work as if the allocation fails the second time:
> 
> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
> index d4de862237..8244e7432c 100644
> --- a/libavfilter/formats.c
> +++ b/libavfilter/formats.c
> @@ -417,12 +417,12 @@ AVFilterChannelLayouts *ff_all_channel_counts(void)
>  }
>  
>  #define FORMATS_REF(f, ref, unref_fn)                                           \
> -    void *tmp;                                                                  \
> +    void *tmp; static int k;                                                    \
>                                                                                  \
>      if (!f || !ref)                                                             \
>          return AVERROR(ENOMEM);                                                 \
>                                                                                  \
> -    tmp = av_realloc_array(f->refs, sizeof(*f->refs), f->refcount + 1);         \
> +    tmp = ++k == 2 ? (av_free(f->refs), NULL) : av_realloc_array(f->refs, sizeof(*f->refs), f->refcount + 1); \
>      if (!tmp) {                                                                 \
>          unref_fn(&f);                                                           \
>          return AVERROR(ENOMEM);                                                 \
> 
> 
> Then run something using it, like:
> 
> ./ffmpeg_g -vaapi_device /dev/dri/renderD128 -i in.mp4 -vf hwupload -frames 1 -f null -
> 
> (Input file doesn't matter as long as it has video.)
> 
> 
> Maybe this is excessive scrutiny, but I know that AVFilterFormats are horrible so a claim to fix memory leaks around them is worth checking to make sure it doesn't introduce crashes.  Tbh I would be fine with the original patch with the noop ff_formats_unref() lines removed - see most filters using ff_formats_ref() for examples of ignoring the problem by just leaking the memory of the format list.
> 
> Thanks,
> 
> - Mark

Ok, I will submit V3 patch just suppress the build warning, then maybe we can find other suitable way for AVFilterFormats resource leaks in AVFilter issue. 

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 


More information about the ffmpeg-devel mailing list