[FFmpeg-devel] [PATCH] avcodec/ff_mpv_encode_end: fix a crash for null s->avctx

[James Almer]

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 at 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 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".
> _______________________________________________
> 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".
>