[FFmpeg-devel] [PATCH 1/2] avcodec/snowdec: Cleanup avmv on errors

[Michael Niedermayer]

On Sun, Aug 15, 2021 at 04:15:25AM +0200, Andreas Rheinhardt wrote:
> Marton Balint:
> > 
> > 
> > On Sun, 15 Aug 2021, "zhilizhao(赵志立)" wrote:
> > 
> >>
> >>
> >>> On Aug 14, 2021, at 11:52 PM, Michael Niedermayer
> >>> <michael at niedermayer.cc> wrote:
> >>>
> >>> On Sat, Aug 14, 2021 at 11:45:59PM +0800, "zhilizhao(赵志立)" wrote:
> >>>>
> >>>>
> >>>>> On Aug 14, 2021, at 11:07 PM, Michael Niedermayer
> >>>>> <michael at niedermayer.cc> wrote:
> >>>>>
> >>>>> Fixes: Assertion failure
> >>>>> Fixes:
> >>>>> 36359/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SNOW_fuzzer-6733238591684608
> >>>>>
> >>>>>
> >>>>> 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 | 8 ++++----
> >>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/libavcodec/snowdec.c b/libavcodec/snowdec.c
> >>>>> index 1355ae6ed1..7ef28c4899 100644
> >>>>> --- a/libavcodec/snowdec.c
> >>>>> +++ b/libavcodec/snowdec.c
> >>>>> @@ -499,7 +499,7 @@ static int decode_frame(AVCodecContext *avctx,
> >>>>> void *data, int *got_frame,
> >>>>>    s->avmv_index = 0;
> >>>>>
> >>>>>    if ((res = decode_blocks(s)) < 0)
> >>>>> -        return res;
> >>>>> +        goto fail;
> >>>>>
> >>>>>    for(plane_index=0; plane_index < s->nb_planes; plane_index++){
> >>>>>        Plane *p= &s->plane[plane_index];
> >>>>> @@ -618,11 +618,11 @@ static int decode_frame(AVCodecContext
> >>>>> *avctx, void *data, int *got_frame,
> >>>>>        AVFrameSideData *sd;
> >>>>>
> >>>>>        sd = av_frame_new_side_data(picture,
> >>>>> AV_FRAME_DATA_MOTION_VECTORS, s->avmv_index * sizeof(AVMotionVector));
> >>>>> -        if (!sd)
> >>>>> -            return AVERROR(ENOMEM);
> >>>>> -        memcpy(sd->data, s->avmv, s->avmv_index *
> >>>>> sizeof(AVMotionVector));
> >>>>> +        if (sd)
> >>>>> +            memcpy(sd->data, s->avmv, s->avmv_index *
> >>>>> sizeof(AVMotionVector));
> >>>>
> >>>> res is not assigned to AVERROR(ENOMEM), so the error is just being
> >>>> ignored. Is it intentional?
> >>>
> >>> the frame was decoded correctly, just exporting the vectors failed.
> >>> Should we fail and then discard the frame as a result ?
> >>> It seemed better to not fail here, but i was a bit undecided here,
> >>> what do others think ?
> >>> so yes it was intentional but maybe it should be done differently,
> >>> depends
> >>> on what people prefer ...
> >>
> >> Understood. In the ENOMEM case, I prefer simple logic than do the best
> >> effort
> >> to give the user a partly success result. Somebody who don’t get the
> >> idea may
> >> try to ‘fix’ it again. Although I don’t have a strong opinion on that.
> > 
> > IMHO ignoring ENOMEM errors is just bad practice. Every ENOMEM error
> > should be propagated back to the user.
> > 
> I agree. After all, this side data is only exported if the user opted to
> do so by setting the appropriate flag, so we already know that the user
> is really interested in this.

> Obviously the assert that is hit is the av_assert0(!s->avmv), isn't it?

yes


> This could be fixed by not throwing away the buffer for each frame, but
> by using av_fast_malloc.

My goal was to fix the bug and then backport and then move to the next
issue.
You are correct that i can optimize the code and while rewriting it the
bug would disappear. ok, will do that, teh result should be better

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210815/562ff386/attachment.sig>