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

Fixes: Null pointer dereference
Fixes: ff_h264_remove_all_refs.mp4

Found-by: Rafael Dutra <rafael.dutra@cispa.de>
Tested-by: Rafael Dutra <rafael.dutra@cispa.de>
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/pthread_frame.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Michael Niedermayer:
> Fixes: Null pointer dereference
> Fixes: ff_h264_remove_all_refs.mp4
> 
> Found-by: Rafael Dutra <rafael.dutra@cispa.de>
> Tested-by: Rafael Dutra <rafael.dutra@cispa.de>
> Signed-off-by: Michael Niedermayer <michael@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.

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.

- Andreas

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


> 
> 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

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

thanks

[...]

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