[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 <michael at 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 at 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 at 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 at 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

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

Modern terrorism, a quick summary: Need oil, start war with country that
has oil, kill hundread thousand in war. Let country fall into chaos,
be surprised about raise of fundamantalists. Drop more bombs, kill more
people, be surprised about them taking revenge and drop even more bombs
and strip your own citizens of their rights and freedoms. to be continued
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180809/8adfc33a/attachment.sig>