Re: [libav-devel] [PATCH 3/4] drawtext: Set an explicit allocation limit

[Anton Khirnov]

Quoting Luca Barbato (2015-08-01 11:22:12)
> And make sure to not have dangling pointers.
> ---
>  libavfilter/vf_drawtext.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c
> index 15c994f..62993cf 100644
> --- a/libavfilter/vf_drawtext.c
> +++ b/libavfilter/vf_drawtext.c
> @@ -524,28 +524,31 @@ static inline int is_newline(uint32_t c)
>  static int expand_strftime(DrawTextContext *s)
>  {
>      struct tm ltime;
> -    time_t now   = time(0);
> -    uint8_t *buf = s->expanded_text;
> -    int buf_size = s->expanded_text_size;
> +    time_t now       = time(0);
> +    uint8_t *buf     = s->expanded_text;
> +    int64_t buf_size = s->expanded_text_size;

Why signed?

> +    int ret;
>  
>      if (!buf)
>          buf_size = 2 * strlen(s->text) + 1;
>  
>      localtime_r(&now, &ltime);
>  
> -    while ((buf = av_realloc(buf, buf_size))) {
> +    while ((ret = av_reallocp(&buf, buf_size)) >= 0) {
>          *buf = 1;
>          if (strftime(buf, buf_size, s->text, &ltime) != 0 || *buf == 0)
>              break;
>          buf_size *= 2;
> +        if (buf_size > SIZE_MAX) {
> +            ret = AVERROR(EINVAL);
> +            break;
> +        }

I think it'd be better to a standard overflow check before modifying
buf_size and not assume some relationships between size_t and int64.

>      }
>  
> -    if (!buf)
> -        return AVERROR(ENOMEM);
>      s->expanded_text      = buf;
>      s->expanded_text_size = buf_size;

When buf is NULL, buf_size is still a random number. Better to
explicitly write zero on failure here.

-- 
Anton Khirnov
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel