| Message ID | 20191223144813.25900-1-lance.lmwang@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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 [...]
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".
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". >
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".
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
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".
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 --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;