[FFmpeg-devel] [PATCH 1/8] avutil/mem: Handle fast allocations near UINT_MAX properly

[Andreas Rheinhardt]

Tomas Härdin:
> tis 2022-07-05 klockan 22:09 +0200 skrev Andreas Rheinhardt:
>> av_fast_realloc and av_fast_mallocz? store the size of
>> the objects they allocate in an unsigned. Yet they overallocate
>> and currently they can allocate more than UINT_MAX bytes
>> in case a user has requested a size of about UINT_MAX * 16 / 17
>> or more if SIZE_MAX > UINT_MAX.
> 
> I think you mean if max_alloc_size > UINT_MAX
> 

Both are correct. I should probably add a note to the commit message
that this whole issue can only be encountered if one has increased the
allocation limit by calling av_max_alloc() before that.

>> In this case it is impossible
>> to store the true size of the buffer via the unsigned*;
>> future requests are likely to use the (re)allocation codepath
>> even if the buffer is actually large enough because of
>> the incorrect size.
>>
>> Fix this by ensuring that the actually allocated size
>> always fits into an unsigned. (This entails erroring out
>> in case the user requested more than UINT_MAX.)
> 
> Who decided unsigned was a good idea in these functions anyway?
> 

git log will tell you.

>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>> ---
>>  libavutil/mem.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/libavutil/mem.c b/libavutil/mem.c
>> index a0c9a42849..18aff5291f 100644
>> --- a/libavutil/mem.c
>> +++ b/libavutil/mem.c
>> @@ -510,6 +510,8 @@ void *av_fast_realloc(void *ptr, unsigned int
>> *size, size_t min_size)
>>          return ptr;
>>  
>>      max_size = atomic_load_explicit(&max_alloc_size,
>> memory_order_relaxed);
>> +    /* *size is an unsigned, so the real maximum is <= UINT_MAX. */
>> +    max_size = FFMIN(max_size, UINT_MAX);
>>  
>>      if (min_size > max_size) {
>>          *size = 0;
>> @@ -542,6 +544,8 @@ static inline void fast_malloc(void *ptr,
>> unsigned int *size, size_t min_size, i
>>      }
>>  
>>      max_size = atomic_load_explicit(&max_alloc_size,
>> memory_order_relaxed);
>> +    /* *size is an unsigned, so the real maximum is <= UINT_MAX. */
>> +    max_size = FFMIN(max_size, UINT_MAX);
>>  
>>      if (min_size > max_size) {
>>          av_freep(ptr);
> 
> Looks OK. This is also why I decided to do formal verification on my
> av_fast_recalloc() patch. I only verify part of it, so it's vulnerable
> to this also.
> 
> This is inspiring me to rework my patch to use size_t instead of
> unsigned for *size

See also 3/8.

- Andreas