[FFmpeg-devel,2/3] lavf/webm_chunk: Fix NULL dereference

Submitted by Andreas Rheinhardt on April 19, 2019, 10:03 p.m.

Details

Message ID 20190419220316.47392-2-andreas.rheinhardt@gmail.com
State New
Headers show

Commit Message

Andreas Rheinhardt April 19, 2019, 10:03 p.m.
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(-)

Comments

Andreas Rheinhardt May 30, 2019, 9:24 a.m.
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 June 7, 2019, 4:55 p.m.
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 June 17, 2019, 8:04 a.m.
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 July 11, 2019, 10:02 p.m.
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
Paul B Mahol July 12, 2019, 8:33 a.m.
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".
Andreas Rheinhardt July 12, 2019, 12:07 p.m.
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

Patch hide | download patch | download mbox

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)