diff mbox series

[FFmpeg-devel,13/21] avfilter/vf_remap: Fix double-free of AVFilterFormats on error

Message ID 20200809155748.30092-7-andreas.rheinhardt@gmail.com
State Accepted
Commit 07240c36c2912cea96dd9d11c8e3ed27995a2b3c
Headers show
Series [FFmpeg-devel,1/6] avfilter/formats: Remove ff_make_formatu64_list()
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Aug. 9, 2020, 3:57 p.m. UTC
The query_formats function of the remap filter tries to allocate
two lists of formats which on success are attached to more permanent objects
(AVFilterLinks) for storage afterwards. If attaching a list to an
AVFilterLink succeeds, it is in turn owned by the AVFilterLink (or more
exactly, the AVFilterLink becomes one of the common owners of the list).
Yet if attaching a list to one of its links succeeds and an error happens
lateron, both lists were manually freed, which means that is wrong if the
list is already owned by one or more links; these links' pointers to
their lists will become dangling and there will be a double-free/use-after-
free when these links are cleaned up automatically.

This commit fixes this by removing the custom free code; this will
temporarily add a leaking codepath (if attaching a list not already
owned by a link to a link fails, the list will leak), but this will
be fixed soon by making sure that an AVFilterFormats without owner will
be automatically freed when attaching it to an AVFilterLink fails.
Notice at most one list leaks because a new list is only allocated
after the old list has been successfully attached to a link.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavfilter/vf_remap.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

Comments

Paul B Mahol Aug. 19, 2020, 7:22 p.m. UTC | #1
On 8/9/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> The query_formats function of the remap filter tries to allocate
> two lists of formats which on success are attached to more permanent objects
> (AVFilterLinks) for storage afterwards. If attaching a list to an
> AVFilterLink succeeds, it is in turn owned by the AVFilterLink (or more
> exactly, the AVFilterLink becomes one of the common owners of the list).
> Yet if attaching a list to one of its links succeeds and an error happens
> lateron, both lists were manually freed, which means that is wrong if the
> list is already owned by one or more links; these links' pointers to
> their lists will become dangling and there will be a double-free/use-after-
> free when these links are cleaned up automatically.
>
> This commit fixes this by removing the custom free code; this will
> temporarily add a leaking codepath (if attaching a list not already
> owned by a link to a link fails, the list will leak), but this will
> be fixed soon by making sure that an AVFilterFormats without owner will
> be automatically freed when attaching it to an AVFilterLink fails.
> Notice at most one list leaks because a new list is only allocated
> after the old list has been successfully attached to a link.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavfilter/vf_remap.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
>

LGTM
diff mbox series

Patch

diff --git a/libavfilter/vf_remap.c b/libavfilter/vf_remap.c
index 6d5d75225b..41a2409f21 100644
--- a/libavfilter/vf_remap.c
+++ b/libavfilter/vf_remap.c
@@ -115,25 +115,15 @@  static int query_formats(AVFilterContext *ctx)
     AVFilterFormats *pix_formats = NULL, *map_formats = NULL;
     int ret;
 
-    if (!(pix_formats = ff_make_format_list(s->format ? gray_pix_fmts : pix_fmts)) ||
-        !(map_formats = ff_make_format_list(map_fmts))) {
-        ret = AVERROR(ENOMEM);
-        goto fail;
-    }
+    pix_formats = ff_make_format_list(s->format ? gray_pix_fmts : pix_fmts);
     if ((ret = ff_formats_ref(pix_formats, &ctx->inputs[0]->out_formats)) < 0 ||
-        (ret = ff_formats_ref(map_formats, &ctx->inputs[1]->out_formats)) < 0 ||
-        (ret = ff_formats_ref(map_formats, &ctx->inputs[2]->out_formats)) < 0 ||
         (ret = ff_formats_ref(pix_formats, &ctx->outputs[0]->in_formats)) < 0)
-        goto fail;
-    return 0;
-fail:
-    if (pix_formats)
-        av_freep(&pix_formats->formats);
-    av_freep(&pix_formats);
-    if (map_formats)
-        av_freep(&map_formats->formats);
-    av_freep(&map_formats);
-    return ret;
+        return ret;
+
+    map_formats = ff_make_format_list(map_fmts);
+    if ((ret = ff_formats_ref(map_formats, &ctx->inputs[1]->out_formats)) < 0)
+        return ret;
+    return ff_formats_ref(map_formats, &ctx->inputs[2]->out_formats);
 }
 
 /**