[FFmpeg-devel] [PATCH] avcodec/snowdec: Check ld/cbd/crd

[James Almer]

On 9/3/2017 10:49 AM, Ronald S. Bultje wrote:
> Hi,
> 
> On Sun, Sep 3, 2017 at 6:23 AM, Michael Niedermayer <michael at niedermayer.cc>
> wrote:
> 
>> Fixes: Timeout
>> Fixes: 3142/clusterfuzz-testcase-5007853163118592
>>
>> Found-by: continuous fuzzing process https://github.com/google/oss-
>> fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>> ---
>>  libavcodec/snowdec.c | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavcodec/snowdec.c b/libavcodec/snowdec.c
>> index b74c468ce3..7e07857a44 100644
>> --- a/libavcodec/snowdec.c
>> +++ b/libavcodec/snowdec.c
>> @@ -183,13 +183,24 @@ static int decode_q_branch(SnowContext *s, int
>> level, int x, int y){
>>          int my_context= av_log2(2*FFABS(left->my - top->my)) +
>> 0*av_log2(2*FFABS(tr->my - top->my));
>>
>>          type= get_rac(&s->c, &s->block_state[1 + left->type + top->type])
>> ? BLOCK_INTRA : 0;
>> -
>>          if(type){
>> +            int ld, cbd, crd;
>>              pred_mv(s, &mx, &my, 0, left, top, tr);
>> -            l += get_symbol(&s->c, &s->block_state[32], 1);
>> +            ld = get_symbol(&s->c, &s->block_state[32], 1);
>> +            if (ld < -255 || ld > 255) {
>> +                av_log(s->avctx, AV_LOG_ERROR, "Invalid ld %d\n", ld);
>> +                return AVERROR_INVALIDDATA;
>> +            }
>> +            l += ld;
>>              if (s->nb_planes > 2) {
>> -                cb+= get_symbol(&s->c, &s->block_state[64], 1);
>> -                cr+= get_symbol(&s->c, &s->block_state[96], 1);
>> +                cbd = get_symbol(&s->c, &s->block_state[64], 1);
>> +                crd = get_symbol(&s->c, &s->block_state[96], 1);
>> +                if (cbd < -255 || cbd > 255 || crd < -255 || crd > 255) {
>> +                    av_log(s->avctx, AV_LOG_ERROR, "Invalid cbd %d, crd
>> %d\n", cbd, crd);
>> +                    return AVERROR_INVALIDDATA;
>> +                }
>> +                cb += cbd;
>> +                cr += crd;
>>              }
> 
> 
> Can you elaborate on how these error messages, which are displayed to the
> user by default, help the user resolve the
> likely-to-occur-with-realworld-files situation where a validly-created file
> doesn't play back?
> 
> If any part of this sentence is not true, then why is there a message here?
> 
> Ronald

Just go straight to the point, please. This fuzzing commit set in the
past few months has been way more controversial than it has any right to.

Michael: Don't add error messages at any level above debug if they are
completely useless and unhelpful for non-developers. And Consider using
ff_tlog() as well, so they don't become binary bloat on a non-debug build.