| Message ID | 20200806080416.17691-3-guangxin.xu@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | avformat/av1dec: add low overhead obu demux | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On 8/6/2020 5:04 AM, Xu Guangxin wrote: > for low overhead obu, we can't forsee the obu size. we can only get it > when we parsed the obu header. > --- > libavcodec/av1_parse.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/av1_parse.h b/libavcodec/av1_parse.h > index a3b39f039c..823bdedd5e 100644 > --- a/libavcodec/av1_parse.h > +++ b/libavcodec/av1_parse.h > @@ -135,7 +135,7 @@ static inline int parse_obu_header(const uint8_t *buf, int buf_size, > > size = *obu_size + *start_pos; > > - if (size > buf_size) > + if (!*has_size_flag && size > buf_size) This check was added in c27c7b49dc to fix out of array reads, so this change will surely reintroduce the issue. Also, when has_size_flag is 0, size will never be bigger than buf_size because it will be derived from it, meaning this change is the same as removing the check altogether. > return AVERROR_INVALIDDATA; > > return size; >
Hi James,
Thanks for the review.
How about we add a new function in av1dec.c like this:
static inline int read_obu_header_with_size_flag(const uint8_t *buf, int buf_size,
int64_t *obu_size, int *type);
then we can remove first two patches and check has_size_flag in the function.
I guess "out of array reads" will not happen in low overhead obu, since it always prepare enough data the obu.
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James
> Almer
> Sent: Thursday, August 6, 2020 10:03 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 2/5] av1_parser: do not check buf_size if
> we have size in obu header
>
> On 8/6/2020 5:04 AM, Xu Guangxin wrote:
> > for low overhead obu, we can't forsee the obu size. we can only get it
> > when we parsed the obu header.
> > ---
> > libavcodec/av1_parse.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/av1_parse.h b/libavcodec/av1_parse.h index
> > a3b39f039c..823bdedd5e 100644
> > --- a/libavcodec/av1_parse.h
> > +++ b/libavcodec/av1_parse.h
> > @@ -135,7 +135,7 @@ static inline int parse_obu_header(const uint8_t
> > *buf, int buf_size,
> >
> > size = *obu_size + *start_pos;
> >
> > - if (size > buf_size)
> > + if (!*has_size_flag && size > buf_size)
>
> This check was added in c27c7b49dc to fix out of array reads, so this change will
> surely reintroduce the issue.
>
> Also, when has_size_flag is 0, size will never be bigger than buf_size because it
> will be derived from it, meaning this change is the same as removing the check
> altogether.
>
> > return AVERROR_INVALIDDATA;
> >
> > return size;
> >
>
> _______________________________________________
> 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 8/6/2020 12:15 PM, Xu, Guangxin wrote: > Hi James, > Thanks for the review. > How about we add a new function in av1dec.c like this: > > static inline int read_obu_header_with_size_flag(const uint8_t *buf, int buf_size, > int64_t *obu_size, int *type); > then we can remove first two patches and check has_size_flag in the function. You mean duplicating the implementation of parse_obu_header() in av1dec.c? > > I guess "out of array reads" will not happen in low overhead obu, since it always prepare enough data the obu. > > >> -----Original Message----- >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James >> Almer >> Sent: Thursday, August 6, 2020 10:03 PM >> To: ffmpeg-devel@ffmpeg.org >> Subject: Re: [FFmpeg-devel] [PATCH 2/5] av1_parser: do not check buf_size if >> we have size in obu header >> >> On 8/6/2020 5:04 AM, Xu Guangxin wrote: >>> for low overhead obu, we can't forsee the obu size. we can only get it >>> when we parsed the obu header. >>> --- >>> libavcodec/av1_parse.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/av1_parse.h b/libavcodec/av1_parse.h index >>> a3b39f039c..823bdedd5e 100644 >>> --- a/libavcodec/av1_parse.h >>> +++ b/libavcodec/av1_parse.h >>> @@ -135,7 +135,7 @@ static inline int parse_obu_header(const uint8_t >>> *buf, int buf_size, >>> >>> size = *obu_size + *start_pos; >>> >>> - if (size > buf_size) >>> + if (!*has_size_flag && size > buf_size) >> >> This check was added in c27c7b49dc to fix out of array reads, so this change will >> surely reintroduce the issue. >> >> Also, when has_size_flag is 0, size will never be bigger than buf_size because it >> will be derived from it, meaning this change is the same as removing the check >> altogether. >> >>> return AVERROR_INVALIDDATA; >>> >>> return size; >>> >> >> _______________________________________________ >> 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". >
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James > Almer > Sent: Friday, August 7, 2020 1:38 AM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH 2/5] av1_parser: do not check buf_size if > we have size in obu header > > On 8/6/2020 12:15 PM, Xu, Guangxin wrote: > > Hi James, > > Thanks for the review. > > How about we add a new function in av1dec.c like this: > > > > static inline int read_obu_header_with_size_flag(const uint8_t *buf, int > buf_size, > > int64_t *obu_size, int *type); then > > we can remove first two patches and check has_size_flag in the function. > > You mean duplicating the implementation of parse_obu_header() in av1dec.c? Yes. if we do not duplicate code. it's hard to fix the issue you mentioned. Low overhead obu can not foresee the obu size. it need get size from the header. > > > > > I guess "out of array reads" will not happen in low overhead obu, since it > always prepare enough data the obu. > > > > > >> -----Original Message----- > >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > >> James Almer > >> Sent: Thursday, August 6, 2020 10:03 PM > >> To: ffmpeg-devel@ffmpeg.org > >> Subject: Re: [FFmpeg-devel] [PATCH 2/5] av1_parser: do not check > >> buf_size if we have size in obu header > >> > >> On 8/6/2020 5:04 AM, Xu Guangxin wrote: > >>> for low overhead obu, we can't forsee the obu size. we can only get > >>> it when we parsed the obu header. > >>> --- > >>> libavcodec/av1_parse.h | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/libavcodec/av1_parse.h b/libavcodec/av1_parse.h index > >>> a3b39f039c..823bdedd5e 100644 > >>> --- a/libavcodec/av1_parse.h > >>> +++ b/libavcodec/av1_parse.h > >>> @@ -135,7 +135,7 @@ static inline int parse_obu_header(const uint8_t > >>> *buf, int buf_size, > >>> > >>> size = *obu_size + *start_pos; > >>> > >>> - if (size > buf_size) > >>> + if (!*has_size_flag && size > buf_size) > >> > >> This check was added in c27c7b49dc to fix out of array reads, so this > >> change will surely reintroduce the issue. > >> > >> Also, when has_size_flag is 0, size will never be bigger than > >> buf_size because it will be derived from it, meaning this change is > >> the same as removing the check altogether. > >> > >>> return AVERROR_INVALIDDATA; > >>> > >>> return size; > >>> > >> > >> _______________________________________________ > >> 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". > > > > _______________________________________________ > 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 8/6/2020 10:25 PM, Xu, Guangxin wrote: > >> -----Original Message----- >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James >> Almer >> Sent: Friday, August 7, 2020 1:38 AM >> To: ffmpeg-devel@ffmpeg.org >> Subject: Re: [FFmpeg-devel] [PATCH 2/5] av1_parser: do not check buf_size if >> we have size in obu header >> >> On 8/6/2020 12:15 PM, Xu, Guangxin wrote: >>> Hi James, >>> Thanks for the review. >>> How about we add a new function in av1dec.c like this: >>> >>> static inline int read_obu_header_with_size_flag(const uint8_t *buf, int >> buf_size, >>> int64_t *obu_size, int *type); then >>> we can remove first two patches and check has_size_flag in the function. >> >> You mean duplicating the implementation of parse_obu_header() in av1dec.c? > > Yes. if we do not duplicate code. it's hard to fix the issue you mentioned. > Low overhead obu can not foresee the obu size. it need get size from the header. Alright. Just make sure to add a comment to the relevant function in av1dec.c explaining why parse_obu_header() is not being used instead.
diff --git a/libavcodec/av1_parse.h b/libavcodec/av1_parse.h index a3b39f039c..823bdedd5e 100644 --- a/libavcodec/av1_parse.h +++ b/libavcodec/av1_parse.h @@ -135,7 +135,7 @@ static inline int parse_obu_header(const uint8_t *buf, int buf_size, size = *obu_size + *start_pos; - if (size > buf_size) + if (!*has_size_flag && size > buf_size) return AVERROR_INVALIDDATA; return size;