| Message ID | 20200313132833.10367-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,v2] ffplay, avcodec, avformat: Don't initialize before av_packet_ref() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Quoting Andreas Rheinhardt (2020-03-13 14:28:33) > It already initializes the packet. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > Resending because of 3117f47f19d051d47ba29c9b78c2ca525f0fdb45. > > fftools/ffplay.c | 2 +- > libavcodec/qsvdec_h2645.c | 2 +- > libavcodec/qsvdec_other.c | 2 +- > libavformat/fifo.c | 1 - > libavformat/img2enc.c | 8 ++++---- > libavformat/tee.c | 1 - > 6 files changed, 7 insertions(+), 9 deletions(-) Generally looks good, but it occurred to me that the semantics of what happens on failure in av_packet_ref() is unspecified, which might lead to those uninitialized packets now being full of random data, which might be dangerous. So we might want to specify that on failure dst is reset to a clean state and replace the call to av_packet_free_side_data() with av_packet_unref().
Anton Khirnov: > Quoting Andreas Rheinhardt (2020-03-13 14:28:33) >> It already initializes the packet. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> Resending because of 3117f47f19d051d47ba29c9b78c2ca525f0fdb45. >> >> fftools/ffplay.c | 2 +- >> libavcodec/qsvdec_h2645.c | 2 +- >> libavcodec/qsvdec_other.c | 2 +- >> libavformat/fifo.c | 1 - >> libavformat/img2enc.c | 8 ++++---- >> libavformat/tee.c | 1 - >> 6 files changed, 7 insertions(+), 9 deletions(-) > > Generally looks good, but it occurred to me that the semantics of what > happens on failure in av_packet_ref() is unspecified, which might lead > to those uninitialized packets now being full of random data, which > might be dangerous. > > So we might want to specify that on failure dst is reset to a clean > state and replace the call to av_packet_free_side_data() with > av_packet_unref(). > Applied, thanks. - Andreas
diff --git a/fftools/ffplay.c b/fftools/ffplay.c index f6511e4afd..2ed4b22d3e 100644 --- a/fftools/ffplay.c +++ b/fftools/ffplay.c @@ -2977,7 +2977,7 @@ static int read_thread(void *arg) } if (is->queue_attachments_req) { if (is->video_st && is->video_st->disposition & AV_DISPOSITION_ATTACHED_PIC) { - AVPacket copy = { 0 }; + AVPacket copy; if ((ret = av_packet_ref(©, &is->video_st->attached_pic)) < 0) goto fail; packet_queue_put(&is->videoq, ©); diff --git a/libavcodec/qsvdec_h2645.c b/libavcodec/qsvdec_h2645.c index 730feed20a..02c41883b6 100644 --- a/libavcodec/qsvdec_h2645.c +++ b/libavcodec/qsvdec_h2645.c @@ -125,7 +125,7 @@ static int qsv_decode_frame(AVCodecContext *avctx, void *data, /* buffer the input packet */ if (avpkt->size) { - AVPacket input_ref = { 0 }; + AVPacket input_ref; if (av_fifo_space(s->packet_fifo) < sizeof(input_ref)) { ret = av_fifo_realloc2(s->packet_fifo, diff --git a/libavcodec/qsvdec_other.c b/libavcodec/qsvdec_other.c index ff2834c20b..b4df76739c 100644 --- a/libavcodec/qsvdec_other.c +++ b/libavcodec/qsvdec_other.c @@ -123,7 +123,7 @@ static int qsv_decode_frame(AVCodecContext *avctx, void *data, /* buffer the input packet */ if (avpkt->size) { - AVPacket input_ref = { 0 }; + AVPacket input_ref; if (av_fifo_space(s->packet_fifo) < sizeof(input_ref)) { ret = av_fifo_realloc2(s->packet_fifo, diff --git a/libavformat/fifo.c b/libavformat/fifo.c index 7b37fff6da..d11dc6626c 100644 --- a/libavformat/fifo.c +++ b/libavformat/fifo.c @@ -536,7 +536,6 @@ static int fifo_write_packet(AVFormatContext *avf, AVPacket *pkt) int ret; if (pkt) { - av_init_packet(&msg.pkt); ret = av_packet_ref(&msg.pkt,pkt); if (ret < 0) return ret; diff --git a/libavformat/img2enc.c b/libavformat/img2enc.c index a2786ec6f8..b303d38239 100644 --- a/libavformat/img2enc.c +++ b/libavformat/img2enc.c @@ -78,7 +78,7 @@ static int write_muxed_file(AVFormatContext *s, AVIOContext *pb, AVPacket *pkt) VideoMuxData *img = s->priv_data; AVCodecParameters *par = s->streams[pkt->stream_index]->codecpar; AVStream *st; - AVPacket pkt2 = {0}; + AVPacket pkt2; AVFormatContext *fmt = NULL; int ret; @@ -88,8 +88,8 @@ static int write_muxed_file(AVFormatContext *s, AVIOContext *pb, AVPacket *pkt) return ret; st = avformat_new_stream(fmt, NULL); if (!st) { - avformat_free_context(fmt); - return AVERROR(ENOMEM); + ret = AVERROR(ENOMEM); + goto out; } st->id = pkt->stream_index; @@ -105,8 +105,8 @@ static int write_muxed_file(AVFormatContext *s, AVIOContext *pb, AVPacket *pkt) (ret = av_interleaved_write_frame(fmt, &pkt2)) < 0 || (ret = av_write_trailer(fmt))) {} -out: av_packet_unref(&pkt2); +out: avformat_free_context(fmt); return ret; } diff --git a/libavformat/tee.c b/libavformat/tee.c index 56669d9d8e..f2b11fcb35 100644 --- a/libavformat/tee.c +++ b/libavformat/tee.c @@ -564,7 +564,6 @@ static int tee_write_packet(AVFormatContext *avf, AVPacket *pkt) if (s2 < 0) continue; - memset(&pkt2, 0, sizeof(AVPacket)); if ((ret = av_packet_ref(&pkt2, pkt)) < 0) if (!ret_all) { ret_all = ret;
It already initializes the packet. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- Resending because of 3117f47f19d051d47ba29c9b78c2ca525f0fdb45. fftools/ffplay.c | 2 +- libavcodec/qsvdec_h2645.c | 2 +- libavcodec/qsvdec_other.c | 2 +- libavformat/fifo.c | 1 - libavformat/img2enc.c | 8 ++++---- libavformat/tee.c | 1 - 6 files changed, 7 insertions(+), 9 deletions(-)