diff mbox series

[FFmpeg-devel,7/7] avfilter/graphparser: Fix memleak when linking filters fails

Message ID 20200823095039.18851-4-andreas.rheinhardt@gmail.com
State Accepted
Commit deb6476fd8bc3a3c2b134704ecb804269843ed89
Headers show
Series [FFmpeg-devel,1/3] avfilter/graphparser: Fix leaks when parsing inputs fails
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. 23, 2020, 9:50 a.m. UTC
Parsing labeled outputs involves a check for an already known match
(a labeled input with the same name) to pair them together. If yes,
it is attempted to create a link between the two filters; in this case
the AVFilterInOuts have fulfilled their purpose and are freed. Yet if
creating the link fails, these AVFilterInOuts have up until now not been
freed, although they had already been removed from their respective lists
(which means that they are not freed automatically). In other words:
They leak. This commit fixes this.

This fixes ticket #7084. Said ticket contains an example program to
reproduce a leak. It can also be reproduced with ffmpeg alone, e.g. with
the complex filters "[0]null[1],[2]anull[0]" or with "[0]abitscope[0]".
All of these three examples involve media type mismatches which make it
impossible to create the links. The bug could also be triggered by other
means, e.g. failure to allocate the necessary AVFilterLink.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavfilter/graphparser.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Nicolas George Aug. 23, 2020, 10:16 a.m. UTC | #1
Andreas Rheinhardt (12020-08-23):
> Parsing labeled outputs involves a check for an already known match
> (a labeled input with the same name) to pair them together. If yes,
> it is attempted to create a link between the two filters; in this case
> the AVFilterInOuts have fulfilled their purpose and are freed. Yet if
> creating the link fails, these AVFilterInOuts have up until now not been
> freed, although they had already been removed from their respective lists
> (which means that they are not freed automatically). In other words:
> They leak. This commit fixes this.
> 
> This fixes ticket #7084. Said ticket contains an example program to
> reproduce a leak. It can also be reproduced with ffmpeg alone, e.g. with
> the complex filters "[0]null[1],[2]anull[0]" or with "[0]abitscope[0]".
> All of these three examples involve media type mismatches which make it
> impossible to create the links. The bug could also be triggered by other
> means, e.g. failure to allocate the necessary AVFilterLink.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavfilter/graphparser.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

LGTM.

Regards,
diff mbox series

Patch

diff --git a/libavfilter/graphparser.c b/libavfilter/graphparser.c
index f55737f8c7..d72e65c09a 100644
--- a/libavfilter/graphparser.c
+++ b/libavfilter/graphparser.c
@@ -372,15 +372,14 @@  static int parse_outputs(const char **buf, AVFilterInOut **curr_inputs,
         match = extract_inout(name, open_inputs);
 
         if (match) {
-            if ((ret = link_filter(input->filter_ctx, input->pad_idx,
-                                   match->filter_ctx, match->pad_idx, log_ctx)) < 0) {
-                av_free(name);
-                return ret;
-            }
+            ret = link_filter(input->filter_ctx, input->pad_idx,
+                              match->filter_ctx, match->pad_idx, log_ctx);
             av_freep(&match->name);
             av_freep(&name);
             av_freep(&match);
             av_freep(&input);
+            if (ret < 0)
+                return ret;
         } else {
             /* Not in the list, so add the first input as an open_output */
             input->name = name;