[FFmpeg-devel] stsz overflow

[Baptiste Coudurier]

On 8/25/2009 11:11 AM, Reimar D?ffinger wrote:
> On Tue, Aug 25, 2009 at 10:16:52AM -0700, Frank Barchard wrote:
>> On Tue, Aug 25, 2009 at 9:16 AM, Reimar D?ffinger
>> <Reimar.Doeffinger at gmx.de>wrote:
>>
>>> On Tue, Aug 25, 2009 at 09:11:21AM -0700, Baptiste Coudurier wrote:
>>>>> Wow, what a mess (IMO). I think we are already at the point where it
>>>>> would be simpler to just get rid of that buffer and directly read the
>>>>> values "one by one" from the file.
>>>> No, it was decided to be done that way when the patch was submitted.
>>> Obviously the review at that time did not take into account the
>>> additional code and complexity of avoiding buffer overflows, which
>>> unless someone comes up with a cleaner check is a really good reason
>>> to reconsider that decision.
>>
>> Here is the simplest change that addresses the math overflow.  It limits
>> stsz to 134,217,728 entries.

Looks reasonable.

>> Index: libavformat/mov.c
>> ===================================================================
>> --- libavformat/mov.c   (revision 19697)
>> +++ libavformat/mov.c   (working copy)
>> @@ -1256,7 +1256,7 @@
>>           return -1;
>>       }
>>
>> -    if(entries>= UINT_MAX / sizeof(int))
>> +    if(entries>= UINT_MAX / 32)  /* avoids buffer overrun */
>>           return -1;
>
> Seems reasonable to me, except for the comment.
> A buffer overrun/overflow is only the secondary effect.
> The right comment should be something like "avoids integer
> overflow in multiplication with field_size".
> Particularly mentioning field_size may reduce the risk of forgetting
> to change this if ever e.g. field_size == 64 should become possible.

I agree.

> Or
> if (entries>= UINT_MAX / sizeof(int) || entries>= (UINT_MAX - 4) / field_size)
> as a compromise.

This one seems good to me as well.

> Anyway, I think we have enough possibilities to choose from, just
> need to pick one and go with it.
> Though I still think that using get_bits isn't worth the 13 lines of
> code it needs - though the alternative is a bit ugly as well.

Yes, I've no problem with the other approach not using a buffer, if 
others prefer it.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org