[FFmpeg-devel] [PATCH] Null check of &s->internal before attempting to free dict and pkt - avoid Null pointer dereference crash

[Robert Beyer]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Andreas Rheinhardt
> Sent: June 8, 2021 1:21 PM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] Null check of &s->internal before attempting to free dict and pkt - avoid Null pointer dereference crash
> 
> Robert Beyer:
>> ---
>>  libavformat/utils.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>> index fe8eaa6cb3..73a7d13123 100644
>> --- a/libavformat/utils.c
>> +++ b/libavformat/utils.c
>> @@ -4331,9 +4331,11 @@ void avformat_free_context(AVFormatContext *s)
>>      }
>>      av_freep(&s->chapters);
>>      av_dict_free(&s->metadata);
>> -    av_dict_free(&s->internal->id3v2_meta);
>> -    av_packet_free(&s->internal->pkt);
>> -    av_packet_free(&s->internal->parse_pkt);
>> +    if (&s->internal) {
>> +        av_dict_free(&s->internal->id3v2_meta);
>> +        av_packet_free(&s->internal->pkt);
>> +        av_packet_free(&s->internal->parse_pkt);
>> +    }
>>      av_freep(&s->streams);
>>      flush_packet_queue(s);
>>      av_freep(&s->internal);
>>
> 1. Checking for &s->internal is nonsense: If s is not NULL and points to
> an AVFormatContext, &s->internal is so, too. You want to check for
> s->internal.
> 2. avformat_alloc_context() (the only function that directly allocates
> AVFormatContexts) ensures that every successfully allocated
> AVFormatContext has an AVFormatInternal set, so the issue should just
> not happen. If it does happen for you, then please provide the necessary
> details to reproduce it.
> 
> - Andreas

> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Andreas Rheinhardt
> Sent: June 8, 2021 1:38 PM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] Null check of &s->internal before attempting to free dict and pkt - avoid Null pointer dereference crash
> 
> Robert Beyer:
> > Andreas Rheinhardt:
> > 
> > In my test case:
> >   avformat_open_input(pInputContext, ...) returns an error code
> > > Attempts to free the (allocated?) context memory then causes a NULL
> reference exception when accessing &s->internal->id3v2_meta, etc. since
> &s->internal is NULL.
> >   avformat_free_context(pInputContext)
> > 
> 
> If avformat_open_input() returns an error, then it has freed the
> AVFormatContext itself (if it was provided one) and set the pointer to
> the AVFormatContext to NULL.

Thank you - it's not obvious that the context is automatically freed on avformat_open_input failure.

> Using this pointer in
> avformat_free_context() is safe, because of the very first check (for
> whether the AVFormatContext is NULL) in avformat_free_context(). But if
> you used a preallocated AVFormatContext (I guess you do?) in
> avformat_open_input() and use multiple pointers to said AVFormatContext,
> then all the other pointers (except the one actually used in
> avformat_open_input()) have become dangling and using them in
> avformat_free_context() is a use-after-free.

And there's the bug! I have the context pointer allocated and retained in a class, but dereferenced in the open call, which results in a use-after-free from the pointer in the class object.

You're also correct about the (s->internal) check ... wouldn't hurt to add it, in case internal is/can ever be null.

Thank you.

> - Andreas
> 
> PS: AVFormatContexts used in conjunction with avformat_open_input() need
> to be closed by avformat_close_input().
> PPS: Don't top-post here.

Fixed. Thanks ... learning the ropes/rules.

Cheers,
    Robert.