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:
> 
> > On Mon, 18 May 2009, Eduardo Habkost wrote:
> >
> >> On Tue, May 19, 2009 at 01:56:55AM +0400, malc wrote:
> >> > On Mon, 18 May 2009, Eduardo Habkost wrote:
> >> > 
> >> > > This patch is similar to a previous qemu_realloc() fix
> >> > > (commit 322691a5c9f1c8531554148d47c078b5be590805), but for 
> >> > > qemu_malloc().
> >> > > 
> >> > > malloc(0) may correctly return NULL if size==0. We don't want to abort 
> >> > > qemu on
> >> > > this case.
> >> > 
> [...]
> >> > Standard (in 7.20.3) says that malloc's behaviour in case of size being
> >> > zero is implementation defined.
> >> > 
> >> > Try `git show 63c75dcd669d011f438421980b4379827da4bb1c'.
> >> > 
> >> > The best(only?) thing to do is to check size passed to qemu_malloc[z]
> >> > and abort the program if this situation is encountered.
> >> 
> >> Why? malloc(0) is as valid as realloc(p, 0). It will either return NULL
> >> or a pointer, and on any case the value can be safely passed to free()
> >> later.
> >
> > I believe you haven't examined the commit that i referenced. Thing is
> 
> Well, I just did, but I don't get it.

Pretty good sign that something is wrong, no?
> 
>     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:
> 
>     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>

> 
> Next hunk:
> 
>     @@ -2023,8 +2029,10 @@ static int qcow_snapshot_create(BlockDriverState 
> *bs,
>          snapshots1 = qemu_malloc((s->nb_snapshots + 1) * 
> sizeof(QCowSnapshot));
>          if (!snapshots1)
>              goto fail;
>     -    memcpy(snapshots1, s->snapshots, s->nb_snapshots * 
> sizeof(QCowSnapshot));
>     -    qemu_free(s->snapshots);
>     +    if (s->snapshots) {
>     +        memcpy(snapshots1, s->snapshots, s->nb_snapshots * 
> sizeof(QCowSnapshot)
>     );
>     +        qemu_free(s->snapshots);
>     +    }
>          s->snapshots = snapshots1;
>          s->snapshots[s->nb_snapshots++] = *sn;
> 
> Again, I can't see the point.  if !s->snapshots, then s->nb_snapshots ==
> 0, doesn't it?  memcpy(snapshots1, NULL, 0) does nothing.  free(NULL)
> does nothing.

Wish i had your confidence and analysis skills, after skimming over
2666 lines of C in block-qcow2.c i can safely assert that this is
indeed the only condition under which s->snapshots can be NULL, hence
this hunk.

> I'm sure you didn't patch this for nothing, so I must miss something.
> Could you enlighten me?
> 

I hope that the above explanation is good enough, please do tell if it's not.

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.

> >                                              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.
> 
> qemu_realloc() currently uses 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.

-- 
mailto:address@hidden