| Message ID | 20200925144318.6194-8-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/8] avcodec/cbs: add a flush callback to CodedBitstreamType | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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@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. - Mark
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@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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@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
On 9/29/2020 3:07 PM, Mark Thompson wrote: > 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@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.) Which option do you prefer, then? If i just set raw_frame_header to NULL on failure and do nothing else, future frames will be parsed and get_buffer() will still be called despite no pix_fmt being set (And thus fail). If i also set raw_seq to NULL on failure then that can be avoided, but it will be more or less functionally the same as this patch seeing that no frame will be handled by this decoder until a new sequence header is found, even if the CBS state was left intact. > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 29/09/2020 19:23, James Almer wrote: > On 9/29/2020 3:07 PM, Mark Thompson wrote: >> 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@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.) > > Which option do you prefer, then? If i just set raw_frame_header to NULL > on failure and do nothing else, future frames will be parsed and > get_buffer() will still be called despite no pix_fmt being set (And thus > fail). I think you can avoid that by making sure that AV1ReferenceFrameState.valid is false for the frame that was lost? Future frames will fail to parse if they refer directly to it, and if they don't then they will continue to work. > If i also set raw_seq to NULL on failure then that can be avoided, but > it will be more or less functionally the same as this patch seeing that > no frame will be handled by this decoder until a new sequence header is > found, even if the CBS state was left intact. Throwing away the sequence header doesn't seem right. - Mark
On 10/1/2020 7:15 PM, Mark Thompson wrote: > On 29/09/2020 19:23, James Almer wrote: >> On 9/29/2020 3:07 PM, Mark Thompson wrote: >>> 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@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.) >> >> Which option do you prefer, then? If i just set raw_frame_header to NULL >> on failure and do nothing else, future frames will be parsed and >> get_buffer() will still be called despite no pix_fmt being set (And thus >> fail). > > I think you can avoid that by making sure that > AV1ReferenceFrameState.valid is false for the frame that was lost? get_buffer() is called even if no hwaccel is set because s->raw_seq will not be NULL when parsing a frame after get_pixel_format() failed while parsing the sequence header. This means get_current_frame() and then av1_frame_alloc() will be reached. Hence setting it to NULL solving it. Another option is to check for pix_fmt == NONE in av1_frame_alloc(), right before the get_buffer() call, at least for the time being. > > Future frames will fail to parse if they refer directly to it, and if > they don't then they will continue to work. How would i do that in the cases where a ff_cbs_read_packet() failure results in refresh_frame_flags not being read? I'll have no way to know what slot/s in the ref frame array the frame in question was meant to take. Not to mention all the other potential frames in the TU CBS never got to parse to begin with. For failures within the decoder after a successful ff_cbs_read_packet() call it sounds good. > >> If i also set raw_seq to NULL on failure then that can be avoided, but >> it will be more or less functionally the same as this patch seeing that >> no frame will be handled by this decoder until a new sequence header is >> found, even if the CBS state was left intact. > > Throwing away the sequence header doesn't seem right. > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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"),
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@gmail.com> --- libavcodec/av1dec.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)