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 <mich...@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-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
