[U-Boot] [PATCH] Prevent malloc with size 0

[Graeme Russ]

Hi Marek,

On Mon, Apr 2, 2012 at 9:45 AM, Marek Vasut <marek.vasut at gmail.com> wrote:
> Dear Graeme Russ,
>
>> Hi All
>>
>> Here we go again ;)
>
> Yay (polishing my flamethrower)!
>
>> On Mon, Apr 2, 2012 at 12:21 AM, Marek Vasut <marek.vasut at gmail.com> wrote:
>> > Dear Joakim Tjernlund,
>> >
>> >> Marek Vasut <marek.vasut at gmail.com> wrote on 2012/04/01 16:01:56:
>> >> > Dear Joakim Tjernlund,
>> >> >
>> >> > > > Dear Mike Frysinger,
>> >> > > >
>> >> > > > > On Thursday, October 21, 2010 17:10:31 Graeme Russ wrote:
>> >> > > > > > On 22/10/10 06:51, Mike Frysinger wrote:
>> >> > > > > > > have u-boot return an error.
>> >> > > > > >
>> >> > > > > > Is NULL what you consider to be an error
>> >> > > > >
>> >> > > > > yes
>> >> > > > >
>> >> > > > > > Besides, is not free(NULL) valid (does nothing) as well?
>> >> > > > >
>> >> > > > > yes, free(NULL) should work fine per POSIX
>> >> > > > > -mike
>> >> > > >
>> >> > > > Well then, this patch wasn't accepted yet and I consider it OK to
>> >> > > > apply. Any objections?
>> >> > >
>> >> > > There was a long debate on the list regarding this where I argued
>> >> > > that malloc(0) should not be an error and malloc should return a
>> >> > > ptr != NULL I guess that is why it hasn't been applied.
>> >> > >
>> >> > >  Jocke
>> >> >
>> >> > Ok, let's restart. Is there any objection why malloc(0) should not
>> >> > return NULL in uboot?
>> >>
>> >> Yes, read the thread to see why.
>> >
>> > Well I did, that's why I have no objections to applying this patch
>> >
>> >> > Is it coliding with any spec?
>> >>
>> >> No, both are valid.
>>
>> <quote author="Reinhard Meyer">
>> Out of principle I would say that malloc(0) should return a non-NULL
>> pointer of an area where exactly 0 bytes may be used. And, of course,
>> free() of that area shall not fail or crash the system.
>> </quote>
>>
>> I'm wondering how exactly this would work - In theory, if you tried to
>> access this pointer you should get a segv. But I suppose if you malloc(1)
>> and try to access beyond the first byte there probably won't be a segv
>> either....
>>
>> So to review the facts:
>>
>> - The original complaint was that malloc(0) corrupts the malloc data
>>   structures, not that U-Boot's malloc(0) behaviour is non-standard
>> - Both the malloc(0) returns NULL and malloc(0) returns a uniquely
>>   free'able block of memory solutions are standard compliant
>> - malloc(0) returning NULL may break code which, for the sake of code
>>   simplicity, does not bother to check for zero-size before calling
>>   malloc()
>
> Well but you said malloc(0) corrupts the mallocator's data structures. Therefore
> malloc(0) used in code right now is broken anyway.

Correct, but the breakage is in malloc() not the caller

>> - malloc(0) returning NULL may help to identify brain-dead use-cases
>
> Agreed.
>
>>
>> My vote:
>>
>>         if ((long)bytes == 0) {
>>                 DEBUG("Warning: malloc of zero block size\n");
>>                 bytes = 1;
>
> Well ... no, how can malloc(0) returning NULL break code that's already broken
> any more? It's silently roughing the mallocator structures up and it means the
> code is sitting on a ticking a-bomb anyway.
>
> So we should add this like:
>
> if (bytes == 0) {
>        debug("You're sitting on a ticking A-Bomb doing this");

Because you just set it off - Right now, that code is assuming malloc(0)
will return a valid pointer and thus not throw an E_NOMEM error - Now
all that code will fail with E_NOMEM

>        return NULL;
> } else if (bytes < 0) {
>        return NULL;
> }
>
>>         } else if ((long)bytes < 0) {
>>                 DEBUG("Error: malloc of negative block size\n");
>>                 return 0;
>>         }

Regards,

Graeme