From patchwork Thu Mar 9 23:49:36 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Thompson X-Patchwork-Id: 2863 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.50.79 with SMTP id y76csp558260vsy; Thu, 9 Mar 2017 15:49:51 -0800 (PST) X-Received: by 10.223.145.97 with SMTP id j88mr12671896wrj.178.1489103390922; Thu, 09 Mar 2017 15:49:50 -0800 (PST) Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id q186si557037wma.35.2017.03.09.15.49.49; Thu, 09 Mar 2017 15:49:50 -0800 (PST) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@jkqxz-net.20150623.gappssmtp.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id B38BA6883D9; Fri, 10 Mar 2017 01:49:32 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wm0-f45.google.com (mail-wm0-f45.google.com [74.125.82.45]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id F34466883A2 for ; Fri, 10 Mar 2017 01:49:25 +0200 (EET) Received: by mail-wm0-f45.google.com with SMTP id t189so67863064wmt.1 for ; Thu, 09 Mar 2017 15:49:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jkqxz-net.20150623.gappssmtp.com; s=20150623; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-transfer-encoding; bh=lLiF+Y9kjXGGTEDEj2Q6n/7GOgCy686nN3cj8+aujqg=; b=BwNVpjE176bI0aflD/mY2+AwUp+Fe/luE2tS7fIBF2+XhreJJs0GbL6euGSV8l5UtZ pesl9T2x80NX6sDsTLFmyauUDxtb+MNhwFCbzopbRIoTXXaiL12lhZXH+ib/fUIQJ4fr sTl6TZwjhiimTC6mz3sF79c/3s6Vs65w1nNCG2+yg1jDyzvUFJgdbhBcwgFURcPG3XnB Aqy2VK6OIGWLGEkBZ3/ts7VpQB028Ar6w5qCa+uo66TtYjkIcVEG6IxEpCYB0iIRBxz1 PFIvrb3qhLBVoO4oSvomr3AYM+lOFkXrhj+cDp17joUMfGty9xjrPU9oyLLyqhTewTsJ JtYQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=lLiF+Y9kjXGGTEDEj2Q6n/7GOgCy686nN3cj8+aujqg=; b=msU1gU6OMewsXDkNrDEK+G65XZ9zKTdr/Zo5RMyN5H/FXsoxtNcd3WtdRvuMHdkghn tjyH4aI1gcO4UCuGA2Psv54rMLhQpFPg66y1N3VRE0joRcpu/D5qfeXVWexd7+qpet8y cGU7dxPxthW7ehu8LYwdwycf0BT8NIzj/nQ7ZCmCJ0k0LhFLraS4BVlCjz+2R2uW/l0K iLwpSOgAUE9mkLM9A6aETxAyfT7e3R+zqzkCM3KOrz77M8Ze3wIuUpmAkVVr0BYduX8n osfAoiWsrezjuiC4qZasRokQwSta4wat8Y3LfRw2ztw2yeW+IXeP/gQCqidVvQ9AUVrd NDIg== X-Gm-Message-State: AMke39mJUk8XWZAcm69k2cC2Hp75+by+xi50dSg40JdliQ7S7zSUpeD1sFrUVl3ioGsx0Q== X-Received: by 10.28.193.193 with SMTP id r184mr12302520wmf.80.1489103378827; Thu, 09 Mar 2017 15:49:38 -0800 (PST) Received: from [192.168.0.8] (cpc91242-cmbg18-2-0-cust650.5-4.cable.virginm.net. [82.8.130.139]) by smtp.gmail.com with ESMTPSA id q4sm626602wme.17.2017.03.09.15.49.37 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 09 Mar 2017 15:49:37 -0800 (PST) To: FFmpeg development discussions and patches References: <7e509f99-3b6b-25a3-47de-c4c2c88ad743@gmail.com> <7349a089-c4c2-68ab-e26e-41dfa5f1931e@jkqxz.net> From: Mark Thompson Message-ID: <7f3da594-6ad5-a11e-7589-f7d37f52825d@jkqxz.net> Date: Thu, 9 Mar 2017 23:49:36 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.6.0 MIME-Version: 1.0 In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH V2] vf_hwupload: Add missing return value check X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" 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 >>>> 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 >>>> --- >>>> 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: 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 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); \