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

[Andreas Rheinhardt]

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

>From the documentation: "Note that a user-supplied AVFormatContext will
be freed on 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.

Then you need to make sure that these pointers are in sync after
avformat_open_input(). The easiest way is to use the pointer in the
class object itself (if that is possible). But anyway, this is no longer
about developing FFmpeg itself, so it doesn't belong on this list any more.

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

It can never be NULL; if it is ever NULL, we should crash so that the
bug that made it NULL is not silently ignored (in your case it meant
that a use-after-free (which already happened before using s->internal
in any way) has been found).

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