Michael Niedermayer: > On Fri, Mar 19, 2021 at 04:39:59PM +0100, Andreas Rheinhardt wrote: >> Michael Niedermayer: >>> Fixes: Null pointer dereference >>> Fixes: ff_h264_remove_all_refs.mp4 >>> >>> Found-by: Rafael Dutra <rafael.du...@cispa.de> >>> Tested-by: Rafael Dutra <rafael.du...@cispa.de> >>> Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc> >>> --- >>> libavcodec/pthread_frame.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c >>> index 7bcb9a7bcc..048e535cb6 100644 >>> --- a/libavcodec/pthread_frame.c >>> +++ b/libavcodec/pthread_frame.c >>> @@ -708,7 +708,7 @@ void ff_frame_thread_free(AVCodecContext *avctx, int >>> thread_count) >>> pthread_join(p->thread, NULL); >>> p->thread_init=0; >>> >>> - if (codec->close && p->avctx) >>> + if (codec->close && p->avctx && p->avctx->priv_data) >>> codec->close(p->avctx); >>> >>> #if FF_API_THREAD_SAFE_CALLBACKS >>> >> This does not fix the whole issue: A codec without >> FF_CODEC_CAP_INIT_CLEANUP set might already have cleaned up internally >> on error and it might not be safe to do it again; and calling close >> before having called init (if existing) is not allowed for any codec. > > Well, this was about the failure to allocate priv_data in the first place > not sure how priv_data would become null from a codec internal cleanup >
It wouldn't (unless the codec would do something weird) and your patch will certainly not hurt, but not every close function is idempotent. If a codec allocates an array of something that is not a POD and frees both the suballocations as well as the actual array, but does not reset the count field, the second time a close function is called the attempt to free the suballocations will crash. Codecs that are not init-threadsafe might also pose problems: The close function of exr must only be called if init succeeded. > >> >> I have already sent a patch for this (which needs to be slightly updated >> due to James having added a new failure path (an av_packet_alloc)) here: >> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20210211155759.309391-1-andreas.rheinha...@gmail.com/ >> I will see whether Nuo's suggestions would lead to an improvement. > > well, if your patchset handles it sure, fine with me. Also note that in > general these cispa testcases do not reproduce for me, they are very sensitiv > to > what the memory limit is and even though ive been provided with a script to > search for this limit, generally the way i tested was to explicitly set the > failing alloc to a forced =NULL. Also meaning i cannot really test if > alternative patches fix these testcases. Just manually set the malloc to NULL > for testing My typical way to test failing allocations is to add an av_max_alloc statement before the allocation. > > I also assume none of these things need the release/4.4 branching to be > delayed and can be backported. > If i should wait with branching release/4.4 then please tell me > I see no reason for a delay. - 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-requ...@ffmpeg.org with subject "unsubscribe".
