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

[Ronald S. Bultje]

Hi,

On Sat, Aug 11, 2018 at 6:55 AM, Michael Niedermayer  wrote:

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

That would be great, thank you for your consideration.

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

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  > 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 

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

[Ronald S. Bultje]

Hi,

On Fri, Aug 10, 2018 at 6:54 PM, Michael Niedermayer  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

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 
> 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


[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ronald S. Bultje]

Hi Michael,

On Thu, Aug 9, 2018 at 8:49 PM, Michael Niedermayer 
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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

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 
> >> 
> >> 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
> >>> 
>  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
> >>>  >>
> >> 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
> >>>  > ffmpegSigned-off-by>:
> >>> Michael Niedermayer 
> >>> ---
> >>>  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.
> >>
> >>
> > 
> >>  Yes, that's how production grade software works.  
> > 
> > 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 

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

[James Almer]

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 
>> 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
>>> 
 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
>>> >
>> 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
>>>  ffmpegSigned-off-by>:
>>> Michael Niedermayer 
>>> ---
>>>  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.
>>
>>
> 
>>  Yes, that's how production grade software works.  
> 
> 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 

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

[Michael Niedermayer]

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 
> 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
> > 
> > > 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
> >  > > > >
> > > > > 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
> > > > > >  > > > ffmpegSigned-off-by>:
> > > > > > Michael Niedermayer 
> > > > > > ---
> > > > > >  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.
> 
> 

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

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 

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

[Ronald S. Bultje]

Hi,

On Tue, Aug 7, 2018 at 7:15 PM, Michael Niedermayer 
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
> 
> > 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
>  > > >
> > > > 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
> > > > >  > > ffmpegSigned-off-by>:
> > > > > Michael Niedermayer 
> > > > > ---
> > > > >  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.


 Yes, that's how production grade software works.  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

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 
> 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  > >
> > > 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
> > > >  > ffmpegSigned-off-by>:
> > > > Michael Niedermayer 
> > > > ---
> > > >  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.


[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ronald S. Bultje]

Hi,

On Mon, Aug 6, 2018 at 3:00 PM, Michael Niedermayer 
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  >
> > 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
> > >  ffmpegSigned-off-by>:
> > > Michael Niedermayer 
> > > ---
> > >  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

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 
> 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
> > :
> > Michael Niedermayer 
> > ---
> >  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

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ronald S. Bultje]

Hi,

On Sun, Aug 5, 2018, 9:17 AM Michael Niedermayer 
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
> :
> Michael Niedermayer 
> ---
>  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

>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel