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

[Andreas Rheinhardt]

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.

- Andreas