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

[Ronald S. Bultje]

Hi,

On Sat, Nov 20, 2010 at 12:29 PM, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de> wrote:
> On Sat, Nov 20, 2010 at 11:05:57AM -0500, Ronald S. Bultje wrote:
>> On Sat, Nov 20, 2010 at 10:53 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> > On Sat, Nov 20, 2010 at 02:20:08PM +0100, 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?
>> >
>> > i dont know, it would slow down pure 32bit systems
>>
>> Not just that, any malloc implementation doesn't allow 64-bit
>> allocations on 32-bit systems anyway (theory or practice), so we're
>> essentially lying and complicating the situation for 32-bit systems
>> for no good reason.
>
> Uh, failing when someone tries to allocate 4.5 GB instead of claiming
> success while only returning 500 MB is "no good reason"?

The type is unsigned int. How did you get a >32-bit number in there in
the first place? I think you should fix the caller.

Where in lavc/f/u is it so difficult to check the value of the number
of bytes allocated that we have to do it for all cases, even though
most don't need it (think avi, or think static allocations such as
sizeof(struct bla))? Or can't you write a wrapper around it
(av_malloc64()) if you really want to for the few cases where the
number could potentially be >32bits?

Again, what is the bug you're trying to fix? The _real_ bug, not the
imaginary one.

Ronald