[FFmpeg-devel] [PATCH] avcodec/frame_thread_encoder: Free AVCodecContext structure on error during init

Andreas Rheinhardt andreas.rheinhardt at outlook.com
Fri Aug 13 21:57:41 EEST 2021


Michael Niedermayer:
> On Fri, Aug 13, 2021 at 08:34:13PM +0200, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> Fixes: MemLeak
>>> Fixes: 8281
>>> Fixes: PoC_option158.jpg
>>> Fixes: CVE-2020-22037
>>>
>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>> ---
>>>  libavcodec/frame_thread_encoder.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavcodec/frame_thread_encoder.c b/libavcodec/frame_thread_encoder.c
>>> index 9cabfc495f..25a308173d 100644
>>> --- a/libavcodec/frame_thread_encoder.c
>>> +++ b/libavcodec/frame_thread_encoder.c
>>> @@ -126,7 +126,7 @@ int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>>>  {
>>>      int i=0;
>>>      ThreadContext *c;
>>> -
>>> +    AVCodecContext *thread_avctx = NULL;
>>>  
>>>      if(   !(avctx->thread_type & FF_THREAD_FRAME)
>>>         || !(avctx->codec->capabilities & AV_CODEC_CAP_FRAME_THREADS))
>>> @@ -202,16 +202,16 @@ int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>>>      for(i=0; i<avctx->thread_count ; i++){
>>>          int ret;
>>>          void *tmpv;
>>> -        AVCodecContext *thread_avctx = avcodec_alloc_context3(avctx->codec);
>>> +        thread_avctx = avcodec_alloc_context3(avctx->codec);
>>>          if(!thread_avctx)
>>>              goto fail;
>>>          tmpv = thread_avctx->priv_data;
>>>          *thread_avctx = *avctx;
>>> +        thread_avctx->priv_data = tmpv;
>>> +        thread_avctx->internal = NULL;
>>>          ret = av_opt_copy(thread_avctx, avctx);
>>>          if (ret < 0)
>>>              goto fail;
>>> -        thread_avctx->priv_data = tmpv;
>>> -        thread_avctx->internal = NULL;
>>>          if (avctx->codec->priv_class) {
>>>              int ret = av_opt_copy(thread_avctx->priv_data, avctx->priv_data);
>>>              if (ret < 0)
>>> @@ -233,6 +233,8 @@ int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>>>  
>>>      return 0;
>>>  fail:
>>> +    avcodec_close(thread_avctx);
>>> +    av_freep(&thread_avctx);
>>>      avctx->thread_count = i;
>>>      av_log(avctx, AV_LOG_ERROR, "ff_frame_thread_encoder_init failed\n");
>>>      ff_frame_thread_encoder_free(avctx);
>>>
>> This has some presumptions: First, it relies on undocumented behaviour
>> from av_opt_copy(), see here:
>> https://ffmpeg.org/pipermail/ffmpeg-devel/2021-August/283645.html
> 
> Its documented after your patch ;)
> 
> 
>> Second, you presume that hw_frames_ctx is not set, although it is a
>> valid field for an encoder (although no hardware encoder seems to have
>> the frame threading flag set). If it were set, it would be freed
>> multiple times. (The same goes for several other fields that are not
>> supposed to be set by encoders, but IMO that would be a user problem.)
> 
> What do you suggest, should i add an av_assert on hw_frames_ctx==NULL ?
> 
> 
>> Notice that the second issue exists even when initializing succeeds.
> 
> then its unrelated to the patch, of course we still need to fix it.
> 
> More specifically, should i change the patch ?
> the hw_frames_ctx seems a bug but unrelated, the undocumentedness is
> fixed by you.
> the code feels fragile and not particularly robust before this already
> and fixing this adding nicer and robust API for all steps cant be backported
> and i need to backport the fix so we need a fix without new&nice API
> 
I'd just set hw_frames_ctx to NULL at the same place where you set
internal to NULL; and add a comment about this that frame threaded
hardware encoders are just not supported atm.

- Andreas


More information about the ffmpeg-devel mailing list