| Message ID | AS8P250MB07448A5BE17FDE107DD04AFA8F182@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/av1dec: Always set ret before goto end | expand |
On Thu, May 2, 2024 at 10:22 AM Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > > Before 0f8763fbea4e8816cd54c2a481d4c048fec58394, av1_frame_ref() > and update_reference_list() could fail and therefore needed to > be checked, which incidentally set ret. This is no longer happening, > leading to a potential use of an uninitialized value which is > also the subject of Coverity ticket #1596605. > > Fix this by always setting ret before goto end; do not return > some random ancient value. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/av1dec.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c > index 79a30a114d..c3f255a29a 100644 > --- a/libavcodec/av1dec.c > +++ b/libavcodec/av1dec.c > @@ -1335,6 +1335,12 @@ static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame) > ret = set_output_frame(avctx, frame); > if (ret < 0) > av_log(avctx, AV_LOG_ERROR, "Set output frame error.\n"); > + } else { > + // CBS checks for us that the frame to be shown actually existed > + // in the bitstream; if it doesn't it could be e.g. due to > + // skip_frame setting. Return EAGAIN to indicate that we are > + // currently unable to produce output. > + ret = AVERROR(EAGAIN); > } > In the vein of this comment, set_output_frame will also return 0 without returning a frame in some cases - eg. with multiple layers. Should this equally return EAGAIN rather than claiming success without a frame? - Hendrik
Hendrik Leppkes: > On Thu, May 2, 2024 at 10:22 AM Andreas Rheinhardt > <andreas.rheinhardt@outlook.com> wrote: >> >> Before 0f8763fbea4e8816cd54c2a481d4c048fec58394, av1_frame_ref() >> and update_reference_list() could fail and therefore needed to >> be checked, which incidentally set ret. This is no longer happening, >> leading to a potential use of an uninitialized value which is >> also the subject of Coverity ticket #1596605. >> >> Fix this by always setting ret before goto end; do not return >> some random ancient value. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavcodec/av1dec.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c >> index 79a30a114d..c3f255a29a 100644 >> --- a/libavcodec/av1dec.c >> +++ b/libavcodec/av1dec.c >> @@ -1335,6 +1335,12 @@ static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame) >> ret = set_output_frame(avctx, frame); >> if (ret < 0) >> av_log(avctx, AV_LOG_ERROR, "Set output frame error.\n"); >> + } else { >> + // CBS checks for us that the frame to be shown actually existed >> + // in the bitstream; if it doesn't it could be e.g. due to >> + // skip_frame setting. Return EAGAIN to indicate that we are >> + // currently unable to produce output. >> + ret = AVERROR(EAGAIN); >> } >> > > In the vein of this comment, set_output_frame will also return 0 > without returning a frame in some cases - eg. with multiple layers. > Should this equally return EAGAIN rather than claiming success without > a frame? > Thanks for pointing this out. There is a translation 0->AVERROR(EAGAIN) at the end of this function if the output frame is unset. Maybe this commit should rather set ret to 0 and rely on this (as it was before 0f8763fbea)? Let's hear what James says about this. - Andreas
On 5/2/2024 6:05 AM, Andreas Rheinhardt wrote: > Hendrik Leppkes: >> On Thu, May 2, 2024 at 10:22 AM Andreas Rheinhardt >> <andreas.rheinhardt@outlook.com> wrote: >>> >>> Before 0f8763fbea4e8816cd54c2a481d4c048fec58394, av1_frame_ref() >>> and update_reference_list() could fail and therefore needed to >>> be checked, which incidentally set ret. This is no longer happening, >>> leading to a potential use of an uninitialized value which is >>> also the subject of Coverity ticket #1596605. >>> >>> Fix this by always setting ret before goto end; do not return >>> some random ancient value. >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>> --- >>> libavcodec/av1dec.c | 12 ++++++++++-- >>> 1 file changed, 10 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c >>> index 79a30a114d..c3f255a29a 100644 >>> --- a/libavcodec/av1dec.c >>> +++ b/libavcodec/av1dec.c >>> @@ -1335,6 +1335,12 @@ static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame) >>> ret = set_output_frame(avctx, frame); >>> if (ret < 0) >>> av_log(avctx, AV_LOG_ERROR, "Set output frame error.\n"); >>> + } else { >>> + // CBS checks for us that the frame to be shown actually existed >>> + // in the bitstream; if it doesn't it could be e.g. due to >>> + // skip_frame setting. Return EAGAIN to indicate that we are >>> + // currently unable to produce output. >>> + ret = AVERROR(EAGAIN); >>> } >>> >> >> In the vein of this comment, set_output_frame will also return 0 >> without returning a frame in some cases - eg. with multiple layers. >> Should this equally return EAGAIN rather than claiming success without >> a frame? >> > > Thanks for pointing this out. There is a translation 0->AVERROR(EAGAIN) > at the end of this function if the output frame is unset. Maybe this > commit should rather set ret to 0 and rely on this (as it was before > 0f8763fbea)? Let's hear what James says about this. Yes, i prefer that.
James Almer: > On 5/2/2024 6:05 AM, Andreas Rheinhardt wrote: >> Hendrik Leppkes: >>> On Thu, May 2, 2024 at 10:22 AM Andreas Rheinhardt >>> <andreas.rheinhardt@outlook.com> wrote: >>>> >>>> Before 0f8763fbea4e8816cd54c2a481d4c048fec58394, av1_frame_ref() >>>> and update_reference_list() could fail and therefore needed to >>>> be checked, which incidentally set ret. This is no longer happening, >>>> leading to a potential use of an uninitialized value which is >>>> also the subject of Coverity ticket #1596605. >>>> >>>> Fix this by always setting ret before goto end; do not return >>>> some random ancient value. >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>>> --- >>>> libavcodec/av1dec.c | 12 ++++++++++-- >>>> 1 file changed, 10 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c >>>> index 79a30a114d..c3f255a29a 100644 >>>> --- a/libavcodec/av1dec.c >>>> +++ b/libavcodec/av1dec.c >>>> @@ -1335,6 +1335,12 @@ static int >>>> av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame) >>>> ret = set_output_frame(avctx, frame); >>>> if (ret < 0) >>>> av_log(avctx, AV_LOG_ERROR, "Set output >>>> frame error.\n"); >>>> + } else { >>>> + // CBS checks for us that the frame to be shown >>>> actually existed >>>> + // in the bitstream; if it doesn't it could be >>>> e.g. due to >>>> + // skip_frame setting. Return EAGAIN to >>>> indicate that we are >>>> + // currently unable to produce output. >>>> + ret = AVERROR(EAGAIN); >>>> } >>>> >>> >>> In the vein of this comment, set_output_frame will also return 0 >>> without returning a frame in some cases - eg. with multiple layers. >>> Should this equally return EAGAIN rather than claiming success without >>> a frame? >>> >> >> Thanks for pointing this out. There is a translation 0->AVERROR(EAGAIN) >> at the end of this function if the output frame is unset. Maybe this >> commit should rather set ret to 0 and rely on this (as it was before >> 0f8763fbea)? Let's hear what James says about this. > > Yes, i prefer that. I already send a v2 that does this. - Andreas
diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c index 79a30a114d..c3f255a29a 100644 --- a/libavcodec/av1dec.c +++ b/libavcodec/av1dec.c @@ -1335,6 +1335,12 @@ static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame) ret = set_output_frame(avctx, frame); if (ret < 0) av_log(avctx, AV_LOG_ERROR, "Set output frame error.\n"); + } else { + // CBS checks for us that the frame to be shown actually existed + // in the bitstream; if it doesn't it could be e.g. due to + // skip_frame setting. Return EAGAIN to indicate that we are + // currently unable to produce output. + ret = AVERROR(EAGAIN); } s->raw_frame_header = NULL; @@ -1439,13 +1445,15 @@ static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame) update_reference_list(avctx); - if (s->raw_frame_header->show_frame && s->cur_frame.f) { + // cur_frame.f needn't exist due to skip_frame. + if (show_frame && s->cur_frame.f) { ret = set_output_frame(avctx, frame); if (ret < 0) { av_log(avctx, AV_LOG_ERROR, "Set output frame error\n"); goto end; } - } + } else + ret = AVERROR(EAGAIN); raw_tile_group = NULL; s->raw_frame_header = NULL; if (show_frame) {
Before 0f8763fbea4e8816cd54c2a481d4c048fec58394, av1_frame_ref() and update_reference_list() could fail and therefore needed to be checked, which incidentally set ret. This is no longer happening, leading to a potential use of an uninitialized value which is also the subject of Coverity ticket #1596605. Fix this by always setting ret before goto end; do not return some random ancient value. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/av1dec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)