Re: [FFmpeg-devel] [PATCH] Respect payload offset in av_grow_packet
> data_offset should probably be size_t, thats also what offsetof() would > give > a pointer difference can be larger than INT_MAX Done > also please add a av_assert0 that pkt->data is not NULL or handle that > case Done NULL data handling but it makes code more complex. Please check. > as pkt->size can be 0 iam not sure pkt->data is guranteed to be non > null It should work with pkt->size = 0 for both reference counted and not-reference counted packets without memory leak. It should allocate just padding and zeroize it. From b92cc763ebb4e9f16989da442af745a78a9c2501 Mon Sep 17 00:00:00 2001 From: Andriy Lysnevych <andriy.lysnev...@gmail.com> Date: Wed, 25 May 2016 17:56:21 +0300 Subject: [PATCH] Respect payload offset in av_grow_packet --- libavcodec/avpacket.c | 28 +--- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index bcc7c79..8988ca2 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -110,24 +110,38 @@ int av_grow_packet(AVPacket *pkt, int grow_by) { int new_size; av_assert0((unsigned)pkt->size <= INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE); -if (!pkt->size) -return av_new_packet(pkt, grow_by); if ((unsigned)grow_by > INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE)) return -1; new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE; if (pkt->buf) { -int ret = av_buffer_realloc(>buf, new_size); -if (ret < 0) -return ret; +size_t data_offset; +uint8_t *old_data = pkt->data; +if (pkt->data == NULL) { +data_offset = 0; +pkt->data = pkt->buf->data; +} else { +data_offset = pkt->data - pkt->buf->data; +if (data_offset > INT_MAX - new_size) +return -1; +} + +if (new_size + data_offset > pkt->buf->size) { +int ret = av_buffer_realloc(>buf, new_size + data_offset); +if (ret < 0) { +pkt->data = old_data; +return ret; +} +pkt->data = pkt->buf->data + data_offset; +} } else { pkt->buf = av_buffer_alloc(new_size); if (!pkt->buf) return AVERROR(ENOMEM); -memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size, pkt->size + grow_by)); +memcpy(pkt->buf->data, pkt->data, pkt->size); +pkt->data = pkt->buf->data; } -pkt->data = pkt->buf->data; pkt->size += grow_by; memset(pkt->data + pkt->size, 0, AV_INPUT_BUFFER_PADDING_SIZE); -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Respect payload offset in av_grow_packet
You are right. Please review updated patch. From 62b31fa4b05fc600eada4fb28b352e5b87bd60f8 Mon Sep 17 00:00:00 2001 From: Andriy Lysnevych <andriy.lysnev...@gmail.com> Date: Wed, 25 May 2016 12:55:39 +0300 Subject: [PATCH] Respect payload offset in av_grow_packet --- libavcodec/avpacket.c | 19 --- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index bcc7c79..68b5202 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -110,24 +110,29 @@ int av_grow_packet(AVPacket *pkt, int grow_by) { int new_size; av_assert0((unsigned)pkt->size <= INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE); -if (!pkt->size) -return av_new_packet(pkt, grow_by); if ((unsigned)grow_by > INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE)) return -1; new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE; if (pkt->buf) { -int ret = av_buffer_realloc(>buf, new_size); -if (ret < 0) -return ret; +int data_offset = pkt->data - pkt->buf->data; +if ((unsigned)data_offset > INT_MAX - new_size) +return -1; + +if (new_size + data_offset > pkt->buf->size) { +int ret = av_buffer_realloc(>buf, new_size + data_offset); +if (ret < 0) +return ret; +pkt->data = pkt->buf->data + data_offset; +} } else { pkt->buf = av_buffer_alloc(new_size); if (!pkt->buf) return AVERROR(ENOMEM); -memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size, pkt->size + grow_by)); +memcpy(pkt->buf->data, pkt->data, pkt->size); +pkt->data = pkt->buf->data; } -pkt->data = pkt->buf->data; pkt->size += grow_by; memset(pkt->data + pkt->size, 0, AV_INPUT_BUFFER_PADDING_SIZE); -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Respect payload offset in av_grow_packet
This one removed: >> -if (!pkt->size) >> -return av_new_packet(pkt, grow_by); pkt->size can be 0 but reference-counted buf allocated. av_new_packet leads to memory leak in this case. (FIXME?) >> -if ((unsigned)grow_by > >> -INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE)) >> -return -1; >> >> new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE; > > you remove the overflow check, which makes this undefined behavior > (note that this is also so when the value is not used) > This check is not removed. It duplicated in two if branches: if (pkt->buf) { +int data_offset = pkt->data - pkt->buf->data; +if ((unsigned)grow_by > +INT_MAX - (pkt->size + data_offset + AV_INPUT_BUFFER_PADDING_SIZE)) +return -1; ... } else { +if ((unsigned)grow_by > +INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE)) +return -1; ... } Please specify more detailed if I missed something. Thanks! ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Respect payload offset in av_grow_packet
Sorry, looks like problems with my mail client. Sending patch as attachment. From 45f69d7f02928ad8abae3fc591082997590c597a Mon Sep 17 00:00:00 2001 From: Andriy Lysnevych <andriy.lysnev...@gmail.com> Date: Mon, 16 May 2016 12:08:33 +0300 Subject: [PATCH] Respect payload offset in av_grow_packet --- libavcodec/avpacket.c | 25 +++-- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index bcc7c79..327cd41 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -110,24 +110,29 @@ int av_grow_packet(AVPacket *pkt, int grow_by) { int new_size; av_assert0((unsigned)pkt->size <= INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE); -if (!pkt->size) -return av_new_packet(pkt, grow_by); -if ((unsigned)grow_by > -INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE)) -return -1; new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE; if (pkt->buf) { -int ret = av_buffer_realloc(>buf, new_size); -if (ret < 0) -return ret; +int data_offset = pkt->data - pkt->buf->data; +if ((unsigned)grow_by > +INT_MAX - (pkt->size + data_offset + AV_INPUT_BUFFER_PADDING_SIZE)) +return -1; +if (new_size + data_offset > pkt->buf->size) { +int ret = av_buffer_realloc(>buf, new_size + data_offset); +if (ret < 0) +return ret; +pkt->data = pkt->buf->data + data_offset; +} } else { +if ((unsigned)grow_by > +INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE)) +return -1; pkt->buf = av_buffer_alloc(new_size); if (!pkt->buf) return AVERROR(ENOMEM); -memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size, pkt->size + grow_by)); +memcpy(pkt->buf->data, pkt->data, pkt->size); +pkt->data = pkt->buf->data; } -pkt->data = pkt->buf->data; pkt->size += grow_by; memset(pkt->data + pkt->size, 0, AV_INPUT_BUFFER_PADDING_SIZE); -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Respect payload offset in av_grow_packet
Patch for latest master --- libavcodec/avpacket.c | 25 +++-- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index bcc7c79..327cd41 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -110,24 +110,29 @@ int av_grow_packet(AVPacket *pkt, int grow_by) { int new_size; av_assert0((unsigned)pkt->size <= INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE); -if (!pkt->size) -return av_new_packet(pkt, grow_by); -if ((unsigned)grow_by > -INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE)) -return -1; new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE; if (pkt->buf) { -int ret = av_buffer_realloc(>buf, new_size); -if (ret < 0) -return ret; +int data_offset = pkt->data - pkt->buf->data; +if ((unsigned)grow_by > +INT_MAX - (pkt->size + data_offset + AV_INPUT_BUFFER_PADDING_SIZE)) +return -1; +if (new_size + data_offset > pkt->buf->size) { +int ret = av_buffer_realloc(>buf, new_size + data_offset); +if (ret < 0) +return ret; +pkt->data = pkt->buf->data + data_offset; +} } else { +if ((unsigned)grow_by > +INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE)) +return -1; pkt->buf = av_buffer_alloc(new_size); if (!pkt->buf) return AVERROR(ENOMEM); -memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size, pkt->size + grow_by)); +memcpy(pkt->buf->data, pkt->data, pkt->size); +pkt->data = pkt->buf->data; } -pkt->data = pkt->buf->data; pkt->size += grow_by; memset(pkt->data + pkt->size, 0, AV_INPUT_BUFFER_PADDING_SIZE); -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Const src in av_packet_clone declaration
> Anyway, example how this change can break: > > typedef struct AVPacket AVPacket; > > AVPacket *av_packet_clone_old(AVPacket *src); > AVPacket *av_packet_clone_new(const AVPacket *src); > > AVPacket *(*unsuspecting_api_user)(AVPacket *src); > > void set(void) > { > unsuspecting_api_user = av_packet_clone_old; > > unsuspecting_api_user = av_packet_clone_new; > } > You are right. But is this the only example how this change breakes compatibility? I don't think many (if any) C++ projects use av_packet_clone this way. Also this incompatibility is very easy to fix in any project. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] Respect payload offset in av_grow_packet
The patch fixes the function when used with reference-counted packets that have payload offset. Also this function is dangerous for not reference-counted packets because it just overwrites pkt->data. Probably it is better to restrict using it with not referenced-counted packets because you simply don't know how to do grow\realloc in this case. h264_mp4toannexb and h265_mp4toannexb already call av_grow_packet on input packets that leads to memory leak in case of not reference counted packets. --- libavcodec/avpacket.c | 25 +++-- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index bcc7c79..327cd41 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -110,24 +110,29 @@ int av_grow_packet(AVPacket *pkt, int grow_by) { int new_size; av_assert0((unsigned)pkt->size <= INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE); -if (!pkt->size) -return av_new_packet(pkt, grow_by); -if ((unsigned)grow_by > -INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE)) -return -1; new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE; if (pkt->buf) { -int ret = av_buffer_realloc(>buf, new_size); -if (ret < 0) -return ret; +int data_offset = pkt->data - pkt->buf->data; +if ((unsigned)grow_by > +INT_MAX - (pkt->size + data_offset + AV_INPUT_BUFFER_PADDING_SIZE)) +return -1; +if (new_size + data_offset > pkt->buf->size) { +int ret = av_buffer_realloc(>buf, new_size + data_offset); +if (ret < 0) +return ret; +pkt->data = pkt->buf->data + data_offset; +} } else { +if ((unsigned)grow_by > +INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE)) +return -1; pkt->buf = av_buffer_alloc(new_size); if (!pkt->buf) return AVERROR(ENOMEM); -memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size, pkt->size + grow_by)); +memcpy(pkt->buf->data, pkt->data, pkt->size); +pkt->data = pkt->buf->data; } -pkt->data = pkt->buf->data; pkt->size += grow_by; memset(pkt->data + pkt->size, 0, AV_INPUT_BUFFER_PADDING_SIZE); -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Respect payload offset in av_packet_ref
You are right --- libavcodec/avpacket.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index dd8b71e..19597f2 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -568,16 +568,18 @@ int av_packet_ref(AVPacket *dst, const AVPacket *src) if (ret < 0) goto fail; memcpy(dst->buf->data, src->data, src->size); +dst->data = dst->buf->data; } else { dst->buf = av_buffer_ref(src->buf); if (!dst->buf) { ret = AVERROR(ENOMEM); goto fail; } +dst->data = src->data; } dst->size = src->size; -dst->data = dst->buf->data; + return 0; fail: av_packet_free_side_data(dst); -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] Respect payload offset in av_packet_ref
Details are in the ticket https://trac.ffmpeg.org/ticket/5543 --- libavcodec/avpacket.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index dd8b71e..842d8ba 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -568,16 +568,18 @@ int av_packet_ref(AVPacket *dst, const AVPacket *src) if (ret < 0) goto fail; memcpy(dst->buf->data, src->data, src->size); +dst->data = dst->buf->data; } else { dst->buf = av_buffer_ref(src->buf); if (!dst->buf) { ret = AVERROR(ENOMEM); goto fail; } +dst->data = dst->buf->data + (src->data - src->buf->data); } dst->size = src->size; -dst->data = dst->buf->data; + return 0; fail: av_packet_free_side_data(dst); -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] Const src in av_packet_clone declaration
--- libavcodec/avcodec.h | 2 +- libavcodec/avpacket.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 97b2128..1ad0412 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -4300,7 +4300,7 @@ AVPacket *av_packet_alloc(void); * @see av_packet_alloc * @see av_packet_ref */ -AVPacket *av_packet_clone(AVPacket *src); +AVPacket *av_packet_clone(const AVPacket *src); /** * Free the packet, if the packet is reference counted, it will be diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index 9cdfafd..dd8b71e 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -584,7 +584,7 @@ fail: return ret; } -AVPacket *av_packet_clone(AVPacket *src) +AVPacket *av_packet_clone(const AVPacket *src) { AVPacket *ret = av_packet_alloc(); -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel