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

[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

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

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

/Tomas