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

[Andreas Rheinhardt]

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

- Andreas

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

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