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

[Marek Vasut]

Dear 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

And what are the consequences of such a breakage?

> >> - 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

Well ... that code worked with invalid memory (most probably not even R/W 
because it was some completely random hunk) and worked only by sheer 
coincidence. Let's break it, it was broken anyway.

Do you know about any such code? That's why I suggest adding such a debug() only 
in case there's malloc(0) called. Maybe even add a printf() instead.

> >        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