From patchwork Sun Aug 9 15:57:44 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 21556 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 2539044B9EE for ; Sun, 9 Aug 2020 18:58:36 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 0C8C968A56E; Sun, 9 Aug 2020 18:58:36 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-ed1-f66.google.com (mail-ed1-f66.google.com [209.85.208.66]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id E735368A9E1 for ; Sun, 9 Aug 2020 18:58:31 +0300 (EEST) Received: by mail-ed1-f66.google.com with SMTP id a14so4684768edx.7 for ; Sun, 09 Aug 2020 08:58:31 -0700 (PDT) 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 :mime-version:content-transfer-encoding; bh=Yb+OdrOzbcItYCRCz5PtdK1OOEH8WJqaSXSv0mgyvlA=; b=PXP1K/22mKdsDGC8PE93fgEnE8dorHzSz5hrQpPTovYdu9ikiZFG+IJ6Kr/u7aUC9v /givAl/1C/g+461CuemPKe1McOwu5zWrWDuyKVwIuokAvS1jFrGrVrjes6F6Z3PWZw6d zUt/9IbGXRzu367qL+pKbHpO1z+jnndHQJjyTvujIygHvYKwBq9TNHUZaSOWQdULZSYy IAPJwNVt2w6cJM4lNm5J19Ui0onTDR1lFLXZ/g0kED1P3PxEw3l2iX1eaw51X6uEOPz/ dKRuZIywVZ6mv0LrEO18DR/vquHupE+zWWu+Qol3J9qfLGySOeyuUk4lUUdXsxnOWzyc WGSQ== 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:mime-version:content-transfer-encoding; bh=Yb+OdrOzbcItYCRCz5PtdK1OOEH8WJqaSXSv0mgyvlA=; b=TZjx9hdmWANax0Wz/+VSM1T2uyUjBYq2PBgGLhFecIVMtxYBI6Ik28Rz/2z7SpXM1x 7UhwDVcU9N6R8fSQclzUgj89/tpXkWv4MQIJQy6+GbnFnFMIh4RQLFe559Hte377GRnZ K7ECok9OavjKSJEF2QDoVgUSy9y0dmr+6BRdDoWdGwibJn0YN/yzer6L8PTUiEEuGNPY C42IJ/9eMvyk7TsC52urE1V5lp3ahD/5jMfvX0sU8DH9DdIXKERI4bWxOfTq3GftHFAO I6ixzr45B/BsJZQoa1GABBR5Pk1f55VUB6ARqmEla70px0FIlQfArmR0pextJBphMR22 eHeQ== X-Gm-Message-State: AOAM531hPLEF3YeVpbynFsIWO58BJv+hsxfe4pW68PZ5OAagDX1dFPxA CR04/Wt/F0JQME3ZWlqSzIJZu3Kv X-Google-Smtp-Source: ABdhPJxRXLph5ljV2kDXlCNftlrwU18bP0IjDFAHFHXomXfDo4yw00kPc0Om0O3nwPH9sQ16JcyoDw== X-Received: by 2002:aa7:cb45:: with SMTP id w5mr17478932edt.77.1596988710740; Sun, 09 Aug 2020 08:58:30 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc10296.dynamic.kabel-deutschland.de. [188.193.2.150]) by smtp.gmail.com with ESMTPSA id g11sm5360290edv.95.2020.08.09.08.58.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 09 Aug 2020 08:58:30 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Sun, 9 Aug 2020 17:57:44 +0200 Message-Id: <20200809155748.30092-11-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200808140202.586-1-andreas.rheinhardt@gmail.com> References: <20200808140202.586-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 17/21] avfilter/formats: Fix double frees and memleaks on 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" The formats API deals with lists of channel layouts, sample rates, pixel formats and sample formats. These lists are refcounted in a way in which the list structure itself contains pointers to all of its owners. Furthermore, it is possible for a list to be not owned by anyone yet; this status is temporary until the list has been attached to an owner. Adding an owner to a list involves reallocating the list's list of owners and can therefore fail. In order to reduce the amount of checks and cleanup code for the users of this API, the API is supposed to be lenient when faced with input lists that are NULL and it is supposed to clean up if adding an owner to a list fails, so that a simple use case like list = ff_make_format_list(foo_fmts); if ((ret = ff_formats_ref(list, &ctx->inputs[0]->out_formats)) < 0) return ret; needn't check whether list could be successfully allocated (ff_formats_ref() return AVERROR(ENOMEM) if it couldn't) and it also needn't free list if ff_formats_ref() couldn't add an owner for it. But the cleaning up after itself was broken. The root cause was that the refcount was decremented during unreferencing whether or not the element to be unreferenced was actually an owner of the list or not. This means that if the above sample code is continued by if ((ret = ff_formats_ref(list, &ctx->inputs[1]->out_formats)) < 0) return ret; and that if an error happens at the second ff_formats_ref() call, the automatic cleaning of list will decrement the refcount from 1 (the sole owner of list at this moment is ctx->input[0]->out_formats) to 0 and so the list will be freed; yet ctx->input[0]->out_formats still points to the list and this will lead to a double free/use-after-free when ctx->input[0] is freed later. Presumably in order to work around such an issue, commit 93afb338a405eac0f9e7b092bc26603378bfcca6 restricted unreferencing to lists with owners. This does not solve the root cause (the above example is not fixed by this) at all, but it solves some crashs. This commit fixes the API: The list's refcount is only decremented if an owner is removed from the list of owners and not if the unref-function is called with a pointer that is not among the owners of the list. Furtermore, the requirement for the list to have owners is dropped. This implies that if the first call to ff_formats_ref() in the above example fails, the refcount which is initially zero during unreferencing is not modified, so that the list will be freed automatically in said call to ff_formats_ref() as every list whose refcount reaches zero is. If on the other hand, the second call to ff_formats_ref() is the first to fail, the refcount would stay at one during the automatic unreferencing in ff_formats_ref(). The list would later be freed when its last (and in this case sole) owner (namely ctx->inputs[0]->out_formats) gets unreferenced. The issues described here for ff_formats_ref() also affected the other functions of this API. E.g. ff_add_format() failed to clean up after itself if adding an entry to an already existing list failed (the case of a freshly allocated list was handled specially and this commit also removes said code). E.g. ff_all_formats() inherited the flaw. Signed-off-by: Andreas Rheinhardt --- The count variable in SET_COMMON_FORMATS is now btw unnecessary; it would be safe to always unref fmt in this macro (which does nothing except when fmt has no owner in which case it frees fmt). libavfilter/formats.c | 32 ++++++++++---------------------- 1 file changed, 10 insertions(+), 22 deletions(-) diff --git a/libavfilter/formats.c b/libavfilter/formats.c index 3118aa0925..2379be1518 100644 --- a/libavfilter/formats.c +++ b/libavfilter/formats.c @@ -327,7 +327,6 @@ AVFilterChannelLayouts *avfilter_make_format64_list(const int64_t *fmts) #define ADD_FORMAT(f, fmt, unref_fn, type, list, nb) \ do { \ type *fmts; \ - void *oldf = *f; \ \ if (!(*f) && !(*f = av_mallocz(sizeof(**f)))) { \ return AVERROR(ENOMEM); \ @@ -337,8 +336,6 @@ do { \ sizeof(*(*f)->list)); \ if (!fmts) { \ unref_fn(f); \ - if (!oldf) \ - av_freep(f); \ return AVERROR(ENOMEM); \ } \ \ @@ -499,16 +496,17 @@ do { \ do { \ int idx = -1; \ \ - if (!ref || !*ref || !(*ref)->refs) \ + if (!ref || !*ref) \ return; \ \ FIND_REF_INDEX(ref, idx); \ \ - if (idx >= 0) \ + if (idx >= 0) { \ memmove((*ref)->refs + idx, (*ref)->refs + idx + 1, \ sizeof(*(*ref)->refs) * ((*ref)->refcount - idx - 1)); \ - \ - if(!--(*ref)->refcount) { \ + --(*ref)->refcount; \ + } \ + if (!(*ref)->refcount) { \ av_free((*ref)->list); \ av_free((*ref)->refs); \ av_free(*ref); \ @@ -550,7 +548,7 @@ void ff_formats_changeref(AVFilterFormats **oldref, AVFilterFormats **newref) FORMATS_CHANGEREF(oldref, newref); } -#define SET_COMMON_FORMATS(ctx, fmts, in_fmts, out_fmts, ref_fn, unref_fn, list) \ +#define SET_COMMON_FORMATS(ctx, fmts, in_fmts, out_fmts, ref_fn, unref_fn) \ int count = 0, i; \ \ if (!fmts) \ @@ -560,10 +558,6 @@ void ff_formats_changeref(AVFilterFormats **oldref, AVFilterFormats **newref) if (ctx->inputs[i] && !ctx->inputs[i]->out_fmts) { \ int ret = ref_fn(fmts, &ctx->inputs[i]->out_fmts); \ if (ret < 0) { \ - unref_fn(&fmts); \ - if (fmts) \ - av_freep(&fmts->list); \ - av_freep(&fmts); \ return ret; \ } \ count++; \ @@ -573,10 +567,6 @@ void ff_formats_changeref(AVFilterFormats **oldref, AVFilterFormats **newref) if (ctx->outputs[i] && !ctx->outputs[i]->in_fmts) { \ int ret = ref_fn(fmts, &ctx->outputs[i]->in_fmts); \ if (ret < 0) { \ - unref_fn(&fmts); \ - if (fmts) \ - av_freep(&fmts->list); \ - av_freep(&fmts); \ return ret; \ } \ count++; \ @@ -584,9 +574,7 @@ void ff_formats_changeref(AVFilterFormats **oldref, AVFilterFormats **newref) } \ \ if (!count) { \ - av_freep(&fmts->list); \ - av_freep(&fmts->refs); \ - av_freep(&fmts); \ + unref_fn(&fmts); \ } \ \ return 0; @@ -595,14 +583,14 @@ int ff_set_common_channel_layouts(AVFilterContext *ctx, AVFilterChannelLayouts *layouts) { SET_COMMON_FORMATS(ctx, layouts, in_channel_layouts, out_channel_layouts, - ff_channel_layouts_ref, ff_channel_layouts_unref, channel_layouts); + ff_channel_layouts_ref, ff_channel_layouts_unref); } int ff_set_common_samplerates(AVFilterContext *ctx, AVFilterFormats *samplerates) { SET_COMMON_FORMATS(ctx, samplerates, in_samplerates, out_samplerates, - ff_formats_ref, ff_formats_unref, formats); + ff_formats_ref, ff_formats_unref); } /** @@ -613,7 +601,7 @@ int ff_set_common_samplerates(AVFilterContext *ctx, int ff_set_common_formats(AVFilterContext *ctx, AVFilterFormats *formats) { SET_COMMON_FORMATS(ctx, formats, in_formats, out_formats, - ff_formats_ref, ff_formats_unref, formats); + ff_formats_ref, ff_formats_unref); } static int default_query_formats_common(AVFilterContext *ctx,