On 5/2/2021 1:10 AM, Andreas Rheinhardt wrote:
James Almer:
As avpriv_dv_get_packet can fail now, make it return < 0 on error, 0 on no
packet found, and > 0 on packet found.

Signed-off-by: James Almer <jamr...@gmail.com>
---
  libavdevice/iec61883.c |  2 +-
  libavformat/avidec.c   |  4 +++-
  libavformat/dv.c       | 51 ++++++++++++++++++++++++++----------------
  3 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/libavdevice/iec61883.c b/libavdevice/iec61883.c
index 18ad704066..de9f48b8fc 100644
--- a/libavdevice/iec61883.c
+++ b/libavdevice/iec61883.c
@@ -191,7 +191,7 @@ static int iec61883_parse_queue_dv(struct iec61883_data 
*dv, AVPacket *pkt)
      int size;
size = avpriv_dv_get_packet(dv->dv_demux, pkt);
-    if (size > 0)
+    if (size)
          return size;
packet = dv->queue_first;
diff --git a/libavformat/avidec.c b/libavformat/avidec.c
index 2d0d2a7389..2f493e42a6 100644
--- a/libavformat/avidec.c
+++ b/libavformat/avidec.c
@@ -1440,8 +1440,10 @@ static int avi_read_packet(AVFormatContext *s, AVPacket 
*pkt)
if (CONFIG_DV_DEMUXER && avi->dv_demux) {
          int size = avpriv_dv_get_packet(avi->dv_demux, pkt);
-        if (size >= 0)
+        if (size > 0)
              return size;
+        else if (size < 0)
+            return AVERROR(ENOMEM);
          else
              goto resync;
      }
diff --git a/libavformat/dv.c b/libavformat/dv.c
index a948fc0b98..1adc9fdb7b 100644
--- a/libavformat/dv.c
+++ b/libavformat/dv.c
@@ -45,7 +45,7 @@ struct DVDemuxContext {
      AVFormatContext*  fctx;
      AVStream*         vst;
      AVStream*         ast[4];
-    AVPacket          audio_pkt[4];
+    AVPacket         *audio_pkt[4];
      uint8_t           audio_buf[4][8192];
      int               ach;
      int               frames;
@@ -261,11 +261,11 @@ static int dv_extract_audio_info(DVDemuxContext *c, const 
uint8_t *frame)
              c->ast[i]->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
              c->ast[i]->codecpar->codec_id   = AV_CODEC_ID_PCM_S16LE;
- av_init_packet(&c->audio_pkt[i]);
-            c->audio_pkt[i].size         = 0;
-            c->audio_pkt[i].data         = c->audio_buf[i];
-            c->audio_pkt[i].stream_index = c->ast[i]->index;
-            c->audio_pkt[i].flags       |= AV_PKT_FLAG_KEY;
+            av_packet_unref(c->audio_pkt[i]);
+            c->audio_pkt[i]->size         = 0;
+            c->audio_pkt[i]->data         = c->audio_buf[i];
+            c->audio_pkt[i]->stream_index = c->ast[i]->index;
+            c->audio_pkt[i]->flags       |= AV_PKT_FLAG_KEY;
          }
          c->ast[i]->codecpar->sample_rate    = dv_audio_frequency[freq];
          c->ast[i]->codecpar->channels       = 2;
@@ -327,6 +327,9 @@ void avpriv_dv_close_demux(DVDemuxContext **pc)
      if (!c)
          return;
+ for (int i = 0; i < 4; i++)
+        av_packet_free(&c->audio_pkt[i]);
+
      av_freep(pc);
  }
@@ -336,6 +339,12 @@ static int dv_init_demux(AVFormatContext *s, DVDemuxContext *c)
      if (!c->vst)
          return AVERROR(ENOMEM);
+ for (int i = 0; i < 4; i++) {
+        c->audio_pkt[i] = av_packet_alloc();
+        if (!c->audio_pkt[i])
+           return AVERROR(ENOMEM);
+    }
+
      c->fctx                   = s;
      c->vst->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
      c->vst->codecpar->codec_id   = AV_CODEC_ID_DVVIDEO;
@@ -361,13 +370,14 @@ DVDemuxContext *avpriv_dv_init_demux(AVFormatContext *s)
int avpriv_dv_get_packet(DVDemuxContext *c, AVPacket *pkt)
  {
-    int size = -1;
+    int size = 0;
      int i;
for (i = 0; i < c->ach; i++) {
-        if (c->ast[i] && c->audio_pkt[i].size) {
-            *pkt                 = c->audio_pkt[i];
-            c->audio_pkt[i].size = 0;
+        if (c->ast[i] && c->audio_pkt[i]->size) {
+            if (av_packet_ref(pkt, c->audio_pkt[i]) < 0)
+                return -1;
+            c->audio_pkt[i]->size = 0;
              size                 = pkt->size;
              break;
          }
@@ -392,9 +402,9 @@ int avpriv_dv_produce_packet(DVDemuxContext *c, AVPacket 
*pkt,
      /* FIXME: in case of no audio/bad audio we have to do something */
      size = dv_extract_audio_info(c, buf);
      for (i = 0; i < c->ach; i++) {
-        c->audio_pkt[i].pos  = pos;
-        c->audio_pkt[i].size = size;
-        c->audio_pkt[i].pts  = (c->sys->height == 720) ? (c->frames & ~1) : 
c->frames;
+        c->audio_pkt[i]->pos  = pos;
+        c->audio_pkt[i]->size = size;
+        c->audio_pkt[i]->pts  = (c->sys->height == 720) ? (c->frames & ~1) : 
c->frames;
          ppcm[i] = c->audio_buf[i];
      }
      if (c->ach)
@@ -404,15 +414,15 @@ int avpriv_dv_produce_packet(DVDemuxContext *c, AVPacket 
*pkt,
       * channels 0,1 and odd 2,3. */
      if (c->sys->height == 720) {
          if (buf[1] & 0x0C) {
-            c->audio_pkt[2].size = c->audio_pkt[3].size = 0;
+            c->audio_pkt[2]->size = c->audio_pkt[3]->size = 0;
          } else {
-            c->audio_pkt[0].size = c->audio_pkt[1].size = 0;
+            c->audio_pkt[0]->size = c->audio_pkt[1]->size = 0;
          }
      }
/* Now it's time to return video packet */
      size = dv_extract_video_info(c, buf);
-    av_init_packet(pkt);
+    av_packet_unref(pkt);

This code predates the introduction of refcounted AVPackets; it
therefore doesn't handle this case very well.
There are four callers of avpriv_dv_produce_packet(); of these, two have
refcounted packets and therefore store the packet's AVBufferRef* and
reattach it later. Here is the avi demuxer, the mov demuxer does the same:

             AVBufferRef *avbuf = pkt->buf;
             size = avpriv_dv_produce_packet(avi->dv_demux, pkt,
                                             pkt->data, pkt->size, pkt->pos);
             pkt->buf    = avbuf;
             pkt->flags |= AV_PKT_FLAG_KEY;
             if (size < 0)
                 av_packet_unref(pkt);

With your code, the reference (and the underlying buffer) will be freed
in avpriv_dv_produce_packet(), leading to use-after-free.

The simple fix is of course to reset pkt->buf, but I think whoever
touches this code should make it properly support refcounted packets.
(Btw: Is the av_init_packet() even necessary? None of the packets it
gets is uninitialized.)

Probably not, so i can just remove it and prevent what you describe above.


Furthermore, I don't like that you are adding another avpriv symbol that
could be easily avoided by adding a struct that has exactly the members
of the packet that are actually used.

A struct with the subset of AVPacket members used here will have more than half of them. And I don't see introducing a close() avpriv_ symbol here as a problem (its signature isn't going to require changes, and it can do more things in the future if required). But if you really dislike it, i can look into implementing it.


      pkt->data         = buf;
      pkt->pos          = pos;
      pkt->size         = size;
@@ -447,8 +457,8 @@ static int64_t dv_frame_offset(AVFormatContext *s, 
DVDemuxContext *c,
  void ff_dv_offset_reset(DVDemuxContext *c, int64_t frame_offset)
  {
      c->frames = frame_offset;
-    c->audio_pkt[0].size = c->audio_pkt[1].size = 0;
-    c->audio_pkt[2].size = c->audio_pkt[3].size = 0;
+    c->audio_pkt[0]->size = c->audio_pkt[1]->size = 0;
+    c->audio_pkt[2]->size = c->audio_pkt[3]->size = 0;
  }
/************************************************************
@@ -547,7 +557,10 @@ static int dv_read_packet(AVFormatContext *s, AVPacket 
*pkt)
size = avpriv_dv_get_packet(c->dv_demux, pkt); - if (size < 0) {
+    if (size < 0)
+        return AVERROR(ENOMEM);
+
+    if (!size) {
          int ret;
          int64_t pos = avio_tell(s->pb);
          if (!c->dv_demux->sys)


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Reply via email to