diff mbox

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

Message ID 20191223144813.25900-1-lance.lmwang@gmail.com
State Superseded
Headers show

Commit Message

Limin Wang Dec. 23, 2019, 2:48 p.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavutil/frame.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

Michael Niedermayer Dec. 23, 2019, 11:32 p.m. UTC | #1
On Mon, Dec 23, 2019 at 10:48:13PM +0800, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@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 ?

thx

[...]
Limin Wang Dec. 24, 2019, 12:57 a.m. UTC | #2
On Tue, Dec 24, 2019 at 12:32:07AM +0100, Michael Niedermayer wrote:
> On Mon, Dec 23, 2019 at 10:48:13PM +0800, lance.lmwang@gmail.com wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@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 ?

no, I have add the check for the overflow and update the patch.

> 
> thx
> 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Many things microsoft did are stupid, but not doing something just because
> microsoft did it is even more stupid. If everything ms did were stupid they
> would be bankrupt already.



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer Dec. 24, 2019, 1:20 a.m. UTC | #3
On 12/23/2019 8:32 PM, Michael Niedermayer wrote:
> On Mon, Dec 23, 2019 at 10:48:13PM +0800, lance.lmwang@gmail.com wrote:
>> From: Limin Wang <lance.lmwang@gmail.com>
>>
>> Signed-off-by: Limin Wang <lance.lmwang@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).

The version 2 of this patch is pointless since it adds an extra check to
the process, so if this one isn't applied then IMO none should.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Limin Wang Dec. 24, 2019, 1:38 a.m. UTC | #4
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@gmail.com wrote:
> >> From: Limin Wang <lance.lmwang@gmail.com>
> >>
> >> Signed-off-by: Limin Wang <lance.lmwang@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.

> 
> The version 2 of this patch is pointless since it adds an extra check to
> the process, so if this one isn't applied then IMO none should.
> 
> > 
> > thx
> > 
> > [...]
> > 
> > 
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Andreas Rheinhardt Dec. 24, 2019, 1:46 a.m. UTC | #5
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@gmail.com wrote:
>>>> From: Limin Wang <lance.lmwang@gmail.com>
>>>>
>>>> Signed-off-by: Limin Wang <lance.lmwang@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
Limin Wang Dec. 24, 2019, 1:59 a.m. UTC | #6
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@gmail.com wrote:
> >>>> From: Limin Wang <lance.lmwang@gmail.com>
> >>>>
> >>>> Signed-off-by: Limin Wang <lance.lmwang@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer Dec. 24, 2019, 1:11 p.m. UTC | #7
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@gmail.com wrote:
> >>>> From: Limin Wang <lance.lmwang@gmail.com>
> >>>>
> >>>> Signed-off-by: Limin Wang <lance.lmwang@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?

If it becomes documented behaviour then it would be difficult to
raise the INT_MAX limit with a bigger data type

thx

[...]
diff mbox

Patch

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));
     if (!tmp)
         return NULL;
     frame->side_data = tmp;