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

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

Regards,
Marton