diff mbox series

[FFmpeg-devel,16/21] avfilter/af_channelmap: Fix double-free of AVFilterChannelLayouts on error

Message ID 20200809155748.30092-10-andreas.rheinhardt@gmail.com
State Accepted
Commit 44bcd6f74922ba490e680e79eae897b249c29d62
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 channelmap filter tries to allocate
a list of channel layouts which on success are attached to more permanent
objects (an AVFilterLink) for storage afterwards. If attaching succeeds,
the link becomes one of the common owners (in this case, the only owner)
of the list. Yet if the list has been successfully attached to the link
and an error happens lateron, the list was manually freed, which is wrong,
because it is owned by its link so that the link's pointer to the list will
become dangling and there will be a double-free/use-after-free when the link
is later cleaned up automatically.

This commit fixes this by removing the custom freeing code; this will
temporarily add a leaking codepath (if attaching the list fails, the list
will leak), but this will be fixed soon by making sure that an
AVFilterChannelLayouts without owner will be automatically freed when
attaching it to an AVFilterLink fails.

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

Comments

Nicolas George Aug. 23, 2020, 2:10 p.m. UTC | #1
Andreas Rheinhardt (12020-08-09):
> The query_formats function of the channelmap filter tries to allocate
> a list of channel layouts which on success are attached to more permanent
> objects (an AVFilterLink) for storage afterwards. If attaching succeeds,
> the link becomes one of the common owners (in this case, the only owner)
> of the list. Yet if the list has been successfully attached to the link
> and an error happens lateron, the list was manually freed, which is wrong,
> because it is owned by its link so that the link's pointer to the list will
> become dangling and there will be a double-free/use-after-free when the link
> is later cleaned up automatically.
> 
> This commit fixes this by removing the custom freeing code; this will
> temporarily add a leaking codepath (if attaching the list fails, the list
> will leak), but this will be fixed soon by making sure that an
> AVFilterChannelLayouts without owner will be automatically freed when
> attaching it to an AVFilterLink fails.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavfilter/af_channelmap.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)

Patches 8-16 LGTM.

As a matter of style, I would like it better if the last case was
handled the same way as the others rather than using the final return,
i.e., instead of:

    if ((ret = a()) < 0 ||
        (ret = b()) < 0)
        return ret;
    return c();

I like better :

    if ((ret = a()) < 0 ||
        (ret = b()) < 0 ||
        (ret = c()) < 0)
        return ret;
    return 0;

But it is minor and mostly a matter of taste.

Regards,
diff mbox series

Patch

diff --git a/libavfilter/af_channelmap.c b/libavfilter/af_channelmap.c
index 285d76a3ef..1f79f89ce3 100644
--- a/libavfilter/af_channelmap.c
+++ b/libavfilter/af_channelmap.c
@@ -280,28 +280,18 @@  static av_cold int channelmap_init(AVFilterContext *ctx)
 static int channelmap_query_formats(AVFilterContext *ctx)
 {
     ChannelMapContext *s = ctx->priv;
-    AVFilterChannelLayouts *layouts;
     AVFilterChannelLayouts *channel_layouts = NULL;
     int ret;
 
-    layouts = ff_all_channel_counts();
-    if (!layouts) {
-        ret = AVERROR(ENOMEM);
-        goto fail;
-    }
-    if ((ret = ff_add_channel_layout     (&channel_layouts, s->output_layout                    )) < 0 ||
-        (ret = ff_set_common_formats     (ctx             , ff_planar_sample_fmts()             )) < 0 ||
+    if ((ret = ff_set_common_formats    (ctx,  ff_planar_sample_fmts()))  < 0 ||
         (ret = ff_set_common_samplerates (ctx             , ff_all_samplerates()                )) < 0 ||
-        (ret = ff_channel_layouts_ref    (layouts         , &ctx->inputs[0]->out_channel_layouts)) < 0 ||
-        (ret = ff_channel_layouts_ref    (channel_layouts , &ctx->outputs[0]->in_channel_layouts)) < 0)
-            goto fail;
+        (ret = ff_add_channel_layout(&channel_layouts, s->output_layout)) < 0 ||
+        (ret = ff_channel_layouts_ref(channel_layouts,
+                                      &ctx->outputs[0]->in_channel_layouts)) < 0)
+        return ret;
 
-    return 0;
-fail:
-    if (layouts)
-        av_freep(&layouts->channel_layouts);
-    av_freep(&layouts);
-    return ret;
+    return ff_channel_layouts_ref(ff_all_channel_counts(),
+                                  &ctx->inputs[0]->out_channel_layouts);
 }
 
 static int channelmap_filter_frame(AVFilterLink *inlink, AVFrame *buf)