diff mbox series

[FFmpeg-devel,2/5] av1_parser: do not check buf_size if we have size in obu header

Message ID 20200806080416.17691-3-guangxin.xu@intel.com
State Changes Requested
Headers show
Series avformat/av1dec: add low overhead obu demux
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Xu Guangxin Aug. 6, 2020, 8:04 a.m. UTC
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(-)

Comments

James Almer Aug. 6, 2020, 2:03 p.m. UTC | #1
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;
>
Xu Guangxin Aug. 6, 2020, 3:15 p.m. UTC | #2
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".
James Almer Aug. 6, 2020, 5:38 p.m. UTC | #3
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".
>
Xu Guangxin Aug. 7, 2020, 1:25 a.m. UTC | #4
> -----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".
James Almer Aug. 7, 2020, 1:27 a.m. UTC | #5
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 mbox series

Patch

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;