diff mbox series

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

Message ID 20210608170812.18976-1-robertbeyer@woodgrovetech.com
State New
Headers show
Series [FFmpeg-devel] Null check of &s->internal before attempting to free dict and pkt - avoid Null pointer dereference crash
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Robert Beyer June 8, 2021, 5:08 p.m. UTC
---
 libavformat/utils.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Andreas Rheinhardt June 8, 2021, 5:20 p.m. UTC | #1
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
Robert Beyer June 8, 2021, 5:28 p.m. UTC | #2
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)

- Robert.

-----Original Message-----
From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas Rheinhardt
Sent: June 8, 2021 1:21 PM
To: ffmpeg-devel@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
Andreas Rheinhardt June 8, 2021, 5:38 p.m. UTC | #3
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@ffmpeg.org> On Behalf Of Andreas Rheinhardt
> Sent: June 8, 2021 1:21 PM
> To: ffmpeg-devel@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
Robert Beyer June 8, 2021, 5:47 p.m. UTC | #4
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas Rheinhardt
> Sent: June 8, 2021 1:21 PM
> To: ffmpeg-devel@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@ffmpeg.org> On Behalf Of Andreas Rheinhardt
> Sent: June 8, 2021 1:38 PM
> To: ffmpeg-devel@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.
Andreas Rheinhardt June 8, 2021, 5:58 p.m. UTC | #5
Robert Beyer:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas Rheinhardt
>> Sent: June 8, 2021 1:21 PM
>> To: ffmpeg-devel@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@ffmpeg.org> On Behalf Of Andreas Rheinhardt
>> Sent: June 8, 2021 1:38 PM
>> To: ffmpeg-devel@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.
>
diff mbox series

Patch

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