[07/10] sierravmd: Do sanity checking of frame sizes
diff mbox

Message ID 1378932481-98398-7-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö Sept. 11, 2013, 8:47 p.m. 
Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC: libav-stable@libav.org
---
 libavformat/sierravmd.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Luca Barbato Sept. 11, 2013, 9:05 p.m. | #1 
On 11/09/13 22:47, Martin Storsjö wrote:
> Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> CC: libav-stable@libav.org
> ---
>  libavformat/sierravmd.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/libavformat/sierravmd.c b/libavformat/sierravmd.c
> index 645b99b..8915001 100644
> --- a/libavformat/sierravmd.c
> +++ b/libavformat/sierravmd.c
> @@ -199,6 +199,10 @@ static int vmd_read_header(AVFormatContext *s)
>              avio_read(pb, chunk, BYTES_PER_FRAME_RECORD);
>              type = chunk[0];
>              size = AV_RL32(&chunk[2]);
> +            if (size > INT_MAX/2) {
                                ^^^ spaces

Why INT_MAX / 2 btw?
Martin Storsjö Sept. 11, 2013, 9:18 p.m. | #2 
On Wed, 11 Sep 2013, Luca Barbato wrote:

> On 11/09/13 22:47, Martin Storsjö wrote:
>> Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
>> CC: libav-stable@libav.org
>> ---
>>  libavformat/sierravmd.c |    9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/libavformat/sierravmd.c b/libavformat/sierravmd.c
>> index 645b99b..8915001 100644
>> --- a/libavformat/sierravmd.c
>> +++ b/libavformat/sierravmd.c
>> @@ -199,6 +199,10 @@ static int vmd_read_header(AVFormatContext *s)
>>              avio_read(pb, chunk, BYTES_PER_FRAME_RECORD);
>>              type = chunk[0];
>>              size = AV_RL32(&chunk[2]);
>> +            if (size > INT_MAX/2) {
>                                ^^^ spaces
>
> Why INT_MAX / 2 btw?

Because we need to be sure that size + BYTES_PER_FRAME_RECORD doesn't 
wrap. We could explicitly check for INT_MAX - BYTES_PER_FRAME_RECORD, but 
INT_MAX / 2 gives a bit more marginal since it really shouldn't be 
all that big mostly anyway (Kostya suggested this for another patch 
earlier today, http://patches.libav.org/patch/42128/).

I amended this one as well to add spaces around the operator.

// Martin
Luca Barbato Sept. 11, 2013, 9:21 p.m. | #3 
On 11/09/13 23:18, Martin Storsjö wrote:
> On Wed, 11 Sep 2013, Luca Barbato wrote:
> 
>> On 11/09/13 22:47, Martin Storsjö wrote:
>>> Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
>>> CC: libav-stable@libav.org
>>> ---
>>>  libavformat/sierravmd.c |    9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/libavformat/sierravmd.c b/libavformat/sierravmd.c
>>> index 645b99b..8915001 100644
>>> --- a/libavformat/sierravmd.c
>>> +++ b/libavformat/sierravmd.c
>>> @@ -199,6 +199,10 @@ static int vmd_read_header(AVFormatContext *s)
>>>              avio_read(pb, chunk, BYTES_PER_FRAME_RECORD);
>>>              type = chunk[0];
>>>              size = AV_RL32(&chunk[2]);
>>> +            if (size > INT_MAX/2) {
>>                                ^^^ spaces
>>
>> Why INT_MAX / 2 btw?
> 
> Because we need to be sure that size + BYTES_PER_FRAME_RECORD doesn't
> wrap. We could explicitly check for INT_MAX - BYTES_PER_FRAME_RECORD,
> but INT_MAX / 2 gives a bit more marginal since it really shouldn't be
> all that big mostly anyway (Kostya suggested this for another patch
> earlier today, http://patches.libav.org/patch/42128/).
> 
> I amended this one as well to add spaces around the operator.
> 

I guess the same applies for the other one, thanks for the explanation,
maybe add it to the commit message.

lu
Martin Storsjö Sept. 11, 2013, 9:27 p.m. | #4 
On Wed, 11 Sep 2013, Luca Barbato wrote:

> On 11/09/13 23:18, Martin Storsjö wrote:
>> On Wed, 11 Sep 2013, Luca Barbato wrote:
>> 
>>> On 11/09/13 22:47, Martin Storsjö wrote:
>>>> Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
>>>> CC: libav-stable@libav.org
>>>> ---
>>>>  libavformat/sierravmd.c |    9 +++++++++
>>>>  1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/libavformat/sierravmd.c b/libavformat/sierravmd.c
>>>> index 645b99b..8915001 100644
>>>> --- a/libavformat/sierravmd.c
>>>> +++ b/libavformat/sierravmd.c
>>>> @@ -199,6 +199,10 @@ static int vmd_read_header(AVFormatContext *s)
>>>>              avio_read(pb, chunk, BYTES_PER_FRAME_RECORD);
>>>>              type = chunk[0];
>>>>              size = AV_RL32(&chunk[2]);
>>>> +            if (size > INT_MAX/2) {
>>>                                ^^^ spaces
>>>
>>> Why INT_MAX / 2 btw?
>> 
>> Because we need to be sure that size + BYTES_PER_FRAME_RECORD doesn't
>> wrap. We could explicitly check for INT_MAX - BYTES_PER_FRAME_RECORD,
>> but INT_MAX / 2 gives a bit more marginal since it really shouldn't be
>> all that big mostly anyway (Kostya suggested this for another patch
>> earlier today, http://patches.libav.org/patch/42128/).
>> 
>> I amended this one as well to add spaces around the operator.
>> 
>
> I guess the same applies for the other one, thanks for the explanation,
> maybe add it to the commit message.

Yes, same for both. I'll amend it to the commit messages.

// Martin
Diego Biurrun Sept. 11, 2013, 10:38 p.m. | #5 
On Wed, Sep 11, 2013 at 11:47:58PM +0300, Martin Storsjö wrote:
> --- a/libavformat/sierravmd.c
> +++ b/libavformat/sierravmd.c
> @@ -199,6 +199,10 @@ static int vmd_read_header(AVFormatContext *s)
>              type = chunk[0];
>              size = AV_RL32(&chunk[2]);
> +            if (size > INT_MAX/2) {
> +                av_log(s, AV_LOG_ERROR, "Invalid frame size\n");
> +                goto error;
> +            }
>              if(!size && type != 1)
>                  continue;
> @@ -235,6 +239,11 @@ static int vmd_read_header(AVFormatContext *s)
>  
>      return 0;
> +
> +error:
> +    av_free(raw_frame_table);
> +    av_free(vmd->frame_table);
> +    return AVERROR_INVALIDDATA;
>  }

It feels silly to have the goto for just this one case instead of just
returning directly.

Diego
Martin Storsjö Sept. 12, 2013, 7:45 a.m. | #6 
On Thu, 12 Sep 2013, Diego Biurrun wrote:

> On Wed, Sep 11, 2013 at 11:47:58PM +0300, Martin Storsjö wrote:
>> --- a/libavformat/sierravmd.c
>> +++ b/libavformat/sierravmd.c
>> @@ -199,6 +199,10 @@ static int vmd_read_header(AVFormatContext *s)
>>              type = chunk[0];
>>              size = AV_RL32(&chunk[2]);
>> +            if (size > INT_MAX/2) {
>> +                av_log(s, AV_LOG_ERROR, "Invalid frame size\n");
>> +                goto error;
>> +            }
>>              if(!size && type != 1)
>>                  continue;
>> @@ -235,6 +239,11 @@ static int vmd_read_header(AVFormatContext *s)
>>
>>      return 0;
>> +
>> +error:
>> +    av_free(raw_frame_table);
>> +    av_free(vmd->frame_table);
>> +    return AVERROR_INVALIDDATA;
>>  }
>
> It feels silly to have the goto for just this one case instead of just
> returning directly.

There's at least two more identical free+return cases in the same 
function, but I forgot to factorize them into it when doing this patch. 
I'll update it with that so it makes a bit more sense.

// Martin

Patch
diff mbox 

diff --git a/libavformat/sierravmd.c b/libavformat/sierravmd.c
index 645b99b..8915001 100644
--- a/libavformat/sierravmd.c
+++ b/libavformat/sierravmd.c
@@ -199,6 +199,10 @@  static int vmd_read_header(AVFormatContext *s)
             avio_read(pb, chunk, BYTES_PER_FRAME_RECORD);
             type = chunk[0];
             size = AV_RL32(&chunk[2]);
+            if (size > INT_MAX/2) {
+                av_log(s, AV_LOG_ERROR, "Invalid frame size\n");
+                goto error;
+            }
             if(!size && type != 1)
                 continue;
             switch(type) {
@@ -235,6 +239,11 @@  static int vmd_read_header(AVFormatContext *s)
     vmd->frame_count = total_frames;
 
     return 0;
+
+error:
+    av_free(raw_frame_table);
+    av_free(vmd->frame_table);
+    return AVERROR_INVALIDDATA;
 }
 
 static int vmd_read_packet(AVFormatContext *s,