| Message ID | 20240323140054.4222-1-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
Quoting Andreas Rheinhardt (2024-03-23 15:11:59) > 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. So?
Anton Khirnov: > Quoting Andreas Rheinhardt (2024-03-23 15:11:59) >> 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. > > So? > It means there is a scenario where you break something. - Andreas
Quoting Andreas Rheinhardt (2024-03-24 11:19:19) > Anton Khirnov: > > Quoting Andreas Rheinhardt (2024-03-23 15:11:59) > >> 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. > > > > So? > > > > It means there is a scenario where you break something. There is no way for the caller to know whether, and how much, will lavc read from those tables, so it's invalid API use.
Anton Khirnov: > Quoting Andreas Rheinhardt (2024-03-24 11:19:19) >> Anton Khirnov: >>> Quoting Andreas Rheinhardt (2024-03-23 15:11:59) >>>> 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. >>> >>> So? >>> >> >> It means there is a scenario where you break something. > > There is no way for the caller to know whether, and how much, will lavc > read from those tables, so it's invalid API use. > Incorrect: Given that these fields do not a length field, it is legal to put pointers to matrices of arbitrary length in the relevant AVCodecContext field. Just because the current code presumes this to have at least 64 elements for certain mpegvideo encoders and just because there are no other encoders using different values does not make it illegal. The reason we are in this predicament is of course that the API itself is broken. - Andreas
Quoting Andreas Rheinhardt (2024-03-24 12:10:24) > Anton Khirnov: > > Quoting Andreas Rheinhardt (2024-03-24 11:19:19) > >> Anton Khirnov: > >>> Quoting Andreas Rheinhardt (2024-03-23 15:11:59) > >>>> 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. > >>> > >>> So? > >>> > >> > >> It means there is a scenario where you break something. > > > > There is no way for the caller to know whether, and how much, will lavc > > read from those tables, so it's invalid API use. > > > > Incorrect: Given that these fields do not a length field, it is legal to > put pointers to matrices of arbitrary length in the relevant > AVCodecContext field. No. Users are not allowed to write random values to random places. The only meaningful way to use these arrays is with 64 elements, therefore any caller using a different number is broken.
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);