[FFmpeg-devel] [PATCH 2/2] avformat/dv: use av_packet_alloc() to allocate packets
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Sun May 2 07:10:18 EEST 2021
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 at 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)
>
More information about the ffmpeg-devel
mailing list