> -Original Message-
> From: ffmpeg-devel 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 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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
