[FFmpeg-devel] [PATCH] *alloc(type)

[Yuriy Kaminskiy]

Reimar D?ffinger wrote:
> On Sat, Nov 20, 2010 at 04:05:32PM +0300, Yuriy Kaminskiy wrote:
>> Reimar D?ffinger wrote:
>>> On Sat, Nov 20, 2010 at 04:37:30AM +0100, Michael Niedermayer wrote:
>>>> patchset below fixes the type used in malloc and co
>>>> The sense behind this patch is that feeding things that dont fit in unsigned
>>>> int into *alloc() can lead to successfull allocation of too small arrays which
>>>> is pretty bad.
>>>> There are probably more functions that should be changed like av_new_packet()
>>>> but i had to start somewhere and will look into the others too if noone else
>>>> does.
>>>> Note, i will apply this in a few days if there are no objections
>>> This has some side-effects I do not like.
>>> For example, allocating more than 4 GB now becomes possible, even
>>> though such an allocation is almost certain to be a bug.
>> No. A bit more context:
>> === cut ===
>> void *av_malloc(unsigned int size)
>> {
>>     void *ptr = NULL;
>> #if CONFIG_MEMALIGN_HACK
>>     long diff;
>> #endif
>>
>>     /* let's disallow possible ambiguous cases */
>>     if(size > (INT_MAX-16) )
>>         return NULL;
> 
> Ok, I'll change my suggestions to:
> How about using uint64_t always?

Are you sure it won't break e.g. those matroska demuxer trickery? (fwiw, *if* it
will - *then* size_t is unsafe too) [maybe there are other places, that relies
on that too, have no idea].
FWIW, relying on "fate did not break, so let's deploy it" is very wrong, IMO -
brokenness in this area won't show up with normal non-broken files (or even just
randomly fuzzied files), you need carefully craft broken file to test it.

> How about adding a
> #if SIZE_MAX < INT_MAX
> #error unsupported system (size_t < int)
> #endif

I think it cannot harm (but very unlikely to help anything too).