Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[malc <av1474@comtv.ru>]

On Tue, 19 May 2009, Eduardo Habkost wrote:

> On Tue, May 19, 2009 at 06:37:18PM +0400, malc wrote:
> > On Tue, 19 May 2009, Eduardo Habkost wrote:
> > 
> > > On Tue, May 19, 2009 at 05:00:27PM +0400, malc wrote:
> > > > On Tue, 19 May 2009, Markus Armbruster wrote:
> > > <snip>
> > > > > >                                              IOW making qemu_malloc[z]
> > > > > > return whatever the underlying system returns is just hiding the bugs,
> > > > > > the code becomes unportable.
> > > > > 
> > > > > Matter of taste.
> > > > > 
> > > > > 1. Deal with the implementation-definedness.  Every caller that could
> > > > >    pass zero needs to take care not to confuse empty allocation with an
> > > > >    out of memory condition.
> > > > > 
> > > > >    This is easier than it sounds when you check for out of memory in
> > > > >    just one place, like we do.
> > > > > 
> > > > > 2. Remove the implementation-definedness.  Easiest way is to detect zero
> > > > >    size in a wrapper (for us: qemu_malloc()) and bump it to one.
> > > > 
> > > > And mine:
> > > >   3. Abort the program if somebody tries it. Because so far history thought
> > > >      me that nobody does 1.
> > > 
> > > Are you sure about that? There may be cases where qemu_malloc(0) is
> > > called correctly, without the wrong assumptions about the returned
> > > value.
> > > 
> > > You are proposing to make the qemu_malloc() API behavior diverge from
> > > the standard C malloc() behavior and prevent usage that is valid for
> > > malloc()/free() usage. Do you volunteer to audit all Qemu code to make
> > > sure the new behavior is safe?  ;)
> > 
> > That's the problem standard C does _not_ define the behaviour, and leaves
> > that to implementation.
> 
> The only thing it doesn't define is either the returned pointer is NULL
> or not, and that doesn't make malloc(0) automatically unportable,
> because all the rest is perfectly defined:
> 
> 1) You can't dereference the pointer (just like you can't
>    dereference p[n] on a malloc(n) block)
> 2) You should pass the returned pointer to free() later
> 

Alas your list is not exhaustive:

  3) Test the returned value against NULL

[Which is precisely what the qcow2 code did]

And

  4) Accidentally derefence it

And here one would get away with it on certain subset of systems.

> 
> > As for audit, that's precisely what aborting on
> > zero will try (and fail) to accomplish the offenders will be (given
> > unlimited time) caught.
> 
> My point is that malloc(0) is not a bug, and I don't see a reason to
> make it an offense and diverge from standard malloc() and free().
> 
> > oom_check was added despite the fact that there
> > were places that correctly handled malloc's returning NULL. And i for
> > one do not know if there are/were places that called qemu_malloc with
> > zero and expected Linux behaviour.
> 
> I agree that expecting the Linux behaviour (non-NULL) is a bug. My point
> is that there is no reason to consider malloc(0) a bug.

There is, due to the possibility of performing a 3) and a hard time 
catching that (unless someone solves halting problem or subset applicable
to QEMU thereof)

-- 
mailto:av1474@comtv.ru