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

[Michael Niedermayer]

On Thu, Aug 12, 2021 at 05:16:30PM -0300, James Almer wrote:
> 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().

calling avcodec_close() immedeatly after fail crashes the code 
during multiple fate tests if one forces the error path to be taken after the
first av_opt_copy()

Ill post a patch that reorders things a bit so this is working

Thanks


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

What does censorship reveal? It reveals fear. -- Julian Assange
-------------- 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/fca79e8c/attachment.sig>