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

[Andreas Rheinhardt]

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