diff mbox series

[FFmpeg-devel] avformat/mpegts: Don't leave context in inconsistent state upon error

Message ID 20200810125047.3101-1-andreas.rheinhardt@gmail.com
State Accepted
Commit 1ead176d874acb489827ace3935fc71e1eea7e0e
Headers show
Series [FFmpeg-devel] avformat/mpegts: Don't leave context in inconsistent state upon error
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Aug. 10, 2020, 12:50 p.m. UTC
Up until now, opening a section filter works as follows: A filter is
opened and (on success) attached to the MpegTSContext. Then a buffer for
said filter is allocated and upon success attached to the section
filter; on error, the filter is simply freed without removing it from
the MpegTSContext, leaving the latter in an inconsistent state. This
leads to use-after-frees lateron.

This commit fixes this by allocating the buffer first; the filter is
only opened if the buffer could be successfully allocated.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
At first I wanted to use a flexible array member for this buffer (do we
actually support any compiler that doesn't support flexible array
members?), yet for some reason unknown to me a structure containing a
flexible array member must not be contained in another structure or
union (not even at the end, although it makes sense to do so). GCC gives
a warning in pedantic mode for this.

 libavformat/mpegts.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Marton Balint Aug. 10, 2020, 6:33 p.m. UTC | #1
On Mon, 10 Aug 2020, Andreas Rheinhardt wrote:

> Up until now, opening a section filter works as follows: A filter is
> opened and (on success) attached to the MpegTSContext. Then a buffer for
> said filter is allocated and upon success attached to the section
> filter; on error, the filter is simply freed without removing it from
> the MpegTSContext, leaving the latter in an inconsistent state. This
> leads to use-after-frees lateron.
>
> This commit fixes this by allocating the buffer first; the filter is
> only opened if the buffer could be successfully allocated.

LGTM, thanks.

Marton

>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> At first I wanted to use a flexible array member for this buffer (do we
> actually support any compiler that doesn't support flexible array
> members?), yet for some reason unknown to me a structure containing a
> flexible array member must not be contained in another structure or
> union (not even at the end, although it makes sense to do so). GCC gives
> a warning in pedantic mode for this.
>
> libavformat/mpegts.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> index c6fd3e1cef..f71f18a57d 100644
> --- a/libavformat/mpegts.c
> +++ b/libavformat/mpegts.c
> @@ -510,20 +510,22 @@ static MpegTSFilter *mpegts_open_section_filter(MpegTSContext *ts,
> {
>     MpegTSFilter *filter;
>     MpegTSSectionFilter *sec;
> +    uint8_t *section_buf = av_mallocz(MAX_SECTION_SIZE);
> 
> -    if (!(filter = mpegts_open_filter(ts, pid, MPEGTS_SECTION)))
> +    if (!section_buf)
>         return NULL;
> +
> +    if (!(filter = mpegts_open_filter(ts, pid, MPEGTS_SECTION))) {
> +        av_free(section_buf);
> +        return NULL;
> +    }
>     sec = &filter->u.section_filter;
>     sec->section_cb  = section_cb;
>     sec->opaque      = opaque;
> -    sec->section_buf = av_mallocz(MAX_SECTION_SIZE);
> +    sec->section_buf = section_buf;
>     sec->check_crc   = check_crc;
>     sec->last_ver    = -1;
> 
> -    if (!sec->section_buf) {
> -        av_free(filter);
> -        return NULL;
> -    }
>     return filter;
> }
> 
> -- 
> 2.20.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
index c6fd3e1cef..f71f18a57d 100644
--- a/libavformat/mpegts.c
+++ b/libavformat/mpegts.c
@@ -510,20 +510,22 @@  static MpegTSFilter *mpegts_open_section_filter(MpegTSContext *ts,
 {
     MpegTSFilter *filter;
     MpegTSSectionFilter *sec;
+    uint8_t *section_buf = av_mallocz(MAX_SECTION_SIZE);
 
-    if (!(filter = mpegts_open_filter(ts, pid, MPEGTS_SECTION)))
+    if (!section_buf)
         return NULL;
+
+    if (!(filter = mpegts_open_filter(ts, pid, MPEGTS_SECTION))) {
+        av_free(section_buf);
+        return NULL;
+    }
     sec = &filter->u.section_filter;
     sec->section_cb  = section_cb;
     sec->opaque      = opaque;
-    sec->section_buf = av_mallocz(MAX_SECTION_SIZE);
+    sec->section_buf = section_buf;
     sec->check_crc   = check_crc;
     sec->last_ver    = -1;
 
-    if (!sec->section_buf) {
-        av_free(filter);
-        return NULL;
-    }
     return filter;
 }