| 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 | expand |
| 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 |
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: 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
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
> -----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.
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 --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);