diff mbox series

[FFmpeg-devel,1/3] avfilter/avfilter: Fix leaks upon filter creation error

Message ID AM7PR03MB66604AF57B655E8787E04A068FF79@AM7PR03MB6660.eurprd03.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,1/3] avfilter/avfilter: Fix leaks upon filter creation error
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Aug. 10, 2021, 11:42 p.m. UTC
Both the name as well as the options need to be freed.
(Right now there is no option for the AVFilterContext itself that could
leak, but some filters have options (e.g. of type AV_OPT_TYPE_STRING)
that can leak.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
I intend to remove this duplicated freeing code and call avfilter_free()
directly once it is ensured that it is safe to do so; a few checks will
have to be added to it and the filters will have to be checked to be
compatible with it. The only thing I already found out is that libvamf
is buggy (even without calling avfilter_free), because it just presumes
that a mutex and a condition variable have always been properly
initialized.

 libavfilter/avfilter.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Nicolas George Aug. 11, 2021, 8:15 a.m. UTC | #1
Andreas Rheinhardt (12021-08-11):
> Both the name as well as the options need to be freed.
> (Right now there is no option for the AVFilterContext itself that could
> leak, but some filters have options (e.g. of type AV_OPT_TYPE_STRING)
> that can leak.)
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>

LGTM.

> I intend to remove this duplicated freeing code and call avfilter_free()
> directly once it is ensured that it is safe to do so; a few checks will
> have to be added to it and the filters will have to be checked to be
> compatible with it. The only thing I already found out is that libvamf
> is buggy (even without calling avfilter_free), because it just presumes
> that a mutex and a condition variable have always been properly
> initialized.

Useful work, thanks.

Regards,
Nicolas George Aug. 11, 2021, 8:25 a.m. UTC | #2
Andreas Rheinhardt (12021-08-11):
> The current way of doing it involves writing the ctx parameter twice.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>

LGTM, but it is not my area of expertise.

Regards,
Andreas Rheinhardt Aug. 11, 2021, 7:19 p.m. UTC | #3
Nicolas George:
> Andreas Rheinhardt (12021-08-11):
>> Both the name as well as the options need to be freed.
>> (Right now there is no option for the AVFilterContext itself that could
>> leak, but some filters have options (e.g. of type AV_OPT_TYPE_STRING)
>> that can leak.)
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> 
> LGTM.
> 
I have now found an issue with this patch: If preinit fails (no current
preinit seems to be able to fail), then av_opt_free() will be called
before the AVClass has been set on the private context (presuming the
filter to have a private class).

Furthermore, there are more problems here: The documentation states that
uninit will be called on preinit failure. This is just not true.
Finally, nb_(in|out)puts is set before the structures necessary for this
have been allocated and if uninit is called (due to preinit success),
then freeing the pads names might crash; seems like none of the filters
that currently have a preinit are affected by this (only the xmedian
filter has dynamic inputs, but it has no static inputs, so it is safe).
I will therefore send an updated patch; I will also send a patch to free
the name of pads generically based upon a internal flag.

- Andreas
diff mbox series

Patch

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index 358bf8a853..908e812b5c 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -684,13 +684,19 @@  AVFilterContext *ff_filter_alloc(const AVFilter *filter, const char *inst_name)
 err:
     if (preinited)
         filter->uninit(ret);
+    av_freep(&ret->name);
     av_freep(&ret->inputs);
     av_freep(&ret->input_pads);
     ret->nb_inputs = 0;
     av_freep(&ret->outputs);
     av_freep(&ret->output_pads);
     ret->nb_outputs = 0;
-    av_freep(&ret->priv);
+    if (ret->priv) {
+        if (filter->priv_class)
+            av_opt_free(ret->priv);
+        av_freep(&ret->priv);
+    }
+    av_opt_free(ret);
     av_freep(&ret->internal);
     av_free(ret);
     return NULL;