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

[malc]

On Tue, 19 May 2009, Markus Armbruster wrote:

> malc <address@hidden> 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.

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.


> >>     offset = s->snapshots_offset;
> >>     s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
> >>     s->snapshots_size = offset - s->snapshots_offset;
> >>     return 0;
> >
> > Uh, no.
> >
> > $ git cat-file -p 63c75dcd669d011f438421980b4379827da4bb1c^:block-qcow2.c | 
> > sed -n 1811,1815p
> >     offset = s->snapshots_offset;
> >     s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
> >     if (!s->snapshots)
> >         goto fail;
> >
> > So before the patch qemu_mallocz(0) call would return NULL, goto fail 
> > would be executed and `qcow_read_snapshots' will return -1 and QEMU
> > will fail to start with very helpful message:
> > qemu: could not open disk image <image name>
> 
> The test went away in commit 3ec88e80.  The hunk became pointless only
> then.

Again, it's pointless only with your proposed addition, otherwise
instead of 'could not open disk image' one would get an out of memory
error.

[..snip..]

> 
> > Also please notice how throwing second implemenation-defined behaviour of
> > malloc in to the mix led to the hunk #2, in which you see no point.
> >
> >> > existing code used to, i'd venture a guess accidentaly, rely on the
> >> > behaviour that current GLIBC provides and consequently failed to
> >> > operate on AIX where malloc(0) returns NULL,
> >> 
> >> Common error.
> >
> > Made by a very skilled programmer i might add.
> 
> I've fallen into this trap myself.
> 
> >> >                                              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.
> 
> 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. 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(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));
}

> 
> I don't care whether we pick 1., 2. or 3., but I'd prefer we pick the
> same for qemu_malloc() and qemu_realloc().  We currently use 1. for
> qemu_realloc().  My point is: if you pick something else for
> qemu_malloc(), please consider changing qemu_realloc().  That's all.

I see.

-- 
mailto:address@hidden