| Message ID | 20210814150800.13974-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/snowdec: Cleanup avmv on errors | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | fail | Make fate failed |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | warning | Make fate failed |
> On Aug 14, 2021, at 11:07 PM, Michael Niedermayer <michael@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@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? > } > > +fail: > av_freep(&s->avmv); > > if (res < 0) > -- > 2.17.1 > > _______________________________________________ > 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 Sat, Aug 14, 2021 at 11:45:59PM +0800, "zhilizhao(赵志立)" wrote: > > > > On Aug 14, 2021, at 11:07 PM, Michael Niedermayer <michael@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@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 ... thx [...]
> On Aug 14, 2021, at 11:52 PM, Michael Niedermayer <michael@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@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@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@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 Sun, 15 Aug 2021, "zhilizhao(赵志立)" wrote: > > >> On Aug 14, 2021, at 11:52 PM, Michael Niedermayer <michael@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@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@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
Marton Balint: > > > On Sun, 15 Aug 2021, "zhilizhao(赵志立)" wrote: > >> >> >>> On Aug 14, 2021, at 11:52 PM, Michael Niedermayer >>> <michael@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@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@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? This could be fixed by not throwing away the buffer for each frame, but by using av_fast_malloc. - Andreas PS: Does clusterfuzz now also test memory failures and the related error paths? Awesome!
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@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@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@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 [...]
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)); } +fail: av_freep(&s->avmv); if (res < 0)
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@niedermayer.cc> --- libavcodec/snowdec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)