Message ID | 20210812200339.13903-1-michael@niedermayer.cc |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] avcodec/frame_thread_encoder: Free AVCodecContext structure on error during init | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
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@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); >
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@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 [...]
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); 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);
Fixes: MemLeak Fixes: 8281 Fixes: PoC_option158.jpg Fixes: CVE-2020-22037 Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/frame_thread_encoder.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)