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

[James Almer]

On 8/12/2021 5:03 PM, Michael Niedermayer wrote:
> 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 | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/frame_thread_encoder.c b/libavcodec/frame_thread_encoder.c
> index 9cabfc495f..4a56fedc9d 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))
> @@ -198,11 +198,10 @@ int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>               !(c->tasks[i].outdata = av_packet_alloc()))
>               goto fail;
>       }
> -
>       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;
> @@ -225,6 +224,7 @@ int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>           av_assert0(!thread_avctx->internal->frame_thread_encoder);
>           thread_avctx->internal->frame_thread_encoder = c;
>           if(pthread_create(&c->worker[i], NULL, worker, thread_avctx)) {
> +            avcodec_close(thread_avctx);

If I'm reading this right, there are two av_opt_copy() above that may 
leak if you don't call avcodec_close() on all failure paths after them.
It should be safe to call it even before avcodec_open2() (or if that 
failed), and even if the argument passed to it is NULL, so might as well 
call it below after the fail label. Or could even use 
avcodec_free_context(), assuming it's safe, and remove the av_freep().

>               goto fail;
>           }
>       }
> @@ -233,6 +233,7 @@ int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>   
>       return 0;
>   fail:
> +    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);
>