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

["zhilizhao(赵志立)"]

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

> 
> thx
> 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> If you drop bombs on a foreign country and kill a hundred thousand
> innocent people, expect your government to call the consequence
> "unprovoked inhuman terrorist attacks" and use it to justify dropping
> more bombs and killing more people. The technology changed, the idea is old.
> 
> _______________________________________________
> 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".
>