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

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

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

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

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

 Jocke