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

Thu Aug 9 04:00:42 EEST 2018

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

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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 