Michael Niedermayer: > On Fri, Aug 13, 2021 at 08:34:13PM +0200, Andreas Rheinhardt wrote: >> Michael Niedermayer: >>> Fixes: MemLeak >>> Fixes: 8281 >>> Fixes: PoC_option158.jpg >>> Fixes: CVE-2020-22037 >>> >>> Signed-off-by: Michael Niedermayer <[email protected]> >>> --- >>> libavcodec/frame_thread_encoder.c | 10 ++++++---- >>> 1 file changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/libavcodec/frame_thread_encoder.c >>> b/libavcodec/frame_thread_encoder.c >>> index 9cabfc495f..25a308173d 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,16 @@ 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; >>> 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 +233,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); >>> >> This has some presumptions: First, it relies on undocumented behaviour >> from av_opt_copy(), see here: >> https://ffmpeg.org/pipermail/ffmpeg-devel/2021-August/283645.html > > Its documented after your patch ;) > > >> Second, you presume that hw_frames_ctx is not set, although it is a >> valid field for an encoder (although no hardware encoder seems to have >> the frame threading flag set). If it were set, it would be freed >> multiple times. (The same goes for several other fields that are not >> supposed to be set by encoders, but IMO that would be a user problem.) > > What do you suggest, should i add an av_assert on hw_frames_ctx==NULL ? > > >> Notice that the second issue exists even when initializing succeeds. > > then its unrelated to the patch, of course we still need to fix it. > > More specifically, should i change the patch ? > the hw_frames_ctx seems a bug but unrelated, the undocumentedness is > fixed by you. > the code feels fragile and not particularly robust before this already > and fixing this adding nicer and robust API for all steps cant be backported > and i need to backport the fix so we need a fix without new&nice API > I'd just set hw_frames_ctx to NULL at the same place where you set internal to NULL; and add a comment about this that frame threaded hardware encoders are just not supported atm.
- Andreas _______________________________________________ ffmpeg-devel mailing list [email protected] https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email [email protected] with subject "unsubscribe".
