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

[Mark Thompson]

On 29/09/2020 17:17, James Almer wrote:
> 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.

Only the reference frame state which the frame would have modified has changed, which need not be all of it if the encoder knows it is going to be sending over an unreliable channel.

Also intra refresh, though I haven't seen that implemented in an AV1 encoder yet.

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

I think that answer is preferable, though I guess it could just be ignored for now and fixed when it comes up in future.  (Presumably noone has tried to use dav1d for these sorts of cases yet.)

- Mark