http: Check for negative chunk sizes
diff mbox

Message ID 20161219205637.23895-1-martin@martin.st
State Committed
Headers show

Commit Message

Martin Storsjö Dec. 19, 2016, 8:56 p.m. 
A negative chunk size is illegal and would end up used as
length for memcpy, where it would lead to memory accesses
out of bounds.

Found-by: Paul Cher <paulcher@icloud.com>

CC: libav-stable@libav.org
---
 libavformat/http.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Vittorio Giovara Dec. 19, 2016, 9:07 p.m. | #1 
On Mon, Dec 19, 2016 at 9:56 PM, Martin Storsjö <martin@martin.st> wrote:
> A negative chunk size is illegal and would end up used as
> length for memcpy, where it would lead to memory accesses
> out of bounds.
>
> Found-by: Paul Cher <paulcher@icloud.com>
>
> CC: libav-stable@libav.org
> ---
>  libavformat/http.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/libavformat/http.c b/libavformat/http.c
> index 8fe8d11..7e3708e 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -784,6 +784,8 @@ static int http_read_stream(URLContext *h, uint8_t *buf, int size)
>
>                  av_log(NULL, AV_LOG_TRACE, "Chunked encoding data size: %"PRId64"'\n",
>                          s->chunksize);
> +                if (s->chunksize < 0)
> +                    return AVERROR_INVALIDDATA;
>
>                  if (!s->chunksize)
>                      return 0;

This is mostly a nit, but would it make sense to coalesce the second
`if` into a `else if`?
Ok with me either way.
Martin Storsjö Dec. 23, 2016, 7:32 p.m. | #2 
On Mon, 19 Dec 2016, Vittorio Giovara wrote:

> On Mon, Dec 19, 2016 at 9:56 PM, Martin Storsjö <martin@martin.st> wrote:
>> A negative chunk size is illegal and would end up used as
>> length for memcpy, where it would lead to memory accesses
>> out of bounds.
>>
>> Found-by: Paul Cher <paulcher@icloud.com>
>>
>> CC: libav-stable@libav.org
>> ---
>>  libavformat/http.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/libavformat/http.c b/libavformat/http.c
>> index 8fe8d11..7e3708e 100644
>> --- a/libavformat/http.c
>> +++ b/libavformat/http.c
>> @@ -784,6 +784,8 @@ static int http_read_stream(URLContext *h, uint8_t *buf, int size)
>>
>>                  av_log(NULL, AV_LOG_TRACE, "Chunked encoding data size: %"PRId64"'\n",
>>                          s->chunksize);
>> +                if (s->chunksize < 0)
>> +                    return AVERROR_INVALIDDATA;
>>
>>                  if (!s->chunksize)
>>                      return 0;
>
> This is mostly a nit, but would it make sense to coalesce the second
> `if` into a `else if`?
> Ok with me either way.

Pushed with this changed as suggested.

// Martin

Patch
diff mbox 

diff --git a/libavformat/http.c b/libavformat/http.c
index 8fe8d11..7e3708e 100644
--- a/libavformat/http.c
+++ b/libavformat/http.c
@@ -784,6 +784,8 @@  static int http_read_stream(URLContext *h, uint8_t *buf, int size)
 
                 av_log(NULL, AV_LOG_TRACE, "Chunked encoding data size: %"PRId64"'\n",
                         s->chunksize);
+                if (s->chunksize < 0)
+                    return AVERROR_INVALIDDATA;
 
                 if (!s->chunksize)
                     return 0;