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.z...@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.z...@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel