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

[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?

>
> - Andreas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".