[Qemu-devel] [PATCH] Permit zero-sized qemu_malloc() & friends

[Markus Armbruster]

Commit a7d27b53 made zero-sized allocations a fatal error, deviating
from ISO C's malloc() & friends.  Revert that, but take care never to
return a null pointer, like malloc() & friends may do (it's
implementation defined), because that's another source of bugs.

Rationale: while zero-sized allocations might occasionally be a sign of
something going wrong, they can also be perfectly legitimate.  The
change broke such legitimate uses.  We've found and "fixed" at least one
of them already (commit eb0b64f7, also reverted by this patch), and
another one just popped up: the change broke qcow2 images with virtual
disk size zero, i.e. images that don't hold real data but only VM state
of snapshots.

If a change breaks two uses, it probably breaks more.  As a quick check,
I reviewed the first six of more than 200 uses of qemu_mallocz(),
qemu_malloc() and qemu_realloc() that don't have an argument of the form
sizeof(...) or similar:

* load_elf32(), load_elf64() in hw/elf_ops.h:

    size = ehdr.e_phnum * sizeof(phdr[0]);
    lseek(fd, ehdr.e_phoff, SEEK_SET);
    phdr = qemu_mallocz(size);

  This aborts when the ELF file has no program header table, because
  then e_phnum is zero (TIS ELF 1.2 page 1-6).  Even if we require the
  ELF file to have a program header table, aborting is not an acceptable
  way to reject an unsuitable or malformed ELF file.

* load_elf32(), load_elf64() in hw/elf_ops.h:

            mem_size = ph->p_memsz;
            /* XXX: avoid allocating */
            data = qemu_mallocz(mem_size);

  This aborts when the segment occupies zero bytes in the image file
  (TIS ELF 1.2 page 2-2).

* bdrv_open2() in block.c:

    bs->opaque = qemu_mallocz(drv->instance_size);

  However, vvfat_write_target.instance_size is zero.  Not sure whether
  this actually bites or is "only" a time bomb.

* qemu_aio_get() in block.c:

        acb = qemu_mallocz(pool->aiocb_size);

  No existing instance of BlockDriverAIOCB has a zero aiocb_size.
  Probably okay.

* defaultallocator_create_displaysurface() in console.c has

    surface->data = (uint8_t*) qemu_mallocz(surface->linesize * 
surface->height);

  With Xen, surface->linesize and surface->height come out of
  xenfb_configure_fb(), which set xenfb->width and xenfb->height to
  values obtained from the guest.  If a buggy guest sets one to zero, we
  abort.  Not an good way to handle that.

  Non-Xen uses aren't obviously correct either, but I didn't dig deeper.

* load_device_tree() in device_tree.c:

    *sizep = 0;
    dt_size = get_image_size(filename_path);
    if (dt_size < 0) {
        printf("Unable to get size of device tree file '%s'\n",
            filename_path);
        goto fail;
    }

    /* Expand to 2x size to give enough room for manipulation.  */
    dt_size *= 2;
    /* First allocate space in qemu for device tree */
    fdt = qemu_mallocz(dt_size);

  We abort if the image is empty.  Not an acceptable way to handle that.

Based on this sample, I'm confident there are many more uses broken by
the change.

Signed-off-by: Markus Armbruster <address@hidden>
---
 block/qcow2-snapshot.c |    5 -----
 qemu-malloc.c          |   14 ++------------
 2 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 94cb838..e3b208c 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -381,11 +381,6 @@ int qcow2_snapshot_list(BlockDriverState *bs, 
QEMUSnapshotInfo **psn_tab)
     QCowSnapshot *sn;
     int i;
 
-    if (!s->nb_snapshots) {
-        *psn_tab = NULL;
-        return s->nb_snapshots;
-    }
-
     sn_tab = qemu_mallocz(s->nb_snapshots * sizeof(QEMUSnapshotInfo));
     for(i = 0; i < s->nb_snapshots; i++) {
         sn_info = sn_tab + i;
diff --git a/qemu-malloc.c b/qemu-malloc.c
index 295d185..aeeb78b 100644
--- a/qemu-malloc.c
+++ b/qemu-malloc.c
@@ -44,22 +44,12 @@ void qemu_free(void *ptr)
 
 void *qemu_malloc(size_t size)
 {
-    if (!size) {
-        abort();
-    }
-    return oom_check(malloc(size));
+    return oom_check(malloc(size ? size : 1));
 }
 
 void *qemu_realloc(void *ptr, size_t size)
 {
-    if (size) {
-        return oom_check(realloc(ptr, size));
-    } else {
-        if (ptr) {
-            return realloc(ptr, size);
-        }
-    }
-    abort();
+    return oom_check(realloc(ptr, size ? size : 1));
 }
 
 void *qemu_mallocz(size_t size)
-- 
1.6.2.5