diff mbox series

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

Message ID 20210814075500.29399-1-michael@niedermayer.cc
State Accepted
Commit 7bba0dd6382e30d646cb406034a66199e071d713
Headers show
Series [FFmpeg-devel,v3] avcodec/frame_thread_encoder: Free AVCodecContext structure on error during init | expand

Checks

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

Commit Message

Michael Niedermayer Aug. 14, 2021, 7:55 a.m. UTC 
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 | 11 +++++++----
 libavcodec/frame_thread_encoder.h |  4 ++++
 2 files changed, 11 insertions(+), 4 deletions(-)

Comments

Andreas Rheinhardt Aug. 15, 2021, 5:35 p.m. UTC | #1 
Michael Niedermayer:
> 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 | 11 +++++++----
>  libavcodec/frame_thread_encoder.h |  4 ++++
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/frame_thread_encoder.c b/libavcodec/frame_thread_encoder.c
> index 9cabfc495f..9bc48c7761 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,17 @@ 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;
> +        thread_avctx->hw_frames_ctx = 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 +234,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);
> diff --git a/libavcodec/frame_thread_encoder.h b/libavcodec/frame_thread_encoder.h
> index 2cdc40a830..201cba2a8f 100644
> --- a/libavcodec/frame_thread_encoder.h
> +++ b/libavcodec/frame_thread_encoder.h
> @@ -23,6 +23,10 @@
>  
>  #include "avcodec.h"
>  
> +/**
> + * Initialize frame thread encoder.
> + * @note hardware encoders are not supported
> + */
>  int ff_frame_thread_encoder_init(AVCodecContext *avctx);
>  void ff_frame_thread_encoder_free(AVCodecContext *avctx);
>  int ff_thread_video_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
> 
LGTM.

- Andreas

PS: I still don't know whether my patch for av_opt_copy needs to bump
minor or micro.
Michael Niedermayer Aug. 16, 2021, 9:18 p.m. UTC | #2 
On Sun, Aug 15, 2021 at 07:35:35PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > 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 | 11 +++++++----
> >  libavcodec/frame_thread_encoder.h |  4 ++++
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libavcodec/frame_thread_encoder.c b/libavcodec/frame_thread_encoder.c
> > index 9cabfc495f..9bc48c7761 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,17 @@ 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;
> > +        thread_avctx->hw_frames_ctx = 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 +234,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);
> > diff --git a/libavcodec/frame_thread_encoder.h b/libavcodec/frame_thread_encoder.h
> > index 2cdc40a830..201cba2a8f 100644
> > --- a/libavcodec/frame_thread_encoder.h
> > +++ b/libavcodec/frame_thread_encoder.h
> > @@ -23,6 +23,10 @@
> >  
> >  #include "avcodec.h"
> >  
> > +/**
> > + * Initialize frame thread encoder.
> > + * @note hardware encoders are not supported
> > + */
> >  int ff_frame_thread_encoder_init(AVCodecContext *avctx);
> >  void ff_frame_thread_encoder_free(AVCodecContext *avctx);
> >  int ff_thread_video_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
> > 
> LGTM.

will apply


> 
> - Andreas
> 
> PS: I still don't know whether my patch for av_opt_copy needs to bump
> minor or micro.

you mean the documentation changing patch ?
IMHO It depends on the viewpoint i guess. is there a bug in the documentation
a bug in the implementation or a AP/ABI change

[...]
Andreas Rheinhardt Aug. 16, 2021, 9:28 p.m. UTC | #3 
Michael Niedermayer:
> On Sun, Aug 15, 2021 at 07:35:35PM +0200, Andreas Rheinhardt wrote:
>>
>> PS: I still don't know whether my patch for av_opt_copy needs to bump
>> minor or micro.
> 
> you mean the documentation changing patch ?
> IMHO It depends on the viewpoint i guess. is there a bug in the documentation
> a bug in the implementation or a AP/ABI change
> 
There was no bug per se; it is also no ABI change (it is only a
documentation change after all). It is just that some previously
undocumented behaviour got documented.

- Andreas
Michael Niedermayer Aug. 17, 2021, 8:36 a.m. UTC | #4 
On Mon, Aug 16, 2021 at 11:28:41PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Sun, Aug 15, 2021 at 07:35:35PM +0200, Andreas Rheinhardt wrote:
> >>
> >> PS: I still don't know whether my patch for av_opt_copy needs to bump
> >> minor or micro.
> > 
> > you mean the documentation changing patch ?
> > IMHO It depends on the viewpoint i guess. is there a bug in the documentation
> > a bug in the implementation or a AP/ABI change
> > 
> There was no bug per se; it is also no ABI change (it is only a
> documentation change after all). It is just that some previously
> undocumented behaviour got documented.

if you see it that way then a micro bump is enough

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/frame_thread_encoder.c b/libavcodec/frame_thread_encoder.c
index 9cabfc495f..9bc48c7761 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,17 @@  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;
+        thread_avctx->hw_frames_ctx = 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 +234,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);
diff --git a/libavcodec/frame_thread_encoder.h b/libavcodec/frame_thread_encoder.h
index 2cdc40a830..201cba2a8f 100644
--- a/libavcodec/frame_thread_encoder.h
+++ b/libavcodec/frame_thread_encoder.h
@@ -23,6 +23,10 @@ 
 
 #include "avcodec.h"
 
+/**
+ * Initialize frame thread encoder.
+ * @note hardware encoders are not supported
+ */
 int ff_frame_thread_encoder_init(AVCodecContext *avctx);
 void ff_frame_thread_encoder_free(AVCodecContext *avctx);
 int ff_thread_video_encode_frame(AVCodecContext *avctx, AVPacket *pkt,