From patchwork Fri Dec 25 15:47:23 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 24652 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id 9A15744B6ED for ; Fri, 25 Dec 2020 17:53:10 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 7E75868AE8C; Fri, 25 Dec 2020 17:53:10 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-ej1-f52.google.com (mail-ej1-f52.google.com [209.85.218.52]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 472AC68AE74 for ; Fri, 25 Dec 2020 17:53:02 +0200 (EET) Received: by mail-ej1-f52.google.com with SMTP id g20so6660208ejb.1 for ; Fri, 25 Dec 2020 07:53:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references:reply-to :mime-version:content-transfer-encoding; bh=ZUtEg9Yn/Kpn/iT1TOyhlDIlpavP32iMofyrKY4qfaQ=; b=JvIQmQkBXEqHiL2wrSFl72ieDpgJW0L7LrdL1d+I6TaKom7ljBK0zvarvFQiZtJN7c G3t6gaEW04JqPuMoEwwHK7XcXZGuop1KSvqcyY7XXwoVRrvMVhfI5ett8qV2HOBI8MYV UEzJBlL+LXARe/+/q+t+jp11WPW3VERazJdeui6p5QjNuSImDhAH+ev9uFbbIuen9U9P TCGXAUexGk1RZcYSVKhRAwDGcvS2fjPzR7WoKrrNR8T8QR5vX1AdbJoDgLtmsRw9JkGK N9fijcscMSCthWlg33I0kXkd5SCRr4I14e6saG0FUDNOp2aDdZFAiSqipovOUtiExtQz BaMQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:reply-to:mime-version:content-transfer-encoding; bh=ZUtEg9Yn/Kpn/iT1TOyhlDIlpavP32iMofyrKY4qfaQ=; b=gTagFMO3Vp7WW2QWnFBmNbftwEs8aTKYVhoZN6kR3r1WNVNK2Xt4ugCBg1GJa67SYE 13k5WEmH6iUbuip7EvXWrRClSqVXEBbrRf5EonWhqdSdAka8ZSpawLfvmAF/2ZoAdMyo MWqpRcYXj1AnnWy63UQ1TRItwOjQIVjMO7xwQgdq2dj9PF/2lkJOIbPZn4KE0JhIPonH GHFnwokJO5t0eZTVDopj0c94scs0gsRt4gBAXgG35ODu84K0TxT4/m6jkk6Pjv5ga2g2 CNX6QdXImHsohLoA2bpxQMELqgCUdUnhmN83YdfLIggjjwmaqQrcXfMzUFdd0FVRkfKO 2J8A== X-Gm-Message-State: AOAM532Z0NsKSApT0q+OLOklL5AQ/E4vDT7HDyh7xJ2NSBWeZM4bD+2y 93ASMq83xHcp2JpGs/gCBSoV5Fpec+M= X-Google-Smtp-Source: ABdhPJzRQzqiAEkuynTh+CwK272zU8fOJ60IGz6d6f9E5A4Cs88imGTuxFfuDrf1cYu/5pmHB5dh0Q== X-Received: by 2002:a17:906:1151:: with SMTP id i17mr32539761eja.250.1608911581449; Fri, 25 Dec 2020 07:53:01 -0800 (PST) Received: from sblaptop.fritz.box (ipbcc1aa4b.dynamic.kabel-deutschland.de. [188.193.170.75]) by smtp.gmail.com with ESMTPSA id ho12sm13733010ejc.45.2020.12.25.07.53.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 25 Dec 2020 07:53:00 -0800 (PST) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Fri, 25 Dec 2020 16:47:23 +0100 Message-Id: <20201225154724.287465-6-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20201225154724.287465-1-andreas.rheinhardt@gmail.com> References: <20201225154724.287465-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 6/7] avcodec/mpegvideo: Fix memleak upon allocation error 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 Cc: Andreas Rheinhardt Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" When slice-threading is used, ff_mpv_common_init() duplicates the first MpegEncContext and allocates some buffers for each MpegEncContext (the first as well as the copies). But the count of allocated MpegEncContexts is not updated until after everything has been allocated and if an error happens after the first one has been allocated, only the first one is freed; the others leak. This commit fixes this: The count is now set before the copies are allocated. Furthermore, the copies are now created and initialized before the first MpegEncContext, so that the buffers exclusively owned by each MpegEncContext are still NULL in the src MpegEncContext so that no double-free happens upon allocation failure. Given that this effectively touches every line of the init code, it has also been factored out in a function of its own in order to remove code duplication with the same code in ff_mpv_common_frame_size_change() (which was never called when using more than one slice (and if it were, there would be potential double-frees)). Signed-off-by: Andreas Rheinhardt --- libavcodec/mpegvideo.c | 89 +++++++++++++++++------------------------- 1 file changed, 36 insertions(+), 53 deletions(-) diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c index 1bf25566b8..3daccb816f 100644 --- a/libavcodec/mpegvideo.c +++ b/libavcodec/mpegvideo.c @@ -364,13 +364,6 @@ static int init_duplicate_context(MpegEncContext *s) if (s->mb_height & 1) yc_size += 2*s->b8_stride + 2*s->mb_stride; - s->sc.edge_emu_buffer = - s->me.scratchpad = - s->me.temp = - s->sc.rd_scratchpad = - s->sc.b_scratchpad = - s->sc.obmc_scratchpad = NULL; - if (s->encoding) { if (!FF_ALLOCZ_TYPED_ARRAY(s->me.map, ME_MAP_SIZE) || !FF_ALLOCZ_TYPED_ARRAY(s->me.score_map, ME_MAP_SIZE)) @@ -411,6 +404,35 @@ static int init_duplicate_context(MpegEncContext *s) return 0; } +/** + * Initialize an MpegEncContext's thread contexts. Presumes that + * slice_context_count is already set and that all the fields + * that are freed/reset in free_duplicate_context() are NULL. + */ +static int init_duplicate_contexts(MpegEncContext *s) +{ + int nb_slices = s->slice_context_count, ret; + + /* We initialize the copies before the original so that + * fields allocated in init_duplicate_context are NULL after + * copying. This prevents double-frees upon allocation error. */ + for (int i = 1; i < nb_slices; i++) { + s->thread_context[i] = av_memdup(s, sizeof(MpegEncContext)); + if (!s->thread_context[i]) + return AVERROR(ENOMEM); + if ((ret = init_duplicate_context(s->thread_context[i])) < 0) + return ret; + s->thread_context[i]->start_mb_y = + (s->mb_height * (i ) + nb_slices / 2) / nb_slices; + s->thread_context[i]->end_mb_y = + (s->mb_height * (i + 1) + nb_slices / 2) / nb_slices; + } + s->start_mb_y = 0; + s->end_mb_y = nb_slices > 1 ? (s->mb_height + nb_slices / 2) / nb_slices + : s->mb_height; + return init_duplicate_context(s); +} + static void free_duplicate_context(MpegEncContext *s) { if (!s) @@ -950,29 +972,12 @@ av_cold int ff_mpv_common_init(MpegEncContext *s) s->context_initialized = 1; memset(s->thread_context, 0, sizeof(s->thread_context)); s->thread_context[0] = s; + s->slice_context_count = nb_slices; // if (s->width && s->height) { - if (nb_slices > 1) { - for (i = 0; i < nb_slices; i++) { - if (i) { - s->thread_context[i] = av_memdup(s, sizeof(MpegEncContext)); - if (!s->thread_context[i]) - goto fail_nomem; - } - if ((ret = init_duplicate_context(s->thread_context[i])) < 0) - goto fail; - s->thread_context[i]->start_mb_y = - (s->mb_height * (i) + nb_slices / 2) / nb_slices; - s->thread_context[i]->end_mb_y = - (s->mb_height * (i + 1) + nb_slices / 2) / nb_slices; - } - } else { - if ((ret = init_duplicate_context(s)) < 0) - goto fail; - s->start_mb_y = 0; - s->end_mb_y = s->mb_height; - } - s->slice_context_count = nb_slices; + ret = init_duplicate_contexts(s); + if (ret < 0) + goto fail; // } return 0; @@ -1082,31 +1087,9 @@ int ff_mpv_common_frame_size_change(MpegEncContext *s) s->thread_context[0] = s; if (s->width && s->height) { - int nb_slices = s->slice_context_count; - if (nb_slices > 1) { - for (i = 0; i < nb_slices; i++) { - if (i) { - s->thread_context[i] = av_memdup(s, sizeof(MpegEncContext)); - if (!s->thread_context[i]) { - err = AVERROR(ENOMEM); - goto fail; - } - } - if ((err = init_duplicate_context(s->thread_context[i])) < 0) - goto fail; - s->thread_context[i]->start_mb_y = - (s->mb_height * (i) + nb_slices / 2) / nb_slices; - s->thread_context[i]->end_mb_y = - (s->mb_height * (i + 1) + nb_slices / 2) / nb_slices; - } - } else { - err = init_duplicate_context(s); - if (err < 0) - goto fail; - s->start_mb_y = 0; - s->end_mb_y = s->mb_height; - } - s->slice_context_count = nb_slices; + err = init_duplicate_contexts(s); + if (err < 0) + goto fail; } return 0;