Re: [FFmpeg-devel] [PATCH v2 08/12] lavc/frame_thread_encoder: avoid assigning a whole AVCodecContext

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Anton Khirnov:
> It is highly unsafe, as AVCodecContext contains many allocated fields.
> Everything needed by worked threads should be covered by
> * routing through AVCodecParameters
> * av_opt_copy()
> * copying quantisation matrices manually
> 
> avcodec_free_context() can now be used for per-thread contexts.
> ---
>  libavcodec/frame_thread_encoder.c | 44 ++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/frame_thread_encoder.c b/libavcodec/frame_thread_encoder.c
> index cda5158117..5ea6386dfb 100644
> --- a/libavcodec/frame_thread_encoder.c
> +++ b/libavcodec/frame_thread_encoder.c
> @@ -28,6 +28,7 @@
>  #include "libavutil/thread.h"
>  #include "avcodec.h"
>  #include "avcodec_internal.h"
> +#include "codec_par.h"
>  #include "encode.h"
>  #include "internal.h"
>  #include "pthread_internal.h"
> @@ -111,8 +112,7 @@ static void * attribute_align_arg worker(void *v){
>          pthread_mutex_unlock(&c->finished_task_mutex);
>      }
>  end:
> -    ff_codec_close(avctx);
> -    av_freep(&avctx);
> +    avcodec_free_context(&avctx);
>      return NULL;
>  }
>  
> @@ -121,6 +121,7 @@ av_cold int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>      int i=0;
>      ThreadContext *c;
>      AVCodecContext *thread_avctx = NULL;
> +    AVCodecParameters *par = NULL;
>      int ret;
>  
>      if(   !(avctx->thread_type & FF_THREAD_FRAME)
> @@ -194,18 +195,27 @@ av_cold int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>          }
>      }
>  
> +    par = avcodec_parameters_alloc();
> +    if (!par) {
> +        ret = AVERROR(ENOMEM);
> +        goto fail;
> +    }
> +
> +    ret = avcodec_parameters_from_context(par, avctx);
> +    if (ret < 0)
> +        goto fail;
> +
>      for(i=0; i<avctx->thread_count ; i++){
> -        void *tmpv;
>          thread_avctx = avcodec_alloc_context3(avctx->codec);
>          if (!thread_avctx) {
>              ret = AVERROR(ENOMEM);
>              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 = avcodec_parameters_to_context(thread_avctx, par);
> +        if (ret < 0)
> +            goto fail;
> +
>          ret = av_opt_copy(thread_avctx, avctx);
>          if (ret < 0)
>              goto fail;
> @@ -217,6 +227,18 @@ av_cold int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>          thread_avctx->thread_count = 1;
>          thread_avctx->active_thread_type &= ~FF_THREAD_FRAME;
>  
> +#define DUP_MATRIX(m)                                                       \
> +        if (avctx->m) {                                                     \
> +            thread_avctx->m = av_memdup(avctx->m, 64 * sizeof(avctx->m));   \
> +            if (!thread_avctx->m) {                                         \
> +                ret = AVERROR(ENOMEM);                                      \
> +                goto fail;                                                  \
> +            }                                                               \
> +        }
> +        DUP_MATRIX(intra_matrix);
> +        DUP_MATRIX(chroma_intra_matrix);
> +        DUP_MATRIX(inter_matrix);
> +
>          if ((ret = avcodec_open2(thread_avctx, avctx->codec, NULL)) < 0)
>              goto fail;
>          av_assert0(!thread_avctx->internal->frame_thread_encoder);
> @@ -227,12 +249,14 @@ av_cold int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>          }
>      }
>  
> +    avcodec_parameters_free(&par);
> +
>      avctx->active_thread_type = FF_THREAD_FRAME;
>  
>      return 0;
>  fail:
> -    ff_codec_close(thread_avctx);
> -    av_freep(&thread_avctx);
> +    avcodec_parameters_free(&par);
> +    avcodec_free_context(&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);

1. The earlier code would just work in case the user used a smaller
number of elements for the matrices if these matrices were not used at
all (which happens for the majority of encoders). This is no longer true
with this patch.
2. Did you test this? Did it work? "av_memdup(avctx->m, 64 *
sizeof(avctx->m))" uses sizeof a pointer and therefore leads to a buffer
overflow.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".