| Message ID | 20190419220316.47392-2-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
Andreas Rheinhardt: > The earlier version of the webm_chunk muxer had several bugs: > > 1. If the first packet of an audio stream didn't have a PTS of zero, > then no chunk will be started before a packet is delivered to the > underlying Matroska/WebM muxer, i.e. the AVFormatContext used to write > these packets had a NULL as AVIOContext for output. This is behind the > crash in ticket #5752. > > 2. If an error happens during writing a packet, the underlyimg > Matroska/WebM muxer context is freed. This leads to a use-after-free > coupled with a double-free in webm_chunk_write_trailer (which supposes > that the underlying AVFormatContext is still valid). > > 3. Even when no error occurs at all, webm_chunk_write_trailer is still > buggy: After the underlying Matroska/WebM muxer has written its trailer, > ending the chunk implicitly flushes it again which is illegal at this > point. > > These bugs have been fixed. > > Fixes #5752. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/webm_chunk.c | 44 +++++++++++++++++++++------------------- > 1 file changed, 23 insertions(+), 21 deletions(-) > > diff --git a/libavformat/webm_chunk.c b/libavformat/webm_chunk.c > index 2c99753b5b..391fee721a 100644 > --- a/libavformat/webm_chunk.c > +++ b/libavformat/webm_chunk.c > @@ -168,7 +168,7 @@ static int chunk_start(AVFormatContext *s) > return 0; > } > > -static int chunk_end(AVFormatContext *s) > +static int chunk_end(AVFormatContext *s, int flush) > { > WebMChunkContext *wc = s->priv_data; > AVFormatContext *oc = wc->avf; > @@ -179,11 +179,14 @@ static int chunk_end(AVFormatContext *s) > char filename[MAX_FILENAME_SIZE]; > AVDictionary *options = NULL; > > - if (wc->chunk_start_index == wc->chunk_index) > + if (!oc->pb) > return 0; > - // Flush the cluster in WebM muxer. > - oc->oformat->write_packet(oc, NULL); > + > + if (flush) > + // Flush the cluster in WebM muxer. > + oc->oformat->write_packet(oc, NULL); > buffer_size = avio_close_dyn_buf(oc->pb, &buffer); > + oc->pb = NULL; > ret = get_chunk_filename(s, 0, filename); > if (ret < 0) > goto fail; > @@ -194,7 +197,6 @@ static int chunk_end(AVFormatContext *s) > goto fail; > avio_write(pb, buffer, buffer_size); > ff_format_io_close(s, &pb); > - oc->pb = NULL; > fail: > av_dict_free(&options); > av_free(buffer); > @@ -216,27 +218,19 @@ static int webm_chunk_write_packet(AVFormatContext *s, AVPacket *pkt) > } > > // For video, a new chunk is started only on key frames. For audio, a new > - // chunk is started based on chunk_duration. > - if ((st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && > + // chunk is started based on chunk_duration. Also, a new chunk is started > + // unconditionally if there is no currently open chunk. > + if (!oc->pb || (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && > (pkt->flags & AV_PKT_FLAG_KEY)) || > (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && > - (pkt->pts == 0 || wc->duration_written >= wc->chunk_duration))) { > + wc->duration_written >= wc->chunk_duration)) { > wc->duration_written = 0; > - if ((ret = chunk_end(s)) < 0 || (ret = chunk_start(s)) < 0) { > - goto fail; > + if ((ret = chunk_end(s, 1)) < 0 || (ret = chunk_start(s)) < 0) { > + return ret; > } > } > > ret = oc->oformat->write_packet(oc, pkt); > - if (ret < 0) > - goto fail; > - > -fail: > - if (ret < 0) { > - oc->streams = NULL; > - oc->nb_streams = 0; > - avformat_free_context(oc); > - } > > return ret; > } > @@ -245,12 +239,20 @@ static int webm_chunk_write_trailer(AVFormatContext *s) > { > WebMChunkContext *wc = s->priv_data; > AVFormatContext *oc = wc->avf; > + int ret; > + > + if (!oc->pb) { > + ret = chunk_start(s); > + if (ret < 0) > + goto fail; > + } > oc->oformat->write_trailer(oc); > - chunk_end(s); > + ret = chunk_end(s, 0); > +fail: > oc->streams = NULL; > oc->nb_streams = 0; > avformat_free_context(oc); > - return 0; > + return ret; > } > > #define OFFSET(x) offsetof(WebMChunkContext, x) > > Ping for the last two patches of this patchset (i.e. this one and https://ffmpeg.org/pipermail/ffmpeg-devel/2019-April/242902.html) - Andreas
Andreas Rheinhardt: > Andreas Rheinhardt: >> The earlier version of the webm_chunk muxer had several bugs: >> >> 1. If the first packet of an audio stream didn't have a PTS of zero, >> then no chunk will be started before a packet is delivered to the >> underlying Matroska/WebM muxer, i.e. the AVFormatContext used to write >> these packets had a NULL as AVIOContext for output. This is behind the >> crash in ticket #5752. >> >> 2. If an error happens during writing a packet, the underlyimg >> Matroska/WebM muxer context is freed. This leads to a use-after-free >> coupled with a double-free in webm_chunk_write_trailer (which supposes >> that the underlying AVFormatContext is still valid). >> >> 3. Even when no error occurs at all, webm_chunk_write_trailer is still >> buggy: After the underlying Matroska/WebM muxer has written its trailer, >> ending the chunk implicitly flushes it again which is illegal at this >> point. >> >> These bugs have been fixed. >> >> Fixes #5752. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavformat/webm_chunk.c | 44 +++++++++++++++++++++------------------- >> 1 file changed, 23 insertions(+), 21 deletions(-) >> >> diff --git a/libavformat/webm_chunk.c b/libavformat/webm_chunk.c >> index 2c99753b5b..391fee721a 100644 >> --- a/libavformat/webm_chunk.c >> +++ b/libavformat/webm_chunk.c >> @@ -168,7 +168,7 @@ static int chunk_start(AVFormatContext *s) >> return 0; >> } >> >> -static int chunk_end(AVFormatContext *s) >> +static int chunk_end(AVFormatContext *s, int flush) >> { >> WebMChunkContext *wc = s->priv_data; >> AVFormatContext *oc = wc->avf; >> @@ -179,11 +179,14 @@ static int chunk_end(AVFormatContext *s) >> char filename[MAX_FILENAME_SIZE]; >> AVDictionary *options = NULL; >> >> - if (wc->chunk_start_index == wc->chunk_index) >> + if (!oc->pb) >> return 0; >> - // Flush the cluster in WebM muxer. >> - oc->oformat->write_packet(oc, NULL); >> + >> + if (flush) >> + // Flush the cluster in WebM muxer. >> + oc->oformat->write_packet(oc, NULL); >> buffer_size = avio_close_dyn_buf(oc->pb, &buffer); >> + oc->pb = NULL; >> ret = get_chunk_filename(s, 0, filename); >> if (ret < 0) >> goto fail; >> @@ -194,7 +197,6 @@ static int chunk_end(AVFormatContext *s) >> goto fail; >> avio_write(pb, buffer, buffer_size); >> ff_format_io_close(s, &pb); >> - oc->pb = NULL; >> fail: >> av_dict_free(&options); >> av_free(buffer); >> @@ -216,27 +218,19 @@ static int webm_chunk_write_packet(AVFormatContext *s, AVPacket *pkt) >> } >> >> // For video, a new chunk is started only on key frames. For audio, a new >> - // chunk is started based on chunk_duration. >> - if ((st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && >> + // chunk is started based on chunk_duration. Also, a new chunk is started >> + // unconditionally if there is no currently open chunk. >> + if (!oc->pb || (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && >> (pkt->flags & AV_PKT_FLAG_KEY)) || >> (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && >> - (pkt->pts == 0 || wc->duration_written >= wc->chunk_duration))) { >> + wc->duration_written >= wc->chunk_duration)) { >> wc->duration_written = 0; >> - if ((ret = chunk_end(s)) < 0 || (ret = chunk_start(s)) < 0) { >> - goto fail; >> + if ((ret = chunk_end(s, 1)) < 0 || (ret = chunk_start(s)) < 0) { >> + return ret; >> } >> } >> >> ret = oc->oformat->write_packet(oc, pkt); >> - if (ret < 0) >> - goto fail; >> - >> -fail: >> - if (ret < 0) { >> - oc->streams = NULL; >> - oc->nb_streams = 0; >> - avformat_free_context(oc); >> - } >> >> return ret; >> } >> @@ -245,12 +239,20 @@ static int webm_chunk_write_trailer(AVFormatContext *s) >> { >> WebMChunkContext *wc = s->priv_data; >> AVFormatContext *oc = wc->avf; >> + int ret; >> + >> + if (!oc->pb) { >> + ret = chunk_start(s); >> + if (ret < 0) >> + goto fail; >> + } >> oc->oformat->write_trailer(oc); >> - chunk_end(s); >> + ret = chunk_end(s, 0); >> +fail: >> oc->streams = NULL; >> oc->nb_streams = 0; >> avformat_free_context(oc); >> - return 0; >> + return ret; >> } >> >> #define OFFSET(x) offsetof(WebMChunkContext, x) >> >> > Ping for the last two patches of this patchset (i.e. this one and > https://ffmpeg.org/pipermail/ffmpeg-devel/2019-April/242902.html) > > - Andreas > And another ping for these two patches. - Andreas
Andreas Rheinhardt: > Andreas Rheinhardt: >> Andreas Rheinhardt: >>> The earlier version of the webm_chunk muxer had several bugs: >>> >>> 1. If the first packet of an audio stream didn't have a PTS of zero, >>> then no chunk will be started before a packet is delivered to the >>> underlying Matroska/WebM muxer, i.e. the AVFormatContext used to write >>> these packets had a NULL as AVIOContext for output. This is behind the >>> crash in ticket #5752. >>> >>> 2. If an error happens during writing a packet, the underlyimg >>> Matroska/WebM muxer context is freed. This leads to a use-after-free >>> coupled with a double-free in webm_chunk_write_trailer (which supposes >>> that the underlying AVFormatContext is still valid). >>> >>> 3. Even when no error occurs at all, webm_chunk_write_trailer is still >>> buggy: After the underlying Matroska/WebM muxer has written its trailer, >>> ending the chunk implicitly flushes it again which is illegal at this >>> point. >>> >>> These bugs have been fixed. >>> >>> Fixes #5752. >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>> --- >>> libavformat/webm_chunk.c | 44 +++++++++++++++++++++------------------- >>> 1 file changed, 23 insertions(+), 21 deletions(-) >>> >>> diff --git a/libavformat/webm_chunk.c b/libavformat/webm_chunk.c >>> index 2c99753b5b..391fee721a 100644 >>> --- a/libavformat/webm_chunk.c >>> +++ b/libavformat/webm_chunk.c >>> @@ -168,7 +168,7 @@ static int chunk_start(AVFormatContext *s) >>> return 0; >>> } >>> >>> -static int chunk_end(AVFormatContext *s) >>> +static int chunk_end(AVFormatContext *s, int flush) >>> { >>> WebMChunkContext *wc = s->priv_data; >>> AVFormatContext *oc = wc->avf; >>> @@ -179,11 +179,14 @@ static int chunk_end(AVFormatContext *s) >>> char filename[MAX_FILENAME_SIZE]; >>> AVDictionary *options = NULL; >>> >>> - if (wc->chunk_start_index == wc->chunk_index) >>> + if (!oc->pb) >>> return 0; >>> - // Flush the cluster in WebM muxer. >>> - oc->oformat->write_packet(oc, NULL); >>> + >>> + if (flush) >>> + // Flush the cluster in WebM muxer. >>> + oc->oformat->write_packet(oc, NULL); >>> buffer_size = avio_close_dyn_buf(oc->pb, &buffer); >>> + oc->pb = NULL; >>> ret = get_chunk_filename(s, 0, filename); >>> if (ret < 0) >>> goto fail; >>> @@ -194,7 +197,6 @@ static int chunk_end(AVFormatContext *s) >>> goto fail; >>> avio_write(pb, buffer, buffer_size); >>> ff_format_io_close(s, &pb); >>> - oc->pb = NULL; >>> fail: >>> av_dict_free(&options); >>> av_free(buffer); >>> @@ -216,27 +218,19 @@ static int webm_chunk_write_packet(AVFormatContext *s, AVPacket *pkt) >>> } >>> >>> // For video, a new chunk is started only on key frames. For audio, a new >>> - // chunk is started based on chunk_duration. >>> - if ((st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && >>> + // chunk is started based on chunk_duration. Also, a new chunk is started >>> + // unconditionally if there is no currently open chunk. >>> + if (!oc->pb || (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && >>> (pkt->flags & AV_PKT_FLAG_KEY)) || >>> (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && >>> - (pkt->pts == 0 || wc->duration_written >= wc->chunk_duration))) { >>> + wc->duration_written >= wc->chunk_duration)) { >>> wc->duration_written = 0; >>> - if ((ret = chunk_end(s)) < 0 || (ret = chunk_start(s)) < 0) { >>> - goto fail; >>> + if ((ret = chunk_end(s, 1)) < 0 || (ret = chunk_start(s)) < 0) { >>> + return ret; >>> } >>> } >>> >>> ret = oc->oformat->write_packet(oc, pkt); >>> - if (ret < 0) >>> - goto fail; >>> - >>> -fail: >>> - if (ret < 0) { >>> - oc->streams = NULL; >>> - oc->nb_streams = 0; >>> - avformat_free_context(oc); >>> - } >>> >>> return ret; >>> } >>> @@ -245,12 +239,20 @@ static int webm_chunk_write_trailer(AVFormatContext *s) >>> { >>> WebMChunkContext *wc = s->priv_data; >>> AVFormatContext *oc = wc->avf; >>> + int ret; >>> + >>> + if (!oc->pb) { >>> + ret = chunk_start(s); >>> + if (ret < 0) >>> + goto fail; >>> + } >>> oc->oformat->write_trailer(oc); >>> - chunk_end(s); >>> + ret = chunk_end(s, 0); >>> +fail: >>> oc->streams = NULL; >>> oc->nb_streams = 0; >>> avformat_free_context(oc); >>> - return 0; >>> + return ret; >>> } >>> >>> #define OFFSET(x) offsetof(WebMChunkContext, x) >>> >>> >> Ping for the last two patches of this patchset (i.e. this one and >> https://ffmpeg.org/pipermail/ffmpeg-devel/2019-April/242902.html) >> >> - Andreas >> > And another ping for these two patches. > > - Andreas > Ping #3. - Andreas
Andreas Rheinhardt: > Andreas Rheinhardt: >> Andreas Rheinhardt: >>> Andreas Rheinhardt: >>>> The earlier version of the webm_chunk muxer had several bugs: >>>> >>>> 1. If the first packet of an audio stream didn't have a PTS of zero, >>>> then no chunk will be started before a packet is delivered to the >>>> underlying Matroska/WebM muxer, i.e. the AVFormatContext used to write >>>> these packets had a NULL as AVIOContext for output. This is behind the >>>> crash in ticket #5752. >>>> >>>> 2. If an error happens during writing a packet, the underlyimg >>>> Matroska/WebM muxer context is freed. This leads to a use-after-free >>>> coupled with a double-free in webm_chunk_write_trailer (which supposes >>>> that the underlying AVFormatContext is still valid). >>>> >>>> 3. Even when no error occurs at all, webm_chunk_write_trailer is still >>>> buggy: After the underlying Matroska/WebM muxer has written its trailer, >>>> ending the chunk implicitly flushes it again which is illegal at this >>>> point. >>>> >>>> These bugs have been fixed. >>>> >>>> Fixes #5752. >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>> --- >>>> libavformat/webm_chunk.c | 44 +++++++++++++++++++++------------------- >>>> 1 file changed, 23 insertions(+), 21 deletions(-) >>>> >>>> diff --git a/libavformat/webm_chunk.c b/libavformat/webm_chunk.c >>>> index 2c99753b5b..391fee721a 100644 >>>> --- a/libavformat/webm_chunk.c >>>> +++ b/libavformat/webm_chunk.c >>>> @@ -168,7 +168,7 @@ static int chunk_start(AVFormatContext *s) >>>> return 0; >>>> } >>>> >>>> -static int chunk_end(AVFormatContext *s) >>>> +static int chunk_end(AVFormatContext *s, int flush) >>>> { >>>> WebMChunkContext *wc = s->priv_data; >>>> AVFormatContext *oc = wc->avf; >>>> @@ -179,11 +179,14 @@ static int chunk_end(AVFormatContext *s) >>>> char filename[MAX_FILENAME_SIZE]; >>>> AVDictionary *options = NULL; >>>> >>>> - if (wc->chunk_start_index == wc->chunk_index) >>>> + if (!oc->pb) >>>> return 0; >>>> - // Flush the cluster in WebM muxer. >>>> - oc->oformat->write_packet(oc, NULL); >>>> + >>>> + if (flush) >>>> + // Flush the cluster in WebM muxer. >>>> + oc->oformat->write_packet(oc, NULL); >>>> buffer_size = avio_close_dyn_buf(oc->pb, &buffer); >>>> + oc->pb = NULL; >>>> ret = get_chunk_filename(s, 0, filename); >>>> if (ret < 0) >>>> goto fail; >>>> @@ -194,7 +197,6 @@ static int chunk_end(AVFormatContext *s) >>>> goto fail; >>>> avio_write(pb, buffer, buffer_size); >>>> ff_format_io_close(s, &pb); >>>> - oc->pb = NULL; >>>> fail: >>>> av_dict_free(&options); >>>> av_free(buffer); >>>> @@ -216,27 +218,19 @@ static int webm_chunk_write_packet(AVFormatContext *s, AVPacket *pkt) >>>> } >>>> >>>> // For video, a new chunk is started only on key frames. For audio, a new >>>> - // chunk is started based on chunk_duration. >>>> - if ((st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && >>>> + // chunk is started based on chunk_duration. Also, a new chunk is started >>>> + // unconditionally if there is no currently open chunk. >>>> + if (!oc->pb || (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && >>>> (pkt->flags & AV_PKT_FLAG_KEY)) || >>>> (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && >>>> - (pkt->pts == 0 || wc->duration_written >= wc->chunk_duration))) { >>>> + wc->duration_written >= wc->chunk_duration)) { >>>> wc->duration_written = 0; >>>> - if ((ret = chunk_end(s)) < 0 || (ret = chunk_start(s)) < 0) { >>>> - goto fail; >>>> + if ((ret = chunk_end(s, 1)) < 0 || (ret = chunk_start(s)) < 0) { >>>> + return ret; >>>> } >>>> } >>>> >>>> ret = oc->oformat->write_packet(oc, pkt); >>>> - if (ret < 0) >>>> - goto fail; >>>> - >>>> -fail: >>>> - if (ret < 0) { >>>> - oc->streams = NULL; >>>> - oc->nb_streams = 0; >>>> - avformat_free_context(oc); >>>> - } >>>> >>>> return ret; >>>> } >>>> @@ -245,12 +239,20 @@ static int webm_chunk_write_trailer(AVFormatContext *s) >>>> { >>>> WebMChunkContext *wc = s->priv_data; >>>> AVFormatContext *oc = wc->avf; >>>> + int ret; >>>> + >>>> + if (!oc->pb) { >>>> + ret = chunk_start(s); >>>> + if (ret < 0) >>>> + goto fail; >>>> + } >>>> oc->oformat->write_trailer(oc); >>>> - chunk_end(s); >>>> + ret = chunk_end(s, 0); >>>> +fail: >>>> oc->streams = NULL; >>>> oc->nb_streams = 0; >>>> avformat_free_context(oc); >>>> - return 0; >>>> + return ret; >>>> } >>>> >>>> #define OFFSET(x) offsetof(WebMChunkContext, x) >>>> >>>> >>> Ping for the last two patches of this patchset (i.e. this one and >>> https://ffmpeg.org/pipermail/ffmpeg-devel/2019-April/242902.html) >>> >>> - Andreas >>> >> And another ping for these two patches. >> >> - Andreas >> > > Ping #3. > > - Andreas > Ping. - Andreas
On 7/12/19, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > Andreas Rheinhardt: >> Andreas Rheinhardt: >>> Andreas Rheinhardt: >>>> Andreas Rheinhardt: >>>>> The earlier version of the webm_chunk muxer had several bugs: >>>>> >>>>> 1. If the first packet of an audio stream didn't have a PTS of zero, >>>>> then no chunk will be started before a packet is delivered to the >>>>> underlying Matroska/WebM muxer, i.e. the AVFormatContext used to write >>>>> these packets had a NULL as AVIOContext for output. This is behind the >>>>> crash in ticket #5752. >>>>> >>>>> 2. If an error happens during writing a packet, the underlyimg >>>>> Matroska/WebM muxer context is freed. This leads to a use-after-free >>>>> coupled with a double-free in webm_chunk_write_trailer (which supposes >>>>> that the underlying AVFormatContext is still valid). >>>>> >>>>> 3. Even when no error occurs at all, webm_chunk_write_trailer is still >>>>> buggy: After the underlying Matroska/WebM muxer has written its >>>>> trailer, >>>>> ending the chunk implicitly flushes it again which is illegal at this >>>>> point. >>>>> >>>>> These bugs have been fixed. >>>>> >>>>> Fixes #5752. >>>>> >>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>>> --- >>>>> libavformat/webm_chunk.c | 44 +++++++++++++++++++++------------------- >>>>> 1 file changed, 23 insertions(+), 21 deletions(-) >>>>> >>>>> diff --git a/libavformat/webm_chunk.c b/libavformat/webm_chunk.c >>>>> index 2c99753b5b..391fee721a 100644 >>>>> --- a/libavformat/webm_chunk.c >>>>> +++ b/libavformat/webm_chunk.c >>>>> @@ -168,7 +168,7 @@ static int chunk_start(AVFormatContext *s) >>>>> return 0; >>>>> } >>>>> >>>>> -static int chunk_end(AVFormatContext *s) >>>>> +static int chunk_end(AVFormatContext *s, int flush) >>>>> { >>>>> WebMChunkContext *wc = s->priv_data; >>>>> AVFormatContext *oc = wc->avf; >>>>> @@ -179,11 +179,14 @@ static int chunk_end(AVFormatContext *s) >>>>> char filename[MAX_FILENAME_SIZE]; >>>>> AVDictionary *options = NULL; >>>>> >>>>> - if (wc->chunk_start_index == wc->chunk_index) >>>>> + if (!oc->pb) >>>>> return 0; >>>>> - // Flush the cluster in WebM muxer. >>>>> - oc->oformat->write_packet(oc, NULL); >>>>> + >>>>> + if (flush) >>>>> + // Flush the cluster in WebM muxer. >>>>> + oc->oformat->write_packet(oc, NULL); >>>>> buffer_size = avio_close_dyn_buf(oc->pb, &buffer); >>>>> + oc->pb = NULL; >>>>> ret = get_chunk_filename(s, 0, filename); >>>>> if (ret < 0) >>>>> goto fail; >>>>> @@ -194,7 +197,6 @@ static int chunk_end(AVFormatContext *s) >>>>> goto fail; >>>>> avio_write(pb, buffer, buffer_size); >>>>> ff_format_io_close(s, &pb); >>>>> - oc->pb = NULL; >>>>> fail: >>>>> av_dict_free(&options); >>>>> av_free(buffer); >>>>> @@ -216,27 +218,19 @@ static int >>>>> webm_chunk_write_packet(AVFormatContext *s, AVPacket *pkt) >>>>> } >>>>> >>>>> // For video, a new chunk is started only on key frames. For >>>>> audio, a new >>>>> - // chunk is started based on chunk_duration. >>>>> - if ((st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && >>>>> + // chunk is started based on chunk_duration. Also, a new chunk is >>>>> started >>>>> + // unconditionally if there is no currently open chunk. >>>>> + if (!oc->pb || (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && >>>>> (pkt->flags & AV_PKT_FLAG_KEY)) || >>>>> (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && >>>>> - (pkt->pts == 0 || wc->duration_written >= >>>>> wc->chunk_duration))) { >>>>> + wc->duration_written >= wc->chunk_duration)) { >>>>> wc->duration_written = 0; >>>>> - if ((ret = chunk_end(s)) < 0 || (ret = chunk_start(s)) < 0) { >>>>> - goto fail; >>>>> + if ((ret = chunk_end(s, 1)) < 0 || (ret = chunk_start(s)) < 0) >>>>> { >>>>> + return ret; >>>>> } >>>>> } >>>>> >>>>> ret = oc->oformat->write_packet(oc, pkt); >>>>> - if (ret < 0) >>>>> - goto fail; >>>>> - >>>>> -fail: >>>>> - if (ret < 0) { >>>>> - oc->streams = NULL; >>>>> - oc->nb_streams = 0; >>>>> - avformat_free_context(oc); >>>>> - } >>>>> >>>>> return ret; >>>>> } >>>>> @@ -245,12 +239,20 @@ static int >>>>> webm_chunk_write_trailer(AVFormatContext *s) >>>>> { >>>>> WebMChunkContext *wc = s->priv_data; >>>>> AVFormatContext *oc = wc->avf; >>>>> + int ret; >>>>> + >>>>> + if (!oc->pb) { >>>>> + ret = chunk_start(s); >>>>> + if (ret < 0) >>>>> + goto fail; >>>>> + } >>>>> oc->oformat->write_trailer(oc); >>>>> - chunk_end(s); >>>>> + ret = chunk_end(s, 0); >>>>> +fail: >>>>> oc->streams = NULL; >>>>> oc->nb_streams = 0; >>>>> avformat_free_context(oc); >>>>> - return 0; >>>>> + return ret; >>>>> } >>>>> >>>>> #define OFFSET(x) offsetof(WebMChunkContext, x) >>>>> >>>>> >>>> Ping for the last two patches of this patchset (i.e. this one and >>>> https://ffmpeg.org/pipermail/ffmpeg-devel/2019-April/242902.html) >>>> >>>> - Andreas >>>> >>> And another ping for these two patches. >>> >>> - Andreas >>> >> >> Ping #3. >> >> - Andreas >> > Ping. Will apply first patch from this thread. Is this OK? > > - Andreas > > _______________________________________________ > 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".
Paul B Mahol: > On 7/12/19, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: >> Andreas Rheinhardt: >>> Andreas Rheinhardt: >>>> Andreas Rheinhardt: >>>>> Andreas Rheinhardt: >>>>>> The earlier version of the webm_chunk muxer had several bugs: >>>>>> >>>>>> 1. If the first packet of an audio stream didn't have a PTS of zero, >>>>>> then no chunk will be started before a packet is delivered to the >>>>>> underlying Matroska/WebM muxer, i.e. the AVFormatContext used to write >>>>>> these packets had a NULL as AVIOContext for output. This is behind the >>>>>> crash in ticket #5752. >>>>>> >>>>>> 2. If an error happens during writing a packet, the underlyimg >>>>>> Matroska/WebM muxer context is freed. This leads to a use-after-free >>>>>> coupled with a double-free in webm_chunk_write_trailer (which supposes >>>>>> that the underlying AVFormatContext is still valid). >>>>>> >>>>>> 3. Even when no error occurs at all, webm_chunk_write_trailer is still >>>>>> buggy: After the underlying Matroska/WebM muxer has written its >>>>>> trailer, >>>>>> ending the chunk implicitly flushes it again which is illegal at this >>>>>> point. >>>>>> >>>>>> These bugs have been fixed. >>>>>> >>>>>> Fixes #5752. >>>>>> >>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>>>> --- >>>>>> libavformat/webm_chunk.c | 44 +++++++++++++++++++++------------------- >>>>>> 1 file changed, 23 insertions(+), 21 deletions(-) >>>>>> >>>>>> diff --git a/libavformat/webm_chunk.c b/libavformat/webm_chunk.c >>>>>> index 2c99753b5b..391fee721a 100644 >>>>>> --- a/libavformat/webm_chunk.c >>>>>> +++ b/libavformat/webm_chunk.c >>>>>> @@ -168,7 +168,7 @@ static int chunk_start(AVFormatContext *s) >>>>>> return 0; >>>>>> } >>>>>> >>>>>> -static int chunk_end(AVFormatContext *s) >>>>>> +static int chunk_end(AVFormatContext *s, int flush) >>>>>> { >>>>>> WebMChunkContext *wc = s->priv_data; >>>>>> AVFormatContext *oc = wc->avf; >>>>>> @@ -179,11 +179,14 @@ static int chunk_end(AVFormatContext *s) >>>>>> char filename[MAX_FILENAME_SIZE]; >>>>>> AVDictionary *options = NULL; >>>>>> >>>>>> - if (wc->chunk_start_index == wc->chunk_index) >>>>>> + if (!oc->pb) >>>>>> return 0; >>>>>> - // Flush the cluster in WebM muxer. >>>>>> - oc->oformat->write_packet(oc, NULL); >>>>>> + >>>>>> + if (flush) >>>>>> + // Flush the cluster in WebM muxer. >>>>>> + oc->oformat->write_packet(oc, NULL); >>>>>> buffer_size = avio_close_dyn_buf(oc->pb, &buffer); >>>>>> + oc->pb = NULL; >>>>>> ret = get_chunk_filename(s, 0, filename); >>>>>> if (ret < 0) >>>>>> goto fail; >>>>>> @@ -194,7 +197,6 @@ static int chunk_end(AVFormatContext *s) >>>>>> goto fail; >>>>>> avio_write(pb, buffer, buffer_size); >>>>>> ff_format_io_close(s, &pb); >>>>>> - oc->pb = NULL; >>>>>> fail: >>>>>> av_dict_free(&options); >>>>>> av_free(buffer); >>>>>> @@ -216,27 +218,19 @@ static int >>>>>> webm_chunk_write_packet(AVFormatContext *s, AVPacket *pkt) >>>>>> } >>>>>> >>>>>> // For video, a new chunk is started only on key frames. For >>>>>> audio, a new >>>>>> - // chunk is started based on chunk_duration. >>>>>> - if ((st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && >>>>>> + // chunk is started based on chunk_duration. Also, a new chunk is >>>>>> started >>>>>> + // unconditionally if there is no currently open chunk. >>>>>> + if (!oc->pb || (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && >>>>>> (pkt->flags & AV_PKT_FLAG_KEY)) || >>>>>> (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && >>>>>> - (pkt->pts == 0 || wc->duration_written >= >>>>>> wc->chunk_duration))) { >>>>>> + wc->duration_written >= wc->chunk_duration)) { >>>>>> wc->duration_written = 0; >>>>>> - if ((ret = chunk_end(s)) < 0 || (ret = chunk_start(s)) < 0) { >>>>>> - goto fail; >>>>>> + if ((ret = chunk_end(s, 1)) < 0 || (ret = chunk_start(s)) < 0) >>>>>> { >>>>>> + return ret; >>>>>> } >>>>>> } >>>>>> >>>>>> ret = oc->oformat->write_packet(oc, pkt); >>>>>> - if (ret < 0) >>>>>> - goto fail; >>>>>> - >>>>>> -fail: >>>>>> - if (ret < 0) { >>>>>> - oc->streams = NULL; >>>>>> - oc->nb_streams = 0; >>>>>> - avformat_free_context(oc); >>>>>> - } >>>>>> >>>>>> return ret; >>>>>> } >>>>>> @@ -245,12 +239,20 @@ static int >>>>>> webm_chunk_write_trailer(AVFormatContext *s) >>>>>> { >>>>>> WebMChunkContext *wc = s->priv_data; >>>>>> AVFormatContext *oc = wc->avf; >>>>>> + int ret; >>>>>> + >>>>>> + if (!oc->pb) { >>>>>> + ret = chunk_start(s); >>>>>> + if (ret < 0) >>>>>> + goto fail; >>>>>> + } >>>>>> oc->oformat->write_trailer(oc); >>>>>> - chunk_end(s); >>>>>> + ret = chunk_end(s, 0); >>>>>> +fail: >>>>>> oc->streams = NULL; >>>>>> oc->nb_streams = 0; >>>>>> avformat_free_context(oc); >>>>>> - return 0; >>>>>> + return ret; >>>>>> } >>>>>> >>>>>> #define OFFSET(x) offsetof(WebMChunkContext, x) >>>>>> >>>>>> >>>>> Ping for the last two patches of this patchset (i.e. this one and >>>>> https://ffmpeg.org/pipermail/ffmpeg-devel/2019-April/242902.html) >>>>> >>>>> - Andreas >>>>> >>>> And another ping for these two patches. >>>> >>>> - Andreas >>>> >>> >>> Ping #3. >>> >>> - Andreas >>> >> Ping. > > Will apply first patch from this thread. > Is this OK? > If the first patch you are referring to is https://ffmpeg.org/pipermail/ffmpeg-devel/2019-April/242903.html then it is OK. - Andreas
diff --git a/libavformat/webm_chunk.c b/libavformat/webm_chunk.c index 2c99753b5b..391fee721a 100644 --- a/libavformat/webm_chunk.c +++ b/libavformat/webm_chunk.c @@ -168,7 +168,7 @@ static int chunk_start(AVFormatContext *s) return 0; } -static int chunk_end(AVFormatContext *s) +static int chunk_end(AVFormatContext *s, int flush) { WebMChunkContext *wc = s->priv_data; AVFormatContext *oc = wc->avf; @@ -179,11 +179,14 @@ static int chunk_end(AVFormatContext *s) char filename[MAX_FILENAME_SIZE]; AVDictionary *options = NULL; - if (wc->chunk_start_index == wc->chunk_index) + if (!oc->pb) return 0; - // Flush the cluster in WebM muxer. - oc->oformat->write_packet(oc, NULL); + + if (flush) + // Flush the cluster in WebM muxer. + oc->oformat->write_packet(oc, NULL); buffer_size = avio_close_dyn_buf(oc->pb, &buffer); + oc->pb = NULL; ret = get_chunk_filename(s, 0, filename); if (ret < 0) goto fail; @@ -194,7 +197,6 @@ static int chunk_end(AVFormatContext *s) goto fail; avio_write(pb, buffer, buffer_size); ff_format_io_close(s, &pb); - oc->pb = NULL; fail: av_dict_free(&options); av_free(buffer); @@ -216,27 +218,19 @@ static int webm_chunk_write_packet(AVFormatContext *s, AVPacket *pkt) } // For video, a new chunk is started only on key frames. For audio, a new - // chunk is started based on chunk_duration. - if ((st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && + // chunk is started based on chunk_duration. Also, a new chunk is started + // unconditionally if there is no currently open chunk. + if (!oc->pb || (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && (pkt->flags & AV_PKT_FLAG_KEY)) || (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && - (pkt->pts == 0 || wc->duration_written >= wc->chunk_duration))) { + wc->duration_written >= wc->chunk_duration)) { wc->duration_written = 0; - if ((ret = chunk_end(s)) < 0 || (ret = chunk_start(s)) < 0) { - goto fail; + if ((ret = chunk_end(s, 1)) < 0 || (ret = chunk_start(s)) < 0) { + return ret; } } ret = oc->oformat->write_packet(oc, pkt); - if (ret < 0) - goto fail; - -fail: - if (ret < 0) { - oc->streams = NULL; - oc->nb_streams = 0; - avformat_free_context(oc); - } return ret; } @@ -245,12 +239,20 @@ static int webm_chunk_write_trailer(AVFormatContext *s) { WebMChunkContext *wc = s->priv_data; AVFormatContext *oc = wc->avf; + int ret; + + if (!oc->pb) { + ret = chunk_start(s); + if (ret < 0) + goto fail; + } oc->oformat->write_trailer(oc); - chunk_end(s); + ret = chunk_end(s, 0); +fail: oc->streams = NULL; oc->nb_streams = 0; avformat_free_context(oc); - return 0; + return ret; } #define OFFSET(x) offsetof(WebMChunkContext, x)
The earlier version of the webm_chunk muxer had several bugs: 1. If the first packet of an audio stream didn't have a PTS of zero, then no chunk will be started before a packet is delivered to the underlying Matroska/WebM muxer, i.e. the AVFormatContext used to write these packets had a NULL as AVIOContext for output. This is behind the crash in ticket #5752. 2. If an error happens during writing a packet, the underlyimg Matroska/WebM muxer context is freed. This leads to a use-after-free coupled with a double-free in webm_chunk_write_trailer (which supposes that the underlying AVFormatContext is still valid). 3. Even when no error occurs at all, webm_chunk_write_trailer is still buggy: After the underlying Matroska/WebM muxer has written its trailer, ending the chunk implicitly flushes it again which is illegal at this point. These bugs have been fixed. Fixes #5752. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/webm_chunk.c | 44 +++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 21 deletions(-)