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

[Markus Armbruster <armbru@redhat.com>]

malc <av1474@comtv.ru> 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.

    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;

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.

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

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

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

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