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 | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
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 --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; }
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(-)