| Message ID | 20180805011642.5290-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Headers | show |
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 >
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 [...]
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
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. [...]
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
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 [...]
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 >
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 [...]
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
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 [...]
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
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 [...]
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
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);
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(+)