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

[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