| Message ID | 20210813174815.13273-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/13/2021 2:48 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 | 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); Should be ok.
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 | 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 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.) Notice that the second issue exists even when initializing succeeds. (@James: Using avcodec_free_context() is even more problematic because this also frees (intra|inter)_matrix, to fields which are allowed to be set by the user. It also frees rc_override although the documentation says it is freed by the user.) - Andreas
On Fri, Aug 13, 2021 at 03:22:41PM -0300, James Almer wrote: > On 8/13/2021 2:48 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 | 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); > > Should be ok. will apply thx [...]
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 <michael@niedermayer.cc> > > --- > > 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 thx [...]
On 8/13/2021 3:51 PM, Michael Niedermayer wrote: > 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 <michael@niedermayer.cc> >>> --- >>> 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 ? We should not av_assert on user set arguments like this. Since no encoder uses frame threading, and none will, a simple check and EINVAL, or simply setting hw_frames_ctx to NULL on the copied contexts (same as with internal), should be enough. > > >> 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 > > thx > > [...] > > > _______________________________________________ > 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". >
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 <michael@niedermayer.cc> >>> --- >>> 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
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);
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 | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)