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

[Eduardo Habkost <ehabkost@redhat.com>]

On Tue, May 19, 2009 at 06:06:56PM +0400, malc wrote:
> On Tue, 19 May 2009, Markus Armbruster wrote:
> 
> > malc <av1474@comtv.ru> writes:
> > 
> 
> [..snip..]
> 
> > >>     diff --git a/block-qcow2.c b/block-qcow2.c
> > >>     index 9aa7261..d4556ef 100644
> > >>     --- a/block-qcow2.c
> > >>     +++ b/block-qcow2.c
> > >>     @@ -1809,6 +1809,12 @@ static int qcow_read_snapshots(BlockDriverState *bs)
> > >>          int64_t offset;
> > >>          uint32_t extra_data_size;
> > >> 
> > >>     +    if (!s->nb_snapshots) {
> > >>     +        s->snapshots = NULL;
> > >>     +        s->snapshots_size = 0;
> > >>     +        return 0;
> > >>     +    }
> > >>     +
> > >>          offset = s->snapshots_offset;
> > >>          s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
> > >>          if (!s->snapshots)
> > >> 
> > >> Can't see what this hunk accomplishes.  If we remove it, the loop
> > >> rejects, and we thus execute:
> > >> 
> 
> Once again, on Linux/GLIBC it will, on AIX it wont.

Why not? It will. If nb_snapshots is 0, it won't enter the loop. The
problem with that code was the "if (!s->snapshots)" check, not the
qemu_mallocz(0) call.


> 
> And FWIW despite behaviour of malloc(0) being marked as implementation
> defined i have sa far was unable to find any documentaiton (Linux man
> pages, GLIBC info files) witht the actual definition, unlike on AIX
> where man pages make it crystal clear what happens.

You don't need to have the exact behavior defined, as long as:

1) You call free(p) later
2) You don't dereference the returned pointer (just like you can't
   dereference p[n] on a malloc(n) block)
3) You don't assume anything about the returned value when size==0

My point is that this is valid malloc() usage, and there may be existing
qemu code relying on that, and I don't see any reason to put a trap for
code that would be valid malloc()/free() usage.


> 
> 
<snip>
> > 
> > Tries what?  Passing zero to qemu_malloc()?  That's legitimate.  And
> > with allocation functions that cannot return failure, it's hardly
> > dangerous, isn't it?
> 
> That's legitimate only if one writes unportable code targeting single
> system and knowing how it was defined.

No, that's legitimate and portable. You just can't assume anything about
the returned value.



> As for being dangerous, yes it
> is: dereferencing the returned pointer, while UB, doesn't trigger a
> SEGFAULT on, at least, this machine with Linux.
> 
> > >> qemu_realloc() currently uses 1.
> 
> void *qemu_realloc(void *ptr, size_t size)
> {
>     if (size)
>         return oom_check(realloc(ptr, size));
>     else
>         return realloc(ptr, size);
> }
>  
> There is nothing implementation defined about realloc(whatever, 0), it
> has a defined meaning in POSIX:
> http://opengroup.org/onlinepubs/007908775/xsh/realloc.html
> 
> So it doesn't use 1.
> 

realloc() return value is specified exactly the same way malloc() is:

"If size is 0, either a null pointer or a unique pointer that can be
successfully passed to free() is returned."


> > >> 
> > >> realloc(NULL, sz) is specified to be equivalent to malloc(sz).  It would
> > >> be kind of nice to keep that for qemu_realloc() and qemu_malloc().
> > >> 
> > >
> > > qemu_realloc shouldn't be called qemu_realloc if doesn't do that. The part
> > > about qemu_malloc escapes me.
> > 
> > qemu_malloc() & friends never fail.  Checking their value for failure is
> > pointless.  Therefore, 1. is practical.
> > 
> > 2. is certainly practical as well.
> > 
> > 3. is like 2, with the (size ? size : 1) pushed into callers.  I find
> > that mildly annoying.
> 
> Huh, that's not at all what i proposed. What i had in mind is:
> 
> void *qemu_malloc(size_t size)
> {
>     if (!size) abort();
>     return oom_check(malloc(size));
> }

Understood. And that's exactly what I think we should not do.

-- 
Eduardo