| 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 | expand |
| 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 |
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,
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,
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 --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;
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(-)