[FFmpeg-devel] [PATCH v1] avutil/frame: Use av_realloc_array()

[Andreas Rheinhardt]

Limin Wang:
> On Mon, Dec 23, 2019 at 10:20:37PM -0300, James Almer wrote:
>> On 12/23/2019 8:32 PM, Michael Niedermayer wrote:
>>> On Mon, Dec 23, 2019 at 10:48:13PM +0800, lance.lmwang at gmail.com wrote:
>>>> From: Limin Wang <lance.lmwang at gmail.com>
>>>>
>>>> Signed-off-by: Limin Wang <lance.lmwang at gmail.com>
>>>> ---
>>>>  libavutil/frame.c | 7 ++-----
>>>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/libavutil/frame.c b/libavutil/frame.c
>>>> index 1d0faec687..0a1ba877cc 100644
>>>> --- a/libavutil/frame.c
>>>> +++ b/libavutil/frame.c
>>>> @@ -696,11 +696,8 @@ AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame,
>>>>      if (!buf)
>>>>          return NULL;
>>>>  
>>>> -    if (frame->nb_side_data > INT_MAX / sizeof(*frame->side_data) - 1)
>>>> -        return NULL;
>>>> -
>>>> -    tmp = av_realloc(frame->side_data,
>>>> -                     (frame->nb_side_data + 1) * sizeof(*frame->side_data));
>>>> +    tmp = av_realloc_array(frame->side_data,
>>>> +                     (frame->nb_side_data + 1), sizeof(*frame->side_data));
>>>
>>> does something prevent "frame->nb_side_data + 1" from overflowing ?
>>
>> av_realloc_array() is called with x + 1 as nmemb argument in several
>> places. It checks for "nmemb >= INT_MAX / size", so it will never
>> overflow with a buffer that increases by one element at a time (It would
>> if the check was > alone).
> 
> I think about it, in case nb_side_data is INT_MAX, then frame->nb_side_data + 1 will overflow already
> before enter av_realloc_array, so I add check for this overflow only.
> 
When frame->nb_side_data is INT_MAX - 1, you request to realloc the
array to INT_MAX members. And because of the check James mentioned
this allocation will already fail, hence frame->nb_side_data can never
become INT_MAX (unless it is also set somewhere else in the code). So
no overflow check is necessary in the caller as long as the size of
the array is only increased in steps of 1.

But this relies on undocumented behaviour of av_realloc_array. Maybe
it should be documented?

- Andreas