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 > >>> <mich...@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 > >>>>> <mich...@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 <mich...@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
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
