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

[Andreas Rheinhardt]

Paul B Mahol:
> On 7/12/19, Andreas Rheinhardt <andreas.rheinhardt at 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 at 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