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

Because qemu_mallocz on AIX will be terminated by oom_check.

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

I certainly don't, the standard certainly says the implementation is
obliged to document it, that's what seprates implementation-defined
from unspecified behaviour.
 
> 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.

Okay, at this point we both expressed our points of view and have to
agree to disagree.
 
> 
> > 
> > 
> <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."

Nope, quoting from above page:

If size is 0 and ptr is not a null pointer, the object pointed to is 
freed.

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

-- 
mailto:av1474@comtv.ru