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

[Marek Vasut]

Dear Joakim Tjernlund,

> Marek Vasut <marek.vasut at gmail.com> wrote on 2012/04/02 20:00:03:
> > Dear Joakim Tjernlund,
> > 
> > > Marek Vasut <marek.vasut at gmail.com> wrote on 2012/04/02 18:39:33:
> > > > From: Marek Vasut <marek.vasut at gmail.com>
> > > > 
> > > > Dear Joakim Tjernlund,
> > > > 
> > > > > Marek Vasut <marek.vasut at gmail.com> wrote on 2012/04/02 17:23:03:
> > > > > > Dear Joakim Tjernlund,
> > > > > > 
> > > > > > > Marek Vasut <marek.vasut at gmail.com> wrote on 2012/04/02 16:42:30:
> > > > > > > > Dear Joakim Tjernlund,
> > > > > > > > 
> > > > > > > > > Marek Vasut <marek.vasut at gmail.com> wrote on 2012/04/02 
16:05:13:
> > > > > > > > > > Dear Joakim Tjernlund,
> > > > > > > > > > 
> > > > > > > > > > > Hi Grame
> > > > > > > > > > > 
> > > > > > > > > > > Graeme Russ <graeme.russ at gmail.com> wrote on 2012/04/02
> > 
> > 09:17:44:
> > > > > > > > > > > > Hi Joakim,
> > > > > > > > > > > > 
> > > > > > > > > > > > On Apr 2, 2012 4:55 PM, "Joakim Tjernlund"
> > > > > > > > > > > > <joakim.tjernlund at transmode.se>
> > > > > > > > > > 
> > > > > > > > > > wrote:
> > > > > > > > > > > > > > Hi Marek,
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > On Mon, Apr 2, 2012 at 1:36 PM, Marek Vasut
> > > > > > > > > > > > > > <marek.vasut at gmail.com>
> > > > > > > > > > 
> > > > > > > > > > wrote:
> > > > > > > > > > > > > > > Dear Mike Frysinger,
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > >> On Sunday 01 April 2012 20:25:44 Graeme Russ 
wrote:
> > > > > > > > > > > > > > >> > b) The code calling malloc(0) is making a
> > > > > > > > > > > > > > >> > perfectly legitimate assumption
> > > > > > > > > > > > > > >> > 
> > > > > > > > > > > > > > >> >    based on how glibc handles malloc(0)
> > > > > > > > > > > > > > >> 
> > > > > > > > > > > > > > >> not really.  POSIX says malloc(0) is
> > > > > > > > > > > > > > >> implementation defined (so it may return a
> > > > > > > > > > > > > > >> unique address, or it may return NULL). no
> > > > > > > > > > > > > > >> userspace code assuming malloc(0) will return
> > > > > > > > > > > > > > >> non-NULL is correct.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Which is your implementation-defined ;-) But I
> > > > > > > > > > > > > > > have to agree with this one. So my vote is for
> > > > > > > > > > > > > > > returning NULL.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Also, no userspace code assuming malloc(0) will
> > > > > > > > > > > > > > return NULL is correct
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Point being, no matter which implementation is
> > > > > > > > > > > > > > chosen, it is up to the caller to not assume that
> > > > > > > > > > > > > > the choice that was made was, in fact, the choice
> > > > > > > > > > > > > > that was made.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > I.e. the behaviour of malloc(0) should be able to
> > > > > > > > > > > > > > be changed on a whim with no side-effects
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > So I think I should change my vote to returning
> > > > > > > > > > > > > > NULL for one reason and one reason only - It is
> > > > > > > > > > > > > > faster during run-time
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Then u-boot will be incompatible with both glibc
> > > > > > > > > > > > > and the linux kernel, it seems
> > > > > > > > > > > > 
> > > > > > > > > > > > Forget aboug other implementations...
> > > > > > > > > > > > What matters is that the fact that the behaviour is
> > > > > > > > > > > > undefined and it is up to the caller to take that
> > > > > > > > > > > > into account
> > > > > > > > > > > 
> > > > > > > > > > > Well, u-boot borrows code from both kernel and user
> > > > > > > > > > > space so it would make sense if malloc(0) behaved the
> > > > > > > > > > > same. Especially for kernel code which tend to depend
> > > > > > > > > > > on the kernels impl.(just look at Scotts example)
> > > > > > > > > > > 
> > > > > > > > > > > > > to me that any modern impl. of malloc(0) will
> > > > > > > > > > > > > return a non NULL ptr.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > It does need to be slower, just return ~0 instead,
> > > > > > > > > > > > > the kernel does something similar: if (!size)
> > > > > > > > > > > > > 
> > > > > > > > > > > > >     return ZERO_SIZE_PTR;
> > > > > > > > > > > > 
> > > > > > > > > > > > That could work, but technically I don't think it
> > > > > > > > > > > > complies as it is not a pointer to allocated
> > > > > > > > > > > > memory...
> > > > > > > > > > > 
> > > > > > > > > > > It doesn't not have to be allocated memory, just a ptr
> > > > > > > > > > > != NULL which you can do free() on.
> > > > > > > > > > 
> > > > > > > > > > But kernel has something mapped there to trap these
> > > > > > > > > > pointers I believe.
> > > > > > > > > 
> > > > > > > > > So? That only means that the kernel has extra protection if
> > > > > > > > > someone tries to deference such a ptr. You are not required
> > > > > > > > > to do that(nice to have though) You don have any
> > > > > > > > > protection for deferencing NULL either I think?
> > > > > > > > 
> > > > > > > > Can't GCC track it?
> > > > > > > 
> > > > > > > Track what? NULL ptrs? I don't think so. Possibly when you have
> > > > > > > a static NULL ptr but not in the general case.
> > > > > > 
> > > > > > Well of course.
> > > > > 
> > > > > What did you mean then with "Can't GCC track it?" then? Just a bad
> > > > > joke?
> > > > 
> > > > Never mind, didn't finish my train of thought.
> > > 
> > > I almost figured that ...
> > > 
> > > > > > > I am getting tired of this discussion now. I am just trying to
> > > > > > > tell you that no sane impl. of malloc() these days return NULL
> > > > > > > for malloc(0).
> > > > > > 
> > > > > > And I got your point. Though for u-boot, this would be the best
> > > > > > solution actually. Anyone who uses memory allocated by malloc(0)
> > > > > > is insane.
> > > > > 
> > > > > No, you don't get the point. If you did you would not have have
> > > > > made the "insane" remark.
> > > > 
> > > > No, relying on malloc(0) returning something sane is messed up.
> > > 
> > > Depends, if writing generic code for lots of OS:es you cannot rely
> > > malloc(0). Writing kernel code you can.
> > 
> > No you cannot. It's in the spec you cannot and you have to behave
> > according to the spec, not according to kernel.
> 
> How so? The kernel is its own system and has it own rules and it is wise
> to follow them.

Why? We should follow the C spec first ;-)

> > > Not to mention those devs that
> > > don't
> > > know better and just assumes that what works in glibc/kernel works
> > > every where.
> > 
> > Well, they will be taught it's not like that. Are we gonna support
> > programmers who write crap code or what?
> 
> You do either way, now you support those who assume malloc(0) returns NULL

Is it the lesser of two evils or not? It certainly has benefits over allocating 
some small amount of data (speed).

> > > From Scotts example we already know there is kernel code that relies on
> > > malloc(0) not returning NULL.
> > 
> > Sure, but that means the code is messed up.
> 
> ohh, I don't think the kernel people will agree:
> http://lwn.net/Articles/236920/ But feel free to bring it up.

Ain't no fightin this with kernel folks.

> > > Your argument seems to boil down to "relying on malloc(0) returning
> > > something sane is messed up" so therefore u-boot malloc should take the
> > > easy route and just return NULL for malloc(0).
> > 
> > Basically, yes. It's correct according to the spec and we're not writing
> > on operating system here, it's still a bootloader, so KISS.
> 
> Right, it is a boot loader and it reuses code from both kernel and the open
> source community in general. So KISS here would be to follow suit.

You've got a valid point. On the other hand, if you return NULL, it'll be simple 
to find bugs. If you return a valid pointer, you'll get silent overwrites of 
memory (even mallocator structures), which is what bothers me :(

>  Jocke