[FFmpeg-devel] [PATCH 2/2] avformat/dv: use av_packet_alloc() to allocate packets
James Almer
jamrial at gmail.com
Sun May 2 15:48:04 EEST 2021
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 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.)
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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list