[FFmpeg-devel] stsz overflow
Tue Aug 25 20:19:29 CEST 2009
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.
>> 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.
> 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
More information about the ffmpeg-devel