[FFmpeg-devel] [PATCH 01/35] lavu/fifo: disallow overly large fifo sizes

[Andreas Rheinhardt]

Anton Khirnov:
> The API currently allows creating FIFOs up to
> - UINT_MAX: av_fifo_alloc(), av_fifo_realloc(), av_fifo_grow()
> - SIZE_MAX: av_fifo_alloc_array()
> However the usable limit is determined by
> - rndx/wndx being uint32_t
> - av_fifo_[size,space] returning int
> so no FIFO should be larger than the smallest of
> - INT_MAX
> - UINT32_MAX
> - SIZE_MAX
> (which should be INT_MAX an all commonly used platforms).
> Return an error on trying to allocate FIFOs larger than this limit.
> ---
>  libavutil/fifo.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/libavutil/fifo.c b/libavutil/fifo.c
> index d741bdd395..f2f046b1f3 100644
> --- a/libavutil/fifo.c
> +++ b/libavutil/fifo.c
> @@ -20,14 +20,23 @@
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>   */
>  
> +#include <stdint.h>
> +
>  #include "avassert.h"
>  #include "common.h"
>  #include "fifo.h"
>  
> +#define FIFO_SIZE_MAX FFMIN3((uint64_t)INT_MAX, (uint64_t)UINT32_MAX, (uint64_t)SIZE_MAX)

Aren't these casts unnecessary? And actually dangerous? (They add the
implicit requirement that INT_MAX and SIZE_MAX fit into an uint64_t.)

> +
>  AVFifoBuffer *av_fifo_alloc_array(size_t nmemb, size_t size)
>  {
>      AVFifoBuffer *f;
> -    void *buffer = av_realloc_array(NULL, nmemb, size);
> +    void *buffer;
> +
> +    if (nmemb > FIFO_SIZE_MAX / size)
> +        return NULL;
> +
> +    buffer = av_realloc_array(NULL, nmemb, size);
>      if (!buffer)
>          return NULL;
>      f = av_mallocz(sizeof(AVFifoBuffer));
> @@ -82,6 +91,9 @@ int av_fifo_realloc2(AVFifoBuffer *f, unsigned int new_size)
>  {
>      unsigned int old_size = f->end - f->buffer;
>  
> +    if (new_size > FIFO_SIZE_MAX)
> +        return AVERROR(EINVAL);
> +
>      if (old_size < new_size) {
>          size_t offset_r = f->rptr - f->buffer;
>          size_t offset_w = f->wptr - f->buffer;
>