[FFmpeg-devel,1/2] avcodec/vp9: Check in decode_tiles() if there is data remaining

Submitted by Michael Niedermayer on Aug. 5, 2018, 1:16 a.m.

Details

Message ID 20180805011642.5290-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Aug. 5, 2018, 1:16 a.m.
Fixes: Timeout
Fixes: 9330/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP9_fuzzer-5707345857347584

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/vp9.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Ronald S. Bultje Aug. 6, 2018, 5:05 p.m.
Hi,

On Sun, Aug 5, 2018, 9:17 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> Fixes: Timeout
> Fixes:
> 9330/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP9_fuzzer-5707345857347584
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by
> <https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by>:
> Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/vp9.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> index b1178c9c0c..4ca51ec108 100644
> --- a/libavcodec/vp9.c
> +++ b/libavcodec/vp9.c
> @@ -1302,6 +1302,9 @@ static int decode_tiles(AVCodecContext *avctx,
>                          memset(lflvl_ptr->mask, 0,
> sizeof(lflvl_ptr->mask));
>                      }
>
> +                    if (td->c->end <= td->c->buffer && td->c->bits >= 0) {
> +                        return AVERROR_INVALIDDATA;
> +                    }
>                      if (s->pass == 2) {
>                          decode_sb_mem(td, row, col, lflvl_ptr,
>                                        yoff2, uvoff2, BL_64X64);
>

I don't think this matches spec. Implementations could use this to store
auxiliary data.

Ronald

>
Michael Niedermayer Aug. 6, 2018, 7 p.m.
On Tue, Aug 07, 2018 at 01:05:51AM +0800, Ronald S. Bultje wrote:
> Hi,
> 
> On Sun, Aug 5, 2018, 9:17 AM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > Fixes: Timeout
> > Fixes:
> > 9330/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP9_fuzzer-5707345857347584
> >
> > Found-by: continuous fuzzing process
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by
> > <https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by>:
> > Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/vp9.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> > index b1178c9c0c..4ca51ec108 100644
> > --- a/libavcodec/vp9.c
> > +++ b/libavcodec/vp9.c
> > @@ -1302,6 +1302,9 @@ static int decode_tiles(AVCodecContext *avctx,
> >                          memset(lflvl_ptr->mask, 0,
> > sizeof(lflvl_ptr->mask));
> >                      }
> >
> > +                    if (td->c->end <= td->c->buffer && td->c->bits >= 0) {
> > +                        return AVERROR_INVALIDDATA;
> > +                    }
> >                      if (s->pass == 2) {
> >                          decode_sb_mem(td, row, col, lflvl_ptr,
> >                                        yoff2, uvoff2, BL_64X64);
> >
> 
> I don't think this matches spec. Implementations could use this to store
> auxiliary data.

This checks, or rather is intended to check for a premature end of the input.
Am i missing something? because a premature end of input seems not to lead
to more (auxiliary or other) data in the input.

Of course in principle an encoder could use this and truncate the stream
if the result still matches. Is this allowed in the spec ?

Also i think this if() would be clearer with an error message or some
comment, for example it would have been clear that this is about a end
of input and not unknown additional input. But i omited the message as you
didnt like error messages in similar cases.

Thanks

[...]
Ronald S. Bultje Aug. 7, 2018, 1:50 a.m.
Hi,

On Mon, Aug 6, 2018 at 3:00 PM, Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Tue, Aug 07, 2018 at 01:05:51AM +0800, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Sun, Aug 5, 2018, 9:17 AM Michael Niedermayer <michael@niedermayer.cc
> >
> > wrote:
> >
> > > Fixes: Timeout
> > > Fixes:
> > > 9330/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_
> VP9_fuzzer-5707345857347584
> > >
> > > Found-by: continuous fuzzing process
> > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by
> > > <https://github.com/google/oss-fuzz/tree/master/projects/
> ffmpegSigned-off-by>:
> > > Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/vp9.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> > > index b1178c9c0c..4ca51ec108 100644
> > > --- a/libavcodec/vp9.c
> > > +++ b/libavcodec/vp9.c
> > > @@ -1302,6 +1302,9 @@ static int decode_tiles(AVCodecContext *avctx,
> > >                          memset(lflvl_ptr->mask, 0,
> > > sizeof(lflvl_ptr->mask));
> > >                      }
> > >
> > > +                    if (td->c->end <= td->c->buffer && td->c->bits >=
> 0) {
> > > +                        return AVERROR_INVALIDDATA;
> > > +                    }
> > >                      if (s->pass == 2) {
> > >                          decode_sb_mem(td, row, col, lflvl_ptr,
> > >                                        yoff2, uvoff2, BL_64X64);
> > >
> >
> > I don't think this matches spec. Implementations could use this to store
> > auxiliary data.
>
> This checks, or rather is intended to check for a premature end of the
> input.
> Am i missing something? because a premature end of input seems not to lead
> to more (auxiliary or other) data in the input.


Hm, I misread it, sorry about that, my bad. Please ignore my first review.

Is end == buffer && bits == 0 something that can happen on valid input if
things just align properly? Otherwise looks OK.

Ronald
Michael Niedermayer Aug. 7, 2018, 11:15 p.m.
On Mon, Aug 06, 2018 at 09:50:57PM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Mon, Aug 6, 2018 at 3:00 PM, Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Tue, Aug 07, 2018 at 01:05:51AM +0800, Ronald S. Bultje wrote:
> > > Hi,
> > >
> > > On Sun, Aug 5, 2018, 9:17 AM Michael Niedermayer <michael@niedermayer.cc
> > >
> > > wrote:
> > >
> > > > Fixes: Timeout
> > > > Fixes:
> > > > 9330/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_
> > VP9_fuzzer-5707345857347584
> > > >
> > > > Found-by: continuous fuzzing process
> > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by
> > > > <https://github.com/google/oss-fuzz/tree/master/projects/
> > ffmpegSigned-off-by>:
> > > > Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavcodec/vp9.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> > > > index b1178c9c0c..4ca51ec108 100644
> > > > --- a/libavcodec/vp9.c
> > > > +++ b/libavcodec/vp9.c
> > > > @@ -1302,6 +1302,9 @@ static int decode_tiles(AVCodecContext *avctx,
> > > >                          memset(lflvl_ptr->mask, 0,
> > > > sizeof(lflvl_ptr->mask));
> > > >                      }
> > > >
> > > > +                    if (td->c->end <= td->c->buffer && td->c->bits >=
> > 0) {
> > > > +                        return AVERROR_INVALIDDATA;
> > > > +                    }
> > > >                      if (s->pass == 2) {
> > > >                          decode_sb_mem(td, row, col, lflvl_ptr,
> > > >                                        yoff2, uvoff2, BL_64X64);
> > > >
> > >
> > > I don't think this matches spec. Implementations could use this to store
> > > auxiliary data.
> >
> > This checks, or rather is intended to check for a premature end of the
> > input.
> > Am i missing something? because a premature end of input seems not to lead
> > to more (auxiliary or other) data in the input.
> 
> 
> Hm, I misread it, sorry about that, my bad. Please ignore my first review.
> 

> Is end == buffer && bits == 0 something that can happen on valid input if
> things just align properly? Otherwise looks OK.

The same condition is used in vp5/6/8.
I think this condition only occurs if the input ends. The part i do not know
is if such premature ends might be a "valid compression"

Either way, if this check misbehaves for a valid file then it should be
reverted/removed unless its improv-able and improved.


[...]
Ronald S. Bultje Aug. 8, 2018, 1:07 a.m.
Hi,

On Tue, Aug 7, 2018 at 7:15 PM, Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Mon, Aug 06, 2018 at 09:50:57PM -0400, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Mon, Aug 6, 2018 at 3:00 PM, Michael Niedermayer
> <michael@niedermayer.cc>
> > wrote:
> >
> > > On Tue, Aug 07, 2018 at 01:05:51AM +0800, Ronald S. Bultje wrote:
> > > > Hi,
> > > >
> > > > On Sun, Aug 5, 2018, 9:17 AM Michael Niedermayer
> <michael@niedermayer.cc
> > > >
> > > > wrote:
> > > >
> > > > > Fixes: Timeout
> > > > > Fixes:
> > > > > 9330/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_
> > > VP9_fuzzer-5707345857347584
> > > > >
> > > > > Found-by: continuous fuzzing process
> > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > Signed-off-by
> > > > > <https://github.com/google/oss-fuzz/tree/master/projects/
> > > ffmpegSigned-off-by>:
> > > > > Michael Niedermayer <michael@niedermayer.cc>
> > > > > ---
> > > > >  libavcodec/vp9.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> > > > > index b1178c9c0c..4ca51ec108 100644
> > > > > --- a/libavcodec/vp9.c
> > > > > +++ b/libavcodec/vp9.c
> > > > > @@ -1302,6 +1302,9 @@ static int decode_tiles(AVCodecContext
> *avctx,
> > > > >                          memset(lflvl_ptr->mask, 0,
> > > > > sizeof(lflvl_ptr->mask));
> > > > >                      }
> > > > >
> > > > > +                    if (td->c->end <= td->c->buffer &&
> td->c->bits >=
> > > 0) {
> > > > > +                        return AVERROR_INVALIDDATA;
> > > > > +                    }
> > > > >                      if (s->pass == 2) {
> > > > >                          decode_sb_mem(td, row, col, lflvl_ptr,
> > > > >                                        yoff2, uvoff2, BL_64X64);
> > > > >
> > > >
> > > > I don't think this matches spec. Implementations could use this to
> store
> > > > auxiliary data.
> > >
> > > This checks, or rather is intended to check for a premature end of the
> > > input.
> > > Am i missing something? because a premature end of input seems not to
> lead
> > > to more (auxiliary or other) data in the input.
> >
> >
> > Hm, I misread it, sorry about that, my bad. Please ignore my first
> review.
> >
>
> > Is end == buffer && bits == 0 something that can happen on valid input if
> > things just align properly? Otherwise looks OK.
>
> The same condition is used in vp5/6/8.
> I think this condition only occurs if the input ends. The part i do not
> know
> is if such premature ends might be a "valid compression"
>
> Either way, if this check misbehaves for a valid file then it should be
> reverted/removed unless its improv-able and improved.


<sarcasm> Yes, that's how production grade software works. </sarcasm> Can
you just make it not error out on the end == buffer && bits == 0 condition?
Or does that somehow not fix your timeout?

(Vp5/6 aren't used anywhere so nobody cares. Ffvp9 is used by e.g. Firefox
for Youtube playback so if it breaks 0.01% of files, we're going to
seriously screw a massive number of users.)

Ronald
Michael Niedermayer Aug. 9, 2018, 12:30 a.m.
On Tue, Aug 07, 2018 at 09:07:11PM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Tue, Aug 7, 2018 at 7:15 PM, Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Mon, Aug 06, 2018 at 09:50:57PM -0400, Ronald S. Bultje wrote:
> > > Hi,
> > >
> > > On Mon, Aug 6, 2018 at 3:00 PM, Michael Niedermayer
> > <michael@niedermayer.cc>
> > > wrote:
> > >
> > > > On Tue, Aug 07, 2018 at 01:05:51AM +0800, Ronald S. Bultje wrote:
> > > > > Hi,
> > > > >
> > > > > On Sun, Aug 5, 2018, 9:17 AM Michael Niedermayer
> > <michael@niedermayer.cc
> > > > >
> > > > > wrote:
> > > > >
> > > > > > Fixes: Timeout
> > > > > > Fixes:
> > > > > > 9330/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_
> > > > VP9_fuzzer-5707345857347584
> > > > > >
> > > > > > Found-by: continuous fuzzing process
> > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > Signed-off-by
> > > > > > <https://github.com/google/oss-fuzz/tree/master/projects/
> > > > ffmpegSigned-off-by>:
> > > > > > Michael Niedermayer <michael@niedermayer.cc>
> > > > > > ---
> > > > > >  libavcodec/vp9.c | 3 +++
> > > > > >  1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> > > > > > index b1178c9c0c..4ca51ec108 100644
> > > > > > --- a/libavcodec/vp9.c
> > > > > > +++ b/libavcodec/vp9.c
> > > > > > @@ -1302,6 +1302,9 @@ static int decode_tiles(AVCodecContext
> > *avctx,
> > > > > >                          memset(lflvl_ptr->mask, 0,
> > > > > > sizeof(lflvl_ptr->mask));
> > > > > >                      }
> > > > > >
> > > > > > +                    if (td->c->end <= td->c->buffer &&
> > td->c->bits >=
> > > > 0) {
> > > > > > +                        return AVERROR_INVALIDDATA;
> > > > > > +                    }
> > > > > >                      if (s->pass == 2) {
> > > > > >                          decode_sb_mem(td, row, col, lflvl_ptr,
> > > > > >                                        yoff2, uvoff2, BL_64X64);
> > > > > >
> > > > >
> > > > > I don't think this matches spec. Implementations could use this to
> > store
> > > > > auxiliary data.
> > > >
> > > > This checks, or rather is intended to check for a premature end of the
> > > > input.
> > > > Am i missing something? because a premature end of input seems not to
> > lead
> > > > to more (auxiliary or other) data in the input.
> > >
> > >
> > > Hm, I misread it, sorry about that, my bad. Please ignore my first
> > review.
> > >
> >
> > > Is end == buffer && bits == 0 something that can happen on valid input if
> > > things just align properly? Otherwise looks OK.
> >
> > The same condition is used in vp5/6/8.
> > I think this condition only occurs if the input ends. The part i do not
> > know
> > is if such premature ends might be a "valid compression"
> >
> > Either way, if this check misbehaves for a valid file then it should be
> > reverted/removed unless its improv-able and improved.
> 
> 

> <sarcasm> Yes, that's how production grade software works. </sarcasm> 

yes ;)
but seriously, It only needs a single user hitting a bug and reporting it
for us to know its a issue and revert. This is not in a release just git
master.
Its my oppinion that this is wiser than to never attempt to fix the issue.
Which ultimatly is the alternative. That is unless you know that this is
correct or incorrect for all cases.
Maybe we have very differnt views of how to do software development.
My goal is to minimize the amount of issues in the code per developer time
spend. Time is a limited resource. Doing anything else simply leads to worse code.
If i spend a week testing code that i know is 99.8% right to make it
99.9% right just to save a single user from once hitting and reporting a
non security bug. That is a loss as i could have spend that week fixing
many other issues.
Its even worse if the 99.9% still hits one user and gets reverted, in this
case there is no improvment in user experience at all for the time spend.
In either the 99.8 and 99.9% cases one user ultimatly hits a file that breaks
and teh change gets reverted. but in one case we would have spend alot more
time. 

So yes, it is my philosophy to use our users as beta testers. Not as a
default no absolutely not, but in the rare cases where the extra work
we cause our users is comparably tiny compared to how much work we would
have to do to avoid it. Or if we can never really match user testing anyway
And we also did and do this all over the place. For example, if we do not
know how to handle some case and have no sample, we print a message and ask
the user for a sample. 
This is comparable to the case here. "Is there a sample out there for which this
check misbehaves" vs. "Is there a file out there that uses a special feature"
Either is a question about global file existence and search


> Can
> you just make it not error out on the end == buffer && bits == 0 condition?
> Or does that somehow not fix your timeout?

If the code continues processing "nothing" and does alot of computations on it,
it will need a long time to complete that. Thats the issue.
Its like a network router deciding to spend an hour processing a 100byte
packet that says its 4gb large in a header without there anywhere being 4gb
of input data. simple denial of service.

Thanks

[...]
James Almer Aug. 9, 2018, 1 a.m.
On 8/8/2018 9:30 PM, Michael Niedermayer wrote:
> On Tue, Aug 07, 2018 at 09:07:11PM -0400, Ronald S. Bultje wrote:
>> Hi,
>>
>> On Tue, Aug 7, 2018 at 7:15 PM, Michael Niedermayer <michael@niedermayer.cc>
>> wrote:
>>
>>> On Mon, Aug 06, 2018 at 09:50:57PM -0400, Ronald S. Bultje wrote:
>>>> Hi,
>>>>
>>>> On Mon, Aug 6, 2018 at 3:00 PM, Michael Niedermayer
>>> <michael@niedermayer.cc>
>>>> wrote:
>>>>
>>>>> On Tue, Aug 07, 2018 at 01:05:51AM +0800, Ronald S. Bultje wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Sun, Aug 5, 2018, 9:17 AM Michael Niedermayer
>>> <michael@niedermayer.cc
>>>>>>
>>>>>> wrote:
>>>>>>
>>>>>>> Fixes: Timeout
>>>>>>> Fixes:
>>>>>>> 9330/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_
>>>>> VP9_fuzzer-5707345857347584
>>>>>>>
>>>>>>> Found-by: continuous fuzzing process
>>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>>>> Signed-off-by
>>>>>>> <https://github.com/google/oss-fuzz/tree/master/projects/
>>>>> ffmpegSigned-off-by>:
>>>>>>> Michael Niedermayer <michael@niedermayer.cc>
>>>>>>> ---
>>>>>>>  libavcodec/vp9.c | 3 +++
>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
>>>>>>> index b1178c9c0c..4ca51ec108 100644
>>>>>>> --- a/libavcodec/vp9.c
>>>>>>> +++ b/libavcodec/vp9.c
>>>>>>> @@ -1302,6 +1302,9 @@ static int decode_tiles(AVCodecContext
>>> *avctx,
>>>>>>>                          memset(lflvl_ptr->mask, 0,
>>>>>>> sizeof(lflvl_ptr->mask));
>>>>>>>                      }
>>>>>>>
>>>>>>> +                    if (td->c->end <= td->c->buffer &&
>>> td->c->bits >=
>>>>> 0) {
>>>>>>> +                        return AVERROR_INVALIDDATA;
>>>>>>> +                    }
>>>>>>>                      if (s->pass == 2) {
>>>>>>>                          decode_sb_mem(td, row, col, lflvl_ptr,
>>>>>>>                                        yoff2, uvoff2, BL_64X64);
>>>>>>>
>>>>>>
>>>>>> I don't think this matches spec. Implementations could use this to
>>> store
>>>>>> auxiliary data.
>>>>>
>>>>> This checks, or rather is intended to check for a premature end of the
>>>>> input.
>>>>> Am i missing something? because a premature end of input seems not to
>>> lead
>>>>> to more (auxiliary or other) data in the input.
>>>>
>>>>
>>>> Hm, I misread it, sorry about that, my bad. Please ignore my first
>>> review.
>>>>
>>>
>>>> Is end == buffer && bits == 0 something that can happen on valid input if
>>>> things just align properly? Otherwise looks OK.
>>>
>>> The same condition is used in vp5/6/8.
>>> I think this condition only occurs if the input ends. The part i do not
>>> know
>>> is if such premature ends might be a "valid compression"
>>>
>>> Either way, if this check misbehaves for a valid file then it should be
>>> reverted/removed unless its improv-able and improved.
>>
>>
> 
>> <sarcasm> Yes, that's how production grade software works. </sarcasm> 
> 
> yes ;)
> but seriously, It only needs a single user hitting a bug and reporting it
> for us to know its a issue and revert. This is not in a release just git
> master.
> Its my oppinion that this is wiser than to never attempt to fix the issue.
> Which ultimatly is the alternative. That is unless you know that this is
> correct or incorrect for all cases.
> Maybe we have very differnt views of how to do software development.
> My goal is to minimize the amount of issues in the code per developer time
> spend. Time is a limited resource. Doing anything else simply leads to worse code.
> If i spend a week testing code that i know is 99.8% right to make it
> 99.9% right just to save a single user from once hitting and reporting a
> non security bug. That is a loss as i could have spend that week fixing
> many other issues.
> Its even worse if the 99.9% still hits one user and gets reverted, in this
> case there is no improvment in user experience at all for the time spend.
> In either the 99.8 and 99.9% cases one user ultimatly hits a file that breaks
> and teh change gets reverted. but in one case we would have spend alot more
> time. 
> 
> So yes, it is my philosophy to use our users as beta testers. Not as a
> default no absolutely not, but in the rare cases where the extra work
> we cause our users is comparably tiny compared to how much work we would
> have to do to avoid it. Or if we can never really match user testing anyway
> And we also did and do this all over the place. For example, if we do not
> know how to handle some case and have no sample, we print a message and ask
> the user for a sample. 
> This is comparable to the case here. "Is there a sample out there for which this
> check misbehaves" vs. "Is there a file out there that uses a special feature"
> Either is a question about global file existence and search

The end user is not a beta tester. A user knowingly and willingly acting
as one is a beta tester.
Firefox doesn't use git master, it uses the latest release, and that
means that a sizable bulk of users testing a wide array of vp9 streams
in order to find out if this change breaks any of them will not happen
until it's deployed to the general public.
We commit this change, we branch ffmpeg 4.1 with it, Firefox implements
it, deploys a release, and only then it will truly get tested. With luck
any such stream is detected in the beta channel by the aforementioned
beta testers, but the percentage of users testing vp9 streams there is
minimal. Chances are nothing would be found until it's effectively
deployed in the release channel and the complains that Youtube or some
other streaming service is misbehaving start pouring by the hundreds.

> 
> 
>> Can
>> you just make it not error out on the end == buffer && bits == 0 condition?
>> Or does that somehow not fix your timeout?
> 
> If the code continues processing "nothing" and does alot of computations on it,
> it will need a long time to complete that. Thats the issue.
> Its like a network router deciding to spend an hour processing a 100byte
> packet that says its 4gb large in a header without there anywhere being 4gb
> of input data. simple denial of service.

If continuing when the "end == buffer && bits == 0" condition is met is
the intended behavior as Ronald mentioned, then the theoretical case
where it could end up in a lot of time processing nothing is irrelevant.
It is intended behavior, and breaking it may have consequences with
real, valid files.

Apply this patch with changes to allow that specific condition and lets
not waste more time on this.

> 
> Thanks
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer Aug. 10, 2018, 12:49 a.m.
On Wed, Aug 08, 2018 at 10:00:42PM -0300, James Almer wrote:
> On 8/8/2018 9:30 PM, Michael Niedermayer wrote:
> > On Tue, Aug 07, 2018 at 09:07:11PM -0400, Ronald S. Bultje wrote:
> >> Hi,
> >>
> >> On Tue, Aug 7, 2018 at 7:15 PM, Michael Niedermayer <michael@niedermayer.cc>
> >> wrote:
> >>
> >>> On Mon, Aug 06, 2018 at 09:50:57PM -0400, Ronald S. Bultje wrote:
> >>>> Hi,
> >>>>
> >>>> On Mon, Aug 6, 2018 at 3:00 PM, Michael Niedermayer
> >>> <michael@niedermayer.cc>
> >>>> wrote:
> >>>>
> >>>>> On Tue, Aug 07, 2018 at 01:05:51AM +0800, Ronald S. Bultje wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On Sun, Aug 5, 2018, 9:17 AM Michael Niedermayer
> >>> <michael@niedermayer.cc
> >>>>>>
> >>>>>> wrote:
> >>>>>>
> >>>>>>> Fixes: Timeout
> >>>>>>> Fixes:
> >>>>>>> 9330/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_
> >>>>> VP9_fuzzer-5707345857347584
> >>>>>>>
> >>>>>>> Found-by: continuous fuzzing process
> >>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>>>>>> Signed-off-by
> >>>>>>> <https://github.com/google/oss-fuzz/tree/master/projects/
> >>>>> ffmpegSigned-off-by>:
> >>>>>>> Michael Niedermayer <michael@niedermayer.cc>
> >>>>>>> ---
> >>>>>>>  libavcodec/vp9.c | 3 +++
> >>>>>>>  1 file changed, 3 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> >>>>>>> index b1178c9c0c..4ca51ec108 100644
> >>>>>>> --- a/libavcodec/vp9.c
> >>>>>>> +++ b/libavcodec/vp9.c
> >>>>>>> @@ -1302,6 +1302,9 @@ static int decode_tiles(AVCodecContext
> >>> *avctx,
> >>>>>>>                          memset(lflvl_ptr->mask, 0,
> >>>>>>> sizeof(lflvl_ptr->mask));
> >>>>>>>                      }
> >>>>>>>
> >>>>>>> +                    if (td->c->end <= td->c->buffer &&
> >>> td->c->bits >=
> >>>>> 0) {
> >>>>>>> +                        return AVERROR_INVALIDDATA;
> >>>>>>> +                    }
> >>>>>>>                      if (s->pass == 2) {
> >>>>>>>                          decode_sb_mem(td, row, col, lflvl_ptr,
> >>>>>>>                                        yoff2, uvoff2, BL_64X64);
> >>>>>>>
> >>>>>>
> >>>>>> I don't think this matches spec. Implementations could use this to
> >>> store
> >>>>>> auxiliary data.
> >>>>>
> >>>>> This checks, or rather is intended to check for a premature end of the
> >>>>> input.
> >>>>> Am i missing something? because a premature end of input seems not to
> >>> lead
> >>>>> to more (auxiliary or other) data in the input.
> >>>>
> >>>>
> >>>> Hm, I misread it, sorry about that, my bad. Please ignore my first
> >>> review.
> >>>>
> >>>
> >>>> Is end == buffer && bits == 0 something that can happen on valid input if
> >>>> things just align properly? Otherwise looks OK.
> >>>
> >>> The same condition is used in vp5/6/8.
> >>> I think this condition only occurs if the input ends. The part i do not
> >>> know
> >>> is if such premature ends might be a "valid compression"
> >>>
> >>> Either way, if this check misbehaves for a valid file then it should be
> >>> reverted/removed unless its improv-able and improved.
> >>
> >>
> > 
> >> <sarcasm> Yes, that's how production grade software works. </sarcasm> 
> > 
> > yes ;)
> > but seriously, It only needs a single user hitting a bug and reporting it
> > for us to know its a issue and revert. This is not in a release just git
> > master.
> > Its my oppinion that this is wiser than to never attempt to fix the issue.
> > Which ultimatly is the alternative. That is unless you know that this is
> > correct or incorrect for all cases.
> > Maybe we have very differnt views of how to do software development.
> > My goal is to minimize the amount of issues in the code per developer time
> > spend. Time is a limited resource. Doing anything else simply leads to worse code.
> > If i spend a week testing code that i know is 99.8% right to make it
> > 99.9% right just to save a single user from once hitting and reporting a
> > non security bug. That is a loss as i could have spend that week fixing
> > many other issues.
> > Its even worse if the 99.9% still hits one user and gets reverted, in this
> > case there is no improvment in user experience at all for the time spend.
> > In either the 99.8 and 99.9% cases one user ultimatly hits a file that breaks
> > and teh change gets reverted. but in one case we would have spend alot more
> > time. 
> > 
> > So yes, it is my philosophy to use our users as beta testers. Not as a
> > default no absolutely not, but in the rare cases where the extra work
> > we cause our users is comparably tiny compared to how much work we would
> > have to do to avoid it. Or if we can never really match user testing anyway
> > And we also did and do this all over the place. For example, if we do not
> > know how to handle some case and have no sample, we print a message and ask
> > the user for a sample. 
> > This is comparable to the case here. "Is there a sample out there for which this
> > check misbehaves" vs. "Is there a file out there that uses a special feature"
> > Either is a question about global file existence and search
> 

> The end user is not a beta tester. A user knowingly and willingly acting
> as one is a beta tester.

i did not mean "beta tester" in the litteral sense of the word.
You are correct of course if you read it litterally


> Firefox doesn't use git master, it uses the latest release, and that
> means that a sizable bulk of users testing a wide array of vp9 streams
> in order to find out if this change breaks any of them will not happen
> until it's deployed to the general public.
> We commit this change, we branch ffmpeg 4.1 with it, Firefox implements
> it, deploys a release, and only then it will truly get tested. With luck
> any such stream is detected in the beta channel by the aforementioned
> beta testers, but the percentage of users testing vp9 streams there is
> minimal. Chances are nothing would be found until it's effectively
> deployed in the release channel and the complains that Youtube or some
> other streaming service is misbehaving start pouring by the hundreds.

This is a valid concern but without this fix theres one more way with which
to do a denial of service attack against firefox.


> 
> > 
> > 
> >> Can
> >> you just make it not error out on the end == buffer && bits == 0 condition?
> >> Or does that somehow not fix your timeout?
> > 
> > If the code continues processing "nothing" and does alot of computations on it,
> > it will need a long time to complete that. Thats the issue.
> > Its like a network router deciding to spend an hour processing a 100byte
> > packet that says its 4gb large in a header without there anywhere being 4gb
> > of input data. simple denial of service.
> 
> If continuing when the "end == buffer && bits == 0" condition is met is
> the intended behavior as Ronald mentioned, then the theoretical case
> where it could end up in a lot of time processing nothing is irrelevant.
> It is intended behavior, and breaking it may have consequences with
> real, valid files.
> 

> Apply this patch with changes to allow that specific condition and lets
> not waste more time on this.

this is the only change the patch does. Without it there is no patch.

Either we stop when the input ends -> that might break decoding a file
that was designed so as to expect a decoder not to stop.
or we do not stop then that allows doing denial of service

Of course we could do X iterations beyond the end. But not only has noone
suggested this, we then would have to decide on how many and why.

Its not the Implementations that is in dispute here. At least not in a
way from which i could write a different working Implementation


[...]
Ronald S. Bultje Aug. 10, 2018, 6:16 a.m.
Hi Michael,

On Thu, Aug 9, 2018 at 8:49 PM, Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Wed, Aug 08, 2018 at 10:00:42PM -0300, James Almer wrote:
>
> Apply this patch with changes to allow that specific condition and lets
> > not waste more time on this.
>
> this is the only change the patch does. Without it there is no patch.
>
> Either we stop when the input ends -> that might break decoding a file
> that was designed so as to expect a decoder not to stop.
> or we do not stop then that allows doing denial of service
>

OK, ok, hold on. I'll try to explain my problem with the patch and I will
suggest a possible solution. Please store your objections in the closet for
a second. I'm not a terrible person.

The situation you're fixing and not breaking:
let's say there is a file that is 1 byte long (8 bits), but we claim it's a
16k x 16k file. This will take ages to decode, even though it's likely
broken. Right? A one-byte file is unlikely anyway, but sure, it will run
out of data after a few symbols. I get it. I really do. And I agree that
this must be fixed. Yes.

Also, if a valid file of 1 byte (8 bits) has only 1 symbol of approximately
4 real bits, then at the end, there's still 4 bits left in the arithcoder.
So nothing breaks. Great!

My objection:
if a file has exactly symbols of 8 bits in arithdata, then after all this,
the arithcoder will signal empty and EOF, even though no error occured.
Imagine a bitcoder (non-arith) of this situation. After get_bits(gb, 8),
the data pointer will have reached the end, and the bits_left is 0, but
that does not indicate an error, quite the contrary. It just means that the
byte boundary happened to align with the exact end of the file. This can
happen.

My suggestion:
add an eof flag to the arithcoder. When we have reached the above condition
where new data is needed but not present, simply set the EOF flag, and
check that for errors. If it's set, you can error out.

Ronald
Michael Niedermayer Aug. 10, 2018, 10:54 p.m.
On Fri, Aug 10, 2018 at 02:16:56AM -0400, Ronald S. Bultje wrote:
> Hi Michael,
> 
> On Thu, Aug 9, 2018 at 8:49 PM, Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Wed, Aug 08, 2018 at 10:00:42PM -0300, James Almer wrote:
> >
> > Apply this patch with changes to allow that specific condition and lets
> > > not waste more time on this.
> >
> > this is the only change the patch does. Without it there is no patch.
> >
> > Either we stop when the input ends -> that might break decoding a file
> > that was designed so as to expect a decoder not to stop.
> > or we do not stop then that allows doing denial of service
> >
> 
> OK, ok, hold on. I'll try to explain my problem with the patch and I will
> suggest a possible solution. Please store your objections in the closet for
> a second. I'm not a terrible person.

you arnt a terrible person but some patch reviews from you have been 
frustrating. And i dont mean in the sense that the code quality improved
through them. Of course many of your reviews have been excelent too


> 
> The situation you're fixing and not breaking:
> let's say there is a file that is 1 byte long (8 bits), but we claim it's a
> 16k x 16k file. This will take ages to decode, even though it's likely
> broken. Right? A one-byte file is unlikely anyway, but sure, it will run
> out of data after a few symbols. I get it. I really do. And I agree that
> this must be fixed. Yes.
> 
> Also, if a valid file of 1 byte (8 bits) has only 1 symbol of approximately
> 4 real bits, then at the end, there's still 4 bits left in the arithcoder.
> So nothing breaks. Great!
> 

> My objection:
> if a file has exactly symbols of 8 bits in arithdata, then after all this,
> the arithcoder will signal empty and EOF, even though no error occured.
> Imagine a bitcoder (non-arith) of this situation. 


> After get_bits(gb, 8),
> the data pointer will have reached the end, and the bits_left is 0, but
> that does not indicate an error, quite the contrary. It just means that the
> byte boundary happened to align with the exact end of the file. This can
> happen.

Yes but thats not something we do with bitcoders.
bits_left being 0 does indicate an error when the next
step unconditionally reads 1 or more bits.
The same was the intend of the patch here, check if the end was reached
before more reads.
vp56_rac_renorm() always reads data if(bits >= 0 && c->buffer < c->end) 

I had thought that will get executed in all cases, i may have missed
something and the check should be moved by a few lines

> 
> My suggestion:
> add an eof flag to the arithcoder. When we have reached the above condition
> where new data is needed but not present, simply set the EOF flag, and
> check that for errors. If it's set, you can error out.

This is possible but it will make the code likely slower as the reading
happens in a av_always_inline function which itself is used by several
av_always_inline functions. So this else context->flag = 1;
could end up being added to many points in the binary.

I can do this of course if you prefer it just seems sub-optimal to me
unless you have some idea on how to do this without increasing the
complexity of the rac functions

what could make sense is to add a function like vp8_rac_is_end()
but that would not substantially change what the proposed patch does

thanks


[...]
Ronald S. Bultje Aug. 11, 2018, 2:41 a.m.
Hi,

On Fri, Aug 10, 2018 at 6:54 PM, Michael Niedermayer <michael@niedermayer.cc
> wrote:

> On Fri, Aug 10, 2018 at 02:16:56AM -0400, Ronald S. Bultje wrote:
> > My objection:
> > if a file has exactly symbols of 8 bits in arithdata, then after all
> this,
> > the arithcoder will signal empty and EOF, even though no error occured.
> > Imagine a bitcoder (non-arith) of this situation.
> [..]
> > After get_bits(gb, 8),
> > the data pointer will have reached the end, and the bits_left is 0, but
> > that does not indicate an error, quite the contrary. It just means that
> the
> > byte boundary happened to align with the exact end of the file. This can
> > happen.
>
> Yes but thats not something we do with bitcoders.
> bits_left being 0 does indicate an error when the next
> step unconditionally reads 1 or more bits.
> The same was the intend of the patch here, check if the end was reached
> before more reads.
> vp56_rac_renorm() always reads data if(bits >= 0 && c->buffer < c->end)
>
> I had thought that will get executed in all cases, i may have missed
> something and the check should be moved by a few lines


For example, you're checking the arithcoder in pass=2 also, but no
bitstream reading occurs in that pass...

> My suggestion:
> > add an eof flag to the arithcoder. When we have reached the above
> condition
> > where new data is needed but not present, simply set the EOF flag, and
> > check that for errors. If it's set, you can error out.
>
> This is possible but it will make the code likely slower as the reading
> happens in a av_always_inline function which itself is used by several
> av_always_inline functions. So this else context->flag = 1;
> could end up being added to many points in the binary.
>
> I can do this of course if you prefer it just seems sub-optimal to me
> unless you have some idea on how to do this without increasing the
> complexity of the rac functions


But it's an error branch, it is not normally executed. It just makes the
binary a few bytes larger.

Here's another way of looking at this, which isn't so much about exact
bytes and instructions (which will all change in the noise range), but
about code maintainability: you're likely going to want to do fixes like
this for vp8, vp9, vp56, etc., and similar for users of other arithcoders
like cabac or the multisymbol one in daala/av1 and so on. Each of these
decoders are in and by themselves pretty complex creatures, and the people
understanding what's going on under the hood are few and far between, and
half of them are no longer active. I'm not saying you're not intelligent,
but I do think bugs like the one above can creep in because you're not
intimately familiar with all bells and whistles in each decoder codebase.
That being true, a case could be made that noise-range changes in
instruction count or byte size is less important than ease of maintenance.
An EOF flag is easy to maintain and hard to misread, whereas the example
above demonstrates that the inferred checks are brittle and will be much
harder to debug if they do make it into our codebase because someone forgot
to review them.

So it's a balance between priorities. Does every cycle count? Or is
maintenance more important? Each of us is correct in our point, nobody is
wrong, but we need to balance which one is more important...

Ronald
Michael Niedermayer Aug. 11, 2018, 10:55 a.m.
On Fri, Aug 10, 2018 at 10:41:21PM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Fri, Aug 10, 2018 at 6:54 PM, Michael Niedermayer <michael@niedermayer.cc
> > wrote:
> 
> > On Fri, Aug 10, 2018 at 02:16:56AM -0400, Ronald S. Bultje wrote:
> > > My objection:
> > > if a file has exactly symbols of 8 bits in arithdata, then after all
> > this,
> > > the arithcoder will signal empty and EOF, even though no error occured.
> > > Imagine a bitcoder (non-arith) of this situation.
> > [..]
> > > After get_bits(gb, 8),
> > > the data pointer will have reached the end, and the bits_left is 0, but
> > > that does not indicate an error, quite the contrary. It just means that
> > the
> > > byte boundary happened to align with the exact end of the file. This can
> > > happen.
> >
> > Yes but thats not something we do with bitcoders.
> > bits_left being 0 does indicate an error when the next
> > step unconditionally reads 1 or more bits.
> > The same was the intend of the patch here, check if the end was reached
> > before more reads.
> > vp56_rac_renorm() always reads data if(bits >= 0 && c->buffer < c->end)
> >
> > I had thought that will get executed in all cases, i may have missed
> > something and the check should be moved by a few lines
> 
> 
> For example, you're checking the arithcoder in pass=2 also, but no
> bitstream reading occurs in that pass...

this is what i meant by "should be moved by a few lines"
that is as in here:

@@ -1306,6 +1306,9 @@ static int decode_tiles(AVCodecContext *avctx,
                         decode_sb_mem(td, row, col, lflvl_ptr,
                                       yoff2, uvoff2, BL_64X64);
                     } else {
+                        if (td->c->end <= td->c->buffer && td->c->bits >= 0) {
+                            return AVERROR_INVALIDDATA;
+                        }
                         decode_sb(td, row, col, lflvl_ptr,
                                   yoff2, uvoff2, BL_64X64);
     

> 
> > My suggestion:
> > > add an eof flag to the arithcoder. When we have reached the above
> > condition
> > > where new data is needed but not present, simply set the EOF flag, and
> > > check that for errors. If it's set, you can error out.
> >
> > This is possible but it will make the code likely slower as the reading
> > happens in a av_always_inline function which itself is used by several
> > av_always_inline functions. So this else context->flag = 1;
> > could end up being added to many points in the binary.
> >
> > I can do this of course if you prefer it just seems sub-optimal to me
> > unless you have some idea on how to do this without increasing the
> > complexity of the rac functions
> 
> 
> But it's an error branch, it is not normally executed. It just makes the
> binary a few bytes larger.

The condition has to be checked even if there is no error.

Currently the existing branch is a read vs no read check that combines
both the need to read and the end of buffer checks.
with the code to set the flag there would be 3 instead of 1
if (need to read)
    if(space left)
        do read
    else
        set flag
        
vs
if (need to read and space left)
    do read

That means the condition is split in 2 branches instead of 1 and
theres a else
and this is one of the most inner and often executed functions

        
> 
> Here's another way of looking at this, which isn't so much about exact
> bytes and instructions (which will all change in the noise range), but
> about code maintainability: you're likely going to want to do fixes like
> this for vp8, vp9, vp56, etc., and similar for users of other arithcoders
> like cabac or the multisymbol one in daala/av1 and so on. Each of these
> decoders are in and by themselves pretty complex creatures, and the people
> understanding what's going on under the hood are few and far between, and
> half of them are no longer active. I'm not saying you're not intelligent,
> but I do think bugs like the one above can creep in because you're not
> intimately familiar with all bells and whistles in each decoder codebase.
> That being true, a case could be made that noise-range changes in
> instruction count or byte size is less important than ease of maintenance.

Of course iam not intimately familiar with every decoder. Noone is or could
be. But still ive found this pass=2 bug you talk about before you mentioned 
"pass 2" the first time. So either you saw it before and did not mentioning
it directly (which sort of makes your reviews not that usefull)
or your review is just making us find bugs by randomly poking each other.

And Of course changing the code slightly leads to noise in benchmarks.
But the point is that adding operations in inner loops makes them more
complex thus slower on average. turning a single if() into a nested 
if() if/else will make the code slower except by chance of the noise.

Also not only is there more code in the path exeucted. There is more
pressure on the code cache (which is small) as it needs to hold also
some of the not executed instructions that are branched over. These
instructions increase as there are more branches and a flag.
This is a very often executed code.
And the various branch prediction caches and buffers also will see
additional pressure for each of the branches added, there are 2 and
as this is a av_always_inline function used by av_always_inline functions
this amplifies the branches by a bunch so there could be quite a few
that get added


> An EOF flag is easy to maintain and hard to misread, whereas the example
> above demonstrates that the inferred checks are brittle and will be much
> harder to debug if they do make it into our codebase because someone forgot
> to review them.

In the previous mail ive suggested using a vp8_rac_is_end() function for
teh test. This resolves the issue you describe here.

You know, you have snipped the suggested solution and now present the
problem it is intended to solve. Iam not sure what to make of this


> 
> So it's a balance between priorities. Does every cycle count? Or is
> maintenance more important? Each of us is correct in our point, nobody is
> wrong, but we need to balance which one is more important...

See above, the concern of maintainability is solved by using a vp8_rac_is_end()
function instead of the litteral harder to maintain check. In fact such
function is cleaner than directly accessing a flag inside the arith coder
context fron outside the arithcoder code.
And at the same time this avoids the concern about speed loss.

So there seems no need to compromise between priorities. Its easy to have both
fast code and maintainable code at the same time.
But if you still want a flag, i can add one for vp9.
I thought you really cared about your vp9 decoders speed though ...

Thanks

[...]
Ronald S. Bultje Aug. 11, 2018, 12:37 p.m.
Hi,

On Sat, Aug 11, 2018 at 6:55 AM, Michael Niedermayer <michael@niedermayer.cc
> wrote:

> But if you still want a flag, i can add one for vp9.
>

That would be great, thank you for your consideration.

Ronald

Patch hide | download patch | download mbox

diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
index b1178c9c0c..4ca51ec108 100644
--- a/libavcodec/vp9.c
+++ b/libavcodec/vp9.c
@@ -1302,6 +1302,9 @@  static int decode_tiles(AVCodecContext *avctx,
                         memset(lflvl_ptr->mask, 0, sizeof(lflvl_ptr->mask));
                     }
 
+                    if (td->c->end <= td->c->buffer && td->c->bits >= 0) {
+                        return AVERROR_INVALIDDATA;
+                    }
                     if (s->pass == 2) {
                         decode_sb_mem(td, row, col, lflvl_ptr,
                                       yoff2, uvoff2, BL_64X64);