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.rheinha...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
