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

[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


> 
> 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     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If the United States is serious about tackling the national security threats 
related to an insecure 5G network, it needs to rethink the extent to which it
values corporate profits and government espionage over security.-Bruce Schneier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210319/81919ae7/attachment.sig>