Re: [FFmpeg-devel] [PATCH] Respect payload offset in av_grow_packet

[Andriy Lysnevych]

> 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

[Andriy Lysnevych]

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

[Andriy Lysnevych]

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

[Andriy Lysnevych]

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

[Andriy Lysnevych]

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

[Andriy Lysnevych]

> 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

[Andriy Lysnevych]

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

[Andriy Lysnevych]

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

[Andriy Lysnevych]

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

[Andriy Lysnevych]

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