diff mbox series

[FFmpeg-devel,2/3] avfilter/graphparser: Don't set pointer to one beyond '\0' of string

Message ID 20200822230434.11347-2-andreas.rheinhardt@gmail.com
State Accepted
Commit f33faa5b9bfb288f83db034fa1f8719ab8a994c6
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. 22, 2020, 11:04 p.m. UTC
This happened in parse_link_name() if there was a '[' without matching
']'. While this is not undefined behaviour (pointer arithmetic one
beyond the end of an array works fine as long as there are no accesses),
it is potentially dangerous. It currently isn't (all callers of
parse_link_name() treat this as an error and don't access the string any
more), but making sure that this will never cause trouble in the future
seems nevertheless worthwhile.

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

Comments

Nicolas George Aug. 23, 2020, 10:05 a.m. UTC | #1
Andreas Rheinhardt (12020-08-23):
> This happened in parse_link_name() if there was a '[' without matching
> ']'. While this is not undefined behaviour (pointer arithmetic one
> beyond the end of an array works fine as long as there are no accesses),
> it is potentially dangerous. It currently isn't (all callers of
> parse_link_name() treat this as an error and don't access the string any
> more), but making sure that this will never cause trouble in the future
> seems nevertheless worthwhile.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavfilter/graphparser.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/libavfilter/graphparser.c b/libavfilter/graphparser.c
> index dfb94788e1..e96b20418e 100644
> --- a/libavfilter/graphparser.c
> +++ b/libavfilter/graphparser.c
> @@ -63,7 +63,7 @@ static char *parse_link_name(const char **buf, void *log_ctx)
>  
>      name = av_get_token(buf, "]");
>      if (!name)
> -        goto fail;
> +        return NULL;

This looks ok.

>  
>      if (!name[0]) {
>          av_log(log_ctx, AV_LOG_ERROR,
> @@ -71,12 +71,14 @@ static char *parse_link_name(const char **buf, void *log_ctx)
>          goto fail;
>      }
>  
> -    if (*(*buf)++ != ']') {
> +    if (**buf != ']') {
>          av_log(log_ctx, AV_LOG_ERROR,
>                 "Mismatched '[' found in the following: \"%s\".\n", start);
>      fail:
>          av_freep(&name);
> +        return NULL;
>      }
> +    (*buf)++;
>  
>      return name;
>  }

I would like it better if you took the opportunity to get rid of this
spaghetti goto. A fail label in an if that uses the side effect of
av_freep() to make sure the final return will return NULL? Brr, that's
bound to come back and bite us at some time.

Regards,
diff mbox series

Patch

diff --git a/libavfilter/graphparser.c b/libavfilter/graphparser.c
index dfb94788e1..e96b20418e 100644
--- a/libavfilter/graphparser.c
+++ b/libavfilter/graphparser.c
@@ -63,7 +63,7 @@  static char *parse_link_name(const char **buf, void *log_ctx)
 
     name = av_get_token(buf, "]");
     if (!name)
-        goto fail;
+        return NULL;
 
     if (!name[0]) {
         av_log(log_ctx, AV_LOG_ERROR,
@@ -71,12 +71,14 @@  static char *parse_link_name(const char **buf, void *log_ctx)
         goto fail;
     }
 
-    if (*(*buf)++ != ']') {
+    if (**buf != ']') {
         av_log(log_ctx, AV_LOG_ERROR,
                "Mismatched '[' found in the following: \"%s\".\n", start);
     fail:
         av_freep(&name);
+        return NULL;
     }
+    (*buf)++;
 
     return name;
 }