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

[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

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210813/e0ad3b19/attachment.sig>