diff mbox series

[FFmpeg-devel,6/7] avfilter/graphparser: Avoid check whose result is known in advance

Message ID 20200823095039.18851-3-andreas.rheinhardt@gmail.com
State New
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
The result of the last check in code like

if (p)
    foo
else
    p = av_mallocz(sizeof(*p));

if (p->ptr)
    bar
else
    bar2

is known in advance if the else branch of the first check was taken
because av_mallocz() returns zeroed buffers. Therefore the above snippet
can be simplified by moving the check and bar into the foo block.

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

Comments

Nicolas George Aug. 23, 2020, 10:24 a.m. UTC | #1
Andreas Rheinhardt (12020-08-23):
> The result of the last check in code like
> 
> if (p)
>     foo
> else
>     p = av_mallocz(sizeof(*p));
> 
> if (p->ptr)
>     bar
> else
>     bar2
> 
> is known in advance if the else branch of the first check was taken
> because av_mallocz() returns zeroed buffers. Therefore the above snippet
> can be simplified by moving the check and bar into the foo block.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavfilter/graphparser.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/libavfilter/graphparser.c b/libavfilter/graphparser.c
> index a52916a146..f55737f8c7 100644
> --- a/libavfilter/graphparser.c
> +++ b/libavfilter/graphparser.c
> @@ -266,20 +266,20 @@ static int link_filter_inouts(AVFilterContext *filt_ctx,
>          if (p) {
>              *curr_inputs = (*curr_inputs)->next;
>              p->next = NULL;
> +            if (p->filter_ctx) {
> +                ret = link_filter(p->filter_ctx, p->pad_idx, filt_ctx, pad, log_ctx);
> +                av_freep(&p->name);
> +                av_freep(&p);
> +                if (ret < 0)
> +                    return ret;
> +                continue;
> +            }
>          } else if (!(p = av_mallocz(sizeof(*p))))
>              return AVERROR(ENOMEM);
>  
> -        if (p->filter_ctx) {
> -            ret = link_filter(p->filter_ctx, p->pad_idx, filt_ctx, pad, log_ctx);
> -            av_freep(&p->name);
> -            av_freep(&p);
> -            if (ret < 0)
> -                return ret;
> -        } else {
> -            p->filter_ctx = filt_ctx;
> -            p->pad_idx = pad;
> -            append_inout(open_inputs, &p);
> -        }
> +        p->filter_ctx = filt_ctx;
> +        p->pad_idx = pad;
> +        append_inout(open_inputs, &p);
>      }
>  
>      if (*curr_inputs) {

I think I like the current version better. I had to read three times
before I noticed the "continue" that guarantees the last three lines
will not overwrite an existing filter. That means somebody could add a
line there, expecting it to be valid for both cases.

There is probably more simplification to be gained by reworking the
whole logic into a more modular structure, but that is a task for later.

Regards,
diff mbox series

Patch

diff --git a/libavfilter/graphparser.c b/libavfilter/graphparser.c
index a52916a146..f55737f8c7 100644
--- a/libavfilter/graphparser.c
+++ b/libavfilter/graphparser.c
@@ -266,20 +266,20 @@  static int link_filter_inouts(AVFilterContext *filt_ctx,
         if (p) {
             *curr_inputs = (*curr_inputs)->next;
             p->next = NULL;
+            if (p->filter_ctx) {
+                ret = link_filter(p->filter_ctx, p->pad_idx, filt_ctx, pad, log_ctx);
+                av_freep(&p->name);
+                av_freep(&p);
+                if (ret < 0)
+                    return ret;
+                continue;
+            }
         } else if (!(p = av_mallocz(sizeof(*p))))
             return AVERROR(ENOMEM);
 
-        if (p->filter_ctx) {
-            ret = link_filter(p->filter_ctx, p->pad_idx, filt_ctx, pad, log_ctx);
-            av_freep(&p->name);
-            av_freep(&p);
-            if (ret < 0)
-                return ret;
-        } else {
-            p->filter_ctx = filt_ctx;
-            p->pad_idx = pad;
-            append_inout(open_inputs, &p);
-        }
+        p->filter_ctx = filt_ctx;
+        p->pad_idx = pad;
+        append_inout(open_inputs, &p);
     }
 
     if (*curr_inputs) {