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

[Andreas Rheinhardt]

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
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.)
Notice that the second issue exists even when initializing succeeds.
(@James: Using avcodec_free_context() is even more problematic because
this also frees (intra|inter)_matrix, to fields which are allowed to be
set by the user. It also frees rc_override although the documentation
says it is freed by the user.)

- Andreas