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

[malc]

On Mon, 30 Nov 2009, Markus Armbruster wrote:

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

Bottom line: your point is that there are benign uses of zero allocation.
There are, at the expense of memory consumption/fragmentation and useless
code which, as your investigation shows, involve syscalls and what not.

Outlook from my side of the fence: no one audited the code, no one
knows that all of the uses are benign, abort gives the best automatic
way to know for sure one way or the other.

Rationale for keeping it as is: zero-sized allocations - overwhelming
majority of the times, are not anticipated and not well understood,
aborting gives us the ability to eliminate them in an automatic
fashion.

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

If there's a malformed ELF files that must be supported (and by supported
- user notification is meant) then things should be checked, because having
gargantuan size will force qemu_mallocz to abort via OOM check (even
with Linux and overcommit, since malloc will fallback to mmap which
will fail) and this argument falls apart.


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

What is? Let's suppose height is zero, without explicit checks, user
will never know why his screen doesn't update, with abort he will at
least know that something is very wrong.

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

Based on this sample i'm confident, we can eventually fix them should those
become the problem.

> 
> 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));
>  }

http://groups.google.com/group/comp.std.c/browse_thread/thread/4e9af8847613d71f/6f75ad22e0768a0b?q=realloc++group:comp.std.c#6f75ad22e0768a0b

-- 
mailto:address@hidden