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

[James Almer]

On 8/13/2021 3:51 PM, Michael Niedermayer wrote:
> 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 ?

We should not av_assert on user set arguments like this. Since no 
encoder uses frame threading, and none will, a simple check and EINVAL, 
or simply setting hw_frames_ctx to NULL on the copied contexts (same as 
with internal), should be enough.

> 
> 
>> 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
> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>