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

[Limin Wang]

On Tue, Dec 24, 2019 at 01:46:00AM +0000, Andreas Rheinhardt wrote:
> 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.

Yes, please ignore my second patch.

> 
> But this relies on undocumented behaviour of av_realloc_array. Maybe
> it should be documented?
> 
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".