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

[Andreas Rheinhardt]

ffmpeg | branch: master | Andreas Rheinhardt <andreas.rheinhardt at outlook.com> | Wed Sep  1 21:14:41 2021 +0200| [2f710734c878b95eaeb9b84b0b5f367ab976c1bd] | committer: Andreas Rheinhardt

avformat/mux: Fix double-free when using AVPacket.opaque_ref

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.

Reviewed-by: Lynne <dev at lynne.ee>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>

> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=2f710734c878b95eaeb9b84b0b5f367ab976c1bd
---

 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++;
     }