[FFmpeg-devel] [PATCH] avformat/mux: Fix double-free when using AVPacket.opaque_ref

[Lynne]

3 Sept 2021, 17:23 by andreas.rheinhardt at outlook.com:

> Andreas Rheinhardt:
>
>> Up until now, ff_write_chained() copied the packet (manually, not with
>> av_packet_move_ref()) from a packet given to it to a stack packet whose
>> timing and stream_index is then modified before being sent to another
>> muxer via av_(interleaved_)write_frame(). Afterwards it is intended to
>> sync the fields of the packet relevant to freeing again; yet this only
>> encompasses buf, side_data and side_data_elems and not the newly added
>> opaque_ref. The other fields are not synced so that the returned packet
>> can have a size > 0 and data != NULL despite its buf being NULL (this
>> always happens in the interleaved codepath; before commit
>> fe251f77c80b0512ab8907902e1dbed3f4fe1aad it could also happen in the
>> noninterleaved one). This leads to double-frees if the interleaved
>> codepath is used and opaque_ref is set.
>>
>> This commit therefore changes this by directly reusing the packet
>> instead of a spare packet. Given that av_write_frame() does not
>> change the packet given to it, one only needs to restore the timing
>> information to return it as it was; for the interleaved codepath
>> it is not possible to do likewise*, because av_interleaved_write_frame()
>> takes ownership of the packets given to it and returns blank packets.
>> But precisely because of this users of the interleaved codepath
>> have no legitimate expectation that their packet will be returned
>> unchanged. In line with av_interleaved_write_frame() ff_write_chained()
>> therefore returns blank packets when using the interleaved codepath.
>>
>> Making the only user of said codepath compatible with this was trivial.
>>
>> *: Unless one wanted to create a full new reference.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>> ---
>> ff_write_chained() will be redundant when we allow muxers to convert
>> timestamps internally (we can then make the slave muxer do the
>> conversion). So I am not really worried about the case of adding
>> a new timestamp-related field and forgetting to reset it in
>> ff_write_chained().
>>
>>  libavformat/internal.h |  3 ++-
>>  libavformat/mux.c      | 35 ++++++++++++++++++++++-------------
>>  libavformat/segment.c  |  4 +++-
>>  3 files changed, 27 insertions(+), 15 deletions(-)
>>
>> diff --git a/libavformat/internal.h b/libavformat/internal.h
>> index 4fc1154b9d..9d7312c0e2 100644
>> --- a/libavformat/internal.h
>> +++ b/libavformat/internal.h
>> @@ -506,7 +506,8 @@ void ff_sdp_write_media(char *buff, int size, AVStream *st, int idx,
>>  *
>>  * @param dst the muxer to write the packet to
>>  * @param dst_stream the stream index within dst to write the packet to
>> - * @param pkt the packet to be written
>> + * @param pkt the packet to be written. It will be returned blank when
>> + *            av_interleaved_write_frame() is used, unchanged otherwise.
>>  * @param src the muxer the packet originally was intended for
>>  * @param interleave 0->use av_write_frame, 1->av_interleaved_write_frame
>>  * @return the value av_write_frame returned
>> diff --git a/libavformat/mux.c b/libavformat/mux.c
>> index b1ad0dd561..6ba1306f2b 100644
>> --- a/libavformat/mux.c
>> +++ b/libavformat/mux.c
>> @@ -643,12 +643,12 @@ static void guess_pkt_duration(AVFormatContext *s, AVStream *st, AVPacket *pkt)
>>  */
>>  static int write_packet(AVFormatContext *s, AVPacket *pkt)
>>  {
>> +    AVStream *const st = s->streams[pkt->stream_index];
>>  int ret;
>>  
>>  // If the timestamp offsetting below is adjusted, adjust
>>  // ff_interleaved_peek similarly.
>>  if (s->output_ts_offset) {
>> -        AVStream *st = s->streams[pkt->stream_index];
>>  int64_t offset = av_rescale_q(s->output_ts_offset, AV_TIME_BASE_Q, st->time_base);
>>  
>>  if (pkt->dts != AV_NOPTS_VALUE)
>> @@ -658,7 +658,6 @@ static int write_packet(AVFormatContext *s, AVPacket *pkt)
>>  }
>>  
>>  if (s->avoid_negative_ts > 0) {
>> -        AVStream *st = s->streams[pkt->stream_index];
>>  int64_t offset = st->internal->mux_ts_offset;
>>  int64_t ts = s->internal->avoid_negative_ts_use_pts ? pkt->pts : pkt->dts;
>>  
>> @@ -719,7 +718,7 @@ static int write_packet(AVFormatContext *s, AVPacket *pkt)
>>  }
>>  
>>  if (ret >= 0)
>> -        s->streams[pkt->stream_index]->nb_frames++;
>> +        st->nb_frames++;
>>  
>>  return ret;
>>  }
>> @@ -1192,6 +1191,7 @@ int av_write_frame(AVFormatContext *s, AVPacket *in)
>>  pkt = in;
>>  } else {
>>  /* We don't own in, so we have to make sure not to modify it.
>> +         * (ff_write_chained() relies on this fact.)
>>  * The following avoids copying in's data unnecessarily.
>>  * Copying side data is unavoidable as a bitstream filter
>>  * may change it, e.g. free it on errors. */
>> @@ -1291,21 +1291,30 @@ int av_get_output_timestamp(struct AVFormatContext *s, int stream,
>>  int ff_write_chained(AVFormatContext *dst, int dst_stream, AVPacket *pkt,
>>  AVFormatContext *src, int interleave)
>>  {
>> -    AVPacket local_pkt;
>> +    int64_t pts = pkt->pts, dts = pkt->dts, duration = pkt->duration;
>> +    int stream_index = pkt->stream_index;
>> +    AVRational time_base = pkt->time_base;
>>  int ret;
>>  
>> -    local_pkt = *pkt;
>> -    local_pkt.stream_index = dst_stream;
>> +    pkt->stream_index = dst_stream;
>>  
>> -    av_packet_rescale_ts(&local_pkt,
>> -                         src->streams[pkt->stream_index]->time_base,
>> +    av_packet_rescale_ts(pkt,
>> +                         src->streams[stream_index]->time_base,
>>  dst->streams[dst_stream]->time_base);
>>  
>> -    if (interleave) ret = av_interleaved_write_frame(dst, &local_pkt);
>> -    else            ret = av_write_frame(dst, &local_pkt);
>> -    pkt->buf = local_pkt.buf;
>> -    pkt->side_data       = local_pkt.side_data;
>> -    pkt->side_data_elems = local_pkt.side_data_elems;
>> +    if (!interleave) {
>> +        ret = av_write_frame(dst, pkt);
>> +        /* We only have to backup and restore the fields that
>> +         * we changed ourselves, because av_write_frame() does not
>> +         * modify the packet given to it. */
>> +        pkt->pts          = pts;
>> +        pkt->dts          = dts;
>> +        pkt->duration     = duration;
>> +        pkt->stream_index = stream_index;
>> +        pkt->time_base    = time_base;
>> +    } else
>> +        ret = av_interleaved_write_frame(dst, pkt);
>> +
>>  return ret;
>>  }
>>  
>> diff --git a/libavformat/segment.c b/libavformat/segment.c
>> index ed671353d0..7c171b6fa4 100644
>> --- a/libavformat/segment.c
>> +++ b/libavformat/segment.c
>> @@ -952,7 +952,9 @@ calc_times:
>>  seg->initial_offset || seg->reset_timestamps || seg->avf->oformat->interleave_packet);
>>  
>>  fail:
>> -    if (pkt->stream_index == seg->reference_stream_index) {
>> +    /* Use st->index here as the packet returned from ff_write_chained()
>> +     * is blank if interleaving has been used. */
>> +    if (st->index == seg->reference_stream_index) {
>>  seg->frame_count++;
>>  seg->segment_frame_count++;
>>  }
>>
> Will apply tomorrow unless there are objections.
>

Messy to read, but LGTM.