[FFmpeg-devel] [PATCH 2/2] avcodec/pthread_frame: Only attempt to close decoders which have allocated private data

[Andreas Rheinhardt]

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.dutra at cispa.de>
>>> Tested-by: Rafael Dutra <rafael.dutra at cispa.de>
>>> Signed-off-by: Michael Niedermayer <michael at 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.rheinhardt@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