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


[...]
-- 
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.
-------------- 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/20180811/ce6b9973/attachment.sig>