| Message ID | 20200825093541.38010-1-guangxin.xu@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/ff_mpv_encode_end: fix a crash for null s->avctx | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On 8/25/2020 6:35 AM, Xu Guangxin wrote: > Steps to reproduce: > 1. ./configure --enable-debug=3 --disable-libx264 && make install > 2. ffmpeg -i input.mp4 -profile:v baseline output.mp4 -y > > you will see a crash like this: > [mpeg4 @ 0x5555575854c0] [Eval @ 0x7fffffffbf80] Undefined constant or missing '(' in 'baseline' > [mpeg4 @ 0x5555575854c0] Unable to parse option value "baseline" > [mpeg4 @ 0x5555575854c0] Error setting option profile to value baseline. > Thread 1 "ffmpeg" received signal SIGSEGV, Segmentation fault. > > root cause: > If the codec has FF_CODEC_CAP_INIT_CLEANUP flag, and avcodec_open2 got an error before avctx->codec->init, > the ff_mpv_encode_end will face a null s->avctx. > --- > libavcodec/mpegvideo_enc.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c > index 09697d89c8..a79309d1b9 100644 > --- a/libavcodec/mpegvideo_enc.c > +++ b/libavcodec/mpegvideo_enc.c > @@ -1078,9 +1078,10 @@ av_cold int ff_mpv_encode_end(AVCodecContext *avctx) > av_frame_free(&s->tmp_frames[i]); > > ff_free_picture_tables(&s->new_picture); > - ff_mpeg_unref_picture(s->avctx, &s->new_picture); > - > - av_freep(&s->avctx->stats_out); > + if (s->avctx) { Judging by the av_freep(&avctx->extradata) call earlier in the function i assume s->avctx is meant to be the same as avctx, which is guaranteed to not be NULL, so you can use that instead. > + ff_mpeg_unref_picture(s->avctx, &s->new_picture); > + av_freep(&s->avctx->stats_out); > + } > av_freep(&s->ac_stats); > > if(s->q_chroma_intra_matrix != s->q_intra_matrix ) av_freep(&s->q_chroma_intra_matrix); >
Hi James, thanks for the review. We can't assume s->avctx == avctx. in https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/smvjpegdec.c, it will be assigned to a new allocated codec context. Currently, it's no mpeg video encoder doing this, but in the future, some subclass maybe. On Tue, Aug 25, 2020 at 10:38 PM James Almer <jamrial@gmail.com> wrote: > On 8/25/2020 6:35 AM, Xu Guangxin wrote: > > Steps to reproduce: > > 1. ./configure --enable-debug=3 --disable-libx264 && make install > > 2. ffmpeg -i input.mp4 -profile:v baseline output.mp4 -y > > > > you will see a crash like this: > > [mpeg4 @ 0x5555575854c0] [Eval @ 0x7fffffffbf80] Undefined constant or > missing '(' in 'baseline' > > [mpeg4 @ 0x5555575854c0] Unable to parse option value "baseline" > > [mpeg4 @ 0x5555575854c0] Error setting option profile to value baseline. > > Thread 1 "ffmpeg" received signal SIGSEGV, Segmentation fault. > > > > root cause: > > If the codec has FF_CODEC_CAP_INIT_CLEANUP flag, and avcodec_open2 got > an error before avctx->codec->init, > > the ff_mpv_encode_end will face a null s->avctx. > > --- > > libavcodec/mpegvideo_enc.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c > > index 09697d89c8..a79309d1b9 100644 > > --- a/libavcodec/mpegvideo_enc.c > > +++ b/libavcodec/mpegvideo_enc.c > > @@ -1078,9 +1078,10 @@ av_cold int ff_mpv_encode_end(AVCodecContext > *avctx) > > av_frame_free(&s->tmp_frames[i]); > > > > ff_free_picture_tables(&s->new_picture); > > - ff_mpeg_unref_picture(s->avctx, &s->new_picture); > > - > > - av_freep(&s->avctx->stats_out); > > + if (s->avctx) { > > Judging by the av_freep(&avctx->extradata) call earlier in the function > i assume s->avctx is meant to be the same as avctx, which is guaranteed > to not be NULL, so you can use that instead. > > > + ff_mpeg_unref_picture(s->avctx, &s->new_picture); > > + av_freep(&s->avctx->stats_out); > > + } > > av_freep(&s->ac_stats); > > > > if(s->q_chroma_intra_matrix != s->q_intra_matrix ) > av_freep(&s->q_chroma_intra_matrix); > > > > _______________________________________________ > 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 8/25/2020 12:08 PM, Guangxin Xu wrote: > Hi James, > thanks for the review. > We can't assume s->avctx == avctx. in > https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/smvjpegdec.c, it > will be assigned to a new allocated codec context. > Currently, it's no mpeg video encoder doing this, but in the future, some > subclass maybe. I was talking specifically about MpegEncContext, yes. I don't think that will change at all, but i have no strong opinion about it anyway, so your patch LGTM. > > On Tue, Aug 25, 2020 at 10:38 PM James Almer <jamrial@gmail.com> wrote: > >> On 8/25/2020 6:35 AM, Xu Guangxin wrote: >>> Steps to reproduce: >>> 1. ./configure --enable-debug=3 --disable-libx264 && make install >>> 2. ffmpeg -i input.mp4 -profile:v baseline output.mp4 -y >>> >>> you will see a crash like this: >>> [mpeg4 @ 0x5555575854c0] [Eval @ 0x7fffffffbf80] Undefined constant or >> missing '(' in 'baseline' >>> [mpeg4 @ 0x5555575854c0] Unable to parse option value "baseline" >>> [mpeg4 @ 0x5555575854c0] Error setting option profile to value baseline. >>> Thread 1 "ffmpeg" received signal SIGSEGV, Segmentation fault. >>> >>> root cause: >>> If the codec has FF_CODEC_CAP_INIT_CLEANUP flag, and avcodec_open2 got >> an error before avctx->codec->init, >>> the ff_mpv_encode_end will face a null s->avctx. >>> --- >>> libavcodec/mpegvideo_enc.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c >>> index 09697d89c8..a79309d1b9 100644 >>> --- a/libavcodec/mpegvideo_enc.c >>> +++ b/libavcodec/mpegvideo_enc.c >>> @@ -1078,9 +1078,10 @@ av_cold int ff_mpv_encode_end(AVCodecContext >> *avctx) >>> av_frame_free(&s->tmp_frames[i]); >>> >>> ff_free_picture_tables(&s->new_picture); >>> - ff_mpeg_unref_picture(s->avctx, &s->new_picture); >>> - >>> - av_freep(&s->avctx->stats_out); >>> + if (s->avctx) { >> >> Judging by the av_freep(&avctx->extradata) call earlier in the function >> i assume s->avctx is meant to be the same as avctx, which is guaranteed >> to not be NULL, so you can use that instead. >> >>> + ff_mpeg_unref_picture(s->avctx, &s->new_picture); >>> + av_freep(&s->avctx->stats_out); >>> + } >>> av_freep(&s->ac_stats); >>> >>> if(s->q_chroma_intra_matrix != s->q_intra_matrix ) >> av_freep(&s->q_chroma_intra_matrix); >>> >> >> _______________________________________________ >> 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". > _______________________________________________ > 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 Tue, Aug 25, 2020 at 11:37:59AM -0300, James Almer wrote: > On 8/25/2020 6:35 AM, Xu Guangxin wrote: > > Steps to reproduce: > > 1. ./configure --enable-debug=3 --disable-libx264 && make install > > 2. ffmpeg -i input.mp4 -profile:v baseline output.mp4 -y > > > > you will see a crash like this: > > [mpeg4 @ 0x5555575854c0] [Eval @ 0x7fffffffbf80] Undefined constant or missing '(' in 'baseline' > > [mpeg4 @ 0x5555575854c0] Unable to parse option value "baseline" > > [mpeg4 @ 0x5555575854c0] Error setting option profile to value baseline. > > Thread 1 "ffmpeg" received signal SIGSEGV, Segmentation fault. > > > > root cause: > > If the codec has FF_CODEC_CAP_INIT_CLEANUP flag, and avcodec_open2 got an error before avctx->codec->init, > > the ff_mpv_encode_end will face a null s->avctx. > > --- > > libavcodec/mpegvideo_enc.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c > > index 09697d89c8..a79309d1b9 100644 > > --- a/libavcodec/mpegvideo_enc.c > > +++ b/libavcodec/mpegvideo_enc.c > > @@ -1078,9 +1078,10 @@ av_cold int ff_mpv_encode_end(AVCodecContext *avctx) > > av_frame_free(&s->tmp_frames[i]); > > > > ff_free_picture_tables(&s->new_picture); > > - ff_mpeg_unref_picture(s->avctx, &s->new_picture); > > - > > - av_freep(&s->avctx->stats_out); > > + if (s->avctx) { > > Judging by the av_freep(&avctx->extradata) call earlier in the function > i assume s->avctx is meant to be the same as avctx, which is guaranteed > to not be NULL, so you can use that instead. ive just posted a patch which changes these and more s->avctx in the file thx [...]
thanks, James and Michael. Are we ready to merge this? thanks On Wed, Aug 26, 2020 at 5:51 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Tue, Aug 25, 2020 at 11:37:59AM -0300, James Almer wrote: > > On 8/25/2020 6:35 AM, Xu Guangxin wrote: > > > Steps to reproduce: > > > 1. ./configure --enable-debug=3 --disable-libx264 && make install > > > 2. ffmpeg -i input.mp4 -profile:v baseline output.mp4 -y > > > > > > you will see a crash like this: > > > [mpeg4 @ 0x5555575854c0] [Eval @ 0x7fffffffbf80] Undefined constant or > missing '(' in 'baseline' > > > [mpeg4 @ 0x5555575854c0] Unable to parse option value "baseline" > > > [mpeg4 @ 0x5555575854c0] Error setting option profile to value > baseline. > > > Thread 1 "ffmpeg" received signal SIGSEGV, Segmentation fault. > > > > > > root cause: > > > If the codec has FF_CODEC_CAP_INIT_CLEANUP flag, and avcodec_open2 got > an error before avctx->codec->init, > > > the ff_mpv_encode_end will face a null s->avctx. > > > --- > > > libavcodec/mpegvideo_enc.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c > > > index 09697d89c8..a79309d1b9 100644 > > > --- a/libavcodec/mpegvideo_enc.c > > > +++ b/libavcodec/mpegvideo_enc.c > > > @@ -1078,9 +1078,10 @@ av_cold int ff_mpv_encode_end(AVCodecContext > *avctx) > > > av_frame_free(&s->tmp_frames[i]); > > > > > > ff_free_picture_tables(&s->new_picture); > > > - ff_mpeg_unref_picture(s->avctx, &s->new_picture); > > > - > > > - av_freep(&s->avctx->stats_out); > > > + if (s->avctx) { > > > > Judging by the av_freep(&avctx->extradata) call earlier in the function > > i assume s->avctx is meant to be the same as avctx, which is guaranteed > > to not be NULL, so you can use that instead. > > ive just posted a patch which changes these and more s->avctx in the file > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > There will always be a question for which you do not know the correct > answer. > _______________________________________________ > 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, Aug 30, 2020 at 05:07:03PM +0800, Guangxin Xu wrote: > thanks, James and Michael. > Are we ready to merge this? ive applied the avctx change patch which should fix this thx [...]
diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c index 09697d89c8..a79309d1b9 100644 --- a/libavcodec/mpegvideo_enc.c +++ b/libavcodec/mpegvideo_enc.c @@ -1078,9 +1078,10 @@ av_cold int ff_mpv_encode_end(AVCodecContext *avctx) av_frame_free(&s->tmp_frames[i]); ff_free_picture_tables(&s->new_picture); - ff_mpeg_unref_picture(s->avctx, &s->new_picture); - - av_freep(&s->avctx->stats_out); + if (s->avctx) { + ff_mpeg_unref_picture(s->avctx, &s->new_picture); + av_freep(&s->avctx->stats_out); + } av_freep(&s->ac_stats); if(s->q_chroma_intra_matrix != s->q_intra_matrix ) av_freep(&s->q_chroma_intra_matrix);
Steps to reproduce: 1. ./configure --enable-debug=3 --disable-libx264 && make install 2. ffmpeg -i input.mp4 -profile:v baseline output.mp4 -y you will see a crash like this: [mpeg4 @ 0x5555575854c0] [Eval @ 0x7fffffffbf80] Undefined constant or missing '(' in 'baseline' [mpeg4 @ 0x5555575854c0] Unable to parse option value "baseline" [mpeg4 @ 0x5555575854c0] Error setting option profile to value baseline. Thread 1 "ffmpeg" received signal SIGSEGV, Segmentation fault. root cause: If the codec has FF_CODEC_CAP_INIT_CLEANUP flag, and avcodec_open2 got an error before avctx->codec->init, the ff_mpv_encode_end will face a null s->avctx. --- libavcodec/mpegvideo_enc.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)