[FFmpeg-devel] [PATCH 8/8] avcodec/av1dec: clean state on frame decoding errors

[James Almer]

On 9/29/2020 12:57 PM, Mark Thompson wrote:
> On 25/09/2020 15:43, James Almer wrote:
>> Fixes: member access within null pointer of type 'TileGroupInfo' (aka
>> 'struct TileGroupInfo')
>> Fixes:
>> 25725/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AV1_fuzzer-5166692706287616
>>
>>
>> Found-by: continuous fuzzing process
>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>   libavcodec/av1dec.c | 30 ++++++++++++++++--------------
>>   1 file changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
>> index 07026b7aeb..e5cfc3f2f2 100644
>> --- a/libavcodec/av1dec.c
>> +++ b/libavcodec/av1dec.c
>> @@ -381,6 +381,20 @@ fail:
>>       return AVERROR(ENOMEM);
>>   }
>>   +static void av1_decode_flush(AVCodecContext *avctx)
>> +{
>> +    AV1DecContext *s = avctx->priv_data;
>> +
>> +    for (int i = 0; i < FF_ARRAY_ELEMS(s->ref); i++)
>> +        av1_frame_unref(avctx, &s->ref[i]);
>> +
>> +    av1_frame_unref(avctx, &s->cur_frame);
>> +    s->raw_frame_header = NULL;
>> +    s->raw_seq = NULL;
>> +
>> +    ff_cbs_flush(s->cbc);
>> +}
>> +
>>   static av_cold int av1_decode_free(AVCodecContext *avctx)
>>   {
>>       AV1DecContext *s = avctx->priv_data;
>> @@ -841,23 +855,11 @@ static int av1_decode_frame(AVCodecContext
>> *avctx, void *frame,
>>     end:
>>       ff_cbs_fragment_reset(&s->current_obu);
>> +    if (ret < 0)
>> +        av1_decode_flush(avctx);
>>       return ret;
>>   }
>>   -static void av1_decode_flush(AVCodecContext *avctx)
>> -{
>> -    AV1DecContext *s = avctx->priv_data;
>> -
>> -    for (int i = 0; i < FF_ARRAY_ELEMS(s->ref); i++)
>> -        av1_frame_unref(avctx, &s->ref[i]);
>> -
>> -    av1_frame_unref(avctx, &s->cur_frame);
>> -    s->raw_frame_header = NULL;
>> -    s->raw_seq = NULL;
>> -
>> -    ff_cbs_flush(s->cbc);
>> -}
>> -
>>   AVCodec ff_av1_decoder = {
>>       .name                  = "av1",
>>       .long_name             = NULL_IF_CONFIG_SMALL("Alliance for Open
>> Media AV1"),
>>
> 
> This seems questionable - if you have some packet loss and lose an
> intermediate frame, I don't think you want to throw away all the old
> state since it may be able to continue from an earlier frame which was
> received correctly.

If a frame was not parsed correctly, then the reference frame state from
then on will be invalid. Especially if ff_cbs_read_packet() is what
failed, considering everything after the corrupt OBU within the TU will
be discarded by CBS.

I can replace this to simply setting raw_frame_header to NULL if you
prefer, but for reference, dav1d (the decoder that hopefully will
eventually be ported into this native decoder) throws the entire state
away on a frame decoding failure.

> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".