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 <mich...@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.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel