diff mbox series

[FFmpeg-devel,2/2] avformat/dv: use av_packet_alloc() to allocate packets

Message ID 20210430180701.63566-2-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/2] avformat/dv: add avpriv_dv_close_demux
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

James Almer April 30, 2021, 6:07 p.m. UTC
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 <jamrial@gmail.com>
---
 libavdevice/iec61883.c |  2 +-
 libavformat/avidec.c   |  4 +++-
 libavformat/dv.c       | 51 ++++++++++++++++++++++++++----------------
 3 files changed, 36 insertions(+), 21 deletions(-)

Comments

Andreas Rheinhardt May 2, 2021, 4:10 a.m. UTC | #1
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 <jamrial@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.)

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.

>      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)
>
Michael Niedermayer May 2, 2021, 9:21 a.m. UTC | #2
On Fri, Apr 30, 2021 at 03:07:01PM -0300, James Almer wrote:
> 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 <jamrial@gmail.com>
> ---
>  libavdevice/iec61883.c |  2 +-
>  libavformat/avidec.c   |  4 +++-
>  libavformat/dv.c       | 51 ++++++++++++++++++++++++++----------------
>  3 files changed, 36 insertions(+), 21 deletions(-)

crashes, but i guess thats what andreas already reported but
if its something else or you need a testcase then ill retest this and
provide a better report and test sample, just ask me if you need it

thx

[avi @ 0x2ced4680] Switching to NI mode, due to poor interleaving
==21022== Invalid read of size 8
==21022==    at 0x10F6A73: av_buffer_ref (in ffmpeg_g)
==21022==    by 0x72C6D0: av_packet_ref (in ffmpeg_g)
==21022==    by 0x7AA48B: avcodec_send_packet (in ffmpeg_g)
==21022==    by 0x6AE90A: try_decode_frame (in ffmpeg_g)
==21022==    by 0x6B98D8: avformat_find_stream_info (in ffmpeg_g)
==21022==    by 0x2DB293: open_input_file (in ffmpeg_g)
==21022==    by 0x2DEBB3: ffmpeg_parse_options (in ffmpeg_g)
==21022==    by 0x2D3391: main (in ffmpeg_g)
==21022==  Address 0x2d0a8748 is 8 bytes inside a block of size 24 free'd
==21022==    at 0x4C32D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21022==    by 0x10F6ABC: av_buffer_unref (in ffmpeg_g)
==21022==    by 0x72C640: av_packet_unref (in ffmpeg_g)
==21022==    by 0x59B371: avpriv_dv_produce_packet (in ffmpeg_g)
==21022==    by 0x577F4F: avi_read_packet (in ffmpeg_g)
==21022==    by 0x6B3D5A: ff_read_packet (in ffmpeg_g)
==21022==    by 0x6B4AFA: read_frame_internal (in ffmpeg_g)
==21022==    by 0x6B95FC: avformat_find_stream_info (in ffmpeg_g)
==21022==    by 0x2DB293: open_input_file (in ffmpeg_g)
==21022==    by 0x2DEBB3: ffmpeg_parse_options (in ffmpeg_g)
==21022==    by 0x2D3391: main (in ffmpeg_g)
==21022==  Block was alloc'd at
==21022==    at 0x4C33E76: memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21022==    by 0x4C33F91: posix_memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21022==    by 0x110C0E2: av_malloc (in ffmpeg_g)
==21022==    by 0x110C308: av_mallocz (in ffmpeg_g)
==21022==    by 0x10F6925: av_buffer_create (in ffmpeg_g)
==21022==    by 0x10F69E6: av_buffer_alloc (in ffmpeg_g)
==21022==    by 0x72BE07: av_grow_packet (in ffmpeg_g)
==21022==    by 0x6AEFD9: append_packet_chunked (in ffmpeg_g)
==21022==    by 0x577F02: avi_read_packet (in ffmpeg_g)
==21022==    by 0x6B3D5A: ff_read_packet (in ffmpeg_g)
==21022==    by 0x6B4AFA: read_frame_internal (in ffmpeg_g)
==21022==    by 0x6B95FC: avformat_find_stream_info (in ffmpeg_g)
==21022==    by 0x2DB293: open_input_file (in ffmpeg_g)
==21022==    by 0x2DEBB3: ffmpeg_parse_options (in ffmpeg_g)
==21022==    by 0x2D3391: main (in ffmpeg_g)


[...]
James Almer May 2, 2021, 12:48 p.m. UTC | #3
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 <jamrial@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-request@ffmpeg.org with subject "unsubscribe".
>
James Almer May 2, 2021, 1:26 p.m. UTC | #4
On 5/2/2021 6:21 AM, Michael Niedermayer wrote:
> On Fri, Apr 30, 2021 at 03:07:01PM -0300, James Almer wrote:
>> 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 <jamrial@gmail.com>
>> ---
>>   libavdevice/iec61883.c |  2 +-
>>   libavformat/avidec.c   |  4 +++-
>>   libavformat/dv.c       | 51 ++++++++++++++++++++++++++----------------
>>   3 files changed, 36 insertions(+), 21 deletions(-)
> 
> crashes, but i guess thats what andreas already reported but
> if its something else or you need a testcase then ill retest this and
> provide a better report and test sample, just ask me if you need it

Can this file (or a portion of it) be added to FATE? Looking at 
coverage, these avpriv_ functions are only used with the raw DV demuxer. 
Both the AVI and MOV implementations are untested.

I also see three mov files in the FATE samples repo that are not being 
used. None of them have audio, though.

> 
> thx
> 
> [avi @ 0x2ced4680] Switching to NI mode, due to poor interleaving
> ==21022== Invalid read of size 8
> ==21022==    at 0x10F6A73: av_buffer_ref (in ffmpeg_g)
> ==21022==    by 0x72C6D0: av_packet_ref (in ffmpeg_g)
> ==21022==    by 0x7AA48B: avcodec_send_packet (in ffmpeg_g)
> ==21022==    by 0x6AE90A: try_decode_frame (in ffmpeg_g)
> ==21022==    by 0x6B98D8: avformat_find_stream_info (in ffmpeg_g)
> ==21022==    by 0x2DB293: open_input_file (in ffmpeg_g)
> ==21022==    by 0x2DEBB3: ffmpeg_parse_options (in ffmpeg_g)
> ==21022==    by 0x2D3391: main (in ffmpeg_g)
> ==21022==  Address 0x2d0a8748 is 8 bytes inside a block of size 24 free'd
> ==21022==    at 0x4C32D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==21022==    by 0x10F6ABC: av_buffer_unref (in ffmpeg_g)
> ==21022==    by 0x72C640: av_packet_unref (in ffmpeg_g)
> ==21022==    by 0x59B371: avpriv_dv_produce_packet (in ffmpeg_g)
> ==21022==    by 0x577F4F: avi_read_packet (in ffmpeg_g)
> ==21022==    by 0x6B3D5A: ff_read_packet (in ffmpeg_g)
> ==21022==    by 0x6B4AFA: read_frame_internal (in ffmpeg_g)
> ==21022==    by 0x6B95FC: avformat_find_stream_info (in ffmpeg_g)
> ==21022==    by 0x2DB293: open_input_file (in ffmpeg_g)
> ==21022==    by 0x2DEBB3: ffmpeg_parse_options (in ffmpeg_g)
> ==21022==    by 0x2D3391: main (in ffmpeg_g)
> ==21022==  Block was alloc'd at
> ==21022==    at 0x4C33E76: memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==21022==    by 0x4C33F91: posix_memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==21022==    by 0x110C0E2: av_malloc (in ffmpeg_g)
> ==21022==    by 0x110C308: av_mallocz (in ffmpeg_g)
> ==21022==    by 0x10F6925: av_buffer_create (in ffmpeg_g)
> ==21022==    by 0x10F69E6: av_buffer_alloc (in ffmpeg_g)
> ==21022==    by 0x72BE07: av_grow_packet (in ffmpeg_g)
> ==21022==    by 0x6AEFD9: append_packet_chunked (in ffmpeg_g)
> ==21022==    by 0x577F02: avi_read_packet (in ffmpeg_g)
> ==21022==    by 0x6B3D5A: ff_read_packet (in ffmpeg_g)
> ==21022==    by 0x6B4AFA: read_frame_internal (in ffmpeg_g)
> ==21022==    by 0x6B95FC: avformat_find_stream_info (in ffmpeg_g)
> ==21022==    by 0x2DB293: open_input_file (in ffmpeg_g)
> ==21022==    by 0x2DEBB3: ffmpeg_parse_options (in ffmpeg_g)
> ==21022==    by 0x2D3391: main (in ffmpeg_g)
> 
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
diff mbox series

Patch

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);
     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)