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

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(-)

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;
>

Quoting Andreas Rheinhardt (2022-01-13 14:59:49)
> 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?

Possibly yes. I mainly added them so that FIFO_SIZE_MAX is always the
same type, which might prevent surprises. I can drop the casts if you
think they are never necessary.

> And actually dangerous? (They add the implicit requirement that
> INT_MAX and SIZE_MAX fit into an uint64_t.)

I'll grant you _theoretically_ dangerous - I've never heard of a
posix-compliant platform with sizeof(int) > 64.

And in any case that macro should be gone with the old API (was going to
include a patch scheduling its removal, but apparently forgot - will
send it later).

Anton Khirnov:
> Quoting Andreas Rheinhardt (2022-01-13 14:59:49)
>> 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?
> 
> Possibly yes. I mainly added them so that FIFO_SIZE_MAX is always the
> same type, which might prevent surprises. I can drop the casts if you
> think they are never necessary.
> 

FIFO_SIZE is the type that results after integer promotions of an
expression involving an int, an uint32_t and a size_t parameter; all
three constants are representable in it and FIFO_SIZE is representable
in an int, an uint32_t and size_t. Given that it denotes a size I think
you should cast it to size_t if you want to avoid surprises.

>> And actually dangerous? (They add the implicit requirement that
>> INT_MAX and SIZE_MAX fit into an uint64_t.)
> 
> I'll grant you _theoretically_ dangerous - I've never heard of a
> posix-compliant platform with sizeof(int) > 64.

sizeof(int) > 8 you mean? (Or even better: UINT_MAX > UINT64_MAX.)

> 
> And in any case that macro should be gone with the old API (was going to
> include a patch scheduling its removal, but apparently forgot - will
> send it later).
>